| Message ID | 1449743372-17169-10-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 12/10/2015 12:29 PM, Markus Armbruster wrote: > We can have at most one ISA bus. If you try to create another one, > isa_bus_new() complains to stderr and returns null. > > isa_bus_new() is called in two contexts, machine's init() and device's > realize() methods. Since complaining to stderr is not proper in the > latter context, convert isa_bus_new() to Error. > > Machine's init(): > > * mips_jazz_init(), called from the init() methods of machines > "magnum" and "pica" > > * mips_r4k_init(), the init() method of machine "mips" > > * pc_init1() called from the init() methods of non-q35 PC machines > > * typhoon_init(), called from clipper_init(), the init() method of > machine "clipper" > > These callers always create the first ISA bus, hence isa_bus_new() > can't fail. Simply pass &error_abort. > > Device's realize(): > > * i82378_realize(), of PCI device "i82378" > > * ich9_lpc_realize(), of PCI device "ICH9-LPC" > > * pci_ebus_realize(), of PCI device "ebus" > > * piix3_realize(), of PCI device "pci-piix3", abstract parent of > "PIIX3" and "PIIX3-xen" > > * piix4_realize(), of PCI device "PIIX4" > > * vt82c686b_realize(), of PCI device "VT82C686B" > > Propagate the error. Note that these devices are typically created > only by machine init() methods with qdev_init_nofail() or similar. If > we screwed up and created an ISA bus before that call, we now give up > right away. Before, we'd hobble on, and typically die in > isa_bus_irqs(). Similar if someone finds a way to hot-plug one of > these critters. Makes sense. > > Cc: Richard Henderson <rth@twiddle.net> > Cc: "Michael S. Tsirkin" <mst@redhat.com> > Cc: "Hervé Poussineau" <hpoussin@reactos.org> > Cc: Aurelien Jarno <aurelien@aurel32.net> > Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > hw/alpha/typhoon.c | 3 ++- > hw/i386/pc_piix.c | 3 ++- > hw/isa/i82378.c | 5 ++++- > hw/isa/isa-bus.c | 4 ++-- > hw/isa/lpc_ich9.c | 6 +++++- > hw/isa/piix4.c | 6 ++++-- > hw/isa/vt82c686.c | 5 ++++- > hw/mips/mips_jazz.c | 2 +- > hw/mips/mips_r4k.c | 2 +- > hw/pci-host/piix.c | 6 ++++-- > hw/sparc64/sun4u.c | 6 ++++-- > include/hw/isa/isa.h | 2 +- > 12 files changed, 34 insertions(+), 16 deletions(-) > > diff --git a/hw/alpha/typhoon.c b/hw/alpha/typhoon.c > index 421162e..35dc8a5 100644 > --- a/hw/alpha/typhoon.c > +++ b/hw/alpha/typhoon.c > @@ -920,7 +920,8 @@ PCIBus *typhoon_init(ram_addr_t ram_size, ISABus **isa_bus, > { > qemu_irq *isa_irqs; > > - *isa_bus = isa_bus_new(NULL, get_system_memory(), &s->pchip.reg_io); > + *isa_bus = isa_bus_new(NULL, get_system_memory(), &s->pchip.reg_io, > + &error_abort); > isa_irqs = i8259_init(*isa_bus, > qemu_allocate_irq(typhoon_set_isa_irq, s, 0)); > isa_bus_irqs(*isa_bus, isa_irqs); > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > index 2e41efe..48fdad4 100644 > --- a/hw/i386/pc_piix.c > +++ b/hw/i386/pc_piix.c > @@ -200,7 +200,8 @@ static void pc_init1(MachineState *machine, > } else { > pci_bus = NULL; > i440fx_state = NULL; > - isa_bus = isa_bus_new(NULL, get_system_memory(), system_io); > + isa_bus = isa_bus_new(NULL, get_system_memory(), system_io, > + &error_abort); > no_hpet = 1; > } > isa_bus_irqs(isa_bus, gsi); > diff --git a/hw/isa/i82378.c b/hw/isa/i82378.c > index d4c8306..3793c6f 100644 > --- a/hw/isa/i82378.c > +++ b/hw/isa/i82378.c > @@ -75,7 +75,10 @@ static void i82378_realize(PCIDevice *pci, Error **errp) > pci_config_set_interrupt_pin(pci_conf, 1); /* interrupt pin 0 */ > > isabus = isa_bus_new(dev, get_system_memory(), > - pci_address_space_io(pci)); > + pci_address_space_io(pci), errp); > + if (!isabus) { > + return; > + } > > /* This device has: > 2 82C59 (irq) > diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c > index 43e0cd8..af6ffd6 100644 > --- a/hw/isa/isa-bus.c > +++ b/hw/isa/isa-bus.c > @@ -44,10 +44,10 @@ static const TypeInfo isa_bus_info = { > }; > > ISABus *isa_bus_new(DeviceState *dev, MemoryRegion* address_space, > - MemoryRegion *address_space_io) > + MemoryRegion *address_space_io, Error **errp) > { > if (isabus) { > - fprintf(stderr, "Can't create a second ISA bus\n"); > + error_setg(errp, "Can't create a second ISA bus"); > return NULL; > } > if (!dev) { > diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c > index 8e58449..ed9907d 100644 > --- a/hw/isa/lpc_ich9.c > +++ b/hw/isa/lpc_ich9.c > @@ -607,7 +607,11 @@ static void ich9_lpc_realize(PCIDevice *d, Error **errp) > ICH9LPCState *lpc = ICH9_LPC_DEVICE(d); > ISABus *isa_bus; > > - isa_bus = isa_bus_new(DEVICE(d), get_system_memory(), get_system_io()); > + isa_bus = isa_bus_new(DEVICE(d), get_system_memory(), get_system_io(), > + errp); > + if (!isa_bus) { > + return; > + } > > pci_set_long(d->wmask + ICH9_LPC_PMBASE, > ICH9_LPC_PMBASE_BASE_ADDRESS_MASK); > diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c > index 2c59e91..644cfd9 100644 > --- a/hw/isa/piix4.c > +++ b/hw/isa/piix4.c > @@ -90,8 +90,10 @@ static void piix4_realize(PCIDevice *dev, Error **errp) > { > PIIX4State *d = PIIX4_PCI_DEVICE(dev); > > - isa_bus_new(DEVICE(d), pci_address_space(dev), > - pci_address_space_io(dev)); > + if (!isa_bus_new(DEVICE(d), pci_address_space(dev), > + pci_address_space_io(dev), errp)) { > + return; > + } > piix4_dev = &d->dev; > qemu_register_reset(piix4_reset, d); > } > diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c > index 252e1d7..6c2190b 100644 > --- a/hw/isa/vt82c686.c > +++ b/hw/isa/vt82c686.c > @@ -440,7 +440,10 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp) > int i; > > isa_bus = isa_bus_new(DEVICE(d), get_system_memory(), > - pci_address_space_io(d)); > + pci_address_space_io(d), errp); > + if (!isa_bus) { > + return; > + } > > pci_conf = d->config; > pci_config_set_prog_interface(pci_conf, 0x0); > diff --git a/hw/mips/mips_jazz.c b/hw/mips/mips_jazz.c > index 1ab885b..1cfbaa6 100644 > --- a/hw/mips/mips_jazz.c > +++ b/hw/mips/mips_jazz.c > @@ -219,7 +219,7 @@ static void mips_jazz_init(MachineState *machine, > memory_region_init(isa_mem, NULL, "isa-mem", 0x01000000); > memory_region_add_subregion(address_space, 0x90000000, isa_io); > memory_region_add_subregion(address_space, 0x91000000, isa_mem); > - isa_bus = isa_bus_new(NULL, isa_mem, isa_io); > + isa_bus = isa_bus_new(NULL, isa_mem, isa_io, &error_abort); > > /* ISA devices */ > i8259 = i8259_init(isa_bus, env->irq[4]); > diff --git a/hw/mips/mips_r4k.c b/hw/mips/mips_r4k.c > index af10da1..2d4e038 100644 > --- a/hw/mips/mips_r4k.c > +++ b/hw/mips/mips_r4k.c > @@ -272,7 +272,7 @@ void mips_r4k_init(MachineState *machine) > memory_region_init(isa_mem, NULL, "isa-mem", 0x01000000); > memory_region_add_subregion(get_system_memory(), 0x14000000, isa_io); > memory_region_add_subregion(get_system_memory(), 0x10000000, isa_mem); > - isa_bus = isa_bus_new(NULL, isa_mem, get_system_io()); > + isa_bus = isa_bus_new(NULL, isa_mem, get_system_io(), &error_abort); > > /* The PIC is attached to the MIPS CPU INT0 pin */ > i8259 = i8259_init(isa_bus, env->irq[2]); > diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c > index 715208b..775b4bd 100644 > --- a/hw/pci-host/piix.c > +++ b/hw/pci-host/piix.c > @@ -651,8 +651,10 @@ static void piix3_realize(PCIDevice *dev, Error **errp) > { > PIIX3State *d = PIIX3_PCI_DEVICE(dev); > > - isa_bus_new(DEVICE(d), get_system_memory(), > - pci_address_space_io(dev)); > + if (!isa_bus_new(DEVICE(d), get_system_memory(), > + pci_address_space_io(dev), errp)) { > + return; > + } > > memory_region_init_io(&d->rcr_mem, OBJECT(dev), &rcr_ops, d, > "piix3-reset-control", 1); > diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c > index c37e8b0..6c7596b 100644 > --- a/hw/sparc64/sun4u.c > +++ b/hw/sparc64/sun4u.c > @@ -597,8 +597,10 @@ static void pci_ebus_realize(PCIDevice *pci_dev, Error **errp) > { > EbusState *s = DO_UPCAST(EbusState, pci_dev, pci_dev); > > - isa_bus_new(DEVICE(pci_dev), get_system_memory(), > - pci_address_space_io(pci_dev)); > + if (!isa_bus_new(DEVICE(pci_dev), get_system_memory(), > + pci_address_space_io(pci_dev), errp)) { > + return; > + } > > pci_dev->config[0x04] = 0x06; // command = bus master, pci mem > pci_dev->config[0x05] = 0x00; > diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h > index d758b39..de3cd3d 100644 > --- a/include/hw/isa/isa.h > +++ b/include/hw/isa/isa.h > @@ -59,7 +59,7 @@ struct ISADevice { > }; > > ISABus *isa_bus_new(DeviceState *dev, MemoryRegion *address_space, > - MemoryRegion *address_space_io); > + MemoryRegion *address_space_io, Error **errp); > void isa_bus_irqs(ISABus *bus, qemu_irq *irqs); > qemu_irq isa_get_irq(ISADevice *dev, int isairq); > void isa_init_irq(ISADevice *dev, qemu_irq *p, int isairq); > Looks good to me. Reviewed-by: Marcel Apfelbaum <marcel@redhat.com> Thanks Marcel
Le 10/12/2015 11:29, Markus Armbruster a écrit : > We can have at most one ISA bus. If you try to create another one, > isa_bus_new() complains to stderr and returns null. > > isa_bus_new() is called in two contexts, machine's init() and device's > realize() methods. Since complaining to stderr is not proper in the > latter context, convert isa_bus_new() to Error. > > Machine's init(): > > * mips_jazz_init(), called from the init() methods of machines > "magnum" and "pica" > > * mips_r4k_init(), the init() method of machine "mips" > > * pc_init1() called from the init() methods of non-q35 PC machines > > * typhoon_init(), called from clipper_init(), the init() method of > machine "clipper" > > These callers always create the first ISA bus, hence isa_bus_new() > can't fail. Simply pass &error_abort. > > Device's realize(): > > * i82378_realize(), of PCI device "i82378" > > * ich9_lpc_realize(), of PCI device "ICH9-LPC" > > * pci_ebus_realize(), of PCI device "ebus" > > * piix3_realize(), of PCI device "pci-piix3", abstract parent of > "PIIX3" and "PIIX3-xen" > > * piix4_realize(), of PCI device "PIIX4" > > * vt82c686b_realize(), of PCI device "VT82C686B" > > Propagate the error. Note that these devices are typically created > only by machine init() methods with qdev_init_nofail() or similar. If > we screwed up and created an ISA bus before that call, we now give up > right away. Before, we'd hobble on, and typically die in > isa_bus_irqs(). Similar if someone finds a way to hot-plug one of > these critters. > > Cc: Richard Henderson <rth@twiddle.net> > Cc: "Michael S. Tsirkin" <mst@redhat.com> > Cc: "Hervé Poussineau" <hpoussin@reactos.org> > Cc: Aurelien Jarno <aurelien@aurel32.net> > Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- Reviewed-by: Hervé Poussineau <hpoussin@reactos.org>
diff --git a/hw/alpha/typhoon.c b/hw/alpha/typhoon.c index 421162e..35dc8a5 100644 --- a/hw/alpha/typhoon.c +++ b/hw/alpha/typhoon.c @@ -920,7 +920,8 @@ PCIBus *typhoon_init(ram_addr_t ram_size, ISABus **isa_bus, { qemu_irq *isa_irqs; - *isa_bus = isa_bus_new(NULL, get_system_memory(), &s->pchip.reg_io); + *isa_bus = isa_bus_new(NULL, get_system_memory(), &s->pchip.reg_io, + &error_abort); isa_irqs = i8259_init(*isa_bus, qemu_allocate_irq(typhoon_set_isa_irq, s, 0)); isa_bus_irqs(*isa_bus, isa_irqs); diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index 2e41efe..48fdad4 100644 --- a/hw/i386/pc_piix.c +++ b/hw/i386/pc_piix.c @@ -200,7 +200,8 @@ static void pc_init1(MachineState *machine, } else { pci_bus = NULL; i440fx_state = NULL; - isa_bus = isa_bus_new(NULL, get_system_memory(), system_io); + isa_bus = isa_bus_new(NULL, get_system_memory(), system_io, + &error_abort); no_hpet = 1; } isa_bus_irqs(isa_bus, gsi); diff --git a/hw/isa/i82378.c b/hw/isa/i82378.c index d4c8306..3793c6f 100644 --- a/hw/isa/i82378.c +++ b/hw/isa/i82378.c @@ -75,7 +75,10 @@ static void i82378_realize(PCIDevice *pci, Error **errp) pci_config_set_interrupt_pin(pci_conf, 1); /* interrupt pin 0 */ isabus = isa_bus_new(dev, get_system_memory(), - pci_address_space_io(pci)); + pci_address_space_io(pci), errp); + if (!isabus) { + return; + } /* This device has: 2 82C59 (irq) diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c index 43e0cd8..af6ffd6 100644 --- a/hw/isa/isa-bus.c +++ b/hw/isa/isa-bus.c @@ -44,10 +44,10 @@ static const TypeInfo isa_bus_info = { }; ISABus *isa_bus_new(DeviceState *dev, MemoryRegion* address_space, - MemoryRegion *address_space_io) + MemoryRegion *address_space_io, Error **errp) { if (isabus) { - fprintf(stderr, "Can't create a second ISA bus\n"); + error_setg(errp, "Can't create a second ISA bus"); return NULL; } if (!dev) { diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c index 8e58449..ed9907d 100644 --- a/hw/isa/lpc_ich9.c +++ b/hw/isa/lpc_ich9.c @@ -607,7 +607,11 @@ static void ich9_lpc_realize(PCIDevice *d, Error **errp) ICH9LPCState *lpc = ICH9_LPC_DEVICE(d); ISABus *isa_bus; - isa_bus = isa_bus_new(DEVICE(d), get_system_memory(), get_system_io()); + isa_bus = isa_bus_new(DEVICE(d), get_system_memory(), get_system_io(), + errp); + if (!isa_bus) { + return; + } pci_set_long(d->wmask + ICH9_LPC_PMBASE, ICH9_LPC_PMBASE_BASE_ADDRESS_MASK); diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c index 2c59e91..644cfd9 100644 --- a/hw/isa/piix4.c +++ b/hw/isa/piix4.c @@ -90,8 +90,10 @@ static void piix4_realize(PCIDevice *dev, Error **errp) { PIIX4State *d = PIIX4_PCI_DEVICE(dev); - isa_bus_new(DEVICE(d), pci_address_space(dev), - pci_address_space_io(dev)); + if (!isa_bus_new(DEVICE(d), pci_address_space(dev), + pci_address_space_io(dev), errp)) { + return; + } piix4_dev = &d->dev; qemu_register_reset(piix4_reset, d); } diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c index 252e1d7..6c2190b 100644 --- a/hw/isa/vt82c686.c +++ b/hw/isa/vt82c686.c @@ -440,7 +440,10 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp) int i; isa_bus = isa_bus_new(DEVICE(d), get_system_memory(), - pci_address_space_io(d)); + pci_address_space_io(d), errp); + if (!isa_bus) { + return; + } pci_conf = d->config; pci_config_set_prog_interface(pci_conf, 0x0); diff --git a/hw/mips/mips_jazz.c b/hw/mips/mips_jazz.c index 1ab885b..1cfbaa6 100644 --- a/hw/mips/mips_jazz.c +++ b/hw/mips/mips_jazz.c @@ -219,7 +219,7 @@ static void mips_jazz_init(MachineState *machine, memory_region_init(isa_mem, NULL, "isa-mem", 0x01000000); memory_region_add_subregion(address_space, 0x90000000, isa_io); memory_region_add_subregion(address_space, 0x91000000, isa_mem); - isa_bus = isa_bus_new(NULL, isa_mem, isa_io); + isa_bus = isa_bus_new(NULL, isa_mem, isa_io, &error_abort); /* ISA devices */ i8259 = i8259_init(isa_bus, env->irq[4]); diff --git a/hw/mips/mips_r4k.c b/hw/mips/mips_r4k.c index af10da1..2d4e038 100644 --- a/hw/mips/mips_r4k.c +++ b/hw/mips/mips_r4k.c @@ -272,7 +272,7 @@ void mips_r4k_init(MachineState *machine) memory_region_init(isa_mem, NULL, "isa-mem", 0x01000000); memory_region_add_subregion(get_system_memory(), 0x14000000, isa_io); memory_region_add_subregion(get_system_memory(), 0x10000000, isa_mem); - isa_bus = isa_bus_new(NULL, isa_mem, get_system_io()); + isa_bus = isa_bus_new(NULL, isa_mem, get_system_io(), &error_abort); /* The PIC is attached to the MIPS CPU INT0 pin */ i8259 = i8259_init(isa_bus, env->irq[2]); diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c index 715208b..775b4bd 100644 --- a/hw/pci-host/piix.c +++ b/hw/pci-host/piix.c @@ -651,8 +651,10 @@ static void piix3_realize(PCIDevice *dev, Error **errp) { PIIX3State *d = PIIX3_PCI_DEVICE(dev); - isa_bus_new(DEVICE(d), get_system_memory(), - pci_address_space_io(dev)); + if (!isa_bus_new(DEVICE(d), get_system_memory(), + pci_address_space_io(dev), errp)) { + return; + } memory_region_init_io(&d->rcr_mem, OBJECT(dev), &rcr_ops, d, "piix3-reset-control", 1); diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c index c37e8b0..6c7596b 100644 --- a/hw/sparc64/sun4u.c +++ b/hw/sparc64/sun4u.c @@ -597,8 +597,10 @@ static void pci_ebus_realize(PCIDevice *pci_dev, Error **errp) { EbusState *s = DO_UPCAST(EbusState, pci_dev, pci_dev); - isa_bus_new(DEVICE(pci_dev), get_system_memory(), - pci_address_space_io(pci_dev)); + if (!isa_bus_new(DEVICE(pci_dev), get_system_memory(), + pci_address_space_io(pci_dev), errp)) { + return; + } pci_dev->config[0x04] = 0x06; // command = bus master, pci mem pci_dev->config[0x05] = 0x00; diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h index d758b39..de3cd3d 100644 --- a/include/hw/isa/isa.h +++ b/include/hw/isa/isa.h @@ -59,7 +59,7 @@ struct ISADevice { }; ISABus *isa_bus_new(DeviceState *dev, MemoryRegion *address_space, - MemoryRegion *address_space_io); + MemoryRegion *address_space_io, Error **errp); void isa_bus_irqs(ISABus *bus, qemu_irq *irqs); qemu_irq isa_get_irq(ISADevice *dev, int isairq); void isa_init_irq(ISADevice *dev, qemu_irq *p, int isairq);
We can have at most one ISA bus. If you try to create another one, isa_bus_new() complains to stderr and returns null. isa_bus_new() is called in two contexts, machine's init() and device's realize() methods. Since complaining to stderr is not proper in the latter context, convert isa_bus_new() to Error. Machine's init(): * mips_jazz_init(), called from the init() methods of machines "magnum" and "pica" * mips_r4k_init(), the init() method of machine "mips" * pc_init1() called from the init() methods of non-q35 PC machines * typhoon_init(), called from clipper_init(), the init() method of machine "clipper" These callers always create the first ISA bus, hence isa_bus_new() can't fail. Simply pass &error_abort. Device's realize(): * i82378_realize(), of PCI device "i82378" * ich9_lpc_realize(), of PCI device "ICH9-LPC" * pci_ebus_realize(), of PCI device "ebus" * piix3_realize(), of PCI device "pci-piix3", abstract parent of "PIIX3" and "PIIX3-xen" * piix4_realize(), of PCI device "PIIX4" * vt82c686b_realize(), of PCI device "VT82C686B" Propagate the error. Note that these devices are typically created only by machine init() methods with qdev_init_nofail() or similar. If we screwed up and created an ISA bus before that call, we now give up right away. Before, we'd hobble on, and typically die in isa_bus_irqs(). Similar if someone finds a way to hot-plug one of these critters. Cc: Richard Henderson <rth@twiddle.net> Cc: "Michael S. Tsirkin" <mst@redhat.com> Cc: "Hervé Poussineau" <hpoussin@reactos.org> Cc: Aurelien Jarno <aurelien@aurel32.net> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> Signed-off-by: Markus Armbruster <armbru@redhat.com> --- hw/alpha/typhoon.c | 3 ++- hw/i386/pc_piix.c | 3 ++- hw/isa/i82378.c | 5 ++++- hw/isa/isa-bus.c | 4 ++-- hw/isa/lpc_ich9.c | 6 +++++- hw/isa/piix4.c | 6 ++++-- hw/isa/vt82c686.c | 5 ++++- hw/mips/mips_jazz.c | 2 +- hw/mips/mips_r4k.c | 2 +- hw/pci-host/piix.c | 6 ++++-- hw/sparc64/sun4u.c | 6 ++++-- include/hw/isa/isa.h | 2 +- 12 files changed, 34 insertions(+), 16 deletions(-)