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

[Jonathan Cameron]

On Sun, 6 Mar 2022 16:31:05 -0500
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> 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>

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(-)
> > 
> > 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?

Added.

> 
> > +    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.

Wise advice.  Having added documentation it is clear this was strangely ordered
and contained at least one bug.  Guess I took this on a bit too much trust.

> 
> 
> > +    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 named
Local0 when disassembled  which isn't particularly intuitive.

> > +
> > +    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 */

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

 
> > +    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  
>