| Message ID | 20241121100152.65476-1-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/ide/ahci: Check for PCI device once in ahci_init() | expand |
Am 21. November 2024 10:01:52 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>: >object_dynamic_cast() is expensive; IRQ helpers are certainly >a bad place to call it. Since the device type won't change at >runtime, resolve it once when the AHCI context is initialized >in ahci_init(). > >Reported-by: Peter Xu <peterx@redhat.com> >Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >--- > include/hw/ide/ahci.h | 2 +- > hw/ide/ahci.c | 17 +++++------------ > 2 files changed, 6 insertions(+), 13 deletions(-) > >diff --git a/include/hw/ide/ahci.h b/include/hw/ide/ahci.h >index ba31e75ff9..f6d977610d 100644 >--- a/include/hw/ide/ahci.h >+++ b/include/hw/ide/ahci.h >@@ -37,7 +37,7 @@ typedef struct AHCIControlRegs { > } AHCIControlRegs; > > typedef struct AHCIState { >- DeviceState *container; >+ PCIDevice *pci_dev; > > AHCIDevice *dev; > AHCIControlRegs control_regs; >diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c >index 0eb24304ee..f2eb3b527b 100644 >--- a/hw/ide/ahci.c >+++ b/hw/ide/ahci.c >@@ -181,14 +181,10 @@ static uint32_t ahci_port_read(AHCIState *s, int port, int offset) > > static void ahci_irq_raise(AHCIState *s) > { >- DeviceState *dev_state = s->container; >- PCIDevice *pci_dev = (PCIDevice *) object_dynamic_cast(OBJECT(dev_state), >- TYPE_PCI_DEVICE); >- > trace_ahci_irq_raise(s); > >- if (pci_dev && msi_enabled(pci_dev)) { >- msi_notify(pci_dev, 0); >+ if (s->pci_dev && msi_enabled(s->pci_dev)) { >+ msi_notify(s->pci_dev, 0); > } else { > qemu_irq_raise(s->irq); > } >@@ -196,13 +192,9 @@ static void ahci_irq_raise(AHCIState *s) > > static void ahci_irq_lower(AHCIState *s) > { >- DeviceState *dev_state = s->container; >- PCIDevice *pci_dev = (PCIDevice *) object_dynamic_cast(OBJECT(dev_state), >- TYPE_PCI_DEVICE); >- > trace_ahci_irq_lower(s); > >- if (!pci_dev || !msi_enabled(pci_dev)) { >+ if (!s->pci_dev || !msi_enabled(s->pci_dev)) { > qemu_irq_lower(s->irq); > } > } By always triggering the "irq" property, it might be possible to push out the above two methods to the caller, i.e. the parent PCI device. This would make this device model independent from PCI, essentially turning it into an "IP block". At the same time this eliminates the need for the dynamic casts and AFAICS would also fix the missing PCI dependency in the Kconfig file. I could send a patch. Best regards, Bernhard >@@ -1608,7 +1600,8 @@ static const IDEDMAOps ahci_dma_ops = { > > void ahci_init(AHCIState *s, DeviceState *qdev) > { >- s->container = qdev; >+ s->pci_dev = (PCIDevice *)object_dynamic_cast(OBJECT(qdev), TYPE_PCI_DEVICE); >+ > /* XXX BAR size should be 1k, but that breaks, so bump it to 4k for now */ > memory_region_init_io(&s->mem, OBJECT(qdev), &ahci_mem_ops, s, > "ahci", AHCI_MEM_BAR_SIZE);
On 3/12/24 07:46, Bernhard Beschow wrote: > Am 21. November 2024 10:01:52 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>: >> object_dynamic_cast() is expensive; IRQ helpers are certainly >> a bad place to call it. Since the device type won't change at >> runtime, resolve it once when the AHCI context is initialized >> in ahci_init(). >> >> Reported-by: Peter Xu <peterx@redhat.com> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> --- >> include/hw/ide/ahci.h | 2 +- >> hw/ide/ahci.c | 17 +++++------------ >> 2 files changed, 6 insertions(+), 13 deletions(-) >> @@ -196,13 +192,9 @@ static void ahci_irq_raise(AHCIState *s) >> >> static void ahci_irq_lower(AHCIState *s) >> { >> - DeviceState *dev_state = s->container; >> - PCIDevice *pci_dev = (PCIDevice *) object_dynamic_cast(OBJECT(dev_state), >> - TYPE_PCI_DEVICE); >> - >> trace_ahci_irq_lower(s); >> >> - if (!pci_dev || !msi_enabled(pci_dev)) { >> + if (!s->pci_dev || !msi_enabled(s->pci_dev)) { >> qemu_irq_lower(s->irq); >> } >> } > > By always triggering the "irq" property, it might be possible to push out the above two methods to the caller, i.e. the parent PCI device. This would make this device model independent from PCI, essentially turning it into an "IP block". At the same time this eliminates the need for the dynamic casts and AFAICS would also fix the missing PCI dependency in the Kconfig file. I could send a patch. Oh. Please go ahead, that is appreciated! > > Best regards, > Bernhard
Am 3. Dezember 2024 09:12:08 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>: >On 3/12/24 07:46, Bernhard Beschow wrote: >> Am 21. November 2024 10:01:52 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>: >>> object_dynamic_cast() is expensive; IRQ helpers are certainly >>> a bad place to call it. Since the device type won't change at >>> runtime, resolve it once when the AHCI context is initialized >>> in ahci_init(). >>> >>> Reported-by: Peter Xu <peterx@redhat.com> >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>> --- >>> include/hw/ide/ahci.h | 2 +- >>> hw/ide/ahci.c | 17 +++++------------ >>> 2 files changed, 6 insertions(+), 13 deletions(-) > > >>> @@ -196,13 +192,9 @@ static void ahci_irq_raise(AHCIState *s) >>> >>> static void ahci_irq_lower(AHCIState *s) >>> { >>> - DeviceState *dev_state = s->container; >>> - PCIDevice *pci_dev = (PCIDevice *) object_dynamic_cast(OBJECT(dev_state), >>> - TYPE_PCI_DEVICE); >>> - >>> trace_ahci_irq_lower(s); >>> >>> - if (!pci_dev || !msi_enabled(pci_dev)) { >>> + if (!s->pci_dev || !msi_enabled(s->pci_dev)) { >>> qemu_irq_lower(s->irq); >>> } >>> } >> >> By always triggering the "irq" property, it might be possible to push out the above two methods to the caller, i.e. the parent PCI device. This would make this device model independent from PCI, essentially turning it into an "IP block". At the same time this eliminates the need for the dynamic casts and AFAICS would also fix the missing PCI dependency in the Kconfig file. I could send a patch. > >Oh. Please go ahead, that is appreciated! Patch sent: <https://lore.kernel.org/qemu-devel/20241205114453.1848-1-shentey@gmail.com/> Best regards, Bernhard > >> >> Best regards, >> Bernhard
diff --git a/include/hw/ide/ahci.h b/include/hw/ide/ahci.h index ba31e75ff9..f6d977610d 100644 --- a/include/hw/ide/ahci.h +++ b/include/hw/ide/ahci.h @@ -37,7 +37,7 @@ typedef struct AHCIControlRegs { } AHCIControlRegs; typedef struct AHCIState { - DeviceState *container; + PCIDevice *pci_dev; AHCIDevice *dev; AHCIControlRegs control_regs; diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 0eb24304ee..f2eb3b527b 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -181,14 +181,10 @@ static uint32_t ahci_port_read(AHCIState *s, int port, int offset) static void ahci_irq_raise(AHCIState *s) { - DeviceState *dev_state = s->container; - PCIDevice *pci_dev = (PCIDevice *) object_dynamic_cast(OBJECT(dev_state), - TYPE_PCI_DEVICE); - trace_ahci_irq_raise(s); - if (pci_dev && msi_enabled(pci_dev)) { - msi_notify(pci_dev, 0); + if (s->pci_dev && msi_enabled(s->pci_dev)) { + msi_notify(s->pci_dev, 0); } else { qemu_irq_raise(s->irq); } @@ -196,13 +192,9 @@ static void ahci_irq_raise(AHCIState *s) static void ahci_irq_lower(AHCIState *s) { - DeviceState *dev_state = s->container; - PCIDevice *pci_dev = (PCIDevice *) object_dynamic_cast(OBJECT(dev_state), - TYPE_PCI_DEVICE); - trace_ahci_irq_lower(s); - if (!pci_dev || !msi_enabled(pci_dev)) { + if (!s->pci_dev || !msi_enabled(s->pci_dev)) { qemu_irq_lower(s->irq); } } @@ -1608,7 +1600,8 @@ static const IDEDMAOps ahci_dma_ops = { void ahci_init(AHCIState *s, DeviceState *qdev) { - s->container = qdev; + s->pci_dev = (PCIDevice *)object_dynamic_cast(OBJECT(qdev), TYPE_PCI_DEVICE); + /* XXX BAR size should be 1k, but that breaks, so bump it to 4k for now */ memory_region_init_io(&s->mem, OBJECT(qdev), &ahci_mem_ops, s, "ahci", AHCI_MEM_BAR_SIZE);
object_dynamic_cast() is expensive; IRQ helpers are certainly a bad place to call it. Since the device type won't change at runtime, resolve it once when the AHCI context is initialized in ahci_init(). Reported-by: Peter Xu <peterx@redhat.com> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- include/hw/ide/ahci.h | 2 +- hw/ide/ahci.c | 17 +++++------------ 2 files changed, 6 insertions(+), 13 deletions(-)