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

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

[Ani Sinha]

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.

The patch is initial version and is a rough implementation. 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 d706360..dad1bf4 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -82,6 +82,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;
@@ -676,6 +677,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 2bc8117..526feb2 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -94,6 +94,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;
@@ -220,6 +221,9 @@ static void acpi_get_pm_info(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)
@@ -430,7 +434,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;
@@ -458,11 +463,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)) {
+                    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);
@@ -516,12 +524,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);
             }
@@ -532,7 +542,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);
@@ -2133,7 +2144,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(tpm_find())) {
                 dev = aml_device("ISA.TPM");
-- 
1.9.4

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

[Michael S. Tsirkin]

On Fri, Apr 17, 2020 at 03:13:31PM +0000, Ani Sinha 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.
> 
> The patch is initial version and is a rough implementation. It has
> been tested on Windows guests.
> 
> Signed-off-by: Ani Sinha <ani.sinha@nutanix.com>

Is there a real reason to do this? Can't we just limit the
hotplug control to pcie ports? At some point I'd like us to
start leaving piix alone...


> ---
>  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 d706360..dad1bf4 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -82,6 +82,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;
> @@ -676,6 +677,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 2bc8117..526feb2 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -94,6 +94,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;
> @@ -220,6 +221,9 @@ static void acpi_get_pm_info(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)
> @@ -430,7 +434,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;
> @@ -458,11 +463,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)) {
> +                    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);
> @@ -516,12 +524,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);
>              }
> @@ -532,7 +542,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);
> @@ -2133,7 +2144,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(tpm_find())) {
>                  dev = aml_device("ISA.TPM");
> -- 
> 1.9.4

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

[Ani Sinha]

> On Apr 17, 2020, at 8:57 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> Is there a real reason to do this? Can't we just limit the
> hotplug control to pcie ports? At some point I'd like us to
> start leaving piix alone..

Yes we really need this feature as want to be able to hot plug devices into the guest but prevent customers from hot unplugging them from say Windows system tray.

ani

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

[Michael S. Tsirkin]

On Fri, Apr 17, 2020 at 03:36:14PM +0000, Ani Sinha wrote:
> 
> 
> > On Apr 17, 2020, at 8:57 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > 
> > Is there a real reason to do this? Can't we just limit the
> > hotplug control to pcie ports? At some point I'd like us to
> > start leaving piix alone..
> 
> Yes we really need this feature as want to be able to hot plug devices into the guest but prevent customers from hot unplugging them from say Windows system tray.
> 
> ani

Problem is, I think this is not something we can support with pcie or shpc.
I'm reluctant to add features that only ACPI can support,
we are trying to phase that out.
Better ideas?

-- 
MST

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

[Ani Sinha]

+Laine

> On Apr 17, 2020, at 9:39 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> Problem is, I think this is not something we can support with pcie or shpc.
> I'm reluctant to add features that only ACPI can support,
> we are trying to phase that out.

Hmm. I see. We use conventional PCI and hence was looking for providing this feature for conventional PCI only. Laine might be able to throw some lights as to feasibility of the in PCIE world.

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

[Laine Stump]

On 4/17/20 12:35 PM, Ani Sinha wrote:
> +Laine
> 
>> On Apr 17, 2020, at 9:39 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>
>> Problem is, I think this is not something we can support with pcie or shpc.
>> I'm reluctant to add features that only ACPI can support,
>> we are trying to phase that out.
> 
> Hmm. I see. We use conventional PCI and hence was looking for providing this feature for conventional PCI only. Laine might be able to throw some lights as to feasibility of the in PCIE world.
> 

Sorry, my knowledge doesn't go that low. If there's a qemu option I can 
expose it in libvirt, but am by no means an expert of qemu internals or 
the pci/pcie specs :-)

