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

[Ani Sinha]

On Fri, Sep 18, 2020 at 11:54 AM Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Thu, 17 Sep 2020 20:30:41 +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 ACPI code 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.
> >
> > In this change, we also make sure ACPI code for the PCNT method are only
> > added when bsel is enabled for the corresponding pci bus or bridge hotplug
> > is turned on.
> >
> > As q35 machines do not use bsel for it's pci buses at this point in time, 
> > this
> > change affects DSDT acpi table for q35 machines as well. Therefore, we will
> > also need to commit the updated golden master DSDT table acpi binary blobs 
> > as
> > well. Follwoing is the list of blobs which needs updating:
> >
> >  tests/data/acpi/q35/DSDT
> >  tests/data/acpi/q35/DSDT.acpihmat
> >  tests/data/acpi/q35/DSDT.bridge
> >  tests/data/acpi/q35/DSDT.cphp
> >  tests/data/acpi/q35/DSDT.dimmpxm
> >  tests/data/acpi/q35/DSDT.ipmibt
> >  tests/data/acpi/q35/DSDT.memhp
> >  tests/data/acpi/q35/DSDT.mmio64
> >  tests/data/acpi/q35/DSDT.numamem
> >  tests/data/acpi/q35/DSDT.tis
> >
> > These tables are updated in the following commit. Without the updated table
> > blobs, the unit tests would fail with this patch.
>
> adding this to commit description doesn't help,
> 'make check' still fails if we apply patches till this point.
> which leads to broken bisection.
>
> I'm curious, what is not clear in process description at
> the start of tests/qtest/bios-tables-test.c.
> You did that for new tests, but so some reason you are avoiding
> to  follow the same rules with this patch.

Yes because I assumed the instructions were for tests only not for
patches which modify tables. It starts with the lines:
" How to add or update the tests:"

>
>
> > Signed-off-by: Ani Sinha <ani@anisinha.ca>
> > ---
> >  hw/acpi/piix4.c      |  6 ++++--
> >  hw/i386/acpi-build.c | 25 ++++++++++++++++++-------
> >  2 files changed, 22 insertions(+), 9 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..e41bb0992b 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)
> > @@ -450,10 +454,12 @@ static void build_append_pci_bus_devices(Aml 
> > *parent_scope, PCIBus *bus,
> >      }
> >
> >      /* Append PCNT method to notify about events on local and child buses.
> > -     * Add unconditionally for root since DSDT expects it.
> > +     * Add this method for root bus only when hotplug is enabled since DSDT
> > +     * expects it.
> >       */
> > -    method = aml_method("PCNT", 0, AML_NOTSERIALIZED);
> > -
> > +    if (bsel || pcihp_bridge_en) {
> > +        method = aml_method("PCNT", 0, AML_NOTSERIALIZED);
> > +    }
> >      /* If bus supports hotplug select it and notify about local events */
> >      if (bsel) {
> >          uint64_t bsel_val = qnum_get_uint(qobject_to(QNum, bsel));
> > @@ -479,7 +485,10 @@ 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 || pcihp_bridge_en) {
> > +        aml_append(parent_scope, method);
> > +    }
> >      qobject_unref(bsel);
> >  }
> >
> > @@ -1504,7 +1513,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");
> > @@ -1546,7 +1557,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> >      {
> >          aml_append(scope, aml_name_decl("_HID", aml_string("ACPI0006")));
> >
> > -        if (misc->is_piix4) {
> > +        if (misc->is_piix4 && (pm->pcihp_bridge_en || pm->pcihp_root_en)) {
> >              method = aml_method("_E01", 0, AML_NOTSERIALIZED);
> >              aml_append(method,
> >                  aml_acquire(aml_name("\\_SB.PCI0.BLCK"), 0xFFFF));
> > @@ -1698,7 +1709,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,
>