Re: [PATCH v7 24/46] acpi/cxl: Add _OSC implementation (9.14.2)

["Michael S. Tsirkin" <mst@redhat.com>]

On Sun, Mar 06, 2022 at 05:41:15PM +0000, Jonathan Cameron wrote:
> From: Ben Widawsky <ben.widawsky@intel.com>
> 
> 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.
> 
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  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(-)
> 
> 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 <http://www.gnu.org/licenses/>
> + */
> +
> +#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_caps_masked;
> +    Aml *a_ctrl = aml_local(0);
> +    Aml *a_cdw1 = aml_name("CDW1");
> +
> +    method = 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 */

List spec name and version pls?

> +    if_uuid = aml_if(
> +        aml_lor(aml_equal(aml_arg(0),
> +                          aml_touuid("33DB4D5B-1FF7-401C-9657-7441C03DD766")),
> +                aml_equal(aml_arg(0),
> +                          aml_touuid("68F2D50B-C469-4D8A-BD3D-941A103FD3FC"))));
> +    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 */

Referring to what exactly?
Better to also document the meaning.


> +    aml_append(if_uuid,
> +               aml_and(aml_name("CTRL"), aml_int(0x1F), aml_name("CTRL")));
> +
> +    if_arg1_not_1 = aml_if(aml_lnot(aml_equal(aml_arg(1), aml_int(0x1))));
> +    /* 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 = aml_if(aml_lnot(aml_equal(aml_name("CDW3"), a_ctrl)));
> +    /* 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 = 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 */
> +    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 = 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_false: 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-stub.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_event_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-stub.c', 'acpi-cpu-hotplug-stub.c',
> -                                                  'acpi-pci-hotplug-stub.c', 'acpi-nvdimm-stub.c'))
> +                                                  'acpi-pci-hotplug-stub.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"
>  
>  #include "qom/qom-qobject.h"
>  #include "hw/i386/amd_iommu.h"
> @@ -1574,11 +1575,15 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>              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("PNP0A08")));
> -                aml_append(dev, aml_name_decl("_CID", aml_eisaid("PNP0A03")));
> -
> -                /* Expander bridges do not have ACPI PCI Hot-plug enabled */
> -                aml_append(dev, build_q35_osc_method(true));
> +                struct Aml *pkg = aml_package(2);
> +
> +                aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0016")));
> +                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("PNP0A08")));
>                  aml_append(dev, aml_name_decl("_CID", aml_eisaid("PNP0A03")));
> 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 along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef HW_ACPI_CXL_H
> +#define HW_ACPI_CXL_H
> +
> +void build_cxl_osc_method(Aml *dev);
> +
> +#endif
> -- 
> 2.32.0