[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v5 09/11] piix4: don't reserve hw resources when hotplug is o
From: |
Igor Mammedov |
Subject: |
Re: [PATCH v5 09/11] piix4: don't reserve hw resources when hotplug is off globally |
Date: |
Thu, 17 Sep 2020 09:05:57 +0200 |
On Thu, 17 Sep 2020 12:09:46 +0530
Ani Sinha <ani@anisinha.ca> wrote:
> On Thu, Sep 17, 2020 at 11:46 AM Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > On Wed, 16 Sep 2020 13:49:08 +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 AMLs for the PCNT method are only added
> > > when
> > > bsel is enabled for the corresponding pci bus or bridge hotplug is turned
> > > on.
> > > As a result, when the hotplug for both the root pci device and bridge are
> > > disabled, this AML in the DSDT acpi table is turned off.
> > >
> > > 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
> > > also
> > > commit the updated golden master DSDT table acpi binary blobs as well.
> >
> > Please split binary blobs into separate patch, as described in process.
> > Otherwise the patch is very likely to become un-merge-able due to
> > conflicts.
>
> I wanted to make sure that the changes in this patch and the blobs go
> together as a single atomic commit since the unit test will break if
> one has the patch but not the blobs.
it would work if it were the only patch that touches ACPI,
process instructs splitting acpi changes in 3 part to avoid issues
with merging and bisecting. (it saves time not only of maintainer
but your as well)
Additionally, one can not review (without going into trouble of
testing/disassembling)
binary blobs, so it's good to split them out into a separate patch.
> >
> > > Following is a typical diff between the q35 acpi DSDT table blobs:
> > >
> > > @@ -1,30 +1,30 @@
> > > /*
> > > * Intel ACPI Component Architecture
> > > * AML/ASL+ Disassembler version 20180105 (64-bit version)
> > > * Copyright (c) 2000 - 2018 Intel Corporation
> > > *
> > > * Disassembling to symbolic ASL+ operators
> > > *
> > > - * Disassembly of tests/data/acpi/q35/DSDT, Tue Sep 15 18:52:47 2020
> > > + * Disassembly of /tmp/aml-3O0DR0, Tue Sep 15 18:52:47 2020
> > > *
> > > * Original Table Header:
> > > * Signature "DSDT"
> > > - * Length 0x00001DFE (7678)
> > > + * Length 0x00001DF6 (7670)
> > > * Revision 0x01 **** 32-bit table (V1), no 64-bit math
> > > support
> > > - * Checksum 0xAC
> > > + * Checksum 0x17
> > > * OEM ID "BOCHS "
> > > * OEM Table ID "BXPCDSDT"
> > > * OEM Revision 0x00000001 (1)
> > > * Compiler ID "BXPC"
> > > * Compiler Version 0x00000001 (1)
> > > */
> > > DefinitionBlock ("", "DSDT", 1, "BOCHS ", "BXPCDSDT", 0x00000001)
> > > {
> > > Scope (\)
> > > {
> > > OperationRegion (DBG, SystemIO, 0x0402, One)
> > > Field (DBG, ByteAcc, NoLock, Preserve)
> > > {
> > > DBGB, 8
> > > }
> > >
> > > @@ -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)
> > > - {
> > > - }
> > > }
> > > }
> > > }
> > >
> > > Signed-off-by: Ani Sinha <ani@anisinha.ca>
> > > ---
> > > hw/acpi/piix4.c | 6 ++++--
> > > hw/i386/acpi-build.c | 25 ++++++++++++++++++-------
> > > tests/data/acpi/q35/DSDT | Bin 7678 -> 7670 bytes
> > > tests/data/acpi/q35/DSDT.acpihmat | Bin 9002 -> 8994 bytes
> > > tests/data/acpi/q35/DSDT.bridge | Bin 7695 -> 7688 bytes
> > > tests/data/acpi/q35/DSDT.cphp | Bin 8141 -> 8133 bytes
> > > tests/data/acpi/q35/DSDT.dimmpxm | Bin 9331 -> 9323 bytes
> > > tests/data/acpi/q35/DSDT.ipmibt | Bin 7753 -> 7745 bytes
> > > tests/data/acpi/q35/DSDT.memhp | Bin 9037 -> 9029 bytes
> > > tests/data/acpi/q35/DSDT.mmio64 | Bin 8808 -> 8801 bytes
> > > tests/data/acpi/q35/DSDT.numamem | Bin 7684 -> 7676 bytes
> > > tests/data/acpi/q35/DSDT.tis | Bin 8283 -> 8276 bytes
> > > 12 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,
> > > diff --git a/tests/data/acpi/q35/DSDT b/tests/data/acpi/q35/DSDT
> > > index
> > > bba8884073a27427b88ac0d733c9c87330a59366..4fad91b72e279b744b0528fd687c862d3a3d5cfa
> > > 100644
> > > GIT binary patch
> > > delta 33
> > > pcmexo{mq)oCD<k8n=AtZqxeRyWwM-ZEHUxHPVoYElXuD50sy=Q3Pb<^
> > >
> > > delta 42
> > > ycmexn{m+`qCD<k8pDY6d<C=|J%VfFySYqOXo#F-DSSIh3wPhD!3vl)eVE_Owj0{Tv
> > >
> > > diff --git a/tests/data/acpi/q35/DSDT.acpihmat
> > > b/tests/data/acpi/q35/DSDT.acpihmat
> > > index
> > > 9cac92418b5fcc2767dc74603d599642b59623fe..e4df7d1ca89578dd81be3539c8f83f073bb8db25
> > > 100644
> > > GIT binary patch
> > > delta 33
> > > pcmZ4Gw#bdkCD<iINtuCx@ySN6OG=z>EHUxHPVoYElb<Qs0syHh3KIYT
> > >
> > > delta 42
> > > xcmZ4Fw#tpmCD<iIOPPUzv2r8VB_%FDmYDcpr+5K3mdQ_*Y}rNF0-XIq7y$jZ3mO0b
> > >
> > > diff --git a/tests/data/acpi/q35/DSDT.bridge
> > > b/tests/data/acpi/q35/DSDT.bridge
> > > index
> > > f08b7245f59aad491fcaa60e2bab1085c369ea1c..065399174575442201cc09ffa4939ddf90ac81b4
> > > 100644
> > > GIT binary patch
> > > delta 33
> > > ocmeCT>9FB)33dtLkYiwAT)&ZPnJlLVYfOBwQ@nt~<ejqq0G06xDgXcg
> > >
> > > delta 41
> > > wcmeCM>9^r>33dtLmt$aH^xnv|OqSE1H6}jTDPF*R@=jTQb`iD!XTJ~z0NaZSF#rGn
> > >
> > > diff --git a/tests/data/acpi/q35/DSDT.cphp b/tests/data/acpi/q35/DSDT.cphp
> > > index
> > > 57d859cef9fa16a8f125c4b338611c8472699f38..8d2395e3cb4383b30e3840caed0d09ccad0c7323
> > > 100644
> > > GIT binary patch
> > > delta 33
> > > ocmX?Wf7G7KCD<k8s5}D$<AjY|rShC^EHUxHPVoYElRM>Y0kO^r)&Kwi
> > >
> > > delta 42
> > > xcmX?Vf7YJMCD<k8tULn)qv}SkQh6>vmYDcpr+5K3mdPFRw(KHo0nUCQ3;+bB3f%wz
> > >
> > > diff --git a/tests/data/acpi/q35/DSDT.dimmpxm
> > > b/tests/data/acpi/q35/DSDT.dimmpxm
> > > index
> > > 9d5bd5744e2ba2e0f6126c3aba0bb36af865e499..df7422051c6feadeaa3b6733ad7efa67c339b49d
> > > 100644
> > > GIT binary patch
> > > delta 33
> > > ocmezD@!EsSCD<h-TZMsvF>)i9v<jyiOH6#QQ@nuPWPKG|0ILxQ{r~^~
> > >
> > > delta 42
> > > xcmaFu@!5mRCD<jTScQRs@!du)X%#L%mYDcpr+5K3mdSc5w(KHo0nUCQ3;+_P3k3iG
> > >
> > > diff --git a/tests/data/acpi/q35/DSDT.ipmibt
> > > b/tests/data/acpi/q35/DSDT.ipmibt
> > > index
> > > 5cd11de6a8fe47324e5f922823a22746882f19f5..c4ce5cc0ede822ea82656d078d7a8b7eee4a7516
> > > 100644
> > > GIT binary patch
> > > delta 33
> > > ocmX?UbI^v%CD<jzQI3Iu(Q6~uM_EocmYDcpr+5Lo$*gj=0Hl8i@&Et;
> > >
> > > delta 42
> > > xcmX?TbJB*(CD<jzQ;vaw@%~1xkFs2TEHUxHPVoY6ER$K}Y}rNF0-XIq7yt{`3i$v4
> > >
> > > diff --git a/tests/data/acpi/q35/DSDT.memhp
> > > b/tests/data/acpi/q35/DSDT.memhp
> > > index
> > > 05a7a73ec43130d5c3018bb462fd84981bfb151c..84614ffc1452358053b4c2be4b2edcb4d56a9ae6
> > > 100644
> > > GIT binary patch
> > > delta 33
> > > ocmX@>cGQi_CD<jzRhfZ-kz*s*S0zq2mYDcpr+5Lo$(+iz0Hk>c=Kufz
> > >
> > > delta 42
> > > xcmX@=cGiu{CD<jzSDAr<aqdR0uS#5gEHUxHPVoY6ER#8uZP`WG0-XIq7yt`p3hn>^
> > >
> > > diff --git a/tests/data/acpi/q35/DSDT.mmio64
> > > b/tests/data/acpi/q35/DSDT.mmio64
> > > index
> > > efd3f1188f2b55da1514212d4be081a61c2a96e9..d8dd702b69cc24a6b58b8eaa79ea02439a2a7dd9
> > > 100644
> > > GIT binary patch
> > > delta 33
> > > ocmaFi^3a9LCD<h-QHg<paqmX1|B9R*tTFMyPVoW`lZBN00lUBo5&!@I
> > >
> > > delta 41
> > > wcmaFp^1_A7CD<h-Ly3WbF>)i<e??Az)|mKUr+5MP$wEs0>>_Ld&VC^b00^lI82|tP
> > >
> > > diff --git a/tests/data/acpi/q35/DSDT.numamem
> > > b/tests/data/acpi/q35/DSDT.numamem
> > > index
> > > 1978b55f1255402bf9bade0b91150b5cb49789a4..f36d22063a6eed4fb107ffd0e10477a2d6d7a983
> > > 100644
> > > GIT binary patch
> > > delta 33
> > > pcmZp%`D4xH66_N4N0xzs@!&?THL{#;EHUxHPVoYElMl$+0sy+k3XK2&
> > >
> > > delta 42
> > > xcmexk-D1P#66_MfBFDhM7`l;bjVzZROH6#QQ@ns1%jEsCw(KHo0nUCQ3;^@W3X}i<
> > >
> > > diff --git a/tests/data/acpi/q35/DSDT.tis b/tests/data/acpi/q35/DSDT.tis
> > > index
> > > 638de3872673d17b1958497d0e62c83653de1602..203030a61a92c204bb93c43fe79e546471ae2985
> > > 100644
> > > GIT binary patch
> > > delta 38
> > > ucmccZaK(YkCD<h-M1g^U@x?~2O|qOGnlbUgPVoW`laI>UZRU~-WC8&31`DbH
> > >
> > > delta 45
> > > zcmccOaNB{)CD<h-T7iLqv1KFICRt8@&6xOLr+5MP$wy`F*hJU@oc%&JGsy)p0RTF+
> > > B45<JB
> > >
> >
>
- [PATCH v5 03/11] tests/acpi: add a new ACPI table in order to test root pci hotplug on/off, (continued)
- [PATCH v5 03/11] tests/acpi: add a new ACPI table in order to test root pci hotplug on/off, Ani Sinha, 2020/09/16
- [PATCH v5 04/11] Fix a gap where acpi_pcihp_find_hotplug_bus() returns a non-hotpluggable bus, Ani Sinha, 2020/09/16
- [PATCH v5 05/11] i440fx/acpi: do not add hotplug related amls for cold plugged bridges, Ani Sinha, 2020/09/16
- [PATCH v5 06/11] tests/acpi: list added acpi table binary file for pci bridge hotplug test, Ani Sinha, 2020/09/16
- [PATCH v5 07/11] tests/acpi: unit test for 'acpi-pci-hotplug-with-bridge-support' bridge flag, Ani Sinha, 2020/09/16
- [PATCH v5 08/11] tests/acpi: add newly added acpi DSDT table blob for pci bridge hotplug flag, Ani Sinha, 2020/09/16
- [PATCH v5 09/11] piix4: don't reserve hw resources when hotplug is off globally, Ani Sinha, 2020/09/16
- [PATCH v5 10/11] tests/acpi: unit test exercising hotplug off for pci root bus & bridge in i440fx, Ani Sinha, 2020/09/16
- [PATCH v5 11/11] tests/acpi: add DSDT.hpbrroot DSDT table blob to test global i440fx hotplug, Ani Sinha, 2020/09/16
- Re: [PATCH v5 00/11] i440fx/acpi: addition of feature and bug fixes., Igor Mammedov, 2020/09/17