[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 10/13] isa: Clean up error handling around is
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH v2 10/13] isa: Clean up error handling around isa_bus_new() |
Date: |
Thu, 17 Dec 2015 15:41:32 +0200 |
On Thu, Dec 17, 2015 at 01:19:52PM +0100, 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.
>
> Cc: Richard Henderson <address@hidden>
> Cc: "Michael S. Tsirkin" <address@hidden>
> Cc: "Hervé Poussineau" <address@hidden>
> Cc: Aurelien Jarno <address@hidden>
> Cc: Mark Cave-Ayland <address@hidden>
> Signed-off-by: Markus Armbruster <address@hidden>
> Reviewed-by: Marcel Apfelbaum <address@hidden>
> Reviewed-by: Hervé Poussineau <address@hidden>
Reviewed-by: Michael S. Tsirkin <address@hidden>
> ---
> 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);
> --
> 2.4.3
- Re: [Qemu-devel] [PATCH v2 02/13] omap: Don't use hw_error() in device init() methods, (continued)
- [Qemu-devel] [PATCH v2 10/13] isa: Clean up error handling around isa_bus_new(), Markus Armbruster, 2015/12/17
- Re: [Qemu-devel] [PATCH v2 10/13] isa: Clean up error handling around isa_bus_new(),
Michael S. Tsirkin <=
- [Qemu-devel] [PATCH v2 08/13] sysbus: Don't use hw_error() in machine_init_done_notifiers, Markus Armbruster, 2015/12/17
- [Qemu-devel] [PATCH v2 13/13] xen-hvm: Mark inappropriate error handling FIXME, Markus Armbruster, 2015/12/17
- [Qemu-devel] [PATCH v2 03/13] arm_mptimer: Don't use hw_error() in realize() method, Markus Armbruster, 2015/12/17
- [Qemu-devel] [PATCH v2 01/13] hw: Don't use hw_error() for machine initialization errors, Markus Armbruster, 2015/12/17
- [Qemu-devel] [PATCH v2 07/13] hw/arm/virt: Fix property "gic-version" error handling, Markus Armbruster, 2015/12/17