Message ID | 20220212113519.69527-3-shentey@gmail.com |
---|---|
State | New |
Headers | show |
Series | [v2,1/5] malta: Move PCI interrupt handling from gt64xxx to piix4 | expand |
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; >
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; >>
On Sat, 12 Feb 2022, Bernhard Beschow wrote: > 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 I'm still not quite sure what you mean considering these typedefs in include/hw/pci/pci.h: typedef void (*pci_set_irq_fn)(void *opaque, int irq_num, int level); typedef int (*pci_map_irq_fn)(PCIDevice *pci_dev, int irq_num); typedef PCIINTxRoute (*pci_route_irq_fn)(void *opaque, int pin); pci_map_irq_fn already has PCIDevice *, it's pci_set_irq_fn that has void *opaque and is what this patch appears to be changing. Am I looking at the wrong typedefs? > 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. Yes that seems to be an independent cahange so maybe it's better to just change piix4 in this series and then have a separate patch for changing the void *opaque to DeviceState * independent of this series. But I'm not sure that's necessarily a good idea. It may give some more checks but for callbacks used internally by device implementations I think it can be expected that code that registers the callback also knows what its opaque data should be so it does not have to be checked additionally, especially in code that may be called frequently. Although in a similar via-ide callback I could not measure a big penalty the last time I tried but I suspect there still mey be some overhead involving QOM casts (maybe there are just bigger bottle necks in ide emulation so it was masked) so I'm not sure it's worth the added complexity but if others prefer that I'm not that much opposed to it but it's clearer as a separate change anyway. Regards, BALATON Zoltan
On Sat, 12 Feb 2022 at 13:27, BALATON Zoltan <balaton@eik.bme.hu> wrote: > > 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? Yeah, we don't necessarily need to change all these devices, but I think this is an area where over time we're shifting from an older school of API design that was "we have some basically standalone functions that take an arbitrary opaque pointer" towards a more object-oriented design, where the opaque pointer should probably be the pointer-to-this-object unless there's a good reason why it needs to be something else[*], because having a function that's part of a device being passed something other than the device-instance pointer is a bit unexpected. In some cases there is a good reason for opaque pointers for function callbacks to be something else, because passing in the device pointer would make the code noticeably more complicated. But in the specific cases here, the only change is that the callback functions use "s->somearray[i]" instead of "an_argument[i]", which doesn't seem to me like a significant complexity change. thanks -- PMM
Am 12. Februar 2022 17:13:19 MEZ schrieb BALATON Zoltan <balaton@eik.bme.hu>: >On Sat, 12 Feb 2022, Bernhard Beschow wrote: >> 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 > >I'm still not quite sure what you mean considering these typedefs in >include/hw/pci/pci.h: > >typedef void (*pci_set_irq_fn)(void *opaque, int irq_num, int level); >typedef int (*pci_map_irq_fn)(PCIDevice *pci_dev, int irq_num); >typedef PCIINTxRoute (*pci_route_irq_fn)(void *opaque, int pin); > >pci_map_irq_fn already has PCIDevice *, it's pci_set_irq_fn that has void >*opaque and is what this patch appears to be changing. Am I looking at the >wrong typedefs? Oh sorry, I mixed things up. You're correct: I meant pci_set_irq_fn. >> 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. > >Yes that seems to be an independent cahange so maybe it's better to just >change piix4 in this series and then have a separate patch for changing >the void *opaque to DeviceState * independent of this series. But I'm not >sure that's necessarily a good idea. It may give some more checks but for >callbacks used internally by device implementations I think it can be >expected that code that registers the callback also knows what its opaque >data should be so it does not have to be checked additionally, especially >in code that may be called frequently. Although in a similar via-ide >callback I could not measure a big penalty the last time I tried but I >suspect there still mey be some overhead involving QOM casts (maybe there >are just bigger bottle necks in ide emulation so it was masked) so I'm not >sure it's worth the added complexity but if others prefer that I'm not >that much opposed to it but it's clearer as a separate change anyway. I'll just change piix4, leaving the other devices as is. This also allows for merging this patch with the next. Regards, Bernhard >Regards, >BALATON Zoltan
On Sun, 13 Feb 2022, Bernhard Beschow wrote: > Am 12. Februar 2022 17:13:19 MEZ schrieb BALATON Zoltan <balaton@eik.bme.hu>: >> On Sat, 12 Feb 2022, Bernhard Beschow wrote: >>> 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 >> >> I'm still not quite sure what you mean considering these typedefs in >> include/hw/pci/pci.h: >> >> typedef void (*pci_set_irq_fn)(void *opaque, int irq_num, int level); >> typedef int (*pci_map_irq_fn)(PCIDevice *pci_dev, int irq_num); >> typedef PCIINTxRoute (*pci_route_irq_fn)(void *opaque, int pin); >> >> pci_map_irq_fn already has PCIDevice *, it's pci_set_irq_fn that has void >> *opaque and is what this patch appears to be changing. Am I looking at the >> wrong typedefs? > > Oh sorry, I mixed things up. You're correct: I meant pci_set_irq_fn. > >>> 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. >> >> Yes that seems to be an independent cahange so maybe it's better to just >> change piix4 in this series and then have a separate patch for changing >> the void *opaque to DeviceState * independent of this series. But I'm not >> sure that's necessarily a good idea. It may give some more checks but for >> callbacks used internally by device implementations I think it can be >> expected that code that registers the callback also knows what its opaque >> data should be so it does not have to be checked additionally, especially >> in code that may be called frequently. Although in a similar via-ide >> callback I could not measure a big penalty the last time I tried but I >> suspect there still mey be some overhead involving QOM casts (maybe there >> are just bigger bottle necks in ide emulation so it was masked) so I'm not >> sure it's worth the added complexity but if others prefer that I'm not >> that much opposed to it but it's clearer as a separate change anyway. > > I'll just change piix4, leaving the other devices as is. This also allows for merging this patch with the next. I'm not sure what will be the next patch after all the changes we were talking about but don't put too many changes in one patch. It may worth keeping this as a separate change even if it's simple and only affecting piix4 now just to make it clear and simpler to review and maybe bisect later. Generally one change per patch is preferred even if it results in a lot of small patches (unless maybe combining a small style fix or very simple one line change with another simple patch that may not worth a separate patch). This may be over that limit so better to keep as separate patch but again not sure what patch you meant to combine it with but the patch submission guidelines say separate changes should be separate patches. 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;