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

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

[Thomas Huth]

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. 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)
 {
-- 
2.27.0

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

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 826 bytes --]

On Tue, Apr 27, 2021 at 03:27:40PM +0200, Philippe Mathieu-Daudé wrote:
> 2/ There is no Kconfig dependency IDE_PIIX -> ISA_BUS, apparently
>    we need it.

Adding an ISA_BUS Kconfig dependency won't solve the problem since the
qemu-system-x86_64 binary is built with many machine types. Most of then
include ISA_BUS so the IDE_PIIX device will be linked in and available
when -M x-remote is used. The crash will still occur.

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

I suggest fixing this at the qdev level. Make piix3-ide have a
sub-device that inherits from ISA_DEVICE so it can only be instantiated
when there's an ISA bus.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[John Snow]

On 4/27/21 9:54 AM, Stefan Hajnoczi wrote:
> I suggest fixing this at the qdev level. Make piix3-ide have a
> sub-device that inherits from ISA_DEVICE so it can only be instantiated
> when there's an ISA bus.
> 
> Stefan

My qdev knowledge is shaky. Does this imply that you agree with the 
direction of Thomas's patch, or do you just mean to disagree with Phil 
on his preferred course of action?

--js

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

[Philippe Mathieu-Daudé]

On 4/27/21 7:16 PM, John Snow wrote:
> On 4/27/21 9:54 AM, Stefan Hajnoczi wrote:
>> I suggest fixing this at the qdev level. Make piix3-ide have a
>> sub-device that inherits from ISA_DEVICE so it can only be instantiated
>> when there's an ISA bus.
>>
>> Stefan
> 
> My qdev knowledge is shaky. Does this imply that you agree with the
> direction of Thomas's patch, or do you just mean to disagree with Phil
> on his preferred course of action?

My understanding is a disagreement to both, with a 3rd direction :)

I agree with Stefan direction but I'm not sure (yet) that a sub-device
is the best (long-term) solution. I guess there is a design issue with
this device, and would like to understanding it first.

IIUC Stefan says the piix3-ide is both a PCI and IDE device, but QOM
only allow a single parent. Multiple QOM inheritance is resolved as
interfaces, but PCI/IDE qdev aren't interfaces, rather abstract objects.
So he suggests to embed an IDE device within the PCI piix3-ide device.

My view is the PIIX is a chipset that share stuffs between components,
and the IDE bus belongs to the chipset PCI root (or eventually the
PCI-ISA bridge, function #0). The IDE function would use the IDE bus
from its root parent as a linked property.
My problem is currently this device is user-creatable as a Frankenstein
single PCI function, out of its chipset. I'm not sure yet this is a
dead end or I could work something out.

Regards,

Phil.

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

[John Snow]

On 4/27/21 1:54 PM, Philippe Mathieu-Daudé wrote:
> On 4/27/21 7:16 PM, John Snow wrote:
>> On 4/27/21 9:54 AM, Stefan Hajnoczi wrote:
>>> I suggest fixing this at the qdev level. Make piix3-ide have a
>>> sub-device that inherits from ISA_DEVICE so it can only be instantiated
>>> when there's an ISA bus.
>>>
>>> Stefan
>>
>> My qdev knowledge is shaky. Does this imply that you agree with the
>> direction of Thomas's patch, or do you just mean to disagree with Phil
>> on his preferred course of action?
> 
> My understanding is a disagreement to both, with a 3rd direction :)
> 
> I agree with Stefan direction but I'm not sure (yet) that a sub-device
> is the best (long-term) solution. I guess there is a design issue with
> this device, and would like to understanding it first.
> 
> IIUC Stefan says the piix3-ide is both a PCI and IDE device, but QOM
> only allow a single parent. Multiple QOM inheritance is resolved as
> interfaces, but PCI/IDE qdev aren't interfaces, rather abstract objects.
> So he suggests to embed an IDE device within the PCI piix3-ide device.
> 
> My view is the PIIX is a chipset that share stuffs between components,
> and the IDE bus belongs to the chipset PCI root (or eventually the
> PCI-ISA bridge, function #0). The IDE function would use the IDE bus
> from its root parent as a linked property.
> My problem is currently this device is user-creatable as a Frankenstein
> single PCI function, out of its chipset. I'm not sure yet this is a
> dead end or I could work something out.
> 
> Regards,
> 
> Phil.
> 

It sounds complicated. In the meantime, I think I am favor of taking 
Thomas's patch because it merely adds some error routing to allow us to 
avoid a crash. The core organizational issues of the IDE device(s) will 
remain and can be solved later as needed.

Do you agree?

--js

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

[John Snow]

On 4/16/21 8:52 AM, 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. 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>

Seems OK to me for now. I will trust that this won't get in the way of 
any deeper refactors Phil has planned, since this just adds error 
pathways to avoid something already broken, and doesn't change anything 
else.

I'm OK with that.

Reviewed-by: John Snow <jsnow@redhat.com>

Tentatively staged, pending further discussion.

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


Little funky instead of just checking and returning after the first 
portio list registration, you could leave a little more git blame intact 
by not doing this, but...

...Then again, I just acked a ton of stuff Phil moved around, so, 
whatever O:-)

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2094 bytes --]

