Am 12. Februar 2022 14:27:32 MEZ schrieb BALATON Zoltan <bala...@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 <shen...@gmail.com>
>> Reviewed-by: Peter Maydell <peter.mayd...@linaro.org>
>> Reviewed-by: Philippe Mathieu-Daudé <f4...@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;
>>
