[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v8 09/12] piix4: don't reserve hw resources when hotplug is o
From: |
Ani Sinha |
Subject: |
Re: [PATCH v8 09/12] piix4: don't reserve hw resources when hotplug is off globally |
Date: |
Fri, 18 Sep 2020 12:20:32 +0530 |
On Fri, Sep 18, 2020 at 12:17 PM Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Fri, 18 Sep 2020 12:06:26 +0530
> Ani Sinha <ani@anisinha.ca> wrote:
>
> > 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:"
>
> any change to ACPI tables, that breaks test[s], leads to
> updating relevant blobs
Yes this should be mentioned in the comment in that file as well. I
will send a patch to that effect. It's not just about adding new tests
or updating existing tests.
and you point out in commit message
> that it does break q35 tests.
>
> "
> * How to add or update the tests:
> * Contributor:
> * 1. add empty files for new tables, if any, under tests/data/acpi
> * 2. list any changed files in tests/qtest/bios-tables-test-allowed-diff.h
> * 3. commit the above *before* making changes that affect the tables
> "
>
> Above is pretty clear to me, but if you can improve it
> feel free to post a patch clarifying process.
>
> > >
> > >
> > > > 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,
> > >
> >
>
- [PATCH v8 02/12] tests/acpi: add new unit test to test hotplug off/on feature on the root pci bus, (continued)
- [PATCH v8 02/12] tests/acpi: add new unit test to test hotplug off/on feature on the root pci bus, Ani Sinha, 2020/09/17
- [PATCH v8 04/12] Fix a gap where acpi_pcihp_find_hotplug_bus() returns a non-hotpluggable bus, Ani Sinha, 2020/09/17
- [PATCH v8 03/12] tests/acpi: add a new ACPI table in order to test root pci hotplug on/off, Ani Sinha, 2020/09/17
- [PATCH v8 05/12] i440fx/acpi: do not add hotplug related amls for cold plugged bridges, Ani Sinha, 2020/09/17
- [PATCH v8 06/12] tests/acpi: list added acpi table binary file for pci bridge hotplug test, Ani Sinha, 2020/09/17
- [PATCH v8 07/12] tests/acpi: unit test for 'acpi-pci-hotplug-with-bridge-support' bridge flag, Ani Sinha, 2020/09/17
- [PATCH v8 09/12] piix4: don't reserve hw resources when hotplug is off globally, Ani Sinha, 2020/09/17
[PATCH v8 10/12] tests/acpi: update golden master DSDT binary table blobs for q35, Ani Sinha, 2020/09/17
[PATCH v8 08/12] tests/acpi: add newly added acpi DSDT table blob for pci bridge hotplug flag, Ani Sinha, 2020/09/17
[PATCH v8 11/12] tests/acpi: unit test exercising global pci hotplug off for i440fx, Ani Sinha, 2020/09/17
[PATCH v8 12/12] tests/acpi: add DSDT.hpbrroot DSDT table blob to test global i440fx hotplug, Ani Sinha, 2020/09/17
Re: [PATCH v8 00/12] i440fx/acpi: addition of feature and bug fixes., no-reply, 2020/09/17
Re: [PATCH v8 00/12] i440fx/acpi: addition of feature and bug fixes., Igor Mammedov, 2020/09/18