On Tue, Apr 27, 2021 at 07:54:21PM +0200, Philippe Mathieu-Daudé wrote:
> On 4/27/21 7:16 PM, John Snow wrote:
> > On 4/27/21 9:54 AM, Stefan Hajnoczi wrote:
> >> I suggest fixing this at the qdev level. Make piix3-ide have a
> >> sub-device that inherits from ISA_DEVICE so it can only be instantiated
> >> when there's an ISA bus.
> >>
> >> Stefan
> > 
> > My qdev knowledge is shaky. Does this imply that you agree with the
> > direction of Thomas's patch, or do you just mean to disagree with Phil
> > on his preferred course of action?
> 
> My understanding is a disagreement to both, with a 3rd direction :)
> 
> I agree with Stefan direction but I'm not sure (yet) that a sub-device
> is the best (long-term) solution. I guess there is a design issue with
> this device, and would like to understanding it first.
> 
> IIUC Stefan says the piix3-ide is both a PCI and IDE device, but QOM
> only allow a single parent. Multiple QOM inheritance is resolved as
> interfaces, but PCI/IDE qdev aren't interfaces, rather abstract objects.
> So he suggests to embed an IDE device within the PCI piix3-ide device.
> 
> My view is the PIIX is a chipset that share stuffs between components,
> and the IDE bus belongs to the chipset PCI root (or eventually the
> PCI-ISA bridge, function #0). The IDE function would use the IDE bus
> from its root parent as a linked property.
> My problem is currently this device is user-creatable as a Frankenstein
> single PCI function, out of its chipset. I'm not sure yet this is a
> dead end or I could work something out.

Kevin and Paolo previously pointed out that piix3-ide is sometimes used
with the Q35 machine type. The user-creatable piix3-ide device needs to
be deprecated before it can be dropped. That's a long process that
cannot fix the current crash any time soon.

I do support deprecating the user-creatable piix3-ide device in favor of
a proper Q35 Legacy IDE implementation. The main problem is this
involves a bunch of work and I'm not sure who would do it (the payoff is
not very high).

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2348 bytes --]

On Tue, Apr 27, 2021 at 02:02:27PM -0400, John Snow wrote:
> On 4/27/21 1:54 PM, Philippe Mathieu-Daudé wrote:
> > On 4/27/21 7:16 PM, John Snow wrote:
> > > On 4/27/21 9:54 AM, Stefan Hajnoczi wrote:
> > > > I suggest fixing this at the qdev level. Make piix3-ide have a
> > > > sub-device that inherits from ISA_DEVICE so it can only be instantiated
> > > > when there's an ISA bus.
> > > > 
> > > > Stefan
> > > 
> > > My qdev knowledge is shaky. Does this imply that you agree with the
> > > direction of Thomas's patch, or do you just mean to disagree with Phil
> > > on his preferred course of action?
> > 
> > My understanding is a disagreement to both, with a 3rd direction :)
> > 
> > I agree with Stefan direction but I'm not sure (yet) that a sub-device
> > is the best (long-term) solution. I guess there is a design issue with
> > this device, and would like to understanding it first.
> > 
> > IIUC Stefan says the piix3-ide is both a PCI and IDE device, but QOM
> > only allow a single parent. Multiple QOM inheritance is resolved as
> > interfaces, but PCI/IDE qdev aren't interfaces, rather abstract objects.
> > So he suggests to embed an IDE device within the PCI piix3-ide device.
> > 
> > My view is the PIIX is a chipset that share stuffs between components,
> > and the IDE bus belongs to the chipset PCI root (or eventually the
> > PCI-ISA bridge, function #0). The IDE function would use the IDE bus
> > from its root parent as a linked property.
> > My problem is currently this device is user-creatable as a Frankenstein
> > single PCI function, out of its chipset. I'm not sure yet this is a
> > dead end or I could work something out.
> > 
> > Regards,
> > 
> > Phil.
> > 
> 
> It sounds complicated. In the meantime, I think I am favor of taking
> Thomas's patch because it merely adds some error routing to allow us to
> avoid a crash. The core organizational issues of the IDE device(s) will
> remain and can be solved later as needed.

The approach in this patch is okay but we should keep in mind it only
solves piix3-ide. ISA provides a non-qdev backdoor API and there may be
more instances of this type of bug.

A qdev fix would address the root cause and make it possible to drop the
backdoor API, but that's probably too much work for little benefit.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 706 bytes --]

On Fri, Apr 16, 2021 at 02:52:56PM +0200, Thomas Huth wrote:
> @@ -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)));
> +    }

