Re: [PATCH v2 04/10] hw/ide/piix: Disuse isa_get_irq()

[Mark Cave-Ayland]

On 26/01/2023 21:17, Bernhard Beschow wrote:

> isa_get_irq() asks for an ISADevice which piix-ide doesn't provide.
> Passing a NULL pointer works but causes the isabus global to be used
> then. By fishing out TYPE_ISA_BUS from the QOM tree it is possible to
> achieve the same as isa_get_irq().
>
> This is an alternative solution to commit 9405d87be25d 'hw/ide: Fix
> crash when plugging a piix3-ide device into the x-remote machine' which
> allows for cleaning up the ISA API while keeping PIIX IDE functions
> user-createable.
>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/ide/piix.c | 21 ++++++++++++++++++---
>   1 file changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> index 267dbf37db..a6646d9657 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -126,7 +126,7 @@ static void piix_ide_reset(DeviceState *dev)
>       pci_set_byte(pci_conf + 0x20, 0x01);  /* BMIBA: 20-23h */
>   }
>   
> -static int pci_piix_init_ports(PCIIDEState *d)
> +static int pci_piix_init_ports(PCIIDEState *d, ISABus *isa_bus)
>   {
>       static const struct {
>           int iobase;
> @@ -145,7 +145,7 @@ static int pci_piix_init_ports(PCIIDEState *d)
>           if (ret) {
>               return ret;
>           }
> -        ide_init2(&d->bus[i], isa_get_irq(NULL, port_info[i].isairq));
> +        ide_init2(&d->bus[i], isa_bus->irqs[port_info[i].isairq]);
>   
>           bmdma_init(&d->bus[i], &d->bmdma[i], d);
>           d->bmdma[i].bus = &d->bus[i];
> @@ -159,6 +159,8 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error 
> **errp)
>   {
>       PCIIDEState *d = PCI_IDE(dev);
>       uint8_t *pci_conf = dev->config;
> +    ISABus *isa_bus;
> +    bool ambiguous;
>       int rc;
>   
>       pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode
> @@ -168,7 +170,20 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error 
> **errp)
>   
>       vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_pci, d);
>   
> -    rc = pci_piix_init_ports(d);
> +    isa_bus = ISA_BUS(object_resolve_path_type("", TYPE_ISA_BUS, &ambiguous));
> +    if (ambiguous) {
> +        error_setg(errp,
> +                   "More than one ISA bus found while %s supports only one",
> +                   object_get_typename(OBJECT(dev)));
> +        return;
> +    }
> +    if (!isa_bus) {
> +        error_setg(errp, "No ISA bus found while %s requires one",
> +                   object_get_typename(OBJECT(dev)));
> +        return;
> +    }
> +
> +    rc = pci_piix_init_ports(d, isa_bus);
>       if (rc) {
>           error_setg_errno(errp, -rc, "Failed to realize %s",
>                            object_get_typename(OBJECT(dev)));

I think the approach here to allow the PCI-ISA bridge to locate the ISABus is a good 
one, but I think it's worth keeping isa_get_irq() to avoid exposing the internals to 
devices.

For me the problem here is that isa_get_irq() accepts a NULL argument for ISADevice. 
I'd expect the function to look something like isa_bus_get_irq(ISABus *isa_bus, 
unsigned isairq) and then it is the responsibility of the caller to locate and 
specify the correct ISABus to avoid falling back to the isabus global.

In particular I can see a PCIDevice should be able to attempt a pci_get_isa_bus() or 
similar which would locate the PCI-ISA bridge and return the child ISA bus, which 
again is related to the cmd646/via compatibility mode feature.

This is along the lines of the similar approach I discussed in 
https://lists.gnu.org/archive/html/qemu-devel/2022-12/msg01443.html which is an 
evolution of the ideas discussed in the original thread a couple of years earlier at 
https://lists.nongnu.org/archive/html/qemu-devel/2020-03/msg01707.html.


ATB,

Mark.