1. Patchew
  2. QEMU
  3. [PATCH v2 00/10] Resolve isabus global
  4. View patch

[PATCH v2 08/10] hw/ide: Let ide_init_ioport() take a MemoryRegion argument instead of ISADevice

Bernhard Beschow posted 10 patches 3 years ago
Maintainers: Gerd Hoffmann <kraxel@redhat.com>, "Hervé Poussineau" <hpoussin@reactos.org>, John Snow <jsnow@redhat.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, "Michael S. Tsirkin" <mst@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Aurelien Jarno <aurelien@aurel32.net>, Paolo Bonzini <pbonzini@redhat.com>, Peter Xu <peterx@redhat.com>, David Hildenbrand <david@redhat.com>
[PATCH v2 08/10] hw/ide: Let ide_init_ioport() take a MemoryRegion argument instead of ISADevice
Posted by Bernhard Beschow 3 years ago 
Both callers to ide_init_ioport() have access to the I/O memory region
of the ISA bus, so can pass it directly. This allows ide_init_ioport()
to directly call portio_list_init().

Note, now the callers become the owner of the PortioList.

Inspired-by: <20210518215545.1793947-10-philmd@redhat.com>
  'hw/ide: Let ide_init_ioport() take an ISA bus argument instead of device'
Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/ide/internal.h |  3 ++-
 hw/ide/ioport.c           | 15 ++++++++-------
 hw/ide/isa.c              |  4 +++-
 hw/ide/piix.c             |  8 ++++++--
 4 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index 42c49414f4..c3e4d192fa 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -628,7 +628,8 @@ 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);
+void ide_init_ioport(IDEBus *bus, MemoryRegion *address_space_io, Object *owner,
+                     int iobase, int iobase2);
 void ide_register_restart_cb(IDEBus *bus);
 
 void ide_exec_cmd(IDEBus *bus, uint32_t val);
diff --git a/hw/ide/ioport.c b/hw/ide/ioport.c
index b613ff3bba..00e9baf0d1 100644
--- a/hw/ide/ioport.c
+++ b/hw/ide/ioport.c
@@ -50,15 +50,16 @@ static const MemoryRegionPortio ide_portio2_list[] = {
     PORTIO_END_OF_LIST(),
 };
 