Is there an error message explaining the reason for the failure
somewhere. I can't see one in the patch and imagine users will be
puzzled/frustrated by a generic "Failed to realize" error message. Can
you make it more meaningful?

Thanks,
Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Thomas Huth]

On 28/04/2021 11.24, Stefan Hajnoczi wrote:
> On Fri, Apr 16, 2021 at 02:52:56PM +0200, Thomas Huth wrote:
>> @@ -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)));
>> +    }
> 
> Is there an error message explaining the reason for the failure
> somewhere. I can't see one in the patch and imagine users will be
> puzzled/frustrated by a generic "Failed to realize" error message. Can
> you make it more meaningful?

Do you have a suggestion for a better message?

  Thomas

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

[Philippe Mathieu-Daudé]

On 4/28/21 11:32 AM, Thomas Huth wrote:
> On 28/04/2021 11.24, Stefan Hajnoczi wrote:
>> On Fri, Apr 16, 2021 at 02:52:56PM +0200, Thomas Huth wrote:
>>> @@ -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)));
>>> +    }
>>
>> Is there an error message explaining the reason for the failure
>> somewhere. I can't see one in the patch and imagine users will be
>> puzzled/frustrated by a generic "Failed to realize" error message. Can
>> you make it more meaningful?
> 
> Do you have a suggestion for a better message?

At this point it is hard to do better... Else we'd need to propagate
a meaningful Error* from ide_init_ioport(), and emit something like
"Failed to initialize the ISA bus"?

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

