From: Jonathan Cameron <Jonathan.Cameron@Huawei.com> To: Ben Widawsky <ben.widawsky@intel.com> Cc: qemu-devel@nongnu.org, "David Hildenbrand" <david@redhat.com>, "Vishal Verma" <vishal.l.verma@intel.com>, "John Groves (jgroves)" <jgroves@micron.com>, "Chris Browy" <cbrowy@avery-design.com>, "Markus Armbruster" <armbru@redhat.com>, linux-cxl@vger.kernel.org, "Philippe Mathieu-Daudé" <f4bug@amsat.org>, "Michael S. Tsirkin" <mst@redhat.com>, "Igor Mammedov" <imammedo@redhat.com>, "Dan Williams" <dan.j.williams@intel.com>, "Ira Weiny" <ira.weiny@intel.com> Subject: Re: [RFC PATCH v3 14/31] acpi/pci: Consolidate host bridge setup Date: Thu, 2 Dec 2021 10:32:21 +0000 [thread overview] Message-ID: <20211202103221.0000280b@Huawei.com> (raw) In-Reply-To: <20210202005948.241655-15-ben.widawsky@intel.com> On Mon, 1 Feb 2021 16:59:31 -0800 Ben Widawsky <ben.widawsky@intel.com> wrote: > This cleanup will make it easier to add support for CXL to the mix. > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> Hi Ben / all (particularly PCI experts!) So... I was looking at the large impact this has on needing to update ACPI tables for the tests and that got me wondering. The issue is it reorders things a bit rather than making any functional changes. Why does the pxb expander ACPI not have ADR set and all other PNP0A03 / PNP0A08 root bridge acpi chunks do? I've messed around with the values provided and dug around in Linux and other than them being exposed in the sysfs for firmware blobs associated with /sys/class/pci_bus/devices these particular _ADR entries don't actually seem to be used by Linux. So it becomes a question of what the specification says... As ever clear as mud :) PCI firmware spec doesn't say, but has an example with non _ADR entry. 4.5.3 in the PCI Firmware Specification Revisions 3.3 The ACPI spec has an example with _ADR when describing _SEG. 6.5.6 in ACPI 6.4 _ADR description is the address of the device on it's parent bus. I'm not sure a root bridge parent bus (which is probably the SB, has any such concept of an address (which probably explains why I've never seen it set to anything other than 0). Checking where the _ADR 0 entries came from, they go back to Michael importing tables from seabios in 2013. My current feeling is we don't want to risk breaking some user of these values so perhaps the simple option is just to add _ADR to the PXB ACPI block as well? That way I think we will cause less churn in the ACPI tables needed for tests and end up at least consistent in what QEMU presents. Note I'd ideally like to separate this as a precursor to the CXL series rather than burying it in the middle of that! Jonathan > --- > hw/i386/acpi-build.c | 31 +++++++++++++++++-------------- > 1 file changed, 17 insertions(+), 14 deletions(-) > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index f56d699c7f..cf6eb54c22 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -1194,6 +1194,20 @@ static void build_smb0(Aml *table, I2CBus *smbus, int devnr, int func) > aml_append(table, scope); > } > > +enum { PCI, PCIE }; > +static void init_pci_acpi(Aml *dev, int uid, int type) > +{ > + if (type == PCI) { > + aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A03"))); > + aml_append(dev, aml_name_decl("_UID", aml_int(uid))); > + } else { > + aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A08"))); > + aml_append(dev, aml_name_decl("_CID", aml_eisaid("PNP0A03"))); > + aml_append(dev, aml_name_decl("_UID", aml_int(uid))); > + aml_append(dev, build_q35_osc_method()); > + } > +} > + > static void > build_dsdt(GArray *table_data, BIOSLinker *linker, > AcpiPmInfo *pm, AcpiMiscInfo *misc, > @@ -1222,9 +1236,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, > if (misc->is_piix4) { > sb_scope = aml_scope("_SB"); > dev = aml_device("PCI0"); > - aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A03"))); > + init_pci_acpi(dev, 0, PCI); > aml_append(dev, aml_name_decl("_ADR", aml_int(0))); > - aml_append(dev, aml_name_decl("_UID", aml_int(0))); > aml_append(sb_scope, dev); > aml_append(dsdt, sb_scope); > > @@ -1238,11 +1251,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, > } else { > sb_scope = aml_scope("_SB"); > dev = aml_device("PCI0"); > - aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A08"))); > - aml_append(dev, aml_name_decl("_CID", aml_eisaid("PNP0A03"))); > + init_pci_acpi(dev, 0, PCIE); > aml_append(dev, aml_name_decl("_ADR", aml_int(0))); > - aml_append(dev, aml_name_decl("_UID", aml_int(0))); > - aml_append(dev, build_q35_osc_method()); > aml_append(sb_scope, dev); > > if (pm->smi_on_cpuhp) { > @@ -1345,15 +1355,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, > > scope = aml_scope("\\_SB"); > dev = aml_device("PC%.02X", bus_num); > - aml_append(dev, aml_name_decl("_UID", aml_int(bus_num))); > aml_append(dev, aml_name_decl("_BBN", aml_int(bus_num))); > - if (pci_bus_is_express(bus)) { > - aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A08"))); > - aml_append(dev, aml_name_decl("_CID", aml_eisaid("PNP0A03"))); > - aml_append(dev, build_q35_osc_method()); > - } else { > - aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A03"))); > - } > + init_pci_acpi(dev, bus_num, pci_bus_is_express(bus) ? PCIE : PCI); > > if (numa_node != NUMA_NODE_UNASSIGNED) { > aml_append(dev, aml_name_decl("_PXM", aml_int(numa_node)));
WARNING: multiple messages have this Message-ID (diff)
From: Jonathan Cameron via <qemu-devel@nongnu.org> To: Ben Widawsky <ben.widawsky@intel.com> Cc: qemu-devel@nongnu.org, "David Hildenbrand" <david@redhat.com>, "Vishal Verma" <vishal.l.verma@intel.com>, "John Groves (jgroves)" <jgroves@micron.com>, "Chris Browy" <cbrowy@avery-design.com>, "Markus Armbruster" <armbru@redhat.com>, linux-cxl@vger.kernel.org, "Philippe Mathieu-Daudé" <f4bug@amsat.org>, "Michael S. Tsirkin" <mst@redhat.com>, "Igor Mammedov" <imammedo@redhat.com>, "Dan Williams" <dan.j.williams@intel.com>, "Ira Weiny" <ira.weiny@intel.com> Subject: Re: [RFC PATCH v3 14/31] acpi/pci: Consolidate host bridge setup Date: Thu, 2 Dec 2021 10:32:21 +0000 [thread overview] Message-ID: <20211202103221.0000280b@Huawei.com> (raw) In-Reply-To: <20210202005948.241655-15-ben.widawsky@intel.com> On Mon, 1 Feb 2021 16:59:31 -0800 Ben Widawsky <ben.widawsky@intel.com> wrote: > This cleanup will make it easier to add support for CXL to the mix. > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> Hi Ben / all (particularly PCI experts!) So... I was looking at the large impact this has on needing to update ACPI tables for the tests and that got me wondering. The issue is it reorders things a bit rather than making any functional changes. Why does the pxb expander ACPI not have ADR set and all other PNP0A03 / PNP0A08 root bridge acpi chunks do? I've messed around with the values provided and dug around in Linux and other than them being exposed in the sysfs for firmware blobs associated with /sys/class/pci_bus/devices these particular _ADR entries don't actually seem to be used by Linux. So it becomes a question of what the specification says... As ever clear as mud :) PCI firmware spec doesn't say, but has an example with non _ADR entry. 4.5.3 in the PCI Firmware Specification Revisions 3.3 The ACPI spec has an example with _ADR when describing _SEG. 6.5.6 in ACPI 6.4 _ADR description is the address of the device on it's parent bus. I'm not sure a root bridge parent bus (which is probably the SB, has any such concept of an address (which probably explains why I've never seen it set to anything other than 0). Checking where the _ADR 0 entries came from, they go back to Michael importing tables from seabios in 2013. My current feeling is we don't want to risk breaking some user of these values so perhaps the simple option is just to add _ADR to the PXB ACPI block as well? That way I think we will cause less churn in the ACPI tables needed for tests and end up at least consistent in what QEMU presents. Note I'd ideally like to separate this as a precursor to the CXL series rather than burying it in the middle of that! Jonathan > --- > hw/i386/acpi-build.c | 31 +++++++++++++++++-------------- > 1 file changed, 17 insertions(+), 14 deletions(-) > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index f56d699c7f..cf6eb54c22 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -1194,6 +1194,20 @@ static void build_smb0(Aml *table, I2CBus *smbus, int devnr, int func) > aml_append(table, scope); > } > > +enum { PCI, PCIE }; > +static void init_pci_acpi(Aml *dev, int uid, int type) > +{ > + if (type == PCI) { > + aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A03"))); > + aml_append(dev, aml_name_decl("_UID", aml_int(uid))); > + } else { > + aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A08"))); > + aml_append(dev, aml_name_decl("_CID", aml_eisaid("PNP0A03"))); > + aml_append(dev, aml_name_decl("_UID", aml_int(uid))); > + aml_append(dev, build_q35_osc_method()); > + } > +} > + > static void > build_dsdt(GArray *table_data, BIOSLinker *linker, > AcpiPmInfo *pm, AcpiMiscInfo *misc, > @@ -1222,9 +1236,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, > if (misc->is_piix4) { > sb_scope = aml_scope("_SB"); > dev = aml_device("PCI0"); > - aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A03"))); > + init_pci_acpi(dev, 0, PCI); > aml_append(dev, aml_name_decl("_ADR", aml_int(0))); > - aml_append(dev, aml_name_decl("_UID", aml_int(0))); > aml_append(sb_scope, dev); > aml_append(dsdt, sb_scope); > > @@ -1238,11 +1251,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, > } else { > sb_scope = aml_scope("_SB"); > dev = aml_device("PCI0"); > - aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A08"))); > - aml_append(dev, aml_name_decl("_CID", aml_eisaid("PNP0A03"))); > + init_pci_acpi(dev, 0, PCIE); > aml_append(dev, aml_name_decl("_ADR", aml_int(0))); > - aml_append(dev, aml_name_decl("_UID", aml_int(0))); > - aml_append(dev, build_q35_osc_method()); > aml_append(sb_scope, dev); > > if (pm->smi_on_cpuhp) { > @@ -1345,15 +1355,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, > > scope = aml_scope("\\_SB"); > dev = aml_device("PC%.02X", bus_num); > - aml_append(dev, aml_name_decl("_UID", aml_int(bus_num))); > aml_append(dev, aml_name_decl("_BBN", aml_int(bus_num))); > - if (pci_bus_is_express(bus)) { > - aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A08"))); > - aml_append(dev, aml_name_decl("_CID", aml_eisaid("PNP0A03"))); > - aml_append(dev, build_q35_osc_method()); > - } else { > - aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A03"))); > - } > + init_pci_acpi(dev, bus_num, pci_bus_is_express(bus) ? PCIE : PCI); > > if (numa_node != NUMA_NODE_UNASSIGNED) { > aml_append(dev, aml_name_decl("_PXM", aml_int(numa_node)));
next prev parent reply other threads:[~2021-12-02 10:32 UTC|newest] Thread overview: 117+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-02-02 0:59 [RFC PATCH v3 00/31] CXL 2.0 Support Ben Widawsky 2021-02-02 0:59 ` Ben Widawsky 2021-02-02 0:59 ` [RFC PATCH v3 01/31] hw/pci/cxl: Add a CXL component type (interface) Ben Widawsky 2021-02-02 0:59 ` Ben Widawsky 2021-02-02 0:59 ` [RFC PATCH v3 02/31] hw/cxl/component: Introduce CXL components (8.1.x, 8.2.5) Ben Widawsky 2021-02-02 0:59 ` Ben Widawsky 2021-02-02 11:48 ` Jonathan Cameron 2021-02-02 11:48 ` Jonathan Cameron 2021-02-17 18:36 ` Ben Widawsky 2021-02-11 17:08 ` Jonathan Cameron 2021-02-11 17:08 ` Jonathan Cameron 2021-02-17 16:40 ` Ben Widawsky 2021-02-02 0:59 ` [RFC PATCH v3 03/31] hw/cxl/device: Introduce a CXL device (8.2.8) Ben Widawsky 2021-02-02 0:59 ` Ben Widawsky 2021-02-02 12:03 ` Jonathan Cameron 2021-02-02 12:03 ` Jonathan Cameron 2021-02-02 0:59 ` [RFC PATCH v3 04/31] hw/cxl/device: Implement the CAP array (8.2.8.1-2) Ben Widawsky 2021-02-02 0:59 ` Ben Widawsky 2021-02-02 12:23 ` Jonathan Cameron 2021-02-02 12:23 ` Jonathan Cameron 2021-02-17 22:15 ` Ben Widawsky 2021-02-02 0:59 ` [RFC PATCH v3 05/31] hw/cxl/device: Implement basic mailbox (8.2.8.4) Ben Widawsky 2021-02-02 0:59 ` Ben Widawsky 2021-02-02 14:58 ` Jonathan Cameron 2021-02-02 14:58 ` Jonathan Cameron 2021-02-11 17:46 ` Jonathan Cameron 2021-02-18 0:55 ` Ben Widawsky 2021-02-18 16:50 ` Jonathan Cameron 2021-02-11 18:09 ` Jonathan Cameron 2021-02-11 18:09 ` Jonathan Cameron 2021-02-02 0:59 ` [RFC PATCH v3 06/31] hw/cxl/device: Add memory device utilities Ben Widawsky 2021-02-02 0:59 ` Ben Widawsky 2021-02-02 0:59 ` [RFC PATCH v3 07/31] hw/cxl/device: Add cheap EVENTS implementation (8.2.9.1) Ben Widawsky 2021-02-02 0:59 ` Ben Widawsky 2021-02-02 13:44 ` Jonathan Cameron 2021-02-02 13:44 ` Jonathan Cameron 2021-02-11 17:59 ` Jonathan Cameron 2021-02-11 17:59 ` Jonathan Cameron 2021-02-02 0:59 ` [RFC PATCH v3 08/31] hw/cxl/device: Timestamp implementation (8.2.9.3) Ben Widawsky 2021-02-02 0:59 ` Ben Widawsky 2021-02-02 0:59 ` [RFC PATCH v3 09/31] hw/cxl/device: Add log commands (8.2.9.4) + CEL Ben Widawsky 2021-02-02 0:59 ` Ben Widawsky 2021-02-02 0:59 ` [RFC PATCH v3 10/31] hw/pxb: Use a type for realizing expanders Ben Widawsky 2021-02-02 0:59 ` Ben Widawsky 2021-02-02 13:50 ` Jonathan Cameron 2021-02-02 13:50 ` Jonathan Cameron 2021-02-02 0:59 ` [RFC PATCH v3 11/31] hw/pci/cxl: Create a CXL bus type Ben Widawsky 2021-02-02 0:59 ` Ben Widawsky 2021-02-02 0:59 ` [RFC PATCH v3 12/31] hw/pxb: Allow creation of a CXL PXB (host bridge) Ben Widawsky 2021-02-02 0:59 ` Ben Widawsky 2021-02-02 0:59 ` [RFC PATCH v3 13/31] qtest: allow DSDT acpi table changes Ben Widawsky 2021-02-02 0:59 ` Ben Widawsky 2021-02-02 0:59 ` [RFC PATCH v3 14/31] acpi/pci: Consolidate host bridge setup Ben Widawsky 2021-02-02 0:59 ` Ben Widawsky 2021-02-02 13:56 ` Jonathan Cameron 2021-02-02 13:56 ` Jonathan Cameron 2021-12-02 10:32 ` Jonathan Cameron [this message] 2021-12-02 10:32 ` Jonathan Cameron via 2021-02-02 0:59 ` [RFC PATCH v3 15/31] tests/acpi: remove stale allowed tables Ben Widawsky 2021-02-02 0:59 ` Ben Widawsky 2021-02-02 0:59 ` [RFC PATCH v3 16/31] hw/pci: Plumb _UID through host bridges Ben Widawsky 2021-02-02 0:59 ` Ben Widawsky 2021-02-02 15:00 ` Jonathan Cameron 2021-02-02 15:00 ` Jonathan Cameron 2021-02-02 15:24 ` Michael S. Tsirkin 2021-02-02 15:24 ` Michael S. Tsirkin 2021-02-02 15:42 ` Ben Widawsky 2021-02-02 15:42 ` Ben Widawsky 2021-02-02 15:51 ` Michael S. Tsirkin 2021-02-02 15:51 ` Michael S. Tsirkin 2021-02-02 16:20 ` Ben Widawsky 2021-02-02 16:20 ` Ben Widawsky 2021-02-02 0:59 ` [RFC PATCH v3 17/31] hw/cxl/component: Implement host bridge MMIO (8.2.5, table 142) Ben Widawsky 2021-02-02 0:59 ` Ben Widawsky 2021-02-02 19:21 ` Jonathan Cameron 2021-02-02 19:21 ` Jonathan Cameron 2021-02-02 19:45 ` Ben Widawsky 2021-02-02 20:43 ` Jonathan Cameron 2021-02-02 21:03 ` Ben Widawsky 2021-02-02 22:06 ` Jonathan Cameron 2021-02-02 0:59 ` [RFC PATCH v3 18/31] acpi/pxb/cxl: Reserve host bridge MMIO Ben Widawsky 2021-02-02 0:59 ` Ben Widawsky 2021-02-02 0:59 ` [RFC PATCH v3 19/31] hw/pxb/cxl: Add "windows" for host bridges Ben Widawsky 2021-02-02 0:59 ` Ben Widawsky 2021-02-02 0:59 ` [RFC PATCH v3 20/31] hw/cxl/rp: Add a root port Ben Widawsky 2021-02-02 0:59 ` Ben Widawsky 2021-02-02 0:59 ` [RFC PATCH v3 21/31] hw/cxl/device: Add a memory device (8.2.8.5) Ben Widawsky 2021-02-02 0:59 ` Ben Widawsky 2021-02-02 14:26 ` Eric Blake 2021-02-02 15:06 ` Ben Widawsky 2021-02-02 15:06 ` Ben Widawsky 2021-02-02 0:59 ` [RFC PATCH v3 22/31] hw/cxl/device: Implement MMIO HDM decoding (8.2.5.12) Ben Widawsky 2021-02-02 0:59 ` Ben Widawsky 2021-02-02 0:59 ` [RFC PATCH v3 23/31] acpi/cxl: Add _OSC implementation (9.14.2) Ben Widawsky 2021-02-02 0:59 ` Ben Widawsky 2021-02-02 0:59 ` [RFC PATCH v3 24/31] tests/acpi: allow CEDT table addition Ben Widawsky 2021-02-02 0:59 ` Ben Widawsky 2021-02-02 0:59 ` [RFC PATCH v3 25/31] acpi/cxl: Create the CEDT (9.14.1) Ben Widawsky 2021-02-02 0:59 ` Ben Widawsky 2021-02-02 0:59 ` [RFC PATCH v3 26/31] tests/acpi: Add new CEDT files Ben Widawsky 2021-02-02 0:59 ` Ben Widawsky 2021-02-02 0:59 ` [RFC PATCH v3 27/31] hw/cxl/device: Add some trivial commands Ben Widawsky 2021-02-02 0:59 ` Ben Widawsky 2021-02-02 0:59 ` [RFC PATCH v3 28/31] hw/cxl/device: Plumb real LSA sizing Ben Widawsky 2021-02-02 0:59 ` Ben Widawsky 2021-02-02 0:59 ` [RFC PATCH v3 29/31] hw/cxl/device: Implement get/set LSA Ben Widawsky 2021-02-02 0:59 ` Ben Widawsky 2021-02-02 0:59 ` [RFC PATCH v3 30/31] qtest/cxl: Add very basic sanity tests Ben Widawsky 2021-02-02 0:59 ` Ben Widawsky 2021-02-02 0:59 ` [RFC PATCH v3 31/31] WIP: i386/cxl: Initialize a host bridge Ben Widawsky 2021-02-02 0:59 ` Ben Widawsky 2021-02-02 1:33 ` [RFC PATCH v3 00/31] CXL 2.0 Support no-reply 2021-02-02 1:33 ` no-reply 2021-02-03 17:42 ` Ben Widawsky 2021-02-11 18:51 ` Jonathan Cameron 2021-02-11 18:51 ` Jonathan Cameron 2021-03-11 23:27 ` [RFC PATCH] hw/mem/cxl_type3: Go back to subregions Ben Widawsky
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20211202103221.0000280b@Huawei.com \ --to=jonathan.cameron@huawei.com \ --cc=armbru@redhat.com \ --cc=ben.widawsky@intel.com \ --cc=cbrowy@avery-design.com \ --cc=dan.j.williams@intel.com \ --cc=david@redhat.com \ --cc=f4bug@amsat.org \ --cc=imammedo@redhat.com \ --cc=ira.weiny@intel.com \ --cc=jgroves@micron.com \ --cc=linux-cxl@vger.kernel.org \ --cc=mst@redhat.com \ --cc=qemu-devel@nongnu.org \ --cc=vishal.l.verma@intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.