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

[Philippe Mathieu-Daudé]

Hi Thomas,

On 4/16/21 2:52 PM, Thomas Huth wrote:
> QEMU currently crashes when the user tries to do something like:
> 
>  qemu-system-x86_64 -M x-remote -device piix3-ide
> 
> This happens because the "isabus" variable is not initialized with
> the x-remote machine yet.

The qdev contract is everything must be ready at realize() time,
so your safe check in pci_piix_ide_realize() seems alright.

But something doesn't sound right... If no ISA bus is present,
we shouldn't have executed so many code, rather have bailed out
earlier.

How does it work with the x-remote machine? The bus will become
available later upon remote connection?

piix3-ide is a PCI function device. Personally I consider it part
of the PIIX3 chipset, but we prefer to instantiate it separately.
So per Kconfig, piix3-ide is user-creatable by any machine providing
a PCI bus.

See hw/ide/Kconfig:

config IDE_CORE
    bool

config IDE_PCI
    bool
    depends on PCI
    select IDE_CORE

config IDE_ISA
    bool
    depends on ISA_BUS
    select IDE_QDEV

config IDE_PIIX
    bool
    select IDE_PCI
    select IDE_QDEV

and x-remote machine:

config MULTIPROCESS
    bool
    depends on PCI && PCI_EXPRESS && KVM
    select REMOTE_PCIHOST

1/ This KVM check is dubious, and isn't enforced at runtime
   in the machine.

2/ There is no Kconfig dependency IDE_PIIX -> ISA_BUS, apparently
   we need it.


Anyhow I think it would be easier to manage the ISA code if the bus
were created explicitly, not under our feet. I have a WIP series
doing that, but it isn't easy (as with all very old QEMU code/devices).

> 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(-)

>  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)));
> +    }
>  }