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

[Ani Sinha]

> On May 12, 2020, at 12:23 AM, Igor Mammedov <address@hidden> wrote:
> 
>> 
>> 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?

Yes, the user will see the eject option from system tray for devices plugged 
into the root bus. The idea is that whereas we disallow some devices from 
hot-unplugging, other devices which are plugged into the root bus can be hot 
plugged and unplugged. This leaves some room for flexibility across devices and 
VMs.

> 
> 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’)

So the main objection against this patch is that with this option enabled, we 
are violating what real HW does and since we want emulated HW to mimic real HW 
behavior as close as possible, we are breaking this assumption. Am I correct?

> 
> which is possible is one puts device on pci bridge without hotplug, i.e.
> 
> -global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=off

right.

> 
> 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.

Right. However, I have tested that even though the PCI bridge shows up as a 
device in the “safely remove HW” option in the system tray, trying to eject a 
PCI bridge with devices attached will result in failure with the error message 
“this device is currently in use”.

> 
> 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

Given that my patch is not acceptable, I’d prefer the following in the order of 
preference:

(a) Have an option to disable hot ejection of PCI-PCI bridge so that Windows 
does not even show this HW in the “safely remove HW” option. If we can do this 
then from OS perspective the GUI options will be same as what is available with 
PCIE/q35 - none of the devices will be hot ejectable if the hot plug option is 
turned off from the PCIE slots where devices are plugged into.
I looked at the code. It seems to manipulate ACPI tables of the empty slots of 
the root bus where no devices are attached (see comment "/* add hotplug slots 
for non present devices */ “). For cold plugged bridges, it recurses down to 
scan the slots of the bridge. Is it possible to disable hot plug for the slot 
to which the bridge is attached?

(b) Failing above, having a global option to disable all hot plug, including 
the 32 slots of the root bus would be good. However, this does not give us the 
flexibility we have with PCIE (that is, to hot plug a  device, we can always 
plug it to a slot with hot plug enabled).


Thanks for looking into my requirement more seriously,
ani


> 
> 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) {