Re: [PATCH v3 05/18] hw/ide: Rename ISA specific ide_init_ioport -> ide_bus_init_ioport_isa

[Mark Cave-Ayland]

On 02/03/2023 22:40, Philippe Mathieu-Daudé wrote:

> Rename ide_init_ioport() as ide_bus_init_ioport_isa() to make
> explicit it expects an ISA device. Move the declaration to
> "hw/ide/isa.h" where it belongs.
>
> Message-Id: <20230215161641.32663-13-philmd@linaro.org>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/ide/ioport.c           | 4 +++-
>   hw/ide/isa.c              | 2 +-
>   hw/ide/piix.c             | 5 +++--
>   include/hw/ide/internal.h | 1 -
>   include/hw/ide/isa.h      | 3 +++
>   5 files changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/hw/ide/ioport.c b/hw/ide/ioport.c
> index e2ecc6230c..d869f8018a 100644
> --- a/hw/ide/ioport.c
> +++ b/hw/ide/ioport.c
> @@ -25,6 +25,7 @@
>   
>   #include "qemu/osdep.h"
>   #include "hw/isa/isa.h"
> +#include "hw/ide/isa.h"
>   #include "hw/ide/internal.h"
>   #include "trace.h"
>   
> @@ -40,7 +41,8 @@ static const MemoryRegionPortio ide_portio2_list[] = {
>       PORTIO_END_OF_LIST(),
>   };
>   
> -int ide_init_ioport(IDEBus *bus, ISADevice *dev, int iobase, int iobase2)
> +int ide_bus_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 95053e026f..6eed16bf87 100644
> --- a/hw/ide/isa.c
> +++ b/hw/ide/isa.c
> @@ -71,7 +71,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_bus_init_ioport_isa(&s->bus, isadev, s->iobase, s->iobase2);
>       ide_bus_init_output_irq(&s->bus, isa_get_irq(isadev, s->irqnum));
>       vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_isa, s);
>       ide_bus_register_restart_cb(&s->bus);
> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> index 7cb96ef67f..cb527553e2 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -33,6 +33,7 @@
>   #include "hw/pci/pci.h"
>   #include "hw/ide/piix.h"
>   #include "hw/ide/pci.h"
> +#include "hw/ide/isa.h"
>   #include "trace.h"
>   
>   static uint64_t bmdma_read(void *opaque, hwaddr addr, unsigned size)
> @@ -142,8 +143,8 @@ static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, 
> Error **errp)
>   
>       qemu_irq irq_out = d->isa_irq[i] ? : isa_get_irq(NULL, port_info[i].isairq);
>       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_bus_init_ioport_isa(&d->bus[i], NULL, port_info[i].iobase,
> +                                  port_info[i].iobase2);
>       if (ret) {
>           error_setg_errno(errp, -ret, "Failed to realize %s port %u",
>                            object_get_typename(OBJECT(d)), i);
> diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
> index d9f1f77dd5..d3b7fdc504 100644
> --- a/include/hw/ide/internal.h
> +++ b/include/hw/ide/internal.h
> @@ -618,7 +618,6 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, 
> IDEDriveKind kind,
>                      int chs_trans, Error **errp);
>   void ide_exit(IDEState *s);
>   void ide_bus_init_output_irq(IDEBus *bus, qemu_irq irq_out);
> -int ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int iobase2);
>   void ide_bus_set_irq(IDEBus *bus);
>   void ide_bus_register_restart_cb(IDEBus *bus);
>   
> diff --git a/include/hw/ide/isa.h b/include/hw/ide/isa.h
> index 1cd0ff1fa6..7f7a850265 100644
> --- a/include/hw/ide/isa.h
> +++ b/include/hw/ide/isa.h
> @@ -10,11 +10,14 @@
>   #define HW_IDE_ISA_H
>   
>   #include "qom/object.h"
> +#include "hw/ide/internal.h"
>   
>   #define TYPE_ISA_IDE "isa-ide"
>   OBJECT_DECLARE_SIMPLE_TYPE(ISAIDEState, ISA_IDE)
>   
>   ISADevice *isa_ide_init(ISABus *bus, int iobase, int iobase2, int irqnum,
>                           DriveInfo *hd0, DriveInfo *hd1);
> +int ide_bus_init_ioport_isa(IDEBus *bus, ISADevice *isa,
> +                            int iobase, int iobase2);
>   
>   #endif

I have a similar, but opposite patch to this in one of my branches where I have a PCI 
IDE controller that can switch between legacy and native modes :)

From my perspective the use of ide_init_ioport() in hw/ide/isa.c is the outlier 
here, because that is the only instance that works on ISADevice, all the other uses 
are PCIDevices. Hence I've gone the other way which is to inline the ISA ioport 
initialisation into isa_ide_realizefn(): 
https://github.com/mcayland/qemu/commit/e94b004d259e5831beadface100e6bb41beca92c.


ATB,

Mark.