| Message ID | 20241212110926.23548-2-shentey@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | AHCI cleanup | expand |
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? > } > > 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)
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) >
diff --git a/hw/ide/ahci-internal.h b/hw/ide/ahci-internal.h index 7e63ea2310..a318f36811 100644 --- a/hw/ide/ahci-internal.h +++ b/hw/ide/ahci-internal.h @@ -25,7 +25,6 @@ #define HW_IDE_AHCI_INTERNAL_H #include "hw/ide/ahci.h" -#include "hw/pci/pci_device.h" #include "ide-internal.h" #define AHCI_MEM_BAR_SIZE 0x1000 diff --git a/include/hw/ide/ahci-pci.h b/include/hw/ide/ahci-pci.h index c2ee616962..face1a9a4a 100644 --- a/include/hw/ide/ahci-pci.h +++ b/include/hw/ide/ahci-pci.h @@ -9,6 +9,7 @@ #include "qom/object.h" #include "hw/ide/ahci.h" #include "hw/pci/pci_device.h" +#include "hw/irq.h" #define TYPE_ICH9_AHCI "ich9-ahci" OBJECT_DECLARE_SIMPLE_TYPE(AHCIPCIState, ICH9_AHCI) @@ -17,6 +18,7 @@ struct AHCIPCIState { PCIDevice parent_obj; AHCIState ahci; + IRQState irq; }; #endif diff --git a/include/hw/ide/ahci.h b/include/hw/ide/ahci.h index ba31e75ff9..ac0292c634 100644 --- a/include/hw/ide/ahci.h +++ b/include/hw/ide/ahci.h @@ -37,8 +37,6 @@ typedef struct AHCIControlRegs { } AHCIControlRegs; typedef struct AHCIState { - DeviceState *container; - AHCIDevice *dev; AHCIControlRegs control_regs; MemoryRegion mem; diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 0eb24304ee..5836aa924b 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -23,8 +23,6 @@ #include "qemu/osdep.h" #include "hw/irq.h" -#include "hw/pci/msi.h" -#include "hw/pci/pci.h" #include "hw/qdev-properties.h" #include "migration/vmstate.h" @@ -34,8 +32,6 @@ #include "qemu/module.h" #include "sysemu/block-backend.h" #include "sysemu/dma.h" -#include "hw/ide/pci.h" -#include "hw/ide/ahci-pci.h" #include "hw/ide/ahci-sysbus.h" #include "ahci-internal.h" #include "ide-internal.h" @@ -179,34 +175,6 @@ static uint32_t ahci_port_read(AHCIState *s, int port, int offset) return val; } -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); - } else { - qemu_irq_raise(s->irq); - } -} - -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)) { - qemu_irq_lower(s->irq); - } -} - static void ahci_check_irq(AHCIState *s) { int i; @@ -222,9 +190,11 @@ static void ahci_check_irq(AHCIState *s) trace_ahci_check_irq(s, old_irq, s->control_regs.irqstatus); if (s->control_regs.irqstatus && (s->control_regs.ghc & HOST_CTL_IRQ_EN)) { - ahci_irq_raise(s); + trace_ahci_irq_raise(s); + qemu_irq_raise(s->irq); } else { - ahci_irq_lower(s); + trace_ahci_irq_lower(s); + qemu_irq_lower(s->irq); } } @@ -1608,7 +1578,6 @@ static const IDEDMAOps ahci_dma_ops = { void ahci_init(AHCIState *s, DeviceState *qdev) { - s->container = qdev; /* 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); diff --git a/hw/ide/ich.c b/hw/ide/ich.c index b311450c12..c99a44df8e 100644 --- a/hw/ide/ich.c +++ b/hw/ide/ich.c @@ -61,7 +61,6 @@ */ #include "qemu/osdep.h" -#include "hw/irq.h" #include "hw/pci/msi.h" #include "hw/pci/pci.h" #include "migration/vmstate.h" @@ -91,6 +90,19 @@ static const VMStateDescription vmstate_ich9_ahci = { }, }; +static void pci_ich9_ahci_update_irq(void *opaque, int irq_num, int level) +{ + PCIDevice *pci_dev = opaque; + + if (msi_enabled(pci_dev)) { + if (level) { + msi_notify(pci_dev, 0); + } + } else { + pci_set_irq(pci_dev, level); + } +} + 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; } 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)
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(-)