Igor, here’s my attempt in cooking up a patch which disabled global ACPI hot plug. You said something about SHPC also adding AMLs when enabled but I could not figure out where that is being done.
Please provide suggestions. > On May 15, 2020, at 3:36 PM, Ani Sinha <[email protected]> wrote: > > A new option "acpi-pci-hotplug" is introduced for PIIX which will > globally disable hot-plugging of both hot plugged and > cold plugged PCI devices. This will prevent > hot-plugging and hot un-plugging of devices from within Windows based > guests using system tray. > > The patch has been tested on Windows 2016. > > Change-Id: I6a79f5c69dab2c06fc3578b098bf085fb8741ce6 > Signed-off-by: Ani Sinha <[email protected]> > --- > hw/acpi/piix4.c | 19 ++++++++++++------- > hw/i386/acpi-build.c | 46 ++++++++++++++++++++++++++++++---------------- > 2 files changed, 42 insertions(+), 23 deletions(-) > > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c > index 964d6f5..1f86dd9 100644 > --- a/hw/acpi/piix4.c > +++ b/hw/acpi/piix4.c > @@ -78,6 +78,7 @@ typedef struct PIIX4PMState { > > AcpiPciHpState acpi_pci_hotplug; > bool use_acpi_pci_hotplug; > + bool use_acpi_hotplug_bridge; > > uint8_t disable_s3; > uint8_t disable_s4; > @@ -207,13 +208,13 @@ static const VMStateDescription vmstate_pci_status = { > static bool vmstate_test_use_acpi_pci_hotplug(void *opaque, int version_id) > { > PIIX4PMState *s = opaque; > - return s->use_acpi_pci_hotplug; > + return s->use_acpi_hotplug_bridge; > } > > static bool vmstate_test_no_use_acpi_pci_hotplug(void *opaque, int version_id) > { > PIIX4PMState *s = opaque; > - return !s->use_acpi_pci_hotplug; > + return !s->use_acpi_hotplug_bridge; > } > > static bool vmstate_test_use_memhp(void *opaque) > @@ -505,8 +506,6 @@ static void piix4_pm_realize(PCIDevice *dev, Error **errp) > > piix4_acpi_system_hot_add_init(pci_address_space_io(dev), > pci_get_bus(dev), s); > - qbus_set_hotplug_handler(BUS(pci_get_bus(dev)), OBJECT(s), &error_abort); > - I left this as per your suggestion but this was just an optimization to move this into the init routine correct? > piix4_pm_add_propeties(s); > } > > @@ -528,7 +527,7 @@ I2CBus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t > smb_io_base, > s->smi_irq = smi_irq; > s->smm_enabled = smm_enabled; > if (xen_enabled()) { > - s->use_acpi_pci_hotplug = false; > + s->use_acpi_hotplug_bridge = false; > } > > qdev_init_nofail(dev); > @@ -593,8 +592,12 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion > *parent, > memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe); > > acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus, parent, > - s->use_acpi_pci_hotplug); > + s->use_acpi_hotplug_bridge); > > + if (s->use_acpi_pci_hotplug) { > + qbus_set_hotplug_handler(BUS(pci_get_bus(dev)), > + OBJECT(s), &error_abort); > + } Moved this one from the above. Seems we could have used this check in piix4_pm_realize() as well. > s->cpu_hotplug_legacy = true; > object_property_add_bool(OBJECT(s), "cpu-hotplug-legacy", > piix4_get_cpu_hotplug_legacy, > @@ -631,8 +634,10 @@ static Property piix4_pm_properties[] = { > DEFINE_PROP_UINT8(ACPI_PM_PROP_S3_DISABLED, PIIX4PMState, disable_s3, 0), > DEFINE_PROP_UINT8(ACPI_PM_PROP_S4_DISABLED, PIIX4PMState, disable_s4, 0), > DEFINE_PROP_UINT8(ACPI_PM_PROP_S4_VAL, PIIX4PMState, s4_val, 2), > - DEFINE_PROP_BOOL("acpi-pci-hotplug-with-bridge-support", PIIX4PMState, > + DEFINE_PROP_BOOL("acpi-pci-hotplug", PIIX4PMState, > use_acpi_pci_hotplug, true), > + DEFINE_PROP_BOOL("acpi-pci-hotplug-with-bridge-support", PIIX4PMState, > + use_acpi_hotplug_bridge, true), > DEFINE_PROP_BOOL("memory-hotplug-support", PIIX4PMState, > acpi_memory_hotplug.is_enabled, true), > DEFINE_PROP_END_OF_LIST(), > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index 2e15f68..1737378 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -96,6 +96,7 @@ typedef struct AcpiPmInfo { > bool s3_disabled; > bool s4_disabled; > bool pcihp_bridge_en; > + bool acpi_pcihp_en; > uint8_t s4_val; > AcpiFadtData fadt; > uint16_t cpu_hp_io_base; > @@ -246,6 +247,9 @@ static void acpi_get_pm_info(MachineState *machine, > AcpiPmInfo *pm) > pm->pcihp_bridge_en = > object_property_get_bool(obj, "acpi-pci-hotplug-with-bridge-support", > NULL); > + pm->acpi_pcihp_en = > + object_property_get_bool(obj, "acpi-pci-hotplug", > + NULL); > } > > static void acpi_get_misc_info(AcpiMiscInfo *info) > @@ -457,7 +461,8 @@ static void build_append_pcihp_notify_entry(Aml *method, > int slot) > } > > static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, > - bool pcihp_bridge_en) > + bool pcihp_bridge_en, > + bool acpi_pcihp_en) > { > Aml *dev, *notify_method = NULL, *method; > QObject *bsel; > @@ -481,18 +486,25 @@ static void build_append_pci_bus_devices(Aml > *parent_scope, PCIBus *bus, > bool bridge_in_acpi; > > if (!pdev) { > - if (bsel) { /* add hotplug slots for non present devices */ > - dev = aml_device("S%.02X", PCI_DEVFN(slot, 0)); > - aml_append(dev, aml_name_decl("_SUN", aml_int(slot))); > - aml_append(dev, aml_name_decl("_ADR", aml_int(slot << 16))); > - method = aml_method("_EJ0", 1, AML_NOTSERIALIZED); > - aml_append(method, > - aml_call2("PCEJ", aml_name("BSEL"), aml_name("_SUN")) > - ); > - aml_append(dev, method); > - aml_append(parent_scope, dev); > - > - build_append_pcihp_notify_entry(notify_method, slot); > + if (bsel) { > + /* > + * add hotplug slots for non present devices when > + * acpi hotplug is enabled. > + */ > + if (acpi_pcihp_en) { > + dev = aml_device("S%.02X", PCI_DEVFN(slot, 0)); > + aml_append(dev, aml_name_decl("_SUN", aml_int(slot))); > + aml_append(dev, aml_name_decl("_ADR", aml_int(slot << > 16))); > + method = aml_method("_EJ0", 1, AML_NOTSERIALIZED); > + aml_append(method, > + aml_call2("PCEJ", aml_name("BSEL"), > + aml_name("_SUN")) > + ); > + aml_append(dev, method); > + aml_append(parent_scope, dev); > + > + build_append_pcihp_notify_entry(notify_method, slot); > + } > } > continue; > } > @@ -539,7 +551,7 @@ static void build_append_pci_bus_devices(Aml > *parent_scope, PCIBus *bus, > method = aml_method("_S3D", 0, AML_NOTSERIALIZED); > aml_append(method, aml_return(aml_int(s3d))); > aml_append(dev, method); > - } else if (hotplug_enabled_dev) { > + } else if (hotplug_enabled_dev && acpi_pcihp_en) { > /* add _SUN/_EJ0 to make slot hotpluggable */ > aml_append(dev, aml_name_decl("_SUN", aml_int(slot))); > > @@ -559,7 +571,8 @@ static void build_append_pci_bus_devices(Aml > *parent_scope, PCIBus *bus, > */ > PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev)); > > - build_append_pci_bus_devices(dev, sec_bus, pcihp_bridge_en); > + build_append_pci_bus_devices(dev, sec_bus, pcihp_bridge_en, > + acpi_pcihp_en); > } > /* slot descriptor has been composed, add it into parent context */ > aml_append(parent_scope, dev); > @@ -2197,7 +2210,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, > if (bus) { > Aml *scope = aml_scope("PCI0"); > /* Scan all PCI buses. Generate tables to support hotplug. */ > - build_append_pci_bus_devices(scope, bus, pm->pcihp_bridge_en); > + build_append_pci_bus_devices(scope, bus, pm->pcihp_bridge_en, > + pm->acpi_pcihp_en); > > if (TPM_IS_TIS_ISA(tpm)) { > if (misc->tpm_version == TPM_VERSION_2_0) { > -- > 1.9.4 >
