[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v5 3/7] hw/acpi/ich9: Enable ACPI PCI hot-plug
|
From: |
Julia Suvorova |
|
Subject: |
Re: [PATCH v5 3/7] hw/acpi/ich9: Enable ACPI PCI hot-plug |
|
Date: |
Fri, 2 Jul 2021 16:55:47 +0200 |
On Thu, Jul 1, 2021 at 6:59 AM David Gibson <david@gibson.dropbear.id.au> wrote:
>
> On Thu, Jun 17, 2021 at 09:07:35PM +0200, Julia Suvorova wrote:
> > Add acpi_pcihp to ich9_pm as part of
> > 'acpi-pci-hotplug-with-bridge-support' option. Set default to false.
> >
> > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> > Reviewed-by: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> > ---
> > hw/i386/acpi-build.h | 1 +
> > include/hw/acpi/ich9.h | 3 ++
> > hw/acpi/ich9.c | 67 ++++++++++++++++++++++++++++++++++++++++++
> > hw/acpi/pcihp.c | 5 +++-
> > hw/i386/acpi-build.c | 2 +-
> > 5 files changed, 76 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h
> > index 487ec7710f..0dce155c8c 100644
> > --- a/hw/i386/acpi-build.h
> > +++ b/hw/i386/acpi-build.h
> > @@ -10,5 +10,6 @@ extern const struct AcpiGenericAddress
> > x86_nvdimm_acpi_dsmio;
> > #define ACPI_PCIHP_BNMR_BASE 0x10
> >
> > void acpi_setup(void);
> > +Object *acpi_get_i386_pci_host(void);
> >
> > #endif
> > diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
> > index 596120d97f..a329ce43ab 100644
> > --- a/include/hw/acpi/ich9.h
> > +++ b/include/hw/acpi/ich9.h
> > @@ -24,6 +24,7 @@
> > #include "hw/acpi/acpi.h"
> > #include "hw/acpi/cpu_hotplug.h"
> > #include "hw/acpi/cpu.h"
> > +#include "hw/acpi/pcihp.h"
> > #include "hw/acpi/memory_hotplug.h"
> > #include "hw/acpi/acpi_dev_interface.h"
> > #include "hw/acpi/tco.h"
> > @@ -55,6 +56,8 @@ typedef struct ICH9LPCPMRegs {
> > AcpiCpuHotplug gpe_cpu;
> > CPUHotplugState cpuhp_state;
> >
> > + bool use_acpi_hotplug_bridge;
> > + AcpiPciHpState acpi_pci_hotplug;
> > MemHotplugState acpi_memory_hotplug;
> >
> > uint8_t disable_s3;
> > diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> > index 4daa79ec8d..bcbd567cb0 100644
> > --- a/hw/acpi/ich9.c
> > +++ b/hw/acpi/ich9.c
> > @@ -217,6 +217,26 @@ static const VMStateDescription vmstate_cpuhp_state = {
> > }
> > };
> >
> > +static bool vmstate_test_use_pcihp(void *opaque)
> > +{
> > + ICH9LPCPMRegs *s = opaque;
> > +
> > + return s->use_acpi_hotplug_bridge;
> > +}
> > +
> > +static const VMStateDescription vmstate_pcihp_state = {
> > + .name = "ich9_pm/pcihp",
> > + .version_id = 1,
> > + .minimum_version_id = 1,
> > + .needed = vmstate_test_use_pcihp,
> > + .fields = (VMStateField[]) {
> > + VMSTATE_PCI_HOTPLUG(acpi_pci_hotplug,
> > + ICH9LPCPMRegs,
> > + NULL, NULL),
> > + VMSTATE_END_OF_LIST()
> > + }
> > +};
> > +
> > const VMStateDescription vmstate_ich9_pm = {
> > .name = "ich9_pm",
> > .version_id = 1,
> > @@ -238,6 +258,7 @@ const VMStateDescription vmstate_ich9_pm = {
> > &vmstate_memhp_state,
> > &vmstate_tco_io_state,
> > &vmstate_cpuhp_state,
> > + &vmstate_pcihp_state,
> > NULL
> > }
> > };
> > @@ -259,6 +280,7 @@ static void pm_reset(void *opaque)
> > }
> > pm->smi_en_wmask = ~0;
> >
> > + acpi_pcihp_reset(&pm->acpi_pci_hotplug, true);
>
> Doesn't this need to be protected by if (pm->use_acpi_hotplug_bridge)
> ? Otherwise pm->acpi_pci_hotplug won't be initialized.
Yes, you're right. Although it doesn't affect anything now, it should
be fixed. I'll send a patch on top.
> > acpi_update_sci(&pm->acpi_regs, pm->irq);
> > }
> >
> > @@ -297,6 +319,18 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs
> > *pm,
> > pm->enable_tco = true;
> > acpi_pm_tco_init(&pm->tco_regs, &pm->io);
> >
> > + if (pm->use_acpi_hotplug_bridge) {
> > + acpi_pcihp_init(OBJECT(lpc_pci),
> > + &pm->acpi_pci_hotplug,
> > + pci_get_bus(lpc_pci),
> > + pci_address_space_io(lpc_pci),
> > + true,
> > + ACPI_PCIHP_ADDR_ICH9);
> > +
> > + qbus_set_hotplug_handler(BUS(pci_get_bus(lpc_pci)),
> > + OBJECT(lpc_pci));
> > + }
> > +
> > pm->irq = sci_irq;
> > qemu_register_reset(pm_reset, pm);
> > pm->powerdown_notifier.notify = pm_powerdown_req;
> > @@ -368,6 +402,20 @@ static void ich9_pm_set_enable_tco(Object *obj, bool
> > value, Error **errp)
> > s->pm.enable_tco = value;
> > }
> >
> > +static bool ich9_pm_get_acpi_pci_hotplug(Object *obj, Error **errp)
> > +{
> > + ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
> > +
> > + return s->pm.use_acpi_hotplug_bridge;
> > +}
> > +
> > +static void ich9_pm_set_acpi_pci_hotplug(Object *obj, bool value, Error
> > **errp)
> > +{
> > + ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
> > +
> > + s->pm.use_acpi_hotplug_bridge = value;
> > +}
> > +
> > void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
> > {
> > static const uint32_t gpe0_len = ICH9_PMIO_GPE0_LEN;
> > @@ -376,6 +424,7 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs
> > *pm)
> > pm->disable_s3 = 0;
> > pm->disable_s4 = 0;
> > pm->s4_val = 2;
> > + pm->use_acpi_hotplug_bridge = false;
> >
> > object_property_add_uint32_ptr(obj, ACPI_PM_PROP_PM_IO_BASE,
> > &pm->pm_io_base, OBJ_PROP_FLAG_READ);
> > @@ -399,6 +448,9 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs
> > *pm)
> > object_property_add_bool(obj, ACPI_PM_PROP_TCO_ENABLED,
> > ich9_pm_get_enable_tco,
> > ich9_pm_set_enable_tco);
> > + object_property_add_bool(obj, "acpi-pci-hotplug-with-bridge-support",
> > + ich9_pm_get_acpi_pci_hotplug,
> > + ich9_pm_set_acpi_pci_hotplug);
> > }
> >
> > void ich9_pm_device_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState
> > *dev,
> > @@ -406,6 +458,11 @@ void ich9_pm_device_pre_plug_cb(HotplugHandler
> > *hotplug_dev, DeviceState *dev,
> > {
> > ICH9LPCState *lpc = ICH9_LPC_DEVICE(hotplug_dev);
> >
> > + if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> > + acpi_pcihp_device_pre_plug_cb(hotplug_dev, dev, errp);
> > + return;
> > + }
> > +
> > if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) &&
> > !lpc->pm.acpi_memory_hotplug.is_enabled) {
> > error_setg(errp,
> > @@ -441,6 +498,9 @@ void ich9_pm_device_plug_cb(HotplugHandler
> > *hotplug_dev, DeviceState *dev,
> > } else {
> > acpi_cpu_plug_cb(hotplug_dev, &lpc->pm.cpuhp_state, dev, errp);
> > }
> > + } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> > + acpi_pcihp_device_plug_cb(hotplug_dev, &lpc->pm.acpi_pci_hotplug,
> > + dev, errp);
> > } else {
> > error_setg(errp, "acpi: device plug request for not supported
> > device"
> > " type: %s", object_get_typename(OBJECT(dev)));
> > @@ -473,6 +533,10 @@ void ich9_pm_device_unplug_request_cb(HotplugHandler
> > *hotplug_dev,
> >
> > acpi_cpu_unplug_request_cb(hotplug_dev, &lpc->pm.cpuhp_state,
> > dev, errp);
> > + } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> > + acpi_pcihp_device_unplug_request_cb(hotplug_dev,
> > + &lpc->pm.acpi_pci_hotplug,
> > + dev, errp);
> > } else {
> > error_setg(errp, "acpi: device unplug request for not supported
> > device"
> > " type: %s", object_get_typename(OBJECT(dev)));
> > @@ -490,6 +554,9 @@ void ich9_pm_device_unplug_cb(HotplugHandler
> > *hotplug_dev, DeviceState *dev,
> > } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU) &&
> > !lpc->pm.cpu_hotplug_legacy) {
> > acpi_cpu_unplug_cb(&lpc->pm.cpuhp_state, dev, errp);
> > + } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> > + acpi_pcihp_device_unplug_cb(hotplug_dev, &lpc->pm.acpi_pci_hotplug,
> > + dev, errp);
> > } else {
> > error_setg(errp, "acpi: device unplug for not supported device"
> > " type: %s", object_get_typename(OBJECT(dev)));
> > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > index a55992ed9f..5355618608 100644
> > --- a/hw/acpi/pcihp.c
> > +++ b/hw/acpi/pcihp.c
> > @@ -30,6 +30,8 @@
> > #include "hw/pci-host/i440fx.h"
> > #include "hw/pci/pci.h"
> > #include "hw/pci/pci_bridge.h"
> > +#include "hw/pci/pci_host.h"
> > +#include "hw/i386/acpi-build.h"
> > #include "hw/acpi/acpi.h"
> > #include "hw/pci/pci_bus.h"
> > #include "migration/vmstate.h"
> > @@ -103,6 +105,7 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaque)
> > static void acpi_set_pci_info(void)
> > {
> > static bool bsel_is_set;
> > + Object *host = acpi_get_i386_pci_host();
> > PCIBus *bus;
> > unsigned bsel_alloc = ACPI_PCIHP_BSEL_DEFAULT;
> >
> > @@ -111,7 +114,7 @@ static void acpi_set_pci_info(void)
> > }
> > bsel_is_set = true;
> >
> > - bus = find_i440fx(); /* TODO: Q35 support */
> > + bus = PCI_HOST_BRIDGE(host)->bus;
>
> Would it make more sense for this change to go in patch #1, along with
> the similar change to acpi_pcihp_disable_root_bus().
Not really. This is exactly what enables ACPI hotplug, and should be
done with other changes here. The first patch could be merged in this
one, though.
Best regards, Julia Suvorova.
> > if (bus) {
> > /* Scan all PCI buses. Set property to enable acpi based hotplug.
> > */
> > pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL,
> > &bsel_alloc);
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 67753638e4..aa973aa746 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -299,7 +299,7 @@ static void acpi_get_misc_info(AcpiMiscInfo *info)
> > * Because of the PXB hosts we cannot simply query TYPE_PCI_HOST_BRIDGE.
> > * On i386 arch we only have two pci hosts, so we can look only for them.
> > */
> > -static Object *acpi_get_i386_pci_host(void)
> > +Object *acpi_get_i386_pci_host(void)
> > {
> > PCIHostState *host;
> >
>
> --
> David Gibson | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
> | _way_ _around_!
> http://www.ozlabs.org/~dgibson