From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id CE3A8C433EF for ; Mon, 7 Mar 2022 17:06:09 +0000 (UTC) Received: from localhost ([::1]:59840 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1nRGoC-0004cK-K3 for qemu-devel@archiver.kernel.org; Mon, 07 Mar 2022 12:06:08 -0500 Received: from eggs.gnu.org ([209.51.188.92]:38608) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1nRGjp-0004NC-97 for qemu-devel@nongnu.org; Mon, 07 Mar 2022 12:01:37 -0500 Received: from frasgout.his.huawei.com ([185.176.79.56]:2386) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1nRGjm-0001q7-2q for qemu-devel@nongnu.org; Mon, 07 Mar 2022 12:01:36 -0500 Received: from fraeml715-chm.china.huawei.com (unknown [172.18.147.201]) by frasgout.his.huawei.com (SkyGuard) with ESMTP id 4KC4VD2xchz6GD1s; Tue, 8 Mar 2022 01:01:08 +0800 (CST) Received: from lhreml710-chm.china.huawei.com (10.201.108.61) by fraeml715-chm.china.huawei.com (10.206.15.34) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2308.21; Mon, 7 Mar 2022 18:01:30 +0100 Received: from localhost (10.202.226.41) by lhreml710-chm.china.huawei.com (10.201.108.61) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2308.21; Mon, 7 Mar 2022 17:01:29 +0000 Date: Mon, 7 Mar 2022 17:01:27 +0000 To: "Michael S. Tsirkin" CC: , , Alex =?ISO-8859-1?Q?Benn?= =?ISO-8859-1?Q?=E9e?= , Marcel Apfelbaum , Igor Mammedov , Markus Armbruster , , Ben Widawsky , Peter Maydell , Shameerali Kolothum Thodi , Philippe =?ISO-8859-1?Q?Mathieu-Daud=E9?= , Saransh Gupta1 , Shreyas Shah , Chris Browy , Samarth Saxena , "Dan Williams" Subject: Re: [PATCH v7 24/46] acpi/cxl: Add _OSC implementation (9.14.2) Message-ID: <20220307170127.0000582b@Huawei.com> In-Reply-To: <20220306162444-mutt-send-email-mst@kernel.org> References: <20220306174137.5707-1-Jonathan.Cameron@huawei.com> <20220306174137.5707-25-Jonathan.Cameron@huawei.com> <20220306162444-mutt-send-email-mst@kernel.org> Organization: Huawei Technologies Research and Development (UK) Ltd. X-Mailer: Claws Mail 4.0.0 (GTK+ 3.24.29; i686-w64-mingw32) MIME-Version: 1.0 Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: quoted-printable X-Originating-IP: [10.202.226.41] X-ClientProxiedBy: lhreml729-chm.china.huawei.com (10.201.108.80) To lhreml710-chm.china.huawei.com (10.201.108.61) X-CFilter-Loop: Reflected Received-SPF: pass client-ip=185.176.79.56; envelope-from=jonathan.cameron@huawei.com; helo=frasgout.his.huawei.com X-Spam_score_int: -41 X-Spam_score: -4.2 X-Spam_bar: ---- X-Spam_report: (-4.2 / 5.0 requ) BAYES_00=-1.9, RCVD_IN_DNSWL_MED=-2.3, RCVD_IN_MSPIKE_H4=0.001, RCVD_IN_MSPIKE_WL=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" Reply-to: Jonathan Cameron From: Jonathan Cameron via On Sun, 6 Mar 2022 16:31:05 -0500 "Michael S. Tsirkin" wrote: > On Sun, Mar 06, 2022 at 05:41:15PM +0000, Jonathan Cameron wrote: > > From: Ben Widawsky > >=20 > > CXL 2.0 specification adds 2 new dwords to the existing _OSC definition > > from PCIe. The new dwords are accessed with a new uuid. This > > implementation supports what is in the specification. > >=20 > > Signed-off-by: Ben Widawsky > > Signed-off-by: Jonathan Cameron > > Reviewed-by: Alex Benn=E9e Question for Ben inline. > > --- > > hw/acpi/Kconfig | 5 ++ > > hw/acpi/cxl-stub.c | 12 +++++ > > hw/acpi/cxl.c | 104 ++++++++++++++++++++++++++++++++++++++++++ > > hw/acpi/meson.build | 4 +- > > hw/i386/acpi-build.c | 15 ++++-- > > include/hw/acpi/cxl.h | 23 ++++++++++ > > 6 files changed, 157 insertions(+), 6 deletions(-) > >=20 > > diff --git a/hw/acpi/Kconfig b/hw/acpi/Kconfig > > index 19caebde6c..3703aca212 100644 > > --- a/hw/acpi/Kconfig > > +++ b/hw/acpi/Kconfig > > @@ -5,6 +5,7 @@ config ACPI_X86 > > bool > > select ACPI > > select ACPI_NVDIMM > > + select ACPI_CXL > > select ACPI_CPU_HOTPLUG > > select ACPI_MEMORY_HOTPLUG > > select ACPI_HMAT > > @@ -66,3 +67,7 @@ config ACPI_ERST > > bool > > default y > > depends on ACPI && PCI > > + > > +config ACPI_CXL > > + bool > > + depends on ACPI > > diff --git a/hw/acpi/cxl-stub.c b/hw/acpi/cxl-stub.c > > new file mode 100644 > > index 0000000000..15bc21076b > > --- /dev/null > > +++ b/hw/acpi/cxl-stub.c > > @@ -0,0 +1,12 @@ > > + > > +/* > > + * Stubs for ACPI platforms that don't support CXl > > + */ > > +#include "qemu/osdep.h" > > +#include "hw/acpi/aml-build.h" > > +#include "hw/acpi/cxl.h" > > + > > +void build_cxl_osc_method(Aml *dev) > > +{ > > + g_assert_not_reached(); > > +} > > diff --git a/hw/acpi/cxl.c b/hw/acpi/cxl.c > > new file mode 100644 > > index 0000000000..7124d5a1a3 > > --- /dev/null > > +++ b/hw/acpi/cxl.c > > @@ -0,0 +1,104 @@ > > +/* > > + * CXL ACPI Implementation > > + * > > + * Copyright(C) 2020 Intel Corporation. > > + * > > + * This library is free software; you can redistribute it and/or > > + * modify it under the terms of the GNU Lesser General Public > > + * License as published by the Free Software Foundation; either > > + * version 2 of the License, or (at your option) any later version. > > + * > > + * This library is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > > + * Lesser General Public License for more details. > > + * > > + * You should have received a copy of the GNU Lesser General Public > > + * License along with this library; if not, see > > + */ > > + > > +#include "qemu/osdep.h" > > +#include "hw/cxl/cxl.h" > > +#include "hw/acpi/acpi.h" > > +#include "hw/acpi/aml-build.h" > > +#include "hw/acpi/bios-linker-loader.h" > > +#include "hw/acpi/cxl.h" > > +#include "qapi/error.h" > > +#include "qemu/uuid.h" > > + > > +static Aml *__build_cxl_osc_method(void) > > +{ > > + Aml *method, *if_uuid, *else_uuid, *if_arg1_not_1, *if_cxl, *if_ca= ps_masked; > > + Aml *a_ctrl =3D aml_local(0); > > + Aml *a_cdw1 =3D aml_name("CDW1"); > > + > > + method =3D aml_method("_OSC", 4, AML_NOTSERIALIZED); > > + aml_append(method, aml_create_dword_field(aml_arg(3), aml_int(0), = "CDW1")); > > + > > + /* 9.14.2.1.4 */ =20 >=20 > List spec name and version pls? Added. >=20 > > + if_uuid =3D aml_if( > > + aml_lor(aml_equal(aml_arg(0), > > + aml_touuid("33DB4D5B-1FF7-401C-9657-7441C03D= D766")), > > + aml_equal(aml_arg(0), > > + aml_touuid("68F2D50B-C469-4D8A-BD3D-941A103F= D3FC")))); > > + aml_append(if_uuid, aml_create_dword_field(aml_arg(3), aml_int(4),= "CDW2")); > > + aml_append(if_uuid, aml_create_dword_field(aml_arg(3), aml_int(8),= "CDW3")); > > + > > + aml_append(if_uuid, aml_store(aml_name("CDW3"), a_ctrl)); > > + > > + /* This is all the same as what's used for PCIe */ =20 >=20 > Referring to what exactly? > Better to also document the meaning. Wise advice. Having added documentation it is clear this was strangely ord= ered and contained at least one bug. Guess I took this on a bit too much trust. >=20 >=20 > > + aml_append(if_uuid, > > + aml_and(aml_name("CTRL"), aml_int(0x1F), aml_name("CTRL= "))); This should be aml_and(a_ctrl, aml_int(0x1F), a_ctrl) as we haven't stored anything to CTRL yet. The a_ctrl variable ends up nam= ed Local0 when disassembled which isn't particularly intuitive. > > + > > + if_arg1_not_1 =3D aml_if(aml_lnot(aml_equal(aml_arg(1), aml_int(0x= 1)))); > > + /* Unknown revision */ > > + aml_append(if_arg1_not_1, aml_or(a_cdw1, aml_int(0x08), a_cdw1)); > > + aml_append(if_uuid, if_arg1_not_1); > > + > > + if_caps_masked =3D aml_if(aml_lnot(aml_equal(aml_name("CDW3"), a_c= trl))); > > + /* Capability bits were masked */ > > + aml_append(if_caps_masked, aml_or(a_cdw1, aml_int(0x10), a_cdw1)); > > + aml_append(if_uuid, if_caps_masked); > > + > > + aml_append(if_uuid, aml_store(aml_name("CDW2"), aml_name("SUPP"))); > > + aml_append(if_uuid, aml_store(aml_name("CDW3"), aml_name("CTRL"))); > > + > > + if_cxl =3D aml_if(aml_equal( > > + aml_arg(0), aml_touuid("68F2D50B-C469-4D8A-BD3D-941A103FD3FC")= )); > > + /* CXL support field */ > > + aml_append(if_cxl, aml_create_dword_field(aml_arg(3), aml_int(12),= "CDW4")); > > + /* CXL capabilities */ > > + aml_append(if_cxl, aml_create_dword_field(aml_arg(3), aml_int(16),= "CDW5")); > > + aml_append(if_cxl, aml_store(aml_name("CDW4"), aml_name("SUPC"))); > > + aml_append(if_cxl, aml_store(aml_name("CDW5"), aml_name("CTRC"))); > > + > > + /* CXL 2.0 Port/Device Register access */ Ben, this seems to be wrong to me. CDW5 is the CXL control register and the only defined bit in the 2.0 spec (and no ECR or errata seems to have changed it) is bit 0 as CXL Memory Error Reporting Control. We can't control access to the Port/Device registers as that's only specified in the 'supported' value in CDW4. We also should be setting bits the OS didn't ask for. Obvious was a long time ago now, but can you recall what was intended here? Thanks, Jonathan =20 > > + aml_append(if_cxl, > > + aml_or(aml_name("CDW5"), aml_int(0x1), aml_name("CDW5")= )); > > + aml_append(if_uuid, if_cxl); > > + > > + /* Update DWORD3 (the return value) */ > > + aml_append(if_uuid, aml_store(a_ctrl, aml_name("CDW3"))); > > + > > + aml_append(if_uuid, aml_return(aml_arg(3))); > > + aml_append(method, if_uuid); > > + > > + else_uuid =3D aml_else(); > > + > > + /* unrecognized uuid */ > > + aml_append(else_uuid, > > + aml_or(aml_name("CDW1"), aml_int(0x4), aml_name("CDW1")= )); > > + aml_append(else_uuid, aml_return(aml_arg(3))); > > + aml_append(method, else_uuid); > > + > > + return method; > > +} > > + > > +void build_cxl_osc_method(Aml *dev) > > +{ > > + aml_append(dev, aml_name_decl("SUPP", aml_int(0))); > > + aml_append(dev, aml_name_decl("CTRL", aml_int(0))); > > + aml_append(dev, aml_name_decl("SUPC", aml_int(0))); > > + aml_append(dev, aml_name_decl("CTRC", aml_int(0))); > > + aml_append(dev, __build_cxl_osc_method()); > > +} > > diff --git a/hw/acpi/meson.build b/hw/acpi/meson.build > > index 8bea2e6933..cea2f5f93a 100644 > > --- a/hw/acpi/meson.build > > +++ b/hw/acpi/meson.build > > @@ -13,6 +13,7 @@ acpi_ss.add(when: 'CONFIG_ACPI_MEMORY_HOTPLUG', if_fa= lse: files('acpi-mem-hotplu > > acpi_ss.add(when: 'CONFIG_ACPI_NVDIMM', if_true: files('nvdimm.c')) > > acpi_ss.add(when: 'CONFIG_ACPI_NVDIMM', if_false: files('acpi-nvdimm-s= tub.c')) > > acpi_ss.add(when: 'CONFIG_ACPI_PCI', if_true: files('pci.c')) > > +acpi_ss.add(when: 'CONFIG_ACPI_CXL', if_true: files('cxl.c'), if_false= : files('cxl-stub.c')) > > acpi_ss.add(when: 'CONFIG_ACPI_VMGENID', if_true: files('vmgenid.c')) > > acpi_ss.add(when: 'CONFIG_ACPI_HW_REDUCED', if_true: files('generic_ev= ent_device.c')) > > acpi_ss.add(when: 'CONFIG_ACPI_HMAT', if_true: files('hmat.c')) > > @@ -33,4 +34,5 @@ softmmu_ss.add_all(when: 'CONFIG_ACPI', if_true: acpi= _ss) > > softmmu_ss.add(when: 'CONFIG_ALL', if_true: files('acpi-stub.c', 'aml-= build-stub.c', > > 'acpi-x86-stub.c', '= ipmi-stub.c', 'ghes-stub.c', > > 'acpi-mem-hotplug-st= ub.c', 'acpi-cpu-hotplug-stub.c', > > - 'acpi-pci-hotplug-st= ub.c', 'acpi-nvdimm-stub.c')) > > + 'acpi-pci-hotplug-st= ub.c', 'acpi-nvdimm-stub.c', > > + 'cxl-stub.c')) > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > > index 0a28dd6d4e..b5a4b663f2 100644 > > --- a/hw/i386/acpi-build.c > > +++ b/hw/i386/acpi-build.c > > @@ -66,6 +66,7 @@ > > #include "hw/acpi/aml-build.h" > > #include "hw/acpi/utils.h" > > #include "hw/acpi/pci.h" > > +#include "hw/acpi/cxl.h" > > =20 > > #include "qom/qom-qobject.h" > > #include "hw/i386/amd_iommu.h" > > @@ -1574,11 +1575,15 @@ build_dsdt(GArray *table_data, BIOSLinker *link= er, > > 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_cxl(bus)) { > > - aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0= A08"))); > > - aml_append(dev, aml_name_decl("_CID", aml_eisaid("PNP0= A03"))); > > - > > - /* Expander bridges do not have ACPI PCI Hot-plug enab= led */ > > - aml_append(dev, build_q35_osc_method(true)); > > + struct Aml *pkg =3D aml_package(2); > > + > > + aml_append(dev, aml_name_decl("_HID", aml_string("ACPI= 0016"))); > > + aml_append(pkg, aml_eisaid("PNP0A08")); > > + aml_append(pkg, aml_eisaid("PNP0A03")); > > + aml_append(dev, aml_name_decl("_CID", pkg)); > > + aml_append(dev, aml_name_decl("_ADR", aml_int(0))); > > + aml_append(dev, aml_name_decl("_UID", aml_int(bus_num)= )); > > + build_cxl_osc_method(dev); > > } else if (pci_bus_is_express(bus)) { > > aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0= A08"))); > > aml_append(dev, aml_name_decl("_CID", aml_eisaid("PNP0= A03"))); > > diff --git a/include/hw/acpi/cxl.h b/include/hw/acpi/cxl.h > > new file mode 100644 > > index 0000000000..7b8f3b8a2e > > --- /dev/null > > +++ b/include/hw/acpi/cxl.h > > @@ -0,0 +1,23 @@ > > +/* > > + * Copyright (C) 2020 Intel Corporation > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License as published by > > + * the Free Software Foundation; either version 2 of the License, or > > + * (at your option) any later version. > > + > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + > > + * You should have received a copy of the GNU General Public License a= long > > + * with this program; if not, see . > > + */ > > + > > +#ifndef HW_ACPI_CXL_H > > +#define HW_ACPI_CXL_H > > + > > +void build_cxl_osc_method(Aml *dev); > > + > > +#endif > > --=20 > > 2.32.0 =20 >=20