[PATCH] hw/ide/ahci: Check for PCI device once in ahci_init()

[PATCH] hw/ide/ahci: Check for PCI device once in ahci_init()

[Philippe Mathieu-Daudé]

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);
     }
 }
@@ -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);
-- 
2.45.2

Re: [PATCH] hw/ide/ahci: Check for PCI device once in ahci_init()

[Bernhard Beschow]

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

Re: [PATCH] hw/ide/ahci: Check for PCI device once in ahci_init()

[Philippe Mathieu-Daudé]

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

Re: [PATCH] hw/ide/ahci: Check for PCI device once in ahci_init()

[Bernhard Beschow]

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