[PATCH 0/7] hw/ide: Untangle ISA/PCI abuses of ide_init_ioport()

[PATCH 0/7] hw/ide: Untangle ISA/PCI abuses of ide_init_ioport()

[Philippe Mathieu-Daudé]

Background thread:
https://lore.kernel.org/qemu-devel/5095dffc-309b-6c72-d255-8cdaa6fd3d52@ilande.co.uk/

The ide_init_ioport() method expect an ISA device, but is
massaged to accept NULL device (IOW, non-ISA devices...).

A plausible explanation is QOM objects can only inherit one
parent, and ide_init_ioport() was first used with ISA children.
Later it was adapted to accept PCI children in a convoluted
way. The PIIX IDE function abuse it.

This series rename the current ide_init_ioport() as
ide_init_ioport_isa(), then add a generic ide_init_ioport()
which works with PCI devices.

This is required to proceed with more PIIX cleanups.

Philippe Mathieu-Daudé (7):
  hw/isa: Un-inline isa_bus_from_device()
  hw/isa: Use isa_address_space_io() to reduce access on global 'isabus'
  hw/ide: Rename ISA specific ide_init_ioport() as ide_init_ioport_isa()
  hw/ide: Introduce generic ide_init_ioport()
  hw/ide/piix: Use generic ide_init_ioport()
  hw/isa: Assert isa_register_portio_list() gets non-NULL ISA device
  hw/ide/piix: Remove dead code in pci_piix_init_ports()

 hw/ide/ioport.c           | 13 ++++++++++---
 hw/ide/isa.c              |  2 +-
 hw/ide/piix.c             | 21 ++++++---------------
 hw/isa/isa-bus.c          | 13 ++++++++++---
 include/hw/ide/internal.h |  4 +++-
 include/hw/isa/isa.h      |  5 +----
 6 files changed, 31 insertions(+), 27 deletions(-)

-- 
2.38.1

[PATCH 1/7] hw/isa: Un-inline isa_bus_from_device()

[Philippe Mathieu-Daudé]

No point in inlining isa_bus_from_device() which is only
used at device realization time.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/isa/isa-bus.c     | 5 +++++
 include/hw/isa/isa.h | 5 +----
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index 4fe61d6dfe..5bd99379e9 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -162,6 +162,11 @@ bool isa_realize_and_unref(ISADevice *dev, ISABus *bus, Error **errp)
     return qdev_realize_and_unref(&dev->parent_obj, &bus->parent_obj, errp);
 }
 
+ISABus *isa_bus_from_device(ISADevice *dev)
+{
+    return ISA_BUS(qdev_get_parent_bus(DEVICE(dev)));
+}
+
 ISADevice *isa_vga_init(ISABus *bus)
 {
     vga_interface_created = true;
diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
index 25acd5c34c..ad8bdd941f 100644
--- a/include/hw/isa/isa.h
+++ b/include/hw/isa/isa.h
@@ -123,9 +123,6 @@ int isa_register_portio_list(ISADevice *dev,
                              const MemoryRegionPortio *portio,
                              void *opaque, const char *name);
 
-static inline ISABus *isa_bus_from_device(ISADevice *d)
-{
-    return ISA_BUS(qdev_get_parent_bus(DEVICE(d)));
-}
+ISABus *isa_bus_from_device(ISADevice *dev);
 
 #endif
-- 
2.38.1

[PATCH 2/7] hw/isa: Use isa_address_space_io() to reduce access on global 'isabus'

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/isa/isa-bus.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index 5bd99379e9..95fc1ba5f7 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -114,7 +114,7 @@ static inline void isa_init_ioport(ISADevice *dev, uint16_t ioport)
 
 void isa_register_ioport(ISADevice *dev, MemoryRegion *io, uint16_t start)
 {
-    memory_region_add_subregion(isabus->address_space_io, start, io);
+    memory_region_add_subregion(isa_address_space_io(dev), start, io);
     isa_init_ioport(dev, start);
 }
 
@@ -133,7 +133,7 @@ int isa_register_portio_list(ISADevice *dev,
     isa_init_ioport(dev, start);
 
     portio_list_register(piolist, OBJECT(dev), pio_start, opaque, name,
-                         isabus->address_space_io, start);
+                         isa_address_space_io(dev), start);
 
     return 0;
 }
