Re: [PATCH] hw/ide: Fix crash when plugging a piix3-ide device into the x-remote machine

[Thomas Huth <thuth@redhat.com>]

On 16/04/2021 14.52, Thomas Huth wrote:
> QEMU currently crashes when the user tries to do something like:
> 
>   qemu-system-x86_64 -M x-remote -device piix3-ide

It's now several months later already, and this crash has still not been 
fixed yet. The next softfreeze is around the corner and the 
"device-crash-test" still stumbles accross this problem.
If the other solutions are not mergable yet (what's the status here?), could 
we please include my patch for QEMU v6.1 instead, so that we get this 
silenced in the device-crash-test script?

  Thanks,
   Thomas


> This happens because the "isabus" variable is not initialized with
> the x-remote machine yet. Add a proper check for this condition
> and propagate the error to the caller, so we can fail there gracefully.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   hw/ide/ioport.c           | 16 ++++++++++------
>   hw/ide/piix.c             | 22 +++++++++++++++++-----
>   hw/isa/isa-bus.c          | 14 ++++++++++----
>   include/hw/ide/internal.h |  2 +-
>   include/hw/isa/isa.h      | 13 ++++++++-----
>   5 files changed, 46 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/ide/ioport.c b/hw/ide/ioport.c
> index b613ff3bba..e6caa537fa 100644
> --- a/hw/ide/ioport.c
> +++ b/hw/ide/ioport.c
> @@ -50,15 +50,19 @@ static const MemoryRegionPortio ide_portio2_list[] = {
>       PORTIO_END_OF_LIST(),
>   };
>   
> -void ide_init_ioport(IDEBus *bus, ISADevice *dev, int iobase, int iobase2)
> +int ide_init_ioport(IDEBus *bus, ISADevice *dev, int iobase, int iobase2)
>   {
> +    int ret;
> +
>       /* ??? Assume only ISA and PCI configurations, and that the PCI-ISA
>          bridge has been setup properly to always register with ISA.  */
> -    isa_register_portio_list(dev, &bus->portio_list,
> -                             iobase, ide_portio_list, bus, "ide");
> +    ret = isa_register_portio_list(dev, &bus->portio_list,
> +                                   iobase, ide_portio_list, bus, "ide");
>   
> -    if (iobase2) {
> -        isa_register_portio_list(dev, &bus->portio2_list,
> -                                 iobase2, ide_portio2_list, bus, "ide");
> +    if (ret == 0 && iobase2) {
> +        ret = isa_register_portio_list(dev, &bus->portio2_list,
> +                                       iobase2, ide_portio2_list, bus, "ide");
>       }
> +
> +    return ret;
>   }
> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> index b9860e35a5..d3e738320b 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -26,6 +26,7 @@
>   #include "qemu/osdep.h"
>   #include "hw/pci/pci.h"
>   #include "migration/vmstate.h"
> +#include "qapi/error.h"
>   #include "qemu/module.h"
>   #include "sysemu/block-backend.h"
>   #include "sysemu/blockdev.h"
> @@ -123,7 +124,8 @@ static void piix_ide_reset(DeviceState *dev)
>       pci_conf[0x20] = 0x01; /* BMIBA: 20-23h */
>   }
>   
> -static void pci_piix_init_ports(PCIIDEState *d) {
> +static int pci_piix_init_ports(PCIIDEState *d)
> +{
>       static const struct {
>           int iobase;
>           int iobase2;
> @@ -132,24 +134,30 @@ static void pci_piix_init_ports(PCIIDEState *d) {
>           {0x1f0, 0x3f6, 14},
>           {0x170, 0x376, 15},
>       };
> -    int i;
> +    int i, ret;
>   
>       for (i = 0; i < 2; i++) {
>           ide_bus_new(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
> -        ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
> -                        port_info[i].iobase2);
> +        ret = ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
> +                              port_info[i].iobase2);
> +        if (ret) {
> +            return ret;
> +        }
>           ide_init2(&d->bus[i], isa_get_irq(NULL, port_info[i].isairq));
>   
>           bmdma_init(&d->bus[i], &d->bmdma[i], d);
>           d->bmdma[i].bus = &d->bus[i];
>           ide_register_restart_cb(&d->bus[i]);
>       }
> +
> +    return 0;
>   }
>   
>   static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
>   {
>       PCIIDEState *d = PCI_IDE(dev);
>       uint8_t *pci_conf = dev->config;
> +    int rc;
>   
>       pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode
>   
> @@ -158,7 +166,11 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
>   
>       vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_pci, d);
>   
> -    pci_piix_init_ports(d);
> +    rc = pci_piix_init_ports(d);
> +    if (rc) {
> +        error_setg_errno(errp, -rc, "Failed to realize %s",
> +                         object_get_typename(OBJECT(dev)));
> +    }
>   }
>   
>   int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux)
> diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
> index 7820068e6e..cffaa35e9c 100644
> --- a/hw/isa/isa-bus.c
> +++ b/hw/isa/isa-bus.c
> @@ -131,13 +131,17 @@ void isa_register_ioport(ISADevice *dev, MemoryRegion *io, uint16_t start)
>       isa_init_ioport(dev, start);
>   }
>   
> -void isa_register_portio_list(ISADevice *dev,
> -                              PortioList *piolist, uint16_t start,
> -                              const MemoryRegionPortio *pio_start,
> -                              void *opaque, const char *name)
> +int isa_register_portio_list(ISADevice *dev,
> +                             PortioList *piolist, uint16_t start,
> +                             const MemoryRegionPortio *pio_start,
> +                             void *opaque, const char *name)
>   {
>       assert(piolist && !piolist->owner);
>   
> +    if (!isabus) {
> +        return -ENODEV;
> +    }
> +
>       /* START is how we should treat DEV, regardless of the actual
>          contents of the portio array.  This is how the old code
>          actually handled e.g. the FDC device.  */
> @@ -145,6 +149,8 @@ void isa_register_portio_list(ISADevice *dev,
>   
>       portio_list_init(piolist, OBJECT(dev), pio_start, opaque, name);
>       portio_list_add(piolist, isabus->address_space_io, start);
> +
> +    return 0;
>   }
>   
>   static void isa_device_init(Object *obj)
> diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
> index 2d09162eeb..79172217cc 100644
> --- a/include/hw/ide/internal.h
> +++ b/include/hw/ide/internal.h
> @@ -624,7 +624,7 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
>                      int chs_trans, Error **errp);
>   void ide_init2(IDEBus *bus, qemu_irq irq);
>   void ide_exit(IDEState *s);
> -void ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int iobase2);
> +int ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int iobase2);
>   void ide_register_restart_cb(IDEBus *bus);
>   
>   void ide_exec_cmd(IDEBus *bus, uint32_t val);
> diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
> index ddaae89a85..d4417b34b6 100644
> --- a/include/hw/isa/isa.h
> +++ b/include/hw/isa/isa.h
> @@ -132,12 +132,15 @@ void isa_register_ioport(ISADevice *dev, MemoryRegion *io, uint16_t start);
>    * @portio: the ports, sorted by offset.
>    * @opaque: passed into the portio callbacks.
>    * @name: passed into memory_region_init_io.
> + *
> + * Returns: 0 on success, negative error code otherwise (e.g. if the
> + *          ISA bus is not available)
>    */
> -void isa_register_portio_list(ISADevice *dev,
> -                              PortioList *piolist,
> -                              uint16_t start,
> -                              const MemoryRegionPortio *portio,
> -                              void *opaque, const char *name);
> +int isa_register_portio_list(ISADevice *dev,
> +                             PortioList *piolist,
> +                             uint16_t start,
> +                             const MemoryRegionPortio *portio,
> +                             void *opaque, const char *name);
>   
>   static inline ISABus *isa_bus_from_device(ISADevice *d)
>   {
>