Re: [PATCH 9/9] piix4: don't reserve hw resources when hotplug is off globally

[Ani Sinha]

On Mon, Sep 14, 2020 at 6:40 PM Ani Sinha <ani@anisinha.ca> wrote:
>
> On Mon, Sep 14, 2020 at 6:36 PM Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > On Fri, 11 Sep 2020 23:37:55 +0530
> > Ani Sinha <ani@anisinha.ca> wrote:
> >
> > > When acpi hotplug is turned off for both root pci bus as well as for pci
> > > bridges, we should not generate the related amls for DSDT table or 
> > > initialize
> > > related hw ports or reserve hw resources. This change makes sure all those
> > > operations are turned off in the case acpi pci hotplug is off globally.
> >
> > it still leaves around pure PCI hotplug ACPI code:
> >
> >             Method (PCNT, 0, NotSerialized)
> >             {
> >             }
>
> How do you suggest we fix this?

One way to fix this would be to do this:

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 7e3cf3b57b..2bd0c37143 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -483,7 +483,8 @@ static void build_append_pci_bus_devices(Aml
*parent_scope, PCIBus *bus,
             aml_append(method, aml_name("^S%.02X.PCNT", devfn));
         }
     }
-    aml_append(parent_scope, method);
+    if (bsel)
+        aml_append(parent_scope, method);
     qobject_unref(bsel);
 }

This means that if the hotplug on the root bus is disabled, we also
disable PCNT. This will also need the unit test update as the unit
test
will fail with :

@@ -3113,24 +3113,20 @@
                 Name (_ADR, 0x00010000)  // _ADR: Address
                 Method (_S1D, 0, NotSerialized)  // _S1D: S1 Device State
                 {
                     Return (Zero)
                 }

                 Method (_S2D, 0, NotSerialized)  // _S2D: S2 Device State
                 {
                     Return (Zero)
                 }

                 Method (_S3D, 0, NotSerialized)  // _S3D: S3 Device State
                 {
                     Return (Zero)
                 }
             }
-
-            Method (PCNT, 0, NotSerialized)
-            {
-            }
         }
     }
 }

Let's fix this as a separate patch.

>
> > >
> > > Signed-off-by: Ani Sinha <ani@anisinha.ca>
> > > ---
> > >  hw/acpi/piix4.c      |  6 ++++--
> > >  hw/i386/acpi-build.c | 10 ++++++++--
> > >  2 files changed, 12 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> > > index e6163bb6ce..b70b1f98af 100644
> > > --- a/hw/acpi/piix4.c
> > > +++ b/hw/acpi/piix4.c
> > > @@ -596,8 +596,10 @@ 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);
> > > +    }
> > >
> > >      s->cpu_hotplug_legacy = true;
> > >      object_property_add_bool(OBJECT(s), "cpu-hotplug-legacy",
> > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > index e079b686f5..7e3cf3b57b 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)
> > > @@ -1504,7 +1508,9 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> > >          build_hpet_aml(dsdt);
> > >          build_piix4_isa_bridge(dsdt);
> > >          build_isa_devices_aml(dsdt);
> > > -        build_piix4_pci_hotplug(dsdt);
> > > +        if (pm->pcihp_bridge_en || pm->pcihp_root_en) {
> > > +            build_piix4_pci_hotplug(dsdt);
> > > +        }
> > >          build_piix4_pci0_int(dsdt);
> > >      } else {
> > >          sb_scope = aml_scope("_SB");
> > > @@ -1698,7 +1704,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> > >      crs_range_set_free(&crs_range_set);
> > >
> > >      /* reserve PCIHP resources */
> > > -    if (pm->pcihp_io_len) {
> > > +    if (pm->pcihp_io_len && (pm->pcihp_bridge_en || pm->pcihp_root_en)) {
> > >          dev = aml_device("PHPR");
> > >          aml_append(dev, aml_name_decl("_HID", aml_string("PNP0A06")));
> > >          aml_append(dev,
> >