-- 
2.38.1

[PATCH 3/7] hw/ide: Rename ISA specific ide_init_ioport() as ide_init_ioport_isa()

[Philippe Mathieu-Daudé]

Rename ide_init_ioport() as ide_init_ioport_isa() to make
explicit it expects an ISA device.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/ide/ioport.c           | 2 +-
 hw/ide/isa.c              | 2 +-
 hw/ide/piix.c             | 4 ++--
 include/hw/ide/internal.h | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/ide/ioport.c b/hw/ide/ioport.c
index e6caa537fa..ac804a89e8 100644
--- a/hw/ide/ioport.c
+++ b/hw/ide/ioport.c
@@ -50,7 +50,7 @@ static const MemoryRegionPortio ide_portio2_list[] = {
     PORTIO_END_OF_LIST(),
 };
 
-int ide_init_ioport(IDEBus *bus, ISADevice *dev, int iobase, int iobase2)
+int ide_init_ioport_isa(IDEBus *bus, ISADevice *dev, int iobase, int iobase2)
 {
     int ret;
 
diff --git a/hw/ide/isa.c b/hw/ide/isa.c
index 8bedbd13f1..4dbd1e48b8 100644
--- a/hw/ide/isa.c
+++ b/hw/ide/isa.c
@@ -74,7 +74,7 @@ static void isa_ide_realizefn(DeviceState *dev, Error **errp)
     ISAIDEState *s = ISA_IDE(dev);
 
     ide_bus_init(&s->bus, sizeof(s->bus), dev, 0, 2);
-    ide_init_ioport(&s->bus, isadev, s->iobase, s->iobase2);
+    ide_init_ioport_isa(&s->bus, isadev, s->iobase, s->iobase2);
     s->irq = isa_get_irq(isadev, s->isairq);
     ide_init2(&s->bus, s->irq);
     vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_isa, s);
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 267dbf37db..a587541bb2 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -140,8 +140,8 @@ static int pci_piix_init_ports(PCIIDEState *d)
 
     for (i = 0; i < 2; i++) {
         ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
-        ret = ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
-                              port_info[i].iobase2);
+        ret = ide_init_ioport_isa(&d->bus[i], NULL,
+                                  port_info[i].iobase, port_info[i].iobase2);
         if (ret) {
             return ret;
         }
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index fc0aa81a88..88a096f9df 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -628,7 +628,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);
-int ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int iobase2);
+int ide_init_ioport_isa(IDEBus *bus, ISADevice *isa, int iobase, int iobase2);
 void ide_register_restart_cb(IDEBus *bus);
 
 void ide_exec_cmd(IDEBus *bus, uint32_t val);
-- 
2.38.1

[PATCH 4/7] hw/ide: Introduce generic ide_init_ioport()

[Philippe Mathieu-Daudé]

Add ide_init_ioport() which is not restricted to the ISA bus.
(Next commit will use it for a PCI device).

Inspired-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/ide/ioport.c           | 11 +++++++++--
 include/hw/ide/internal.h |  2 ++
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/hw/ide/ioport.c b/hw/ide/ioport.c
index ac804a89e8..494fa88368 100644
--- a/hw/ide/ioport.c
+++ b/hw/ide/ioport.c
@@ -54,8 +54,6 @@ int ide_init_ioport_isa(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.  */
     ret = isa_register_portio_list(dev, &bus->portio_list,
                                    iobase, ide_portio_list, bus, "ide");
 
@@ -66,3 +64,12 @@ int ide_init_ioport_isa(IDEBus *bus, ISADevice *dev, int iobase, int iobase2)
 
     return ret;
 }
