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, 12 May 2020 05:26:47 +0000
Ani Sinha <ani.sinha@nutanix.com> wrote:

> > On May 12, 2020, at 12:23 AM, Igor Mammedov <imammedo@redhat.com> 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?
Yep, from the very begining I was suggesting to use global hotplug/no hotplug switch.

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

I don't think it's possible to have per slot hotplug on conventional PCI hardware.
it's per bridge property.


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

sounds fine to me, at least it will address problem.
Can you post a patch to that effect please?
/It should disable all AML related to hotplug and related hadware code/

As for a way to make it more felixible, theoretically it should be possible to use SHPC on bridges.
So one could have a bridge with SHPC enabled to allow hotplug devices during runtime.
It's rummored that SHPC should be supported since Vista, but it doesn't work
(at least on QEMU with Windows 10 guest and I don't have a real machine to verify that).
That's something to explore (perhaps QEMU doesn't implement it completely so Windows doesn't see it). 
 
> 
> 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

above is a bug in patch below, look like ,shpc=on tricks code to belive that ACPI hotplug is present
and add AML for it.

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