Re: [PATCH v4 1/2] hw/ide/ahci: Decouple from PCI

[Bernhard Beschow]

Am 13. Dezember 2024 14:41:46 UTC schrieb "Philippe Mathieu-Daudé" 
<philmd@linaro.org>:
>On 12/12/24 12:09, Bernhard Beschow wrote:
>> In some adhoc profiling booting Linux VMs, it's observed that 
>> ahci_irq_lower()
>> can be a hot path (10000+ triggers until login prompt appears). Even though 
>> the
>> parent device never changes, this method re-determines whether the parent 
>> device
>> is a PCI device or not using the rather expensive object_dynamic_cast()
>> function. Avoid this overhead by pushing the interrupt handling to the parent
>> device, essentially turning AHCIState into an "IP block".
>> 
>> Note that this change also frees AHCIState from the PCI dependency which 
>> wasn't
>> reflected in Kconfig.
>> 
>> Reported-by: Peter Xu <peterx@redhat.com>
>> Inspired-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   hw/ide/ahci-internal.h    |  1 -
>>   include/hw/ide/ahci-pci.h |  2 ++
>>   include/hw/ide/ahci.h     |  2 --
>>   hw/ide/ahci.c             | 39 ++++-----------------------------------
>>   hw/ide/ich.c              | 19 +++++++++++++++----
>>   5 files changed, 21 insertions(+), 42 deletions(-)
>
>
>>   static void pci_ich9_reset(DeviceState *dev)
>>   {
>>       AHCIPCIState *d = ICH9_AHCI(dev);
>> @@ -102,7 +114,9 @@ static void pci_ich9_ahci_init(Object *obj)
>>   {
>>       AHCIPCIState *d = ICH9_AHCI(obj);
>>   +    qemu_init_irq(&d->irq, pci_ich9_ahci_update_irq, d, 0);
>>       ahci_init(&d->ahci, DEVICE(obj));
>> +    d->ahci.irq = &d->irq;
>
>Pre-existing, but we shouldn't set this directly.
>Does the IRQState belong to AHCIState?

AHCIState isn't an Object, and therefore can't have any properties, so we can 
only set it directly. In the SysBus devices, d->ahci.irq is treated with 
sysbus_init_irq(), so needs to stay a pointer.

I tried to convert AHCIState into a SysBusDevice in order to access it via 
these APIs, but that would create migration compatibility problems for the q35 
machine which was a rabbit hole I didn't want to get into. So I settled on this 
solution. Any better proposals?

Best regards,
Bernhard

>
>>   }
>>     static void pci_ich9_ahci_realize(PCIDevice *dev, Error **errp)
>> @@ -125,8 +139,6 @@ static void pci_ich9_ahci_realize(PCIDevice *dev, Error 
>> **errp)
>>       /* XXX Software should program this register */
>>       dev->config[0x90]   = 1 << 6; /* Address Map Register - AHCI mode */
>>   -    d->ahci.irq = pci_allocate_irq(dev);
>> -
>>       pci_register_bar(dev, ICH9_IDP_BAR, PCI_BASE_ADDRESS_SPACE_IO,
>>                        &d->ahci.idp);
>>       pci_register_bar(dev, ICH9_MEM_BAR, PCI_BASE_ADDRESS_SPACE_MEMORY,
>> @@ -161,7 +173,6 @@ static void pci_ich9_uninit(PCIDevice *dev)
>>         msi_uninit(dev);
>>       ahci_uninit(&d->ahci);
>> -    qemu_free_irq(d->ahci.irq);
>>   }
>>     static void ich_ahci_class_init(ObjectClass *klass, void *data)
>