Re: [PATCH v2 2/5] pci: Always pass own DeviceState to pci_map_irq_fn's

[BALATON Zoltan]

On Sat, 12 Feb 2022, Bernhard Beschow wrote:
> Passing own DeviceState rather than just the IRQs allows for resolving
> global variables.

Do you mean pci_set_irq_fn instead of pci_map_irq_fn in the patch title?

> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> hw/isa/piix4.c          | 6 +++---
> hw/pci-host/sh_pci.c    | 6 +++---
> hw/pci-host/versatile.c | 6 +++---
> hw/ppc/ppc440_pcix.c    | 6 +++---
> hw/ppc/ppc4xx_pci.c     | 6 +++---
> 5 files changed, 15 insertions(+), 15 deletions(-)

You don't seem to change any global function definition that reqires this 
change in all these devices so why can't these decide on their own what 
their preferred opaque data is for their set irq function and only change 
piix4? Am I missing something? I'm not opposed to this change but it seems 
to be unnecessary to touch other device implementations for this and may 
just make them more complex for no good reason.

Regards,
BALATON Zoltan

> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
> index 5a86308689..a31e9714cf 100644
> --- a/hw/isa/piix4.c
> +++ b/hw/isa/piix4.c
> @@ -60,7 +60,7 @@ static int pci_irq_levels[4];
> static void piix4_set_irq(void *opaque, int irq_num, int level)
> {
>     int i, pic_irq, pic_level;
> -    qemu_irq *pic = opaque;
> +    PIIX4State *s = opaque;
>
>     pci_irq_levels[irq_num] = level;
>
> @@ -75,7 +75,7 @@ static void piix4_set_irq(void *opaque, int irq_num, int 
> level)
>                 pic_level |= pci_irq_levels[i];
>             }
>         }
> -        qemu_set_irq(pic[pic_irq], pic_level);
> +        qemu_set_irq(s->i8259[pic_irq], pic_level);
>     }
> }
>
> @@ -323,7 +323,7 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus 
> **isa_bus, I2CBus **smbus)
>                                NULL, 0, NULL);
>     }
>
> -    pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s->i8259, 4);
> +    pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s, 4);
>
>     for (int i = 0; i < ISA_NUM_IRQS; i++) {
>         s->i8259[i] = qdev_get_gpio_in_named(dev, "isa", i);
> diff --git a/hw/pci-host/sh_pci.c b/hw/pci-host/sh_pci.c
> index 719d6ca2a6..ae0aa462b3 100644
> --- a/hw/pci-host/sh_pci.c
> +++ b/hw/pci-host/sh_pci.c
> @@ -111,9 +111,9 @@ static int sh_pci_map_irq(PCIDevice *d, int irq_num)
>
> static void sh_pci_set_irq(void *opaque, int irq_num, int level)
> {
> -    qemu_irq *pic = opaque;
> +    SHPCIState *s = opaque;
>
> -    qemu_set_irq(pic[irq_num], level);
> +    qemu_set_irq(s->irq[irq_num], level);
> }
>
> static void sh_pci_device_realize(DeviceState *dev, Error **errp)
> @@ -128,7 +128,7 @@ static void sh_pci_device_realize(DeviceState *dev, Error 
> **errp)
>     }
>     phb->bus = pci_register_root_bus(dev, "pci",
>                                      sh_pci_set_irq, sh_pci_map_irq,
> -                                     s->irq,
> +                                     s,
>                                      get_system_memory(),
>                                      get_system_io(),
>                                      PCI_DEVFN(0, 0), 4, TYPE_PCI_BUS);
> diff --git a/hw/pci-host/versatile.c b/hw/pci-host/versatile.c
> index f66384fa02..5fbcb72d7d 100644
> --- a/hw/pci-host/versatile.c
> +++ b/hw/pci-host/versatile.c
> @@ -362,9 +362,9 @@ static int pci_vpb_rv_map_irq(PCIDevice *d, int irq_num)
>
> static void pci_vpb_set_irq(void *opaque, int irq_num, int level)
> {
> -    qemu_irq *pic = opaque;
> +    PCIVPBState *s = opaque;
>
> -    qemu_set_irq(pic[irq_num], level);
> +    qemu_set_irq(s->irq[irq_num], level);
> }
>
> static void pci_vpb_reset(DeviceState *d)
> @@ -422,7 +422,7 @@ static void pci_vpb_realize(DeviceState *dev, Error **errp)
>         mapfn = pci_vpb_map_irq;
>     }
>
> -    pci_bus_irqs(&s->pci_bus, pci_vpb_set_irq, mapfn, s->irq, 4);
> +    pci_bus_irqs(&s->pci_bus, pci_vpb_set_irq, mapfn, s, 4);
>
>     /* Our memory regions are:
>      * 0 : our control registers
> diff --git a/hw/ppc/ppc440_pcix.c b/hw/ppc/ppc440_pcix.c
> index 788d25514a..291c1bfbe7 100644
> --- a/hw/ppc/ppc440_pcix.c
> +++ b/hw/ppc/ppc440_pcix.c
> @@ -431,14 +431,14 @@ static int ppc440_pcix_map_irq(PCIDevice *pci_dev, int 
> irq_num)
>
> static void ppc440_pcix_set_irq(void *opaque, int irq_num, int level)
> {
> -    qemu_irq *pci_irq = opaque;
> +    PPC440PCIXState *s = opaque;
>
>     trace_ppc440_pcix_set_irq(irq_num);
>     if (irq_num < 0) {
>         error_report("%s: PCI irq %d", __func__, irq_num);
>         return;
>     }
> -    qemu_set_irq(*pci_irq, level);
> +    qemu_set_irq(s->irq, level);
> }
>
> static AddressSpace *ppc440_pcix_set_iommu(PCIBus *b, void *opaque, int devfn)
> @@ -492,7 +492,7 @@ static void ppc440_pcix_realize(DeviceState *dev, Error 
> **errp)
>     sysbus_init_irq(sbd, &s->irq);
>     memory_region_init(&s->busmem, OBJECT(dev), "pci bus memory", UINT64_MAX);
>     h->bus = pci_register_root_bus(dev, NULL, ppc440_pcix_set_irq,
> -                         ppc440_pcix_map_irq, &s->irq, &s->busmem,
> +                         ppc440_pcix_map_irq, s, &s->busmem,
>                          get_system_io(), PCI_DEVFN(0, 0), 1, TYPE_PCI_BUS);
>
>     s->dev = pci_create_simple(h->bus, PCI_DEVFN(0, 0), "ppc4xx-host-bridge");
> diff --git a/hw/ppc/ppc4xx_pci.c b/hw/ppc/ppc4xx_pci.c
> index 5df97e6d15..f6718746a1 100644
> --- a/hw/ppc/ppc4xx_pci.c
> +++ b/hw/ppc/ppc4xx_pci.c
> @@ -256,11 +256,11 @@ static int ppc4xx_pci_map_irq(PCIDevice *pci_dev, int 
> irq_num)
>
> static void ppc4xx_pci_set_irq(void *opaque, int irq_num, int level)
> {
> -    qemu_irq *pci_irqs = opaque;
> +    PPC4xxPCIState *s = opaque;
>
>     trace_ppc4xx_pci_set_irq(irq_num);
>     assert(irq_num >= 0 && irq_num < PPC4xx_PCI_NUM_DEVS);
> -    qemu_set_irq(pci_irqs[irq_num], level);
> +    qemu_set_irq(s->irq[irq_num], level);
> }
>
> static const VMStateDescription vmstate_pci_master_map = {
> @@ -319,7 +319,7 @@ static void ppc4xx_pcihost_realize(DeviceState *dev, Error 
> **errp)
>     }
>
>     b = pci_register_root_bus(dev, NULL, ppc4xx_pci_set_irq,
> -                              ppc4xx_pci_map_irq, s->irq, get_system_memory(),
> +                              ppc4xx_pci_map_irq, s, get_system_memory(),
>                               get_system_io(), 0, ARRAY_SIZE(s->irq),
>                               TYPE_PCI_BUS);
>     h->bus = b;