[Markus Armbruster]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> On Tue, Apr 27, 2021 at 02:02:27PM -0400, John Snow wrote:
>> On 4/27/21 1:54 PM, Philippe Mathieu-Daudé wrote:
>> > On 4/27/21 7:16 PM, John Snow wrote:
>> > > On 4/27/21 9:54 AM, Stefan Hajnoczi wrote:
>> > > > I suggest fixing this at the qdev level. Make piix3-ide have a
>> > > > sub-device that inherits from ISA_DEVICE so it can only be instantiated
>> > > > when there's an ISA bus.
>> > > > 
>> > > > Stefan
>> > > 
>> > > My qdev knowledge is shaky. Does this imply that you agree with the
>> > > direction of Thomas's patch, or do you just mean to disagree with Phil
>> > > on his preferred course of action?
>> > 
>> > My understanding is a disagreement to both, with a 3rd direction :)
>> > 
>> > I agree with Stefan direction but I'm not sure (yet) that a sub-device
>> > is the best (long-term) solution. I guess there is a design issue with
>> > this device, and would like to understanding it first.
>> > 
>> > IIUC Stefan says the piix3-ide is both a PCI and IDE device, but QOM
>> > only allow a single parent. Multiple QOM inheritance is resolved as
>> > interfaces, but PCI/IDE qdev aren't interfaces, rather abstract objects.
>> > So he suggests to embed an IDE device within the PCI piix3-ide device.
>> > 
>> > My view is the PIIX is a chipset that share stuffs between components,
>> > and the IDE bus belongs to the chipset PCI root (or eventually the
>> > PCI-ISA bridge, function #0). The IDE function would use the IDE bus
>> > from its root parent as a linked property.
>> > My problem is currently this device is user-creatable as a Frankenstein
>> > single PCI function, out of its chipset. I'm not sure yet this is a
>> > dead end or I could work something out.
>> > 
>> > Regards,
>> > 
>> > Phil.
>> > 
>> 
>> It sounds complicated. In the meantime, I think I am favor of taking
>> Thomas's patch because it merely adds some error routing to allow us to
>> avoid a crash. The core organizational issues of the IDE device(s) will
>> remain and can be solved later as needed.
>
> The approach in this patch is okay but we should keep in mind it only
> solves piix3-ide. ISA provides a non-qdev backdoor API and there may be
> more instances of this type of bug.
>
> A qdev fix would address the root cause and make it possible to drop the
> backdoor API, but that's probably too much work for little benefit.

What do you mean by backdoor API?  Global @isabus?

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

[Markus Armbruster]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> On Tue, Apr 27, 2021 at 07:54:21PM +0200, Philippe Mathieu-Daudé wrote:
>> On 4/27/21 7:16 PM, John Snow wrote:
>> > On 4/27/21 9:54 AM, Stefan Hajnoczi wrote:
>> >> I suggest fixing this at the qdev level. Make piix3-ide have a
>> >> sub-device that inherits from ISA_DEVICE so it can only be instantiated
>> >> when there's an ISA bus.
>> >>
>> >> Stefan
>> > 
>> > My qdev knowledge is shaky. Does this imply that you agree with the
>> > direction of Thomas's patch, or do you just mean to disagree with Phil
>> > on his preferred course of action?
>> 
>> My understanding is a disagreement to both, with a 3rd direction :)
>> 
>> I agree with Stefan direction but I'm not sure (yet) that a sub-device
>> is the best (long-term) solution. I guess there is a design issue with
>> this device, and would like to understanding it first.
>> 
>> IIUC Stefan says the piix3-ide is both a PCI and IDE device, but QOM
>> only allow a single parent. Multiple QOM inheritance is resolved as
>> interfaces, but PCI/IDE qdev aren't interfaces, rather abstract objects.
>> So he suggests to embed an IDE device within the PCI piix3-ide device.
>> 
>> My view is the PIIX is a chipset that share stuffs between components,
>> and the IDE bus belongs to the chipset PCI root (or eventually the
>> PCI-ISA bridge, function #0). The IDE function would use the IDE bus
>> from its root parent as a linked property.
>> My problem is currently this device is user-creatable as a Frankenstein
>> single PCI function, out of its chipset. I'm not sure yet this is a
>> dead end or I could work something out.
>
> Kevin and Paolo previously pointed out that piix3-ide is sometimes used
> with the Q35 machine type. The user-creatable piix3-ide device needs to
> be deprecated before it can be dropped. That's a long process that
> cannot fix the current crash any time soon.
>
> I do support deprecating the user-creatable piix3-ide device in favor of
> a proper Q35 Legacy IDE implementation. The main problem is this
> involves a bunch of work and I'm not sure who would do it (the payoff is
> not very high).

In my opinion, letting users plug device models for PCI *functions* as
if they were *devices* was a mistake.  Compounding the mistake of not
modelling the difference between PCI function and PCI device.

The more of them we can deprecate, the better.

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2797 bytes --]

On Wed, Apr 28, 2021 at 04:18:17PM +0200, Markus Armbruster wrote:
> Stefan Hajnoczi <stefanha@redhat.com> writes:
> 
> > On Tue, Apr 27, 2021 at 02:02:27PM -0400, John Snow wrote:
> >> On 4/27/21 1:54 PM, Philippe Mathieu-Daudé wrote:
> >> > On 4/27/21 7:16 PM, John Snow wrote:
> >> > > On 4/27/21 9:54 AM, Stefan Hajnoczi wrote:
> >> > > > I suggest fixing this at the qdev level. Make piix3-ide have a
> >> > > > sub-device that inherits from ISA_DEVICE so it can only be instantiated
> >> > > > when there's an ISA bus.
> >> > > > 
> >> > > > Stefan
> >> > > 
> >> > > My qdev knowledge is shaky. Does this imply that you agree with the
> >> > > direction of Thomas's patch, or do you just mean to disagree with Phil
> >> > > on his preferred course of action?
> >> > 
> >> > My understanding is a disagreement to both, with a 3rd direction :)
> >> > 
> >> > I agree with Stefan direction but I'm not sure (yet) that a sub-device
> >> > is the best (long-term) solution. I guess there is a design issue with
> >> > this device, and would like to understanding it first.
> >> > 
> >> > IIUC Stefan says the piix3-ide is both a PCI and IDE device, but QOM
> >> > only allow a single parent. Multiple QOM inheritance is resolved as
> >> > interfaces, but PCI/IDE qdev aren't interfaces, rather abstract objects.
> >> > So he suggests to embed an IDE device within the PCI piix3-ide device.
> >> > 
> >> > My view is the PIIX is a chipset that share stuffs between components,
> >> > and the IDE bus belongs to the chipset PCI root (or eventually the
> >> > PCI-ISA bridge, function #0). The IDE function would use the IDE bus
> >> > from its root parent as a linked property.
> >> > My problem is currently this device is user-creatable as a Frankenstein
> >> > single PCI function, out of its chipset. I'm not sure yet this is a
> >> > dead end or I could work something out.
> >> > 
> >> > Regards,
> >> > 
> >> > Phil.
> >> > 
> >> 
> >> It sounds complicated. In the meantime, I think I am favor of taking
> >> Thomas's patch because it merely adds some error routing to allow us to
> >> avoid a crash. The core organizational issues of the IDE device(s) will
> >> remain and can be solved later as needed.
> >
> > The approach in this patch is okay but we should keep in mind it only
> > solves piix3-ide. ISA provides a non-qdev backdoor API and there may be
> > more instances of this type of bug.
> >
> > A qdev fix would address the root cause and make it possible to drop the
> > backdoor API, but that's probably too much work for little benefit.
> 
> What do you mean by backdoor API?  Global @isabus?

Yes. It's also strange that isa_get_irq(ISADevice *dev, unsigned isairq)
accepts dev = NULL as a valid argument.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Markus Armbruster]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> On Wed, Apr 28, 2021 at 04:18:17PM +0200, Markus Armbruster wrote:
>> Stefan Hajnoczi <stefanha@redhat.com> writes:

