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

[Igor Mammedov]

On Tue, 12 May 2020 05:26:47 +0000
Ani Sinha <address@hidden> wrote:

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