| Message ID | 20230126211740.66874-9-shentey@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Resolve isabus global | expand |
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>
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.
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.
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);
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(-)