(BTW, I think in the past people have prevented enabling hot-unplug by 
unprivileged users in Windows with some sort of a "system policy" in 
Windows. (whatever that is - I don't use Windows, and have just heard 
this from others when discussing the problem).

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

[Ani Sinha]

+Julia who implemented https://patchwork.kernel.org/patch/11388881/


> On Apr 18, 2020, at 3:26 AM, Laine Stump <laine@redhat.com> wrote:
> 
> On 4/17/20 12:35 PM, Ani Sinha wrote:
>> +Laine
>>> On Apr 17, 2020, at 9:39 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>> 
>>> Problem is, I think this is not something we can support with pcie or shpc.
>>> I'm reluctant to add features that only ACPI can support,
>>> we are trying to phase that out.
>> Hmm. I see. We use conventional PCI and hence was looking for providing this feature for conventional PCI only. Laine might be able to throw some lights as to feasibility of the in PCIE world.
> 
> Sorry, my knowledge doesn't go that low. If there's a qemu option I can expose it in libvirt, but am by no means an expert of qemu internals or the pci/pcie specs :-)
> 
> (BTW, I think in the past people have prevented enabling hot-unplug by unprivileged users in Windows with some sort of a "system policy" in Windows. (whatever that is - I don't use Windows, and have just heard this from others when discussing the problem).
> 

My question to Julia is whether it is possible to disable just hot unplug but keep hot plugging for PCIE devices enabled using PCI_EXP_SLTCAP_HPC  or otherwise in Qemu?

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

[Marcel Apfelbaum]

Hi Ani,

On 4/18/20 6:25 AM, Ani Sinha wrote:
> +Julia who implemented https://patchwork.kernel.org/patch/11388881/
>
>
>> On Apr 18, 2020, at 3:26 AM, Laine Stump <laine@redhat.com> wrote:
>>
>> On 4/17/20 12:35 PM, Ani Sinha wrote:
>>> +Laine
>>>> On Apr 17, 2020, at 9:39 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>
>>>> Problem is, I think this is not something we can support with pcie or shpc.
>>>> I'm reluctant to add features that only ACPI can support,
>>>> we are trying to phase that out.
>>> Hmm. I see. We use conventional PCI and hence was looking for providing this feature for conventional PCI only. Laine might be able to throw some lights as to feasibility of the in PCIE world.
>> Sorry, my knowledge doesn't go that low. If there's a qemu option I can expose it in libvirt, but am by no means an expert of qemu internals or the pci/pcie specs :-)
>>
>> (BTW, I think in the past people have prevented enabling hot-unplug by unprivileged users in Windows with some sort of a "system policy" in Windows. (whatever that is - I don't use Windows, and have just heard this from others when discussing the problem).
>>

A PCIe Root Port or a PCI slot can or cannot support hot-plugging. 
Anything in the middle can't be done at PCIe/PCI level (as far as I know).
I think the answer can be at the modelling level. Use non hot-pluggable 
slots (or PCIe Root Ports without hot-plug support) for the devices
you don't want hot-unplugged, leave an empty PCI Bridge (or some PCIe 
Root Ports with hot-plug support) to be able to hot-plug devices.

Thanks,
Marcel

> My question to Julia is whether it is possible to disable just hot unplug but keep hot plugging for PCIE devices enabled using PCI_EXP_SLTCAP_HPC  or otherwise in Qemu?
>
>

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

[Ani Sinha]

> On Apr 18, 2020, at 6:18 PM, Marcel Apfelbaum <marcel.apfelbaum@gmail.com> wrote:
> 
> Hi Ani,
> 
> On 4/18/20 6:25 AM, Ani Sinha wrote:
>> +Julia who implemented https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.org_patch_11388881_&d=DwICaQ&c=s883GpUCOChKOHiocYtGcg&r=IIUxIyRwG4RGy57y2nvMNYcDkqW-NHozZ2R38VYcg5U&m=cpjHoQeyltZfRWa7a5aUuxM_5-x6_YZZCGb4UixhviE&s=bEc8FsThqIa9W1pzyh3VuOCICwTWwZikECD19MOmttg&e= 
>> 
>>> On Apr 18, 2020, at 3:26 AM, Laine Stump <laine@redhat.com> wrote:
>>> 
>>> On 4/17/20 12:35 PM, Ani Sinha wrote:
>>>> +Laine
>>>>> On Apr 17, 2020, at 9:39 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>> 
>>>>> Problem is, I think this is not something we can support with pcie or shpc.
>>>>> I'm reluctant to add features that only ACPI can support,
>>>>> we are trying to phase that out.
>>>> Hmm. I see. We use conventional PCI and hence was looking for providing this feature for conventional PCI only. Laine might be able to throw some lights as to feasibility of the in PCIE world.
>>> Sorry, my knowledge doesn't go that low. If there's a qemu option I can expose it in libvirt, but am by no means an expert of qemu internals or the pci/pcie specs :-)
>>> 
>>> (BTW, I think in the past people have prevented enabling hot-unplug by unprivileged users in Windows with some sort of a "system policy" in Windows. (whatever that is - I don't use Windows, and have just heard this from others when discussing the problem).
>>> 
> 
> A PCIe Root Port or a PCI slot can or cannot support hot-plugging. Anything in the middle can't be done at PCIe/PCI level (as far as I know).
> I think the answer can be at the modelling level. Use non hot-pluggable slots (or PCIe Root Ports without hot-plug support) for the devices
> you don't want hot-unplugged, leave an empty PCI Bridge (or some PCIe Root Ports with hot-plug support) to be able to hot-plug devices.

Yes, this is exactly the modeling I was going with to prevent users from within Windows guests to hot-unplug a device. Then I realized that if we plug in a device into a slot where hot plugging is disabled with an intention to disable hot unplugging, we can’t even hot plug a device into that slot. When we plug in a device into a slot where hot plugging is enabled, both plugging in and unplugging is possible. In other words, the behavior is symmetric in terms of both plugging and unplugging. Hence for conventional PCI I cooked up this patch where I disable “_EJ0” ACPI functions from pci-pci bridge to order to enable hot plugging but prevent hot unplugging into the slots of that bridge. I tested this on Windows guests and it seems to work. So my next question is, for PCIE, is there something equivalent that can be done? My knowledge is limited in that space.

thanks
ani

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

[Daniel P. Berrangé]

On Fri, Apr 17, 2020 at 12:09:00PM -0400, Michael S. Tsirkin wrote:
> On Fri, Apr 17, 2020 at 03:36:14PM +0000, Ani Sinha wrote:
> > 
> > 
> > > On Apr 17, 2020, at 8:57 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > > 
> > > Is there a real reason to do this? Can't we just limit the
> > > hotplug control to pcie ports? At some point I'd like us to
> > > start leaving piix alone..
> > 
> > Yes we really need this feature as want to be able to hot plug devices into the guest but prevent customers from hot unplugging them from say Windows system tray.
> > 
> > ani
> 
> Problem is, I think this is not something we can support with pcie or shpc.
> I'm reluctant to add features that only ACPI can support,
> we are trying to phase that out.

From the upstream POV, there's been no decision / agreement to phase
out PIIX, this is purely a RHEL downstream decision & plan. If other
distros / users have a different POV, and find the feature useful, we
should accept the patch if it meets the normal QEMU patch requirements.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Ani Sinha]

> On Apr 20, 2020, at 2:54 PM, Daniel P. Berrangé <berrange@redhat.com> wrote:
> 
> From the upstream POV, there's been no decision / agreement to phase
> out PIIX, this is purely a RHEL downstream decision & plan. If other
> distros / users have a different POV, and find the feature useful, we
> should accept the patch if it meets the normal QEMU patch requirements.

Excellent. I will work with anyone who would want to review this patch. Also I just realized this patch is based off Qemu 2.12. So I will send another patch after rebasing it with the latest master. Meanwhile, let the feedbacks keep flowing …

ani

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

[Philippe Mathieu-Daudé]

On 4/20/20 11:24 AM, Daniel P. Berrangé wrote:
> On Fri, Apr 17, 2020 at 12:09:00PM -0400, Michael S. Tsirkin wrote:
>> On Fri, Apr 17, 2020 at 03:36:14PM +0000, Ani Sinha wrote:
>>>
>>>
>>>> On Apr 17, 2020, at 8:57 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>
>>>> Is there a real reason to do this? Can't we just limit the
>>>> hotplug control to pcie ports? At some point I'd like us to
>>>> start leaving piix alone..
>>>
>>> Yes we really need this feature as want to be able to hot plug devices into the guest but prevent customers from hot unplugging them from say Windows system tray.
>>>
>>> ani
>>
>> Problem is, I think this is not something we can support with pcie or shpc.
>> I'm reluctant to add features that only ACPI can support,
>> we are trying to phase that out.
> 
>  From the upstream POV, there's been no decision / agreement to phase
> out PIIX, this is purely a RHEL downstream decision & plan. If other
> distros / users have a different POV, and find the feature useful, we
> should accept the patch if it meets the normal QEMU patch requirements.

Remotely related, this thread suggest to rename the current Frankenstein 
PIIX as 'virt_southbridge' /s
https://www.mail-archive.com/qemu-devel@nongnu.org/msg691232.html

Seriously, apparently thinking to modify PIIX scares various people.
Is there a virt equivalent that could work?
On one hand people want to use Windows guest unmodified (or with 
digitally signed drivers, which seems impossible to do with virtio 
package [*]). OTOH we are not sure how the PIIX model works because it 
is not based on specs, and does not match its datasheet, and do not want 
to modify it.

[*] If you click on 'How to Release-Sign File System Drivers' on
https://www.linux-kvm.org/page/WindowsGuestDrivers/Download_Drivers
it opens https://www.microsoft.com/whdc/driver/tips/IFS_Signing.mspx
and displays "Your request has been blocked."...

> 
> Regards,
> Daniel
> 

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

[Michael S. Tsirkin]

On Mon, Apr 20, 2020 at 10:24:59AM +0100, Daniel P. Berrangé wrote:
> On Fri, Apr 17, 2020 at 12:09:00PM -0400, Michael S. Tsirkin wrote:
> > On Fri, Apr 17, 2020 at 03:36:14PM +0000, Ani Sinha wrote:
> > > 
> > > 
> > > > On Apr 17, 2020, at 8:57 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > 
> > > > Is there a real reason to do this? Can't we just limit the
> > > > hotplug control to pcie ports? At some point I'd like us to
> > > > start leaving piix alone..
> > > 
> > > Yes we really need this feature as want to be able to hot plug devices into the guest but prevent customers from hot unplugging them from say Windows system tray.
> > > 
> > > ani
> > 
> > Problem is, I think this is not something we can support with pcie or shpc.
> > I'm reluctant to add features that only ACPI can support,
> > we are trying to phase that out.
> 
> >From the upstream POV, there's been no decision / agreement to phase
> out PIIX,

Phase out now.  But I for one would like to focus on keeping PIIX stable
and focus development on q35.  Not bloating PIIX with lots of new
features is IMHO a good way to do that.



> this is purely a RHEL downstream decision & plan. If other
> distros / users have a different POV, and find the feature useful, we
> should accept the patch if it meets the normal QEMU patch requirements.
> 
> Regards,
> Daniel

Orthogonality of features is important. It is tough to navigate our
feature matrix as it is. Figuring things out if random features
depend on other random features becomes impossible.


> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Ani Sinha]

> On Apr 18, 2020, at 6:18 PM, Marcel Apfelbaum <marcel.apfelbaum@gmail.com> wrote:
> 
> A PCIe Root Port or a PCI slot can or cannot support hot-plugging. Anything in the middle can't be done at PCIe/PCI level (as far as I know).

Is it possible to dynamically set PCI_EXP_SLTCAP_HPC for a slot at runtime?

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

[Ani Sinha]

> On Apr 20, 2020, at 8:34 PM, Ani Sinha <ani.sinha@nutanix.com> wrote:
> 
> 
> 
>> On Apr 18, 2020, at 6:18 PM, Marcel Apfelbaum <marcel.apfelbaum@gmail.com> wrote:
>> 
>> A PCIe Root Port or a PCI slot can or cannot support hot-plugging. Anything in the middle can't be done at PCIe/PCI level (as far as I know).
> 
> Is it possible to dynamically set PCI_EXP_SLTCAP_HPC for a slot at runtime?

What I mean is, when a slot capability is set an initialization, is it set in stone or can the capabilities be revoked post initialization? I can ’t find anything related to this in PCIE spec.

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

[Julia Suvorova]

On Mon, Apr 20, 2020 at 5:38 PM Ani Sinha <ani.sinha@nutanix.com> wrote:
>
>
>
> > On Apr 20, 2020, at 8:34 PM, Ani Sinha <ani.sinha@nutanix.com> wrote:
> >
> >
> >
> >> On Apr 18, 2020, at 6:18 PM, Marcel Apfelbaum <marcel.apfelbaum@gmail.com> wrote:
> >>
> >> A PCIe Root Port or a PCI slot can or cannot support hot-plugging. Anything in the middle can't be done at PCIe/PCI level (as far as I know).
> >
> > Is it possible to dynamically set PCI_EXP_SLTCAP_HPC for a slot at runtime?
>
> What I mean is, when a slot capability is set an initialization, is it set in stone or can the capabilities be revoked post initialization? I can ’t find anything related to this in PCIE spec.

This bit is hardware initialized. This means that once initialized,
it's fixed and can't be changed. It could be re-initialized after
reset though.

Best regards, Julia Suvorova.

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

[Ani Sinha]

> On Apr 20, 2020, at 8:32 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> But I for one would like to focus on keeping PIIX stable
> and focus development on q35.  Not bloating PIIX with lots of new
> features is IMHO a good way to do that.

Does this mean this patch is a no-go then? :(

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

[Daniel P. Berrangé]

On Tue, Apr 21, 2020 at 02:45:04PM +0000, Ani Sinha wrote:
> 
> 
> > On Apr 20, 2020, at 8:32 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > 
> > But I for one would like to focus on keeping PIIX stable
> > and focus development on q35.  Not bloating PIIX with lots of new
> > features is IMHO a good way to do that.
> 
> Does this mean this patch is a no-go then? :(

I'd support this patch, as I don't think it can really be described as
bloat or destabalizing. It is just adding a simple property to
conditionalize existing functionality.  Telling people to switch to Q35
is unreasonable as it is not a simple 1-1 conversion from existing use
of PIIX. Q35 has much higher complexity in its configuration, has higher
memory overhead per VM too, and lacks certain features of PIIX too.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Ani Sinha]

> On Apr 21, 2020, at 8:32 PM, Daniel P. Berrangé <berrange@redhat.com> wrote:
> 
> On Tue, Apr 21, 2020 at 02:45:04PM +0000, Ani Sinha wrote:
>> 
>> 
>>> On Apr 20, 2020, at 8:32 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>> 
>>> But I for one would like to focus on keeping PIIX stable
>>> and focus development on q35.  Not bloating PIIX with lots of new
>>> features is IMHO a good way to do that.
>> 
>> Does this mean this patch is a no-go then? :(
> 
> I'd support this patch, as I don't think it can really be described as
> bloat or destabalizing. It is just adding a simple property to
> conditionalize existing functionality.  Telling people to switch to Q35
> is unreasonable as it is not a simple 1-1 conversion from existing use
> of PIIX. Q35 has much higher complexity in its configuration, has higher
> memory overhead per VM too, and lacks certain features of PIIX too.

Cool. How do we go forward from here?

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

[Ani Sinha]

> On Apr 22, 2020, at 4:15 PM, Ani Sinha <ani.sinha@nutanix.com> wrote:
> 
> 
> 
>> On Apr 21, 2020, at 8:32 PM, Daniel P. Berrangé <berrange@redhat.com> wrote:
>> 
>> On Tue, Apr 21, 2020 at 02:45:04PM +0000, Ani Sinha wrote:
>>> 
>>> 
>>>> On Apr 20, 2020, at 8:32 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>> 
>>>> But I for one would like to focus on keeping PIIX stable
>>>> and focus development on q35.  Not bloating PIIX with lots of new
>>>> features is IMHO a good way to do that.
>>> 
>>> Does this mean this patch is a no-go then? :(
>> 
>> I'd support this patch, as I don't think it can really be described as
>> bloat or destabalizing. It is just adding a simple property to
>> conditionalize existing functionality.  Telling people to switch to Q35
>> is unreasonable as it is not a simple 1-1 conversion from existing use
>> of PIIX. Q35 has much higher complexity in its configuration, has higher
>> memory overhead per VM too, and lacks certain features of PIIX too.
> 
> Cool. How do we go forward from here?
> 

We would really appreciate if we can add this extra knob in Qemu. Maybe someone else also in the community will find this useful. We don’t want to maintain this patch internally forever but rather prefer we maintain this as a Qemu community.

ani

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

[Eduardo Habkost]

On Fri, Apr 24, 2020 at 03:23:56PM +0000, Ani Sinha wrote:
> 
> 
> > On Apr 22, 2020, at 4:15 PM, Ani Sinha <ani.sinha@nutanix.com> wrote:
> > 
> > 
> > 
> >> On Apr 21, 2020, at 8:32 PM, Daniel P. Berrangé <berrange@redhat.com> wrote:
> >> 
> >> On Tue, Apr 21, 2020 at 02:45:04PM +0000, Ani Sinha wrote:
> >>> 
> >>> 
> >>>> On Apr 20, 2020, at 8:32 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >>>> 
> >>>> But I for one would like to focus on keeping PIIX stable
> >>>> and focus development on q35.  Not bloating PIIX with lots of new
> >>>> features is IMHO a good way to do that.
> >>> 
> >>> Does this mean this patch is a no-go then? :(
> >> 
> >> I'd support this patch, as I don't think it can really be described as
> >> bloat or destabalizing. It is just adding a simple property to
> >> conditionalize existing functionality.  Telling people to switch to Q35
> >> is unreasonable as it is not a simple 1-1 conversion from existing use
> >> of PIIX. Q35 has much higher complexity in its configuration, has higher
> >> memory overhead per VM too, and lacks certain features of PIIX too.
> > 
> > Cool. How do we go forward from here?
> > 
> 
> We would really appreciate if we can add this extra knob in
> Qemu. Maybe someone else also in the community will find this
> useful. We don’t want to maintain this patch internally forever
> but rather prefer we maintain this as a Qemu community.

Michael, I agree with Daniel here and I don't think we should
start refusing PIIX features if they are useful for a portion of
the QEMU community.

Would you reconsider and merge this patch?

-- 
Eduardo

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

[Ani Sinha]

> On Apr 25, 2020, at 12:14 AM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> On Fri, Apr 24, 2020 at 03:23:56PM +0000, Ani Sinha wrote:
>> 
>> 
>>> On Apr 22, 2020, at 4:15 PM, Ani Sinha <ani.sinha@nutanix.com> wrote:
>>> 
>>> 
>>> 
>>>> On Apr 21, 2020, at 8:32 PM, Daniel P. Berrangé <berrange@redhat.com> wrote:
>>>> 
>>>> On Tue, Apr 21, 2020 at 02:45:04PM +0000, Ani Sinha wrote:
>>>>> 
>>>>> 
>>>>>> On Apr 20, 2020, at 8:32 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>> 
>>>>>> But I for one would like to focus on keeping PIIX stable
>>>>>> and focus development on q35.  Not bloating PIIX with lots of new
>>>>>> features is IMHO a good way to do that.
>>>>> 
>>>>> Does this mean this patch is a no-go then? :(
>>>> 
>>>> I'd support this patch, as I don't think it can really be described as
>>>> bloat or destabalizing. It is just adding a simple property to
>>>> conditionalize existing functionality.  Telling people to switch to Q35
>>>> is unreasonable as it is not a simple 1-1 conversion from existing use
>>>> of PIIX. Q35 has much higher complexity in its configuration, has higher
>>>> memory overhead per VM too, and lacks certain features of PIIX too.
>>> 
>>> Cool. How do we go forward from here?
>>> 
>> 
>> We would really appreciate if we can add this extra knob in
>> Qemu. Maybe someone else also in the community will find this
>> useful. We don’t want to maintain this patch internally forever
>> but rather prefer we maintain this as a Qemu community.
> 
> Michael, I agree with Daniel here and I don't think we should
> start refusing PIIX features if they are useful for a portion of
> the QEMU community.
> 
> Would you reconsider and merge this patch?

Thanks!

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

[Igor Mammedov]

On Fri, 24 Apr 2020 14:44:48 -0400
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Fri, Apr 24, 2020 at 03:23:56PM +0000, Ani Sinha wrote:
> > 
> >   
> > > On Apr 22, 2020, at 4:15 PM, Ani Sinha <ani.sinha@nutanix.com> wrote:
> > > 
> > > 
> > >   
> > >> On Apr 21, 2020, at 8:32 PM, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > >> 
> > >> On Tue, Apr 21, 2020 at 02:45:04PM +0000, Ani Sinha wrote:  
> > >>> 
> > >>>   
> > >>>> On Apr 20, 2020, at 8:32 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > >>>> 
> > >>>> But I for one would like to focus on keeping PIIX stable
> > >>>> and focus development on q35.  Not bloating PIIX with lots of new
> > >>>> features is IMHO a good way to do that.  
> > >>> 
> > >>> Does this mean this patch is a no-go then? :(  
> > >> 
> > >> I'd support this patch, as I don't think it can really be described as
> > >> bloat or destabalizing. It is just adding a simple property to
> > >> conditionalize existing functionality.  Telling people to switch to Q35
> > >> is unreasonable as it is not a simple 1-1 conversion from existing use
> > >> of PIIX. Q35 has much higher complexity in its configuration, has higher
> > >> memory overhead per VM too, and lacks certain features of PIIX too.  
> > > 
> > > Cool. How do we go forward from here?
> > >   
> > 
> > We would really appreciate if we can add this extra knob in
> > Qemu. Maybe someone else also in the community will find this
> > useful. We don’t want to maintain this patch internally forever
> > but rather prefer we maintain this as a Qemu community.  
> 
> Michael, I agree with Daniel here and I don't think we should
> start refusing PIIX features if they are useful for a portion of
> the QEMU community.
> 
> Would you reconsider and merge this patch?

I put this patch on my review queue (hopefully next week I'd be able to get to it)

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

[Ani Sinha]

> On Apr 29, 2020, at 9:02 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> 
> On Fri, 24 Apr 2020 14:44:48 -0400
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
>> On Fri, Apr 24, 2020 at 03:23:56PM +0000, Ani Sinha wrote:
>>> 
>>> 
>>>> On Apr 22, 2020, at 4:15 PM, Ani Sinha <ani.sinha@nutanix.com> wrote:
>>>> 
>>>> 
>>>> 
>>>>> On Apr 21, 2020, at 8:32 PM, Daniel P. Berrangé <berrange@redhat.com> wrote:
>>>>> 
>>>>> On Tue, Apr 21, 2020 at 02:45:04PM +0000, Ani Sinha wrote:  
>>>>>> 
>>>>>> 
>>>>>>> On Apr 20, 2020, at 8:32 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>>> 
>>>>>>> But I for one would like to focus on keeping PIIX stable
>>>>>>> and focus development on q35.  Not bloating PIIX with lots of new
>>>>>>> features is IMHO a good way to do that.  
>>>>>> 
>>>>>> Does this mean this patch is a no-go then? :(  
>>>>> 
>>>>> I'd support this patch, as I don't think it can really be described as
>>>>> bloat or destabalizing. It is just adding a simple property to
>>>>> conditionalize existing functionality.  Telling people to switch to Q35
>>>>> is unreasonable as it is not a simple 1-1 conversion from existing use
>>>>> of PIIX. Q35 has much higher complexity in its configuration, has higher
>>>>> memory overhead per VM too, and lacks certain features of PIIX too.  
>>>> 
>>>> Cool. How do we go forward from here?
>>>> 
>>> 
>>> We would really appreciate if we can add this extra knob in
>>> Qemu. Maybe someone else also in the community will find this
>>> useful. We don’t want to maintain this patch internally forever
>>> but rather prefer we maintain this as a Qemu community.  
>> 
>> Michael, I agree with Daniel here and I don't think we should
>> start refusing PIIX features if they are useful for a portion of
>> the QEMU community.
>> 
>> Would you reconsider and merge this patch?
> 
> I put this patch on my review queue (hopefully next week I'd be able to get to it)

Any progress?

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

[Igor Mammedov]

On Sun, 10 May 2020 17:42:16 +0000
Ani Sinha <ani.sinha@nutanix.com> wrote:

> > On Apr 29, 2020, at 9:02 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> > 
> > On Fri, 24 Apr 2020 14:44:48 -0400
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >   
> >> On Fri, Apr 24, 2020 at 03:23:56PM +0000, Ani Sinha wrote:  
> >>> 
> >>>   
> >>>> On Apr 22, 2020, at 4:15 PM, Ani Sinha <ani.sinha@nutanix.com> wrote:
> >>>> 
> >>>> 
> >>>>   
> >>>>> On Apr 21, 2020, at 8:32 PM, Daniel P. Berrangé <berrange@redhat.com> wrote:
> >>>>> 
> >>>>> On Tue, Apr 21, 2020 at 02:45:04PM +0000, Ani Sinha wrote:    
> >>>>>> 
> >>>>>>   
> >>>>>>> On Apr 20, 2020, at 8:32 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >>>>>>> 
> >>>>>>> But I for one would like to focus on keeping PIIX stable
> >>>>>>> and focus development on q35.  Not bloating PIIX with lots of new
> >>>>>>> features is IMHO a good way to do that.    
> >>>>>> 
> >>>>>> Does this mean this patch is a no-go then? :(    
> >>>>> 
> >>>>> I'd support this patch, as I don't think it can really be described as
> >>>>> bloat or destabalizing. It is just adding a simple property to
> >>>>> conditionalize existing functionality.  Telling people to switch to Q35
> >>>>> is unreasonable as it is not a simple 1-1 conversion from existing use
> >>>>> of PIIX. Q35 has much higher complexity in its configuration, has higher
> >>>>> memory overhead per VM too, and lacks certain features of PIIX too.    
> >>>> 
> >>>> Cool. How do we go forward from here?
> >>>>   
> >>> 
> >>> We would really appreciate if we can add this extra knob in
> >>> Qemu. Maybe someone else also in the community will find this
> >>> useful. We don’t want to maintain this patch internally forever
> >>> but rather prefer we maintain this as a Qemu community.    
> >> 
> >> Michael, I agree with Daniel here and I don't think we should
> >> start refusing PIIX features if they are useful for a portion of
> >> the QEMU community.
> >> 
> >> Would you reconsider and merge this patch?  
> > 
> > I put this patch on my review queue (hopefully next week I'd be able to get to it)  
> 
> Any progress?
> 

see my reply on v2