Re: [PATCH V2] Add a new PIIX option to control PCI hot unplugging of devices on non-root buses

[Igor Mammedov <imammedo@redhat.com>]

On Tue, 28 Apr 2020 10:16:52 +0000
Ani Sinha <ani.sinha@nutanix.com> wrote:

> A new option "use_acpi_unplug" is introduced for PIIX which will
> selectively only disable hot unplugging of both hot plugged and
> cold plugged PCI devices on non-root PCI buses. This will prevent
> hot unplugging of devices from Windows based guests from system
> tray but will not prevent devices from being hot plugged into the
> guest.
> 
> It has been tested on Windows guests.
> 
> Signed-off-by: Ani Sinha <ani.sinha@nutanix.com>
> ---
>  hw/acpi/piix4.c      |  3 +++
>  hw/i386/acpi-build.c | 40 ++++++++++++++++++++++++++--------------
>  2 files changed, 29 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 964d6f5..59fa707 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_unplug;
>  
>      uint8_t disable_s3;
>      uint8_t disable_s4;
> @@ -633,6 +634,8 @@ static Property piix4_pm_properties[] = {
>      DEFINE_PROP_UINT8(ACPI_PM_PROP_S4_VAL, PIIX4PMState, s4_val, 2),
>      DEFINE_PROP_BOOL("acpi-pci-hotplug-with-bridge-support", PIIX4PMState,
>                       use_acpi_pci_hotplug, true),
> +    DEFINE_PROP_BOOL("acpi-pci-hotunplug-enable-bridge", PIIX4PMState,
> +                     use_acpi_unplug, 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 23c77ee..71b3ac3 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 pcihup_bridge_en;
>      uint8_t s4_val;
>      AcpiFadtData fadt;
>      uint16_t cpu_hp_io_base;
> @@ -240,6 +241,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->pcihup_bridge_en =
> +        object_property_get_bool(obj, "acpi-pci-hotunplug-enable-bridge",
> +                                 NULL);
>  }
>  
>  static void acpi_get_misc_info(AcpiMiscInfo *info)
> @@ -451,7 +455,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 pcihup_bridge_en)
>  {
>      Aml *dev, *notify_method = NULL, *method;
>      QObject *bsel;
> @@ -479,11 +484,14 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
>                  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);
> +                if (pcihup_bridge_en || pci_bus_is_root(bus)) {

so you are keeping unplug anyway in case of host bridge, so user will see
eject icon if device is in root bus?

Other thing about this patch is that it only partially disable hotplug,
I'd rather do it the way hardware does i.e. full hotplug or no hotplug at all.
(like the other hypervisors have done it, to workaround this Windows 'feature')

which is possible is one puts device on pci bridge without hotplug, i.e.

 -global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=off

that of cause leaves apci hotplug on and as you noticed earlier
Windows will offer to eject any device on root bus including directly
attached bridges. And currently there is no way to disable that.

Will following hack work for you?
possible permutations
1) ACPI hotplug everywhere
-global PIIX4_PM.acpi-pci-hotplug=on -global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=on -device pci-bridge,chassis_nr=1,shpc=doesnt_matter -device e1000,bus=pci.1,addr=01,id=netdev1 

2) No hotplug at all
-global PIIX4_PM.acpi-pci-hotplug=off -global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=on -device pci-bridge,chassis_nr=1,shpc=off -device e1000,bus=pci.1,addr=01,id=netdev1

-global PIIX4_PM.acpi-pci-hotplug=off -global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=off -device pci-bridge,chassis_nr=1,shpc=doesnt_matter  -device e1000,bus=pci.1,addr=01,id=netdev1

3) looks like SHPC kicks in, but it still needs to some bridge description in ACPI that
   acpi-pci-hotplug-with-bridge-support provides, probably with this you can individually flip hotplug on
   colplugged bridges using 'shpc' property (requires Vista or newer, tested win10).

   This needs some investigation so we could remove unsed AML and IO ports, but I'm not really interested
   in PCI stuff. So if 1+2 works for you, I'll post formal patches. If #3 is required feel free
   to use this patch as a starting base to make it complete. 

-global PIIX4_PM.acpi-pci-hotplug=off -global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=on -device pci-bridge,chassis_nr=1,shpc=on -device e1000,bus=pci.1,addr=01,id=netdev1

---

diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 964d6f5990..5f05b2cb87 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_pci_hotplug_on_bridges;
 
     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_pci_hotplug_on_bridges;
 }
 
 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_pci_hotplug_on_bridges;
 }
 
 static bool vmstate_test_use_memhp(void *opaque)
@@ -505,7 +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);
 
     piix4_pm_add_propeties(s);
 }
@@ -528,7 +528,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_pci_hotplug_on_bridges = false;
     }
 
     qdev_init_nofail(dev);
@@ -593,7 +593,10 @@ 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_pci_hotplug_on_bridges);
+    if (s->use_acpi_pci_hotplug) {
+        qbus_set_hotplug_handler(BUS(bus), OBJECT(s), &error_abort);
+    }
 
     s->cpu_hotplug_legacy = true;
     object_property_add_bool(OBJECT(s), "cpu-hotplug-legacy",
@@ -632,6 +635,8 @@ static Property piix4_pm_properties[] = {
     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,
+                     use_acpi_pci_hotplug_on_bridges, true),
+    DEFINE_PROP_BOOL("acpi-pci-hotplug", PIIX4PMState,
                      use_acpi_pci_hotplug, true),
     DEFINE_PROP_BOOL("memory-hotplug-support", PIIX4PMState,
                      acpi_memory_hotplug.is_enabled, true),

---


> +                    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);
> @@ -537,12 +545,14 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
>              /* add _SUN/_EJ0 to make slot hotpluggable  */
>              aml_append(dev, aml_name_decl("_SUN", aml_int(slot)));
>  
> -            method = aml_method("_EJ0", 1, AML_NOTSERIALIZED);
> -            aml_append(method,
> -                aml_call2("PCEJ", aml_name("BSEL"), aml_name("_SUN"))
> -            );
> -            aml_append(dev, method);
> -
> +            if (pcihup_bridge_en || pci_bus_is_root(bus)) {
> +                method = aml_method("_EJ0", 1, AML_NOTSERIALIZED);
> +                aml_append(method,
> +                           aml_call2("PCEJ", aml_name("BSEL"),
> +                                     aml_name("_SUN"))
> +                    );
> +                aml_append(dev, method);
> +            }
>              if (bsel) {
>                  build_append_pcihp_notify_entry(notify_method, slot);
>              }
> @@ -553,7 +563,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,
> +                                         pcihup_bridge_en);
>          }
>          /* slot descriptor has been composed, add it into parent context */
>          aml_append(parent_scope, dev);
> @@ -2196,7 +2207,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->pcihup_bridge_en);
>  
>              if (TPM_IS_TIS_ISA(tpm)) {
>                  if (misc->tpm_version == TPM_VERSION_2_0) {