Am 5. Februar 2023 22:02:02 UTC schrieb Mark Cave-Ayland
<mark.cave-ayl...@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-phi...@redhat.com>
>> 'hw/ide: Let ide_init_ioport() take an ISA bus argument instead of device'
>> Signed-off-by: Bernhard Beschow <shen...@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.
