[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC PATCH 22/25] acpi/cxl: Create the CEDT (9.14.1)
From: |
Ben Widawsky |
Subject: |
Re: [RFC PATCH 22/25] acpi/cxl: Create the CEDT (9.14.1) |
Date: |
Mon, 16 Nov 2020 14:05:02 -0800 |
On 20-11-16 17:15:03, Jonathan Cameron wrote:
> On Tue, 10 Nov 2020 21:47:21 -0800
> Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> > The CXL Early Discovery Table is defined in the CXL 2.0 specification as
> > a way for the OS to get CXL specific information from the system
> > firmware.
> >
> > As of CXL 2.0 spec, only 1 sub structure is defined, the CXL Host Bridge
> > Structure (CHBS) which is primarily useful for telling the OS exactly
> > where the MMIO for the host bridge is.
> >
> > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> Trivial comments inline.
>
> Jonathan
>
> > ---
> > hw/acpi/cxl.c | 72 +++++++++++++++++++++++++++++
> > hw/i386/acpi-build.c | 6 ++-
> > hw/pci-bridge/pci_expander_bridge.c | 21 +--------
> > include/hw/acpi/cxl.h | 4 ++
> > include/hw/pci/pci_bridge.h | 25 ++++++++++
> > 5 files changed, 107 insertions(+), 21 deletions(-)
> >
> > diff --git a/hw/acpi/cxl.c b/hw/acpi/cxl.c
> > index 31ceaeecc3..c9631763ad 100644
> > --- a/hw/acpi/cxl.c
> > +++ b/hw/acpi/cxl.c
> > @@ -18,14 +18,86 @@
> > */
> >
> > #include "qemu/osdep.h"
> > +#include "hw/sysbus.h"
> > +#include "hw/pci/pci_bridge.h"
> > +#include "hw/pci/pci_host.h"
> > #include "hw/cxl/cxl.h"
> > +#include "hw/mem/memory-device.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 "hw/acpi/cxl.h"
> > #include "qapi/error.h"
> > #include "qemu/uuid.h"
> >
> > +static void cedt_build_chbs(GArray *table_data, PXBDev *cxl)
> > +{
> > + SysBusDevice *sbd = SYS_BUS_DEVICE(cxl->cxl.cxl_host_bridge);
> > + struct MemoryRegion *mr = sbd->mmio[0].memory;
> > +
> > + /* Type */
> > + build_append_int_noprefix(table_data, 0, 1);
> > +
> > + /* Reserved */
> > + build_append_int_noprefix(table_data, 0xff, 1);
>
> Why 0xff rather than 0x00? ACPI uses default of 0 for reserved bits
> (5.2.1 in ACPI 6.3 spec)
>
> > +
> > + /* Record Length */
> > + build_append_int_noprefix(table_data, 32, 2);
> > +
> > + /* UID */
> > + build_append_int_noprefix(table_data, cxl->uid, 4);
> > +
> > + /* Version */
> > + build_append_int_noprefix(table_data, 1, 4);
> > +
> > + /* Reserved */
> > + build_append_int_noprefix(table_data, 0xffffffff, 4);
> > +
> > + /* Base */
> > + build_append_int_noprefix(table_data, mr->addr, 8);
> > +
> > + /* Length */
> > + build_append_int_noprefix(table_data, memory_region_size(mr), 4);
>
> Better to just treat this as a 64 bit field as per the spec, even though
> it can only contain 0x10000?
>
Ah, I based this on a pre-release version where it was 32-bit. I'll fix it.
> > +
> > + /* Reserved */
> > + build_append_int_noprefix(table_data, 0xffffffff, 4);
> > +}
> > +
> > +static int cxl_foreach_pxb_hb(Object *obj, void *opaque)
> > +{
> > + Aml *cedt = opaque;
> > +
> > + if (object_dynamic_cast(obj, TYPE_PXB_CXL_DEVICE)) {
> > + PXBDev *pxb = PXB_CXL_DEV(obj);
> > +
> > + cedt_build_chbs(cedt->buf, pxb);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +void cxl_build_cedt(GArray *table_offsets, GArray *table_data,
> > + BIOSLinker *linker)
> > +{
> > + const int cedt_start = table_data->len;
> > + Aml *cedt;
> > +
> > + cedt = init_aml_allocator();
> > +
> > + /* reserve space for CEDT header */
> > + acpi_add_table(table_offsets, table_data);
> > + acpi_data_push(cedt->buf, sizeof(AcpiTableHeader));
> > +
> > + object_child_foreach_recursive(object_get_root(), cxl_foreach_pxb_hb,
> > cedt);
> > +
> > + /* copy AML table into ACPI tables blob and patch header there */
> > + g_array_append_vals(table_data, cedt->buf->data, cedt->buf->len);
> > + build_header(linker, table_data, (void *)(table_data->data +
> > cedt_start),
> > + "CEDT", table_data->len - cedt_start, 1, NULL, NULL);
> > + free_aml_allocator();
> > +}
> > +
> > static Aml *__build_cxl_osc_method(void)
> > {
> > Aml *method, *if_uuid, *else_uuid, *if_arg1_not_1, *if_cxl,
> > *if_caps_masked;
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index dd1f8b39d4..eda62dcd6a 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -75,6 +75,8 @@
> > #include "hw/acpi/ipmi.h"
> > #include "hw/acpi/hmat.h"
> >
> > +#include "hw/acpi/cxl.h"
> > +
> > /* These are used to size the ACPI tables for -M pc-i440fx-1.7 and
> > * -M pc-i440fx-2.0. Even if the actual amount of AML generated grows
> > * a little bit, there should be plenty of free space since the DSDT
> > @@ -1662,7 +1664,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> >
> > scope = aml_scope("\\_SB");
> > if (type == CXL) {
> > - dev = aml_device("CXL%.01X", pci_bus_uid(bus));
> > + dev = aml_device("CXL%.01X", uid);
> > } else {
> > dev = aml_device("PC%.02X", bus_num);
> > }
> > @@ -2568,6 +2570,8 @@ void acpi_build(AcpiBuildTables *tables, MachineState
> > *machine)
> > machine->nvdimms_state, machine->ram_slots);
> > }
> >
> > + cxl_build_cedt(table_offsets, tables_blob, tables->linker);
> > +
> > acpi_add_table(table_offsets, tables_blob);
> > build_waet(tables_blob, tables->linker);
> >
> > diff --git a/hw/pci-bridge/pci_expander_bridge.c
> > b/hw/pci-bridge/pci_expander_bridge.c
> > index 75910f5870..b2c1d9056a 100644
> > --- a/hw/pci-bridge/pci_expander_bridge.c
> > +++ b/hw/pci-bridge/pci_expander_bridge.c
> > @@ -57,26 +57,6 @@ DECLARE_INSTANCE_CHECKER(PXBDev, PXB_DEV,
> > DECLARE_INSTANCE_CHECKER(PXBDev, PXB_PCIE_DEV,
> > TYPE_PXB_PCIE_DEVICE)
> >
> > -#define TYPE_PXB_CXL_DEVICE "pxb-cxl"
> > -DECLARE_INSTANCE_CHECKER(PXBDev, PXB_CXL_DEV,
> > - TYPE_PXB_CXL_DEVICE)
> > -
> > -struct PXBDev {
> > - /*< private >*/
> > - PCIDevice parent_obj;
> > - /*< public >*/
> > -
> > - uint8_t bus_nr;
> > - uint16_t numa_node;
> > - int32_t uid;
> > - struct cxl_dev {
> > - HostMemoryBackend *memory_window[CXL_WINDOW_MAX];
> > -
> > - uint32_t num_windows;
> > - hwaddr *window_base[CXL_WINDOW_MAX];
> > - } cxl;
> > -};
> > -
> > typedef struct CXLHost {
> > PCIHostState parent_obj;
> >
> > @@ -351,6 +331,7 @@ static void pxb_dev_realize_common(PCIDevice *dev, enum
> > BusType type,
> > bus = pci_root_bus_new(ds, dev_name, NULL, NULL, 0,
> > TYPE_PXB_CXL_BUS);
> > bus->flags |= PCI_BUS_CXL;
> > PXB_CXL_HOST(ds)->dev = PXB_CXL_DEV(dev);
> > + PXB_CXL_DEV(dev)->cxl.cxl_host_bridge = ds;
> > } else {
> > bus = pci_root_bus_new(ds, "pxb-internal", NULL, NULL, 0,
> > TYPE_PXB_BUS);
> > bds = qdev_new("pci-bridge");
> > diff --git a/include/hw/acpi/cxl.h b/include/hw/acpi/cxl.h
> > index 7b8f3b8a2e..db2063f8c9 100644
> > --- a/include/hw/acpi/cxl.h
> > +++ b/include/hw/acpi/cxl.h
> > @@ -18,6 +18,10 @@
> > #ifndef HW_ACPI_CXL_H
> > #define HW_ACPI_CXL_H
> >
> > +#include "hw/acpi/bios-linker-loader.h"
> > +
> > +void cxl_build_cedt(GArray *table_offsets, GArray *table_data,
> > + BIOSLinker *linker);
> > void build_cxl_osc_method(Aml *dev);
> >
> > #endif
> > diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
> > index a94d350034..50dd7fdf33 100644
> > --- a/include/hw/pci/pci_bridge.h
> > +++ b/include/hw/pci/pci_bridge.h
> > @@ -28,6 +28,7 @@
> >
> > #include "hw/pci/pci.h"
> > #include "hw/pci/pci_bus.h"
> > +#include "hw/cxl/cxl.h"
> > #include "qom/object.h"
> >
> > typedef struct PCIBridgeWindows PCIBridgeWindows;
> > @@ -81,6 +82,30 @@ struct PCIBridge {
> > #define PCI_BRIDGE_DEV_PROP_MSI "msi"
> > #define PCI_BRIDGE_DEV_PROP_SHPC "shpc"
> >
> > +struct PXBDev {
> > + /*< private >*/
> > + PCIDevice parent_obj;
> > + /*< public >*/
> > +
> > + uint8_t bus_nr;
> > + uint16_t numa_node;
> > + int32_t uid;
> > +
> > + struct cxl_dev {
> > + HostMemoryBackend *memory_window[CXL_WINDOW_MAX];
> > +
> > + uint32_t num_windows;
> > + hwaddr *window_base[CXL_WINDOW_MAX];
> > +
> > + void *cxl_host_bridge; /* Pointer to a CXLHost */
> > + } cxl;
> > +};
> > +
> > +typedef struct PXBDev PXBDev;
> > +#define TYPE_PXB_CXL_DEVICE "pxb-cxl"
> > +DECLARE_INSTANCE_CHECKER(PXBDev, PXB_CXL_DEV,
> > + TYPE_PXB_CXL_DEVICE)
> > +
>
> Seems like this could sensibly be on one line?
> Could have been in earlier patch as well of course.
>
Yeah - this seems to be the convention throughout the code. I didn't follow the
reasoning when it was introduced (which was recently).
> > int pci_bridge_ssvid_init(PCIDevice *dev, uint8_t offset,
> > uint16_t svid, uint16_t ssid,
> > Error **errp);
>
- Re: [RFC PATCH 16/25] hw/pxb/cxl: Add "windows" for host bridges, (continued)
- [RFC PATCH 19/25] hw/cxl/device: Implement MMIO HDM decoding (8.2.5.12), Ben Widawsky, 2020/11/11
- [RFC PATCH 20/25] acpi/cxl: Add _OSC implementation (9.14.2), Ben Widawsky, 2020/11/11
- [RFC PATCH 22/25] acpi/cxl: Create the CEDT (9.14.1), Ben Widawsky, 2020/11/11
- [RFC PATCH 21/25] acpi/cxl: Introduce a compat-driver UUID for CXL _OSC, Ben Widawsky, 2020/11/11
- [RFC PATCH 23/25] Temp: acpi/cxl: Add ACPI0017 (CEDT awareness), Ben Widawsky, 2020/11/11
- [RFC PATCH 25/25] qtest/cxl: Add very basic sanity tests, Ben Widawsky, 2020/11/11
- [RFC PATCH 24/25] WIP: i386/cxl: Initialize a host bridge, Ben Widawsky, 2020/11/11
- Re: [RFC PATCH 00/25] Introduce CXL 2.0 Emulation, Jonathan Cameron, 2020/11/16