From: David Gibson <david@gibson.dropbear.id.au>
To: Julia Suvorova <jusual@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>,
QEMU Developers <qemu-devel@nongnu.org>,
Eduardo Habkost <ehabkost@redhat.com>,
"Michael S. Tsirkin" <mst@redhat.com>
Subject: Re: [PATCH v5 3/7] hw/acpi/ich9: Enable ACPI PCI hot-plug
Date: Sat, 3 Jul 2021 12:51:21 +1000 [thread overview]
Message-ID: <YN/QqS7Uljy46pkW@yekko> (raw)
In-Reply-To: <CAMDeoFXZyOt+O90xrRpTD8k7f0M60GVpg3XWfG2+yOAukOFgeA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 10517 bytes --]
On Fri, Jul 02, 2021 at 04:55:47PM +0200, Julia Suvorova wrote:
> 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.
Ok, understood.
--
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2021-07-03 3:01 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-17 19:07 [PATCH v5 0/7] Use ACPI PCI hot-plug for Q35 Julia Suvorova
2021-06-17 19:07 ` [PATCH v5 1/7] hw/acpi/pcihp: Enhance acpi_pcihp_disable_root_bus() to support Q35 Julia Suvorova
2021-06-23 10:26 ` Marcel Apfelbaum
2021-07-01 4:17 ` David Gibson
2021-07-03 6:19 ` Michael S. Tsirkin
2021-07-03 7:11 ` Michael S. Tsirkin
2021-06-17 19:07 ` [PATCH v5 2/7] hw/i386/acpi-build: Add ACPI PCI hot-plug methods to Q35 Julia Suvorova
2021-06-23 10:39 ` Marcel Apfelbaum
2021-07-01 4:36 ` David Gibson
2021-07-02 14:35 ` Michael S. Tsirkin
2021-06-17 19:07 ` [PATCH v5 3/7] hw/acpi/ich9: Enable ACPI PCI hot-plug Julia Suvorova
2021-06-23 10:46 ` Marcel Apfelbaum
2021-07-01 4:46 ` David Gibson
2021-07-02 14:36 ` Michael S. Tsirkin
2021-07-02 14:55 ` Julia Suvorova
2021-07-02 15:34 ` Michael S. Tsirkin
2021-07-03 2:51 ` David Gibson [this message]
2021-06-17 19:07 ` [PATCH v5 4/7] hw/pci/pcie: Do not set HPC flag if acpihp is used Julia Suvorova
2021-06-17 21:02 ` Michael S. Tsirkin
2021-06-23 11:02 ` Marcel Apfelbaum
2021-06-18 11:54 ` Igor Mammedov
2021-06-18 12:39 ` Michael S. Tsirkin
2021-07-01 4:50 ` David Gibson
2021-06-17 19:07 ` [PATCH v5 5/7] bios-tables-test: Allow changes in DSDT ACPI tables Julia Suvorova
2021-06-23 11:03 ` Marcel Apfelbaum
2021-06-17 19:07 ` [PATCH v5 6/7] hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35 Julia Suvorova
2021-06-23 11:13 ` Marcel Apfelbaum
2021-06-17 19:07 ` [PATCH v5 7/7] bios-tables-test: Update golden binaries Julia Suvorova
2021-07-01 4:54 ` David Gibson
2021-07-01 8:36 ` Igor Mammedov
2021-06-17 21:02 ` [PATCH v5 0/7] Use ACPI PCI hot-plug for Q35 Michael S. Tsirkin
2021-06-23 14:47 ` [PATCH] hw/pci/pcie_port: Rename "enable-native-hotplug" property Julia Suvorova
2021-06-24 6:31 ` Igor Mammedov
2021-06-24 12:29 ` Marcel Apfelbaum
2021-07-01 4:55 ` David Gibson
2021-07-03 16:45 ` [PATCH v5 0/7] Use ACPI PCI hot-plug for Q35 Michael S. Tsirkin
2021-07-06 13:30 ` Marcel Apfelbaum
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YN/QqS7Uljy46pkW@yekko \
--to=david@gibson.dropbear.id.au \
--cc=ehabkost@redhat.com \
--cc=imammedo@redhat.com \
--cc=jusual@redhat.com \
--cc=mst@redhat.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.