Re: [RFC PATCH 02/11] hw/ide: Add PCIIDEState::isa_bus link

[BALATON Zoltan]

On Tue, 18 May 2021, Philippe Mathieu-Daudé wrote:
> IDE bus depends on ISA bus for IRQ/DMA.
>
> Add an ISABus reference in PCIIDEState, and add link properties
> to it in the PIIX and VIA objects (which inherit PCI_IDE).
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> include/hw/ide/pci.h |  1 +
> hw/ide/piix.c        | 11 ++++++++++-
> hw/ide/via.c         | 10 +++++++++-
> 3 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
> index d8384e1c422..e790722ed14 100644
> --- a/include/hw/ide/pci.h
> +++ b/include/hw/ide/pci.h
> @@ -47,6 +47,7 @@ struct PCIIDEState {
>     PCIDevice parent_obj;
>     /*< public >*/
>
> +    ISABus *isa_bus;

I'm not sure that this belongs here. Previously we managed to remove 
device specific fields from this structure so it's now really just holds 
stuff related to PCI IDE (except the remaining "secondary" field specific 
to CMD646). PCI IDE normaly has nothing to do with ISA except for those 
south bridges that have IDE with legacy mode. So this ISABus reference 
should be in those south bridges instead. But that may need a new subclass 
just for this so putting it here is just avoiding boilerplate of declaring 
new subclasses in piix and via-ide. I can sympathise with that but I'd 
still prefer to keep it off here but I wonder if there's a way to do that 
without subclassing and storing an ISABus ref? If I understand correctly 
this ISABus ref is just needed to get appropriate ISA irqs. But could we 
just store a ref to those irqs directly so we don't need to keep the ref 
to the ISA bus? There's already a qemu_irq in BMDMAState but I'm not sure 
how those are set and if you could store an isa irq there to simplify 
this. I don't know the details and could not detangle it by a brief look 
so not sure it can be done but conceptually it feels better to keep PCI 
IDE separate from ISA and let it raise either PCI irq or ISA irq as 
needed. For that a ref to the irq should be enough and that can either 
come from a PCI bus (which is normaly expected for PCI IDE) or an ISA 
bridge for legacy modes. Hope it makes sense and you get what I'm trying 
to say. (Longer term we may want to make it changeable also after the 
device is created to allow switching between legacy and PCI mode but so 
far we could get away without emulating that so it's not a requirement 
just something to consider when you're changing this. The real problem 
that prevents switching modes is not irq I think but ioports and that ISA 
devices are not configurable after creating them but that would need 
QOM'ifying ISA emulation which probably does not worth the effort unless 
we come across some guest that needs this.)

Regards,
BALATON Zoltan

>     IDEBus bus[2];
>     BMDMAState bmdma[2];
>     uint32_t secondary; /* used only for cmd646 */
> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> index b9860e35a5c..48da68da37f 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -30,8 +30,9 @@
> #include "sysemu/block-backend.h"
> #include "sysemu/blockdev.h"
> #include "sysemu/dma.h"
> -
> +#include "qapi/error.h"
> #include "hw/ide/pci.h"
> +#include "hw/isa/isa.h"
> #include "trace.h"
>
> static uint64_t bmdma_read(void *opaque, hwaddr addr, unsigned size)
> @@ -207,6 +208,12 @@ static void pci_piix_ide_exitfn(PCIDevice *dev)
>     }
> }
>
> +static Property piix_ide_properties[] = {
> +    DEFINE_PROP_LINK("isa-bus", PCIIDEState, isa_bus,
> +                     TYPE_ISA_BUS, ISABus *),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> /* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */
> static void piix3_ide_class_init(ObjectClass *klass, void *data)
> {
> @@ -221,6 +228,7 @@ static void piix3_ide_class_init(ObjectClass *klass, void 
> *data)
>     k->class_id = PCI_CLASS_STORAGE_IDE;
>     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>     dc->hotpluggable = false;
> +    device_class_set_props(dc, piix_ide_properties);
> }
>
> static const TypeInfo piix3_ide_info = {
> @@ -249,6 +257,7 @@ static void piix4_ide_class_init(ObjectClass *klass, void 
> *data)
>     k->class_id = PCI_CLASS_STORAGE_IDE;
>     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>     dc->hotpluggable = false;
> +    device_class_set_props(dc, piix_ide_properties);
> }
>
> static const TypeInfo piix4_ide_info = {
> diff --git a/hw/ide/via.c b/hw/ide/via.c
> index be09912b334..65fdca6dcf4 100644
> --- a/hw/ide/via.c
> +++ b/hw/ide/via.c
> @@ -28,8 +28,9 @@
> #include "hw/pci/pci.h"
> #include "migration/vmstate.h"
> #include "qemu/module.h"
> +#include "qapi/error.h"
> #include "sysemu/dma.h"
> -
> +#include "hw/isa/isa.h"
> #include "hw/ide/pci.h"
> #include "trace.h"
>
> @@ -210,6 +211,12 @@ static void via_ide_exitfn(PCIDevice *dev)
>     }
> }
>
> +static Property via_ide_properties[] = {
> +    DEFINE_PROP_LINK("isa-bus", PCIIDEState, isa_bus,
> +                     TYPE_ISA_BUS, ISABus *),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> static void via_ide_class_init(ObjectClass *klass, void *data)
> {
>     DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -224,6 +231,7 @@ static void via_ide_class_init(ObjectClass *klass, void 
> *data)
>     k->revision = 0x06;
>     k->class_id = PCI_CLASS_STORAGE_IDE;
>     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> +    device_class_set_props(dc, via_ide_properties);
> }
>
> static const TypeInfo via_ide_info = {