+
+void ide_init_ioport(IDEBus *bus, Object *owner, MemoryRegion *io,
+                     int iobase, int iobase2)
+{
+    portio_list_register(&bus->portio_list, owner, ide_portio_list,
+                         bus, "ide", io, iobase);
+    portio_list_register(&bus->portio2_list, owner, ide_portio2_list,
+                         bus, "ide", io, iobase2);
+}
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index 88a096f9df..79db902505 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -629,6 +629,8 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
 void ide_init2(IDEBus *bus, qemu_irq irq);
 void ide_exit(IDEState *s);
 int ide_init_ioport_isa(IDEBus *bus, ISADevice *isa, int iobase, int iobase2);
+void ide_init_ioport(IDEBus *bus, Object *owner, MemoryRegion *io,
+                     int iobase, int iobase2);
 void ide_register_restart_cb(IDEBus *bus);
 
 void ide_exec_cmd(IDEBus *bus, uint32_t val);
-- 
2.38.1

[PATCH 5/7] hw/ide/piix: Use generic ide_init_ioport()

[Philippe Mathieu-Daudé]

TYPE_PIIX3_IDE is a PCI function inheriting from QOM
TYPE_PCI_DEVICE. To be able to call the ISA specific
ide_init_ioport_isa(), we call this function passing
a NULL ISADevice argument. Remove this hack by calling
the recently added generic ide_init_ioport(), which
doesn't expect any ISADevice.

Inspired-by: Bernhard Beschow <shentey@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/ide/piix.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index a587541bb2..1cd4389611 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -136,15 +136,13 @@ static int pci_piix_init_ports(PCIIDEState *d)
         {0x1f0, 0x3f6, 14},
         {0x170, 0x376, 15},
     };
