Re: [PATCH V6] Introduce a new flag for i440fx to disable PCI hotplug on the root bus

[Igor Mammedov <imammedo@redhat.com>]

On Fri, 21 Aug 2020 16:43:37 +0530
Ani Sinha <ani@anisinha.ca> wrote:

> On Fri, Aug 21, 2020 at 2:46 PM Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > On Thu, 20 Aug 2020 22:11:41 +0530
> > Ani Sinha <ani@anisinha.ca> wrote:
> >  
> > > > On Aug 20, 2020, at 9:11 PM, Ani Sinha <ani@anisinha.ca> wrote:
> > > >
> > > > On Thu, Aug 20, 2020 at 7:37 PM Igor Mammedov <imammedo@redhat.com> wrote:  
> > > >>  
> > > >>> On Thu, 20 Aug 2020 14:51:56 +0530
> > > >>> Ani Sinha <ani@anisinha.ca> wrote:
> > > >>>
> > > >>> We introduce a new global flag 'acpi-root-pci-hotplug' for i440fx with which
> > > >>> we can turn on or off PCI device hotplug on the root bus. This flag can be
> > > >>> used to prevent all PCI devices from getting hotplugged or unplugged from the
> > > >>> root PCI bus.
> > > >>> This feature is targetted mostly towards Windows VMs. It is useful in cases
> > > >>> where some hypervisor admins want to deploy guest VMs in a way so that the
> > > >>> users of the guest OSes are not able to hot-eject certain PCI devices from
> > > >>> the Windows system tray. Laine has explained the use case here in detail:
> > > >>> https://www.redhat.com/archives/libvir-list/2020-February/msg00110.html
> > > >>>
> > > >>> Julia has resolved this issue for PCIE buses with the following commit:
> > > >>> 530a0963184e57e71a5b538 ("pcie_root_port: Add hotplug disabling option")
> > > >>>
> > > >>> This commit attempts to introduce similar behavior for PCI root buses used in
> > > >>> i440fx machine types (although in this case, we do not have a per-slot
> > > >>> capability to turn hotplug on or off).
> > > >>>
> > > >>> Usage:
> > > >>>   -global PIIX4_PM.acpi-root-pci-hotplug=off
> > > >>>
> > > >>> By default, this option is enabled which means that hotplug is turned on for
> > > >>> the PCI root bus.
> > > >>>
> > > >>> The previously existing flag 'acpi-pci-hotplug-with-bridge-support' for PCI-PCI
> > > >>> bridges remain as is and can be used along with this new flag to control PCI
> > > >>> hotplug on PCI bridges.
> > > >>>
> > > >>> This change has been tested using a Windows 2012R2 server guest image and also
> > > >>> with a Windows 2019 server guest image on a Ubuntu 18.04 host using the latest
> > > >>> master qemu from upstream (v5.1.0 tag).
> > > >>>
> > > >>> Signed-off-by: Ani Sinha <ani@anisinha.ca>
> > > >>> ---
> > > >>> hw/acpi/piix4.c      |  8 ++++++--
> > > >>> hw/i386/acpi-build.c | 26 +++++++++++++++++++-------
> > > >>> 2 files changed, 25 insertions(+), 9 deletions(-)
> > > >>>
> > > >>> Change Log:
> > > >>> V5..V6: specified upstream master tag information off which this patch is
> > > >>> based off of.
> > > >>>
> > > >>> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> > > >>> index 26bac4f16c..4f436e5bf3 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_hotplug_bridge;
> > > >>> +    bool use_acpi_root_pci_hotplug;
> > > >>>
> > > >>>     uint8_t disable_s3;
> > > >>>     uint8_t disable_s4;  
> > > >>  
> > > >>> @@ -595,8 +596,9 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
> > > >>>                           "acpi-gpe0", GPE_LEN);
> > > >>>     memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe);
> > > >>>
> > > >>> -    acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus, parent,
> > > >>> -                    s->use_acpi_hotplug_bridge);
> > > >>> +    if (s->use_acpi_hotplug_bridge || s->use_acpi_root_pci_hotplug)
> > > >>> +        acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus, parent,
> > > >>> +                        s->use_acpi_hotplug_bridge);  
> > > >> If intent was to disable hardware part of ACPI hotplug,
> > > >> then this hunk is not enough. I'd say it introduces bug since you are leaving
> > > >> device_add/del route open and "_E01" AML code around trying to access no longer
> > > >> described/present io ports.
> > > >>
> > > >> Without this hunk patch is fine, as a means to hide hotplug from Windows.
> > > >>
> > > >> If you'd like to disable hw part, you will need to consider case where hotplug is
> > > >> disabled completly and block all related AML and block device_add|del.
> > > >> So it would be a bit more than above hunk.  
> > > >
> > > > Ok maybe I will just remove it.
> > > >  
> > > >>
> > > >>  
> > > >>>     s->cpu_hotplug_legacy = true;
> > > >>>     object_property_add_bool(OBJECT(s), "cpu-hotplug-legacy",
> > > >>> @@ -635,6 +637,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_hotplug_bridge, true),
> > > >>> +    DEFINE_PROP_BOOL("acpi-root-pci-hotplug", PIIX4PMState,
> > > >>> +                     use_acpi_root_pci_hotplug, 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 b7bcbbbb2a..19a1702ad1 100644
> > > >>> --- a/hw/i386/acpi-build.c
> > > >>> +++ b/hw/i386/acpi-build.c
> > > >>> @@ -95,6 +95,7 @@ typedef struct AcpiPmInfo {
> > > >>>     bool s3_disabled;
> > > >>>     bool s4_disabled;
> > > >>>     bool pcihp_bridge_en;
> > > >>> +    bool pcihp_root_en;
> > > >>>     uint8_t s4_val;
> > > >>>     AcpiFadtData fadt;
> > > >>>     uint16_t cpu_hp_io_base;
> > > >>> @@ -245,6 +246,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->pcihp_root_en =
> > > >>> +        object_property_get_bool(obj, "acpi-root-pci-hotplug", NULL);
> > > >>> +
> > > >>> }
> > > >>>
> > > >>> static void acpi_get_misc_info(AcpiMiscInfo *info)
> > > >>> @@ -337,15 +341,18 @@ 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 pcihp_root_en)
> > > >>> {
> > > >>>     Aml *dev, *notify_method = NULL, *method;
> > > >>>     QObject *bsel;
> > > >>>     PCIBus *sec;
> > > >>>     int i;
> > > >>> +    bool root_bus = pci_bus_is_root(bus);
> > > >>> +    bool root_pcihp_disabled = (root_bus && !pcihp_root_en);
> > > >>>
> > > >>>     bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, NULL);
> > > >>> -    if (bsel) {
> > > >>> +    if (bsel && !root_pcihp_disabled) {  
> > > >>
> > > >> have you considered to make bus not hotpluggable,
> > > >> which should make it not to have bsel, and hence skip this branch without
> > > >> root_pcihp_disabled?
> > > >>
> > > >> see: acpi_set_bsel()
> > > >>
> > > >>
> > > >> maybe you can drop pcihp_root_en and use bsel instead of it then.
> > > >>
> > > >> it also should block device_add/del route.  
> > > >
> > > > This is a good idea. Therefore, I tried this:
> > > >
> > > > --- a/hw/acpi/piix4.c
> > > > +++ b/hw/acpi/piix4.c
> > > > @@ -28,6 +28,7 @@
> > > > #include "hw/pci/pci.h"
> > > > #include "hw/qdev-properties.h"
> > > > #include "hw/acpi/acpi.h"
> > > > +#include "hw/pci/pci_bus.h"
> > > > #include "sysemu/runstate.h"
> > > > #include "sysemu/sysemu.h"
> > > > #include "sysemu/xen.h"
> > > > @@ -507,7 +508,9 @@ 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));
> > > > +    if (s->use_acpi_root_pci_hotplug ||
> > > > +        !pci_bus_is_root(pci_get_bus(dev)))
> > > > +        qbus_set_hotplug_handler(BUS(pci_get_bus(dev)), OBJECT(s));
> > > >
> > > >     piix4_pm_add_propeties(s);
> > > > }
> > > >
> > > >
> > > > but this does not seem to be working. I am out of ideas as to why it
> > > > wouldn't work :(  
> > >
> > > By that I mean the devices attached to bridges also seems to be disabled from hotplug from within Windows. I do not understand why that would be the case when the pci_bus_is_root() should be false for the secondary buses.
> > >
> > >  Any ideas?  
> > Looking at acpi_set_pci_info() and debugging it might help.  
> 
> All my efforts in this space have failed to work :( I am feeling a
> little frustrated and out of ideas. I am inclined to resend V2 with
> the suggestions you suggested for the final patch unless you have
> better ideas.

That's why I've suggest to fix up this patch first, and work on hw part separately.

As for ideas, unfortunatly other than debugging it to see where it breaks,
I don't have any other suggestions.
(though that would require a bit more code studying about how ACPI PCI
hotplug works. If You have concrete questions about the code,
I might be able to clarify it for you.)


> >  
> > >  
> > > >  
> > > >>
> > > >>  
> > > >>>         uint64_t bsel_val = qnum_get_uint(qobject_to(QNum, bsel));
> > > >>>
> > > >>>         aml_append(parent_scope, aml_name_decl("BSEL", aml_int(bsel_val)));
> > > >>> @@ -361,6 +368,9 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
> > > >>>         bool bridge_in_acpi;
> > > >>>
> > > >>>         if (!pdev) {
> > > >>> +            /* skip if pci hotplug for the root bus is disabled */
> > > >>> +            if (root_pcihp_disabled)
> > > >>> +                continue;
> > > >>>             if (bsel) { /* add hotplug slots for non present devices */
> > > >>>                 dev = aml_device("S%.02X", PCI_DEVFN(slot, 0));
> > > >>>                 aml_append(dev, aml_name_decl("_SUN", aml_int(slot)));
> > > >>> @@ -419,7 +429,7 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
> > > >>>             method = aml_method("_S3D", 0, AML_NOTSERIALIZED);
> > > >>>             aml_append(method, aml_return(aml_int(s3d)));
> > > >>>             aml_append(dev, method);
> > > >>> -        } else if (hotplug_enabled_dev) {
> > > >>> +        } else if (hotplug_enabled_dev && !root_pcihp_disabled) {
> > > >>>             /* add _SUN/_EJ0 to make slot hotpluggable  */
> > > >>>             aml_append(dev, aml_name_decl("_SUN", aml_int(slot)));
> > > >>>
> > > >>> @@ -439,13 +449,14 @@ 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,
> > > >>> +                                         pcihp_root_en);
> > > >>>         }
> > > >>>         /* slot descriptor has been composed, add it into parent context */
> > > >>>         aml_append(parent_scope, dev);
> > > >>>     }
> > > >>>
> > > >>> -    if (bsel) {
> > > >>> +    if (bsel && !root_pcihp_disabled) {
> > > >>>         aml_append(parent_scope, notify_method);
> > > >>>     }
> > > >>>
> > > >>> @@ -455,7 +466,7 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
> > > >>>     method = aml_method("PCNT", 0, AML_NOTSERIALIZED);
> > > >>>
> > > >>>     /* If bus supports hotplug select it and notify about local events */
> > > >>> -    if (bsel) {
> > > >>> +    if (bsel && !root_pcihp_disabled) {
> > > >>>         uint64_t bsel_val = qnum_get_uint(qobject_to(QNum, bsel));
> > > >>>
> > > >>>         aml_append(method, aml_store(aml_int(bsel_val), aml_name("BNUM")));
> > > >>> @@ -1818,7 +1829,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->pcihp_root_en);
> > > >>>
> > > >>>             if (TPM_IS_TIS_ISA(tpm)) {
> > > >>>                 if (misc->tpm_version == TPM_VERSION_2_0) {  
> > > >>  
> > >  
> >  
>