Re: [PATCH V2] Add a new PIIX option to control global PCI hot-plugging

[Igor Mammedow <imammedo@redhat.com>]

On Fri, 15 May 2020 17:27:30 +0000
Ani Sinha <ani.sinha@nutanix.com> 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.
> 
> Signed-off-by: Ani Sinha <ani.sinha@nutanix.com>
> ---
>  hw/acpi/piix4.c      | 18 ++++++++++++------
>  hw/i386/acpi-build.c | 46
> ++++++++++++++++++++++++++++++---------------- 2 files changed, 42
> insertions(+), 22 deletions(-)
> 
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 964d6f5..91b7e86 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;
It's better to split out renaming into separate patch, so it won't
clutter functional change.

>  
>      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,7 +506,10 @@ 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);
> +    if (s->use_acpi_pci_hotplug) {
> +        qbus_set_hotplug_handler(BUS(pci_get_bus(dev)),
> +                                 OBJECT(s), &error_abort);
that's not enough to disable all hw initialization done for cpi hotplug
you probably should take a look at acpi_pcihp_init()

> +    }
>  
>      piix4_pm_add_propeties(s);
>  }
> @@ -528,7 +532,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,7 +597,7 @@ 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);
>  
>      s->cpu_hotplug_legacy = true;
>      object_property_add_bool(OBJECT(s), "cpu-hotplug-legacy",
> @@ -631,8 +635,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) {
that only disables AML part of it, and that's only partially,
leaving all unused AML methods related to PCI hotplug


it would be better to disable bsel generation, which should disable
code that you are touching here. And some extra work necessary to
disable related AML methods.


> +                /*
> +                 * 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) {