On Mon, Oct 18, 2021 at 12:10:04PM +0200, Philippe Mathieu-Daudé wrote: > On 10/18/21 11:51, BALATON Zoltan wrote: > > On Mon, 18 Oct 2021, Philippe Mathieu-Daudé wrote: > >> On 10/18/21 03:36, BALATON Zoltan wrote: > >>> Cache the pointer to PCI function 0 (ISA bridge, that this IDE device > >>> has to use for IRQs) in the PCIIDEState and pass that as the opaque > >>> data for the interrupt handler to eliminate both the need to look up > >>> function 0 at every interrupt and also a QOM type cast of the opaque > >>> pointer as that's also expensive (mainly due to qom-debug being > >>> enabled by default). > >>> > >>> Suggested-by: Philippe Mathieu-Daudé <f4...@amsat.org> > >>> Signed-off-by: BALATON Zoltan <bala...@eik.bme.hu> > >>> --- > >>> hw/ide/via.c | 11 ++++++----- > >>> include/hw/ide/pci.h | 1 + > >>> 2 files changed, 7 insertions(+), 5 deletions(-) > >>> > >>> diff --git a/hw/ide/via.c b/hw/ide/via.c > >>> index 82def819c4..30566bc409 100644 > >>> --- a/hw/ide/via.c > >>> +++ b/hw/ide/via.c > >>> @@ -104,15 +104,15 @@ static void bmdma_setup_bar(PCIIDEState *d) > >>> > >>> static void via_ide_set_irq(void *opaque, int n, int level) > >>> { > >>> - PCIDevice *d = PCI_DEVICE(opaque); > >>> + PCIIDEState *d = opaque; > >>> > >>> if (level) { > >>> - d->config[0x70 + n * 8] |= 0x80; > >>> + d->parent_obj.config[0x70 + n * 8] |= 0x80; > >>> } else { > >>> - d->config[0x70 + n * 8] &= ~0x80; > >>> + d->parent_obj.config[0x70 + n * 8] &= ~0x80; > >>> } > >> > >> Better not to access parent_obj, so I'd rather keep the previous > >> code as it. The rest is OK, thanks. If you don't want to respin > >> I can fix and take via mips-next. > > > > Why not? If it's OK to access other fields why not parent_obj? That > > avoids the QOM cast PCI_DEVICE(opaque) or PCI_DEVICE(d) after this > > patch. We know it's a PCIIDEState and has PCIDevice parent_obj field > > because we set the opaque pointer when adding this callback so I think > > this works and is the less expensive way. But feel free to change it any > > way you like and use it that way. I'd keep it as it is. > > My understanding of QOM is we shouldn't access internal states that > way, because 1/ this makes object refactors harder and 2/ this is > not the style/example we want in the codebase, but it doesn't seem > documented, so Cc'ing Markus/Eduardo for confirmation.
`PCI_DEVICE(d)` is preferred instead `of d.parent_obj` (parent_obj is just a QOM implementation detail). If there are real performance reasons to avoid it, we need numbers to support that. Also, note that `OBJECT_CHECK(obj)` is just `return obj` if CONFIG_QOM_CAST_DEBUG is disabled. -- Eduardo