[...]

>> > The approach in this patch is okay but we should keep in mind it only
>> > solves piix3-ide. ISA provides a non-qdev backdoor API and there may be
>> > more instances of this type of bug.
>> >
>> > A qdev fix would address the root cause and make it possible to drop the
>> > backdoor API, but that's probably too much work for little benefit.
>> 
>> What do you mean by backdoor API?  Global @isabus?
>
> Yes. It's also strange that isa_get_irq(ISADevice *dev, unsigned isairq)
> accepts dev = NULL as a valid argument.

@isabus is static in hw/isa/isa-bus.c.  Uses:

* Limit isa_bus_new() to one ISA bus.  Arbitrary restriction; multiple
  ISA buses could work with suitable memory mapping and IRQ wiring.
  "Single ISA bus" assumptions could of course hide elsewhere in the
  code.

* Implied argument to isa_get_irq(), isa_register_ioport(),
  isa_register_portio_list(), isa_address_space(),
  isa_address_space_io().

  isa_get_irq() asserts that a non-null @dev is a child of @isabus.
  This means we don't actually need @isabus, except when @dev is null.
  I suspect two separate functions would be cleaner: one taking an
  ISABus * argument, and a wrapper taking an ISADevice * argument.

  isa_address_space() and isa_address_space_io() work the same, less the
  assertion.

  isa_register_ioport() and isa_register_portio_list() take a non-null
  @dev argument.  They don't actually need @isabus.

To eliminate global @isabus, we need to fix up the callers passing null
@dev.  Clean solution: plumb the ISABus returned by isa_bus_new() to the
call sites.  Where that's impractical, we can also get it from QOM, like
build_dsdt_microvm() does.

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

[Philippe Mathieu-Daudé]

Hi Elena,

+Mark

You asked to use the next KVM-external call slot to talk about
the ISA bus issues. I haven't scheduled the call because it seems
this thread helped to figure the problems and Markus's analysis
resumed them all. From here it should be clearer to see what has
to be done to go where we want to be from where we are :)

What do you think (in particular, from Markus list)?

