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

[Michael S. Tsirkin]

On Tue, Apr 28, 2020 at 10:10:18PM +0530, Ani Sinha wrote:
> 
> 
> On Tue, Apr 28, 2020 at 9:51 PM Michael S. Tsirkin <address@hidden> wrote:
> 
>     On Tue, Apr 28, 2020 at 09:39:16PM +0530, Ani Sinha wrote:
>     >
>     > Ani
>     > On Apr 28, 2020, 21:35 +0530, Michael S. Tsirkin <address@hidden>, 
> wrote:
>     >
>     >     On Tue, Apr 28, 2020 at 10:16:52AM +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.
>     >
>     >         It has been tested on Windows guests.
>     >
>     >         Signed-off-by: Ani Sinha <address@hidden>
>     >
>     >
>     >     It's still a non starter until we find something similar for PCIE 
> and
>     >     SHPC. Do guests check command status? Can some unplug commands fail?
>     >
>     >
>     > Ok I  give up! I thought we debated this on the other thread.
> 
>     Sorry to hear that.
>     I'd rather you didn't, and worked on a solution that works for everyone.
> 
> 
> That is extremely hard for one person to do, without inputs and ideas from the
> community.

What kind of input are you looking for?

> Satisfying the entire world requires lot of time and energy
> investment, not to mention a broad expertise in multiple technologies. 
> 
> 
> 
>     Pushing back on merging code is unfortunately the only mechanism
>     maintainers have to make sure features are complete and
>     orthogonal to each other, so I'm not sure I can help otherwise.
> 
>     >
>     >
>     >
>     >
>     >         ---
>     >         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)) {
>     >         + 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) {
>     >         --
>     >         1.9.4
>     >
>     >
>     >
> 
>