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

[Bernhard Beschow <shentey@gmail.com>]

Am 12. Februar 2022 14:27:32 MEZ schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>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?

I'm referring to the typedef in pci.h because the patch establishes
consistency across the board.

>> 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.

This patch is a segway into a direction where the type of the opaque parameter
of the pci_map_irq_fn typedef could be changed from void* to DeviceState* in
order to facilitate the more typesafe QOM casting. I see, though, why this is
confusing in the context of this patch series. I don't have strong feelings for
converting the other devices or not. So I can only change piix4 if desired.

Regards,
Bernhard

>
>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;
>>