-void ide_init_ioport(IDEBus *bus, ISADevice *dev, int iobase, int iobase2)
+void ide_init_ioport(IDEBus *bus, MemoryRegion *address_space_io, Object *owner,
+                     int iobase, int iobase2)
 {
-    /* ??? 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");
+    assert(address_space_io);
+
+    portio_list_init(&bus->portio_list, owner, ide_portio_list, bus, "ide",
+                     address_space_io, iobase);
 
     if (iobase2) {
-        isa_register_portio_list(dev, &bus->portio2_list,
-                                 iobase2, ide_portio2_list, bus, "ide");
+        portio_list_init(&bus->portio2_list, owner, ide_portio2_list, bus,
+                         "ide", address_space_io, iobase2);
     }
 }
diff --git a/hw/ide/isa.c b/hw/ide/isa.c
index 8bedbd13f1..cab5d0a07a 100644
--- a/hw/ide/isa.c
+++ b/hw/ide/isa.c
@@ -72,9 +72,11 @@ static void isa_ide_realizefn(DeviceState *dev, Error **errp)
 {
     ISADevice *isadev = ISA_DEVICE(dev);
     ISAIDEState *s = ISA_IDE(dev);
+    ISABus *isabus = isa_bus_from_device(isadev);
 
     ide_bus_init(&s->bus, sizeof(s->bus), dev, 0, 2);
-    ide_init_ioport(&s->bus, isadev, s->iobase, s->iobase2);
+    ide_init_ioport(&s->bus, isabus->address_space_io, OBJECT(dev),
+                    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 f0d95761ac..236b5b7416 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -29,6 +29,7 @@
 
 #include "qemu/osdep.h"
 #include "hw/pci/pci.h"
+#include "hw/pci/pci_bus.h"
 #include "migration/vmstate.h"
 #include "qapi/error.h"
 #include "qemu/module.h"
@@ -143,8 +144,11 @@ static void pci_piix_init_ports(PCIIDEState *d, ISABus *isa_bus)
         {0x1f0, 0x3f6, 14},
         {0x170, 0x376, 15},
     };
+    PCIBus *pci_bus = pci_get_bus(&d->parent_obj);
     int i;
 
+    assert(pci_bus);
+
     if (isa_bus) {
         d->isa_irqs[0] = isa_bus->irqs[port_info[0].isairq];
         d->isa_irqs[1] = isa_bus->irqs[port_info[1].isairq];
@@ -154,8 +158,8 @@ static void pci_piix_init_ports(PCIIDEState *d, ISABus *isa_bus)
 
     for (i = 0; i < 2; i++) {
         ide_bus_init(&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);
+        ide_init_ioport(&d->bus[i], pci_bus->address_space_io, OBJECT(d),
+                        port_info[i].iobase, port_info[i].iobase2);
         ide_init2(&d->bus[i], qdev_get_gpio_in(DEVICE(d), i));
 
         bmdma_init(&d->bus[i], &d->bmdma[i], d);
-- 
2.39.1
Re: [PATCH v2 08/10] hw/ide: Let ide_init_ioport() take a MemoryRegion argument instead of ISADevice
Posted by Mark Cave-Ayland 3 years ago 
On 26/01/2023 21:17, Bernhard Beschow wrote:

> Both callers to ide_init_ioport() have access to the I/O memory region
> of the ISA bus, so can pass it directly. This allows ide_init_ioport()
> to directly call portio_list_init().
> 
> Note, now the callers become the owner of the PortioList.
> 
> Inspired-by: <20210518215545.1793947-10-philmd@redhat.com>
>    'hw/ide: Let ide_init_ioport() take an ISA bus argument instead of device'
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   include/hw/ide/internal.h |  3 ++-
>   hw/ide/ioport.c           | 15 ++++++++-------
>   hw/ide/isa.c              |  4 +++-
>   hw/ide/piix.c             |  8 ++++++--
>   4 files changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
> index 42c49414f4..c3e4d192fa 100644
> --- a/include/hw/ide/internal.h
> +++ b/include/hw/ide/internal.h
> @@ -628,7 +628,8 @@ 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);
> +void ide_init_ioport(IDEBus *bus, MemoryRegion *address_space_io, Object *owner,
> +                     int iobase, int iobase2);
>   void ide_register_restart_cb(IDEBus *bus);
>   
>   void ide_exec_cmd(IDEBus *bus, uint32_t val);
> diff --git a/hw/ide/ioport.c b/hw/ide/ioport.c
> index b613ff3bba..00e9baf0d1 100644
> --- a/hw/ide/ioport.c
> +++ b/hw/ide/ioport.c
> @@ -50,15 +50,16 @@ static const MemoryRegionPortio ide_portio2_list[] = {
>       PORTIO_END_OF_LIST(),
>   };
>   
> -void ide_init_ioport(IDEBus *bus, ISADevice *dev, int iobase, int iobase2)
> +void ide_init_ioport(IDEBus *bus, MemoryRegion *address_space_io, Object *owner,
> +                     int iobase, int iobase2)
>   {
> -    /* ??? 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");
> +    assert(address_space_io);
> +
> +    portio_list_init(&bus->portio_list, owner, ide_portio_list, bus, "ide",
> +                     address_space_io, iobase);
>   
>       if (iobase2) {
> -        isa_register_portio_list(dev, &bus->portio2_list,
> -                                 iobase2, ide_portio2_list, bus, "ide");
> +        portio_list_init(&bus->portio2_list, owner, ide_portio2_list, bus,
> +                         "ide", address_space_io, iobase2);
>       }
>   }
> diff --git a/hw/ide/isa.c b/hw/ide/isa.c
> index 8bedbd13f1..cab5d0a07a 100644
> --- a/hw/ide/isa.c
> +++ b/hw/ide/isa.c
> @@ -72,9 +72,11 @@ static void isa_ide_realizefn(DeviceState *dev, Error **errp)
>   {
>       ISADevice *isadev = ISA_DEVICE(dev);
>       ISAIDEState *s = ISA_IDE(dev);
> +    ISABus *isabus = isa_bus_from_device(isadev);
>   
>       ide_bus_init(&s->bus, sizeof(s->bus), dev, 0, 2);
> -    ide_init_ioport(&s->bus, isadev, s->iobase, s->iobase2);
> +    ide_init_ioport(&s->bus, isabus->address_space_io, OBJECT(dev),
> +                    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 f0d95761ac..236b5b7416 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -29,6 +29,7 @@
>   
>   #include "qemu/osdep.h"
>   #include "hw/pci/pci.h"
> +#include "hw/pci/pci_bus.h"
>   #include "migration/vmstate.h"
>   #include "qapi/error.h"
>   #include "qemu/module.h"
> @@ -143,8 +144,11 @@ static void pci_piix_init_ports(PCIIDEState *d, ISABus *isa_bus)
>           {0x1f0, 0x3f6, 14},
>           {0x170, 0x376, 15},
>       };
> +    PCIBus *pci_bus = pci_get_bus(&d->parent_obj);
>       int i;
>   
> +    assert(pci_bus);
> +
>       if (isa_bus) {
>           d->isa_irqs[0] = isa_bus->irqs[port_info[0].isairq];
>           d->isa_irqs[1] = isa_bus->irqs[port_info[1].isairq];
> @@ -154,8 +158,8 @@ static void pci_piix_init_ports(PCIIDEState *d, ISABus *isa_bus)
>   
>       for (i = 0; i < 2; i++) {
>           ide_bus_init(&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);
> +        ide_init_ioport(&d->bus[i], pci_bus->address_space_io, OBJECT(d),
> +                        port_info[i].iobase, port_info[i].iobase2);
>           ide_init2(&d->bus[i], qdev_get_gpio_in(DEVICE(d), i));
>   
>           bmdma_init(&d->bus[i], &d->bmdma[i], d);

Again, given that I suspect ioports are specific to x86 I'd be inclined to leave this 
as a reference to ISA. I could see there being a function that exists such as 
isa_get_address_space_io(ISADevice *isa) in the same way as pci_address_space_io(), 
for example.


ATB,

Mark.
Re: [PATCH v2 08/10] hw/ide: Let ide_init_ioport() take a MemoryRegion argument instead of ISADevice
Posted by Bernhard Beschow 2 years, 9 months ago 

Am 5. Februar 2023 22:02:02 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>On 26/01/2023 21:17, Bernhard Beschow wrote:
>
>> Both callers to ide_init_ioport() have access to the I/O memory region
>> of the ISA bus, so can pass it directly. This allows ide_init_ioport()
>> to directly call portio_list_init().
>> 
>> Note, now the callers become the owner of the PortioList.
>> 
>> Inspired-by: <20210518215545.1793947-10-philmd@redhat.com>
>>    'hw/ide: Let ide_init_ioport() take an ISA bus argument instead of device'
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   include/hw/ide/internal.h |  3 ++-
>>   hw/ide/ioport.c           | 15 ++++++++-------
>>   hw/ide/isa.c              |  4 +++-
>>   hw/ide/piix.c             |  8 ++++++--
>>   4 files changed, 19 insertions(+), 11 deletions(-)
>> 
>> diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
>> index 42c49414f4..c3e4d192fa 100644
>> --- a/include/hw/ide/internal.h
>> +++ b/include/hw/ide/internal.h
>> @@ -628,7 +628,8 @@ 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);
>> +void ide_init_ioport(IDEBus *bus, MemoryRegion *address_space_io, Object *owner,
>> +                     int iobase, int iobase2);
>>   void ide_register_restart_cb(IDEBus *bus);
>>     void ide_exec_cmd(IDEBus *bus, uint32_t val);
>> diff --git a/hw/ide/ioport.c b/hw/ide/ioport.c
>> index b613ff3bba..00e9baf0d1 100644
>> --- a/hw/ide/ioport.c
>> +++ b/hw/ide/ioport.c
>> @@ -50,15 +50,16 @@ static const MemoryRegionPortio ide_portio2_list[] = {
>>       PORTIO_END_OF_LIST(),
>>   };
>>   -void ide_init_ioport(IDEBus *bus, ISADevice *dev, int iobase, int iobase2)
>> +void ide_init_ioport(IDEBus *bus, MemoryRegion *address_space_io, Object *owner,
>> +                     int iobase, int iobase2)
>>   {
>> -    /* ??? 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");
>> +    assert(address_space_io);
>> +
>> +    portio_list_init(&bus->portio_list, owner, ide_portio_list, bus, "ide",
>> +                     address_space_io, iobase);
>>         if (iobase2) {
>> -        isa_register_portio_list(dev, &bus->portio2_list,
>> -                                 iobase2, ide_portio2_list, bus, "ide");
>> +        portio_list_init(&bus->portio2_list, owner, ide_portio2_list, bus,
>> +                         "ide", address_space_io, iobase2);
>>       }
>>   }
>> diff --git a/hw/ide/isa.c b/hw/ide/isa.c
>> index 8bedbd13f1..cab5d0a07a 100644
>> --- a/hw/ide/isa.c
>> +++ b/hw/ide/isa.c
>> @@ -72,9 +72,11 @@ static void isa_ide_realizefn(DeviceState *dev, Error **errp)
>>   {
>>       ISADevice *isadev = ISA_DEVICE(dev);
>>       ISAIDEState *s = ISA_IDE(dev);
>> +    ISABus *isabus = isa_bus_from_device(isadev);
>>         ide_bus_init(&s->bus, sizeof(s->bus), dev, 0, 2);
>> -    ide_init_ioport(&s->bus, isadev, s->iobase, s->iobase2);
>> +    ide_init_ioport(&s->bus, isabus->address_space_io, OBJECT(dev),
>> +                    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 f0d95761ac..236b5b7416 100644
>> --- a/hw/ide/piix.c
>> +++ b/hw/ide/piix.c
>> @@ -29,6 +29,7 @@
>>     #include "qemu/osdep.h"
>>   #include "hw/pci/pci.h"
>> +#include "hw/pci/pci_bus.h"
>>   #include "migration/vmstate.h"
>>   #include "qapi/error.h"
>>   #include "qemu/module.h"
>> @@ -143,8 +144,11 @@ static void pci_piix_init_ports(PCIIDEState *d, ISABus *isa_bus)
>>           {0x1f0, 0x3f6, 14},
>>           {0x170, 0x376, 15},
>>       };
>> +    PCIBus *pci_bus = pci_get_bus(&d->parent_obj);
>>       int i;
>>   +    assert(pci_bus);
>> +
>>       if (isa_bus) {
>>           d->isa_irqs[0] = isa_bus->irqs[port_info[0].isairq];
>>           d->isa_irqs[1] = isa_bus->irqs[port_info[1].isairq];
>> @@ -154,8 +158,8 @@ static void pci_piix_init_ports(PCIIDEState *d, ISABus *isa_bus)
>>         for (i = 0; i < 2; i++) {
>>           ide_bus_init(&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);
>> +        ide_init_ioport(&d->bus[i], pci_bus->address_space_io, OBJECT(d),
>> +                        port_info[i].iobase, port_info[i].iobase2);
>>           ide_init2(&d->bus[i], qdev_get_gpio_in(DEVICE(d), i));
>>             bmdma_init(&d->bus[i], &d->bmdma[i], d);
>
>Again, given that I suspect ioports are specific to x86 I'd be inclined to leave this as a reference to ISA. I could see there being a function that exists such as isa_get_address_space_io(ISADevice *isa) in the same way as pci_address_space_io(), for example.

Hi Mark,

I've just posted a series resolving the same problem in a different way.

For some reason I thought you were the maintainer of cmd646. So I was surprised not to see you in the recipients list there...

Best regards,
Bernhard

>
>
>ATB,
>
>Mark.
Re: [PATCH v2 08/10] hw/ide: Let ide_init_ioport() take a MemoryRegion argument instead of ISADevice
Posted by Philippe Mathieu-Daudé 3 years ago 
On 26/1/23 22:17, Bernhard Beschow wrote:
> Both callers to ide_init_ioport() have access to the I/O memory region
> of the ISA bus, so can pass it directly. This allows ide_init_ioport()
> to directly call portio_list_init().
> 
> Note, now the callers become the owner of the PortioList.
> 
> Inspired-by: <20210518215545.1793947-10-philmd@redhat.com>
>    'hw/ide: Let ide_init_ioport() take an ISA bus argument instead of device'
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   include/hw/ide/internal.h |  3 ++-
>   hw/ide/ioport.c           | 15 ++++++++-------
>   hw/ide/isa.c              |  4 +++-
>   hw/ide/piix.c             |  8 ++++++--
>   4 files changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
> index 42c49414f4..c3e4d192fa 100644
> --- a/include/hw/ide/internal.h
> +++ b/include/hw/ide/internal.h
> @@ -628,7 +628,8 @@ 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);
> +void ide_init_ioport(IDEBus *bus, MemoryRegion *address_space_io, Object *owner,
> +                     int iobase, int iobase2);
>   void ide_register_restart_cb(IDEBus *bus);
>   
>   void ide_exec_cmd(IDEBus *bus, uint32_t val);
> diff --git a/hw/ide/ioport.c b/hw/ide/ioport.c
> index b613ff3bba..00e9baf0d1 100644
> --- a/hw/ide/ioport.c
> +++ b/hw/ide/ioport.c
> @@ -50,15 +50,16 @@ static const MemoryRegionPortio ide_portio2_list[] = {
>       PORTIO_END_OF_LIST(),
>   };
>   
> -void ide_init_ioport(IDEBus *bus, ISADevice *dev, int iobase, int iobase2)
> +void ide_init_ioport(IDEBus *bus, MemoryRegion *address_space_io, Object *owner,
> +                     int iobase, int iobase2)
>   {

Eh I have almost the same change locally :)

$ git diff d2968dc5d0940799~..629b479f0b8f082688
diff --git a/hw/ide/ioport.c b/hw/ide/ioport.c
index fcfaa00cb9..f82e00076a 100644
--- a/hw/ide/ioport.c
+++ b/hw/ide/ioport.c
@@ -50,20 +50,21 @@ 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(IDEBus *bus, Object *owner, ISADevice *dev,
+                    MemoryRegion *isa_io, int iobase, int iobase2)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.