-    int i, ret;
+    int i;
 
     for (i = 0; i < 2; i++) {
         ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
-        ret = ide_init_ioport_isa(&d->bus[i], NULL,
-                                  port_info[i].iobase, port_info[i].iobase2);
-        if (ret) {
-            return ret;
-        }
+        ide_init_ioport(&d->bus[i], OBJECT(d),
+                        pci_address_space_io(PCI_DEVICE(d)),
+                        port_info[i].iobase, port_info[i].iobase2);
         ide_init2(&d->bus[i], isa_get_irq(NULL, port_info[i].isairq));
 
         bmdma_init(&d->bus[i], &d->bmdma[i], d);
-- 
2.38.1

[PATCH 6/7] hw/isa: Assert isa_register_portio_list() gets non-NULL ISA device

[Philippe Mathieu-Daudé]

The previous commit removed the single call to
isa_register_portio_list() with dev=NULL. To be
sure we won't reintroduce such weird (ab)use,
add an assertion.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/isa/isa-bus.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index 95fc1ba5f7..3d1996c115 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -107,7 +107,7 @@ IsaDma *isa_get_dma(ISABus *bus, int nchan)
 
 static inline void isa_init_ioport(ISADevice *dev, uint16_t ioport)
 {
-    if (dev && (dev->ioport_id == 0 || ioport < dev->ioport_id)) {
+    if (dev->ioport_id == 0 || ioport < dev->ioport_id) {
         dev->ioport_id = ioport;
     }
 }
@@ -123,6 +123,8 @@ int isa_register_portio_list(ISADevice *dev,
                              const MemoryRegionPortio *pio_start,
                              void *opaque, const char *name)
 {
+    assert(dev);
+
     if (!isabus) {
         return -ENODEV;
     }
-- 
2.38.1

[PATCH 7/7] hw/ide/piix: Remove dead code in pci_piix_init_ports()

[Philippe Mathieu-Daudé]

pci_piix_init_ports() always return '0' so can't fail.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/ide/piix.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 1cd4389611..54d545ce3a 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -126,7 +126,7 @@ static void piix_ide_reset(DeviceState *dev)
     pci_set_byte(pci_conf + 0x20, 0x01);  /* BMIBA: 20-23h */
 }
 
-static int pci_piix_init_ports(PCIIDEState *d)
+static void pci_piix_init_ports(PCIIDEState *d)
 {
     static const struct {
         int iobase;
@@ -149,15 +149,12 @@ static int pci_piix_init_ports(PCIIDEState *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
 
@@ -166,11 +163,7 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
 
     vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_pci, d);
 
-    rc = pci_piix_init_ports(d);
-    if (rc) {
-        error_setg_errno(errp, -rc, "Failed to realize %s",
-                         object_get_typename(OBJECT(dev)));
-    }
+    pci_piix_init_ports(d);
 }
 
 static void pci_piix_ide_exitfn(PCIDevice *dev)
-- 
2.38.1

Re: [PATCH 0/7] hw/ide: Untangle ISA/PCI abuses of ide_init_ioport()

[Philippe Mathieu-Daudé]

On 8/2/23 01:07, Philippe Mathieu-Daudé wrote:
> Background thread:
> https://lore.kernel.org/qemu-devel/5095dffc-309b-6c72-d255-8cdaa6fd3d52@ilande.co.uk/
> 
> The ide_init_ioport() method expect an ISA device, but is
> massaged to accept NULL device (IOW, non-ISA devices...).
> 
> A plausible explanation is QOM objects can only inherit one
> parent, and ide_init_ioport() was first used with ISA children.
> Later it was adapted to accept PCI children in a convoluted
> way. The PIIX IDE function abuse it.
> 
> This series rename the current ide_init_ioport() as
> ide_init_ioport_isa(), then add a generic ide_init_ioport()
> which works with PCI devices.
> 
> This is required to proceed with more PIIX cleanups.
> 
> Philippe Mathieu-Daudé (7):
>    hw/isa: Un-inline isa_bus_from_device()
>    hw/isa: Use isa_address_space_io() to reduce access on global 'isabus'
>    hw/ide: Rename ISA specific ide_init_ioport() as ide_init_ioport_isa()
>    hw/ide: Introduce generic ide_init_ioport()
>    hw/ide/piix: Use generic ide_init_ioport()
>    hw/isa: Assert isa_register_portio_list() gets non-NULL ISA device
>    hw/ide/piix: Remove dead code in pci_piix_init_ports()

Forgot:
Based-on: <20230207234615.77300-1-philmd@linaro.org>
"exec/ioport: Factor portio_list_register[flush_coalesced]() out"
https://lore.kernel.org/qemu-devel/20230207234615.77300-1-philmd@linaro.org/

Re: [PATCH 7/7] hw/ide/piix: Remove dead code in pci_piix_init_ports()

[Richard Henderson]

On 2/7/23 14:07, Philippe Mathieu-Daudé wrote:
> pci_piix_init_ports() always return '0' so can't fail.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/ide/piix.c | 11 ++---------
>   1 file changed, 2 insertions(+), 9 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 5/7] hw/ide/piix: Use generic ide_init_ioport()

[Bernhard Beschow]

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

On Wed, Feb 8, 2023 at 1:08 AM Philippe Mathieu-Daudé <philmd@linaro.org>
wrote:

> TYPE_PIIX3_IDE is a PCI function inheriting from QOM
> TYPE_PCI_DEVICE. To be able to call the ISA specific
> ide_init_ioport_isa(), we call this function passing
> a NULL ISADevice argument. Remove this hack by calling
> the recently added generic ide_init_ioport(), which
> doesn't expect any ISADevice.
>
> Inspired-by: Bernhard Beschow <shentey@gmail.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/ide/piix.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> index a587541bb2..1cd4389611 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -136,15 +136,13 @@ static int pci_piix_init_ports(PCIIDEState *d)
>          {0x1f0, 0x3f6, 14},
>          {0x170, 0x376, 15},
>      };
> -    int i, ret;
> +    int i;
>
>      for (i = 0; i < 2; i++) {
>          ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
> -        ret = ide_init_ioport_isa(&d->bus[i], NULL,
> -                                  port_info[i].iobase,
> port_info[i].iobase2);
> -        if (ret) {
> -            return ret;
> -        }
> +        ide_init_ioport(&d->bus[i], OBJECT(d),
> +                        pci_address_space_io(PCI_DEVICE(d)),
> +                        port_info[i].iobase, port_info[i].iobase2);
>          ide_init2(&d->bus[i], isa_get_irq(NULL, port_info[i].isairq));
>
>          bmdma_init(&d->bus[i], &d->bmdma[i], d);
> --
> 2.38.1
>
> This patch essentially circumvents the mitigations introduced by
https://lore.kernel.org/qemu-devel/20210416125256.2039734-1-thuth@redhat.com/
"hw/ide: Fix crash when plugging a piix3-ide device into the x-remote
machine": `qemu-system-x86_64 -M x-remote -device piix3-ide` now crashes.
This has been considered in
https://lore.kernel.org/qemu-devel/20230126211740.66874-1-shentey@gmail.com/
-- see cover letter there. TBH it's not entirely clear to me why we need
two competing series here at all.

Best regards,
Bernhard

[-- Attachment #2: Type: text/html, Size: 3008 bytes --]

Re: [PATCH 5/7] hw/ide/piix: Use generic ide_init_ioport()

[Bernhard Beschow]

Am 9. Februar 2023 09:04:49 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>On Wed, Feb 8, 2023 at 1:08 AM Philippe Mathieu-Daudé <philmd@linaro.org>
>wrote:
>
>> TYPE_PIIX3_IDE is a PCI function inheriting from QOM
>> TYPE_PCI_DEVICE. To be able to call the ISA specific
>> ide_init_ioport_isa(), we call this function passing
>> a NULL ISADevice argument. Remove this hack by calling
>> the recently added generic ide_init_ioport(), which
>> doesn't expect any ISADevice.
>>
>> Inspired-by: Bernhard Beschow <shentey@gmail.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>  hw/ide/piix.c | 10 ++++------
>>  1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
>> index a587541bb2..1cd4389611 100644
>> --- a/hw/ide/piix.c
>> +++ b/hw/ide/piix.c
>> @@ -136,15 +136,13 @@ static int pci_piix_init_ports(PCIIDEState *d)
>>          {0x1f0, 0x3f6, 14},
>>          {0x170, 0x376, 15},
>>      };
>> -    int i, ret;
>> +    int i;
>>
>>      for (i = 0; i < 2; i++) {
>>          ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
>> -        ret = ide_init_ioport_isa(&d->bus[i], NULL,
>> -                                  port_info[i].iobase,
>> port_info[i].iobase2);
>> -        if (ret) {
>> -            return ret;
>> -        }
>> +        ide_init_ioport(&d->bus[i], OBJECT(d),
>> +                        pci_address_space_io(PCI_DEVICE(d)),
>> +                        port_info[i].iobase, port_info[i].iobase2);
>>          ide_init2(&d->bus[i], isa_get_irq(NULL, port_info[i].isairq));

Let me elaborete a bit on what I mean by the patch essentially circumventing the crash fix:

The reason for the crash with the x-remote machine is now caused by isa_get_irq() which also uses the isabus global behind the scenes. So piix-ide needs to be changed in two places to avoid the global usage and hence the crash.

In his crash fix [1], Thomas was lucky: First, ide_init_ioport() didn't return a value before, so adding one didn't cause changes in other device models. Second, ide_init_ioport() is the first call here to access the global, so it could be used to protect the call to isa_get_irq(). Note that isa_get_irq() couldn't be changed in a similar way without affecting all its call sites.

Fixing ide_init_ioport() to not access the global is certainly a step in the right direction, but this means that ide_init_ioport() is now unable to protect the isa_get_irq() call. Since isa_get_irq() can't conveniently protect itself, we either need to avoid it or need another way to achieve that. That's why in my series GPIOs are used for internal devices and  isa_get_irq() plus fishing out the ISA bus for user-created ones.

Fishing out the ISA bus is still a hack IMO, for two reasons: First, IIUC, QOM'ified devices shall only care about its children while looking up one's parent bus violates this rule. Second, using the global machine pointer to scan for the ISA bus just trades one global for another. That's why I'm only doing this for user-created instances. If we deprecated user-created piix IDE devices we could eventually get rid of this hack.

Best regards,
Bernhard

[1] https://lore.kernel.org/qemu-devel/20210416125256.2039734-1-thuth@redhat.com/ "hw/ide: Fix crash when plugging a piix3-ide device into the x-remote machine"

>>
>>          bmdma_init(&d->bus[i], &d->bmdma[i], d);
>> --
>> 2.38.1
>>
>> This patch essentially circumvents the mitigations introduced by
>https://lore.kernel.org/qemu-devel/20210416125256.2039734-1-thuth@redhat.com/
>"hw/ide: Fix crash when plugging a piix3-ide device into the x-remote
>machine": `qemu-system-x86_64 -M x-remote -device piix3-ide` now crashes.
>This has been considered in
>https://lore.kernel.org/qemu-devel/20230126211740.66874-1-shentey@gmail.com/
>-- see cover letter there. TBH it's not entirely clear to me why we need
>two competing series here at all.
>
>Best regards,
>Bernhard

Re: [PATCH 6/7] hw/isa: Assert isa_register_portio_list() gets non-NULL ISA device

[Philippe Mathieu-Daudé]

On 8/2/23 20:47, Richard Henderson wrote:
> On 2/7/23 14:07, Philippe Mathieu-Daudé wrote:
>> The previous commit removed the single call to
>> isa_register_portio_list() with dev=NULL. To be
>> sure we won't reintroduce such weird (ab)use,
>> add an assertion.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> I wonder how much use of __attribute__((nonnull)) we should be making.

__attribute__((nonnull)) is compile-time, but seems weaker than the
good old runtime assert():

  void a0(void *ptr)
  {
    assert(ptr);
  }

  __attribute__((nonnull)) void a1(void *ptr)
  {
    // can no use assert(ptr) because compiler warning
  }

  void b0(void *x)
  {
    a(NULL); // runtime assertion
  }

  void b(void *x)
  {
    a1(NULL); // compile error
  }

  void c0(void *x)
  {
    a0(x);
  }

  void c1(void *x)
  {
    a1(x);
  }

  void d0(void *x)
  {
    c0(NULL); // runtime assertion
  }

  void d1(void *x)
  {
    c1(NULL); // no compile error, no assertion!
  }

> I realize we'd probably want to add -fno-delete-null-pointer-checks if 
> we make too much use of that.

Re: [PATCH 6/7] hw/isa: Assert isa_register_portio_list() gets non-NULL ISA device

[Richard Henderson]

On 2/14/23 00:18, Philippe Mathieu-Daudé wrote:
>   __attribute__((nonnull)) void a1(void *ptr)
>   {
>     // can no use assert(ptr) because compiler warning
>   }

I briefly glossed over that...

>> I realize we'd probably want to add -fno-delete-null-pointer-checks if we make too much 

... here.  The compiler warning should go away with the right flag.


r~

Re: [PATCH 6/7] hw/isa: Assert isa_register_portio_list() gets non-NULL ISA device

[Philippe Mathieu-Daudé]

On 14/2/23 19:49, Richard Henderson wrote:
> On 2/14/23 00:18, Philippe Mathieu-Daudé wrote:
>>   __attribute__((nonnull)) void a1(void *ptr)
>>   {
>>     // can no use assert(ptr) because compiler warning
>>   }
> 
> I briefly glossed over that...
> 
>>> I realize we'd probably want to add -fno-delete-null-pointer-checks 
>>> if we make too much 
> 
> ... here.  The compiler warning should go away with the right flag.

Doh, got it now!

Re: [PATCH 5/7] hw/ide/piix: Use generic ide_init_ioport()

[Philippe Mathieu-Daudé]

On 10/2/23 17:34, Bernhard Beschow wrote:
> Am 9. Februar 2023 09:04:49 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>> On Wed, Feb 8, 2023 at 1:08 AM Philippe Mathieu-Daudé <philmd@linaro.org>
>> wrote:
>>
>>> TYPE_PIIX3_IDE is a PCI function inheriting from QOM
>>> TYPE_PCI_DEVICE. To be able to call the ISA specific
>>> ide_init_ioport_isa(), we call this function passing
>>> a NULL ISADevice argument. Remove this hack by calling
>>> the recently added generic ide_init_ioport(), which
>>> doesn't expect any ISADevice.
>>>
>>> Inspired-by: Bernhard Beschow <shentey@gmail.com>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>   hw/ide/piix.c | 10 ++++------
>>>   1 file changed, 4 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
>>> index a587541bb2..1cd4389611 100644
>>> --- a/hw/ide/piix.c
>>> +++ b/hw/ide/piix.c
>>> @@ -136,15 +136,13 @@ static int pci_piix_init_ports(PCIIDEState *d)
>>>           {0x1f0, 0x3f6, 14},
>>>           {0x170, 0x376, 15},
>>>       };
>>> -    int i, ret;
>>> +    int i;
>>>
>>>       for (i = 0; i < 2; i++) {
>>>           ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
>>> -        ret = ide_init_ioport_isa(&d->bus[i], NULL,
>>> -                                  port_info[i].iobase,
>>> port_info[i].iobase2);
>>> -        if (ret) {
>>> -            return ret;
>>> -        }
>>> +        ide_init_ioport(&d->bus[i], OBJECT(d),
>>> +                        pci_address_space_io(PCI_DEVICE(d)),
>>> +                        port_info[i].iobase, port_info[i].iobase2);
>>>           ide_init2(&d->bus[i], isa_get_irq(NULL, port_info[i].isairq));
> 
> Let me elaborete a bit on what I mean by the patch essentially circumventing the crash fix:
> 
> The reason for the crash with the x-remote machine is now caused by isa_get_irq() which also uses the isabus global behind the scenes. So piix-ide needs to be changed in two places to avoid the global usage and hence the crash.
> 
> In his crash fix [1], Thomas was lucky: First, ide_init_ioport() didn't return a value before, so adding one didn't cause changes in other device models. Second, ide_init_ioport() is the first call here to access the global, so it could be used to protect the call to isa_get_irq(). Note that isa_get_irq() couldn't be changed in a similar way without affecting all its call sites.
> 
> Fixing ide_init_ioport() to not access the global is certainly a step in the right direction, but this means that ide_init_ioport() is now unable to protect the isa_get_irq() call. Since isa_get_irq() can't conveniently protect itself, we either need to avoid it or need another way to achieve that. That's why in my series GPIOs are used for internal devices and  isa_get_irq() plus fishing out the ISA bus for user-created ones.

The points you raised should be resolved by v2:
https://lore.kernel.org/qemu-devel/20230215161641.32663-1-philmd@linaro.org/
I involved more patches, but hopefully the problem got fixed once for
good without any circumvention.

> Fishing out the ISA bus is still a hack IMO, for two reasons: First, IIUC, QOM'ified devices shall only care about its children while looking up one's parent bus violates this rule. Second, using the global machine pointer to scan for the ISA bus just trades one global for another. That's why I'm only doing this for user-created instances. If we deprecated user-created piix IDE devices we could eventually get rid of this hack.
> 
> Best regards,
> Bernhard
> 
> [1] https://lore.kernel.org/qemu-devel/20210416125256.2039734-1-thuth@redhat.com/ "hw/ide: Fix crash when plugging a piix3-ide device into the x-remote machine"

Re: [PATCH 5/7] hw/ide/piix: Use generic ide_init_ioport()

[Mark Cave-Ayland]

On 10/02/2023 16:34, Bernhard Beschow wrote:

> Fishing out the ISA bus is still a hack IMO, for two reasons: First, IIUC, QOM'ified devices shall only care about its children while looking up one's parent bus violates this rule. Second, using the global machine pointer to scan for the ISA bus just trades one global for another. That's why I'm only doing this for user-created instances. If we deprecated user-created piix IDE devices we could eventually get rid of this hack.

As far as I can tell the solution to QOMified devices finding their parent bus is 
easy: turn DeviceState::parent_bus into a QOM link property called "parent-bus" or 
similar which accepts TYPE_BUS, and then any object of TYPE_DEVICE can locate its 
parent bus using object_property_get_link() with a standardised property name.

I think it may be impossible to completely remove parent bus references, since buses 
like PCI currently make use of upwards hierarchy navigation for things like IRQ 
mapping and PCI-PCI bridge traversal.


ATB,

Mark.