On 4/29/21 8:08 AM, Markus Armbruster wrote:
> Stefan Hajnoczi <stefanha@redhat.com> writes:
> 
>> On Wed, Apr 28, 2021 at 04:18:17PM +0200, Markus Armbruster wrote:
>>> Stefan Hajnoczi <stefanha@redhat.com> writes:
> 
> [...]
> 
>>>> The approach in this patch is okay but we should keep in mind it only
>>>> solves piix3-ide. ISA provides a non-qdev backdoor API and there may be
>>>> more instances of this type of bug.
>>>>
>>>> A qdev fix would address the root cause and make it possible to drop the
>>>> backdoor API, but that's probably too much work for little benefit.
>>>
>>> What do you mean by backdoor API?  Global @isabus?
>>
>> Yes. It's also strange that isa_get_irq(ISADevice *dev, unsigned isairq)
>> accepts dev = NULL as a valid argument.
> 
> @isabus is static in hw/isa/isa-bus.c.  Uses:
> 
> * Limit isa_bus_new() to one ISA bus.  Arbitrary restriction; multiple
>   ISA buses could work with suitable memory mapping and IRQ wiring.
>   "Single ISA bus" assumptions could of course hide elsewhere in the
>   code.
> 
> * Implied argument to isa_get_irq(), isa_register_ioport(),
>   isa_register_portio_list(), isa_address_space(),
>   isa_address_space_io().
> 
>   isa_get_irq() asserts that a non-null @dev is a child of @isabus.
>   This means we don't actually need @isabus, except when @dev is null.
>   I suspect two separate functions would be cleaner: one taking an
>   ISABus * argument, and a wrapper taking an ISADevice * argument.
> 
>   isa_address_space() and isa_address_space_io() work the same, less the
>   assertion.
> 
>   isa_register_ioport() and isa_register_portio_list() take a non-null
>   @dev argument.  They don't actually need @isabus.
> 
> To eliminate global @isabus, we need to fix up the callers passing null
> @dev.  Clean solution: plumb the ISABus returned by isa_bus_new() to the
> call sites.  Where that's impractical, we can also get it from QOM, like
> build_dsdt_microvm() does.
> 

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

[John Snow]

On 4/28/21 5:32 AM, Thomas Huth wrote:
> On 28/04/2021 11.24, Stefan Hajnoczi wrote:
>> On Fri, Apr 16, 2021 at 02:52:56PM +0200, Thomas Huth wrote:
>>> @@ -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)));
>>> +    }
>>
>> Is there an error message explaining the reason for the failure
>> somewhere. I can't see one in the patch and imagine users will be
>> puzzled/frustrated by a generic "Failed to realize" error message. Can
>> you make it more meaningful?
> 
> Do you have a suggestion for a better message?
> 
>   Thomas
> 

All of the rest of this thread looks like longer term fixes. I've kept 
this patch staged and will send a PR soonish, after I collate all of the 
other hanging IDE/FDC fixes that are awaiting my attention.

--js

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

[John Snow]

On 4/28/21 5:32 AM, Thomas Huth wrote:
> On 28/04/2021 11.24, Stefan Hajnoczi wrote:
>> On Fri, Apr 16, 2021 at 02:52:56PM +0200, Thomas Huth wrote:
>>> @@ -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)));
>>> +    }
>>
>> Is there an error message explaining the reason for the failure
>> somewhere. I can't see one in the patch and imagine users will be
>> puzzled/frustrated by a generic "Failed to realize" error message. Can
>> you make it more meaningful?
> 
> Do you have a suggestion for a better message?
> 
>   Thomas
> 

Hi Thomas. Looks like there's some more discussion, and Philippe has a 
counter-proposal he wants to send.

I guess I'll de-queue this for now, and I'll see you on the review for 
Philippe's series?

--js

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

[Thomas Huth]

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

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

[Philippe Mathieu-Daudé]

On 7/6/21 10:24 AM, Thomas Huth wrote:
> 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?),

See this big thread about ISA vs PCI IDE modelling / design:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg809678.html

TL;DR: short term we are screwed. long term, not worth it.

Stefan, IIRC the multi-process conclusion was we have to reject
PCI devices briding another (non-PCI) bus, such ISA / I2C / USB
/ SD / ... because QEMU register the bus type globally and the
command line machinery resolves it to plug user-creatable devices,
so we can not share such buses. Is that correct?

> could we please include my patch for QEMU v6.1 instead, so that we get
> this silenced in the device-crash-test script?

Yes please.

Regards,

Phil.

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

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 966 bytes --]

On Tue, Jul 06, 2021 at 10:37:03AM +0200, Philippe Mathieu-Daudé wrote:
> Stefan, IIRC the multi-process conclusion was we have to reject
> PCI devices briding another (non-PCI) bus, such ISA / I2C / USB
> / SD / ... because QEMU register the bus type globally and the
> command line machinery resolves it to plug user-creatable devices,
> so we can not share such buses. Is that correct?

I'm not sure I understand, but I'll try:

You can implement an out-of-process USB host controller (a PCI device),
but QEMU will not be aware of devices on this out-of-process USB bus.

If you're referring to a PCI IDE controller that is also exposed on the
ISA bus, then that's hard to do. Maybe there would need to be a separate
ISA-to-PCI bridge device so there's a clean separation between the PCI
device and the ISA portion. The current multi-process QEMU protocol (and
the upcoming vfio-user protocol) support PCI devices but not ISA
devices.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]