[RFC PATCH 0/3] QEMU ACPI generic port support

[RFC PATCH 0/3] QEMU ACPI generic port support

[Dave Jiang]

s small RFC patch series is really a hack on what I need from qemu rather
than a proper implementation. I'm hoping to get some guidance from the list on
how to implement this correctly for qemu upstream. Thank you!

The patch series provides support for the ACPI Generic Port support that's
defined by ACPI spec 6.5 5.2.16.7 (Generic Port Affinity Structure). The
series also adds a genport object that allows locality data to be injected via
qemu commandline to the HMAT tables. The generic port support is to allow a hot
plugged CXL memory device to calculate the locality data from the CPU to
the CXL device. The generic port related data provides the locality data from
the CPU to the CXL host bridge (latency and bandwidth). These data in
addition to the PCIe link data, CDAT from device, and CXL switch CDAT if switch
exist, provides the locality data for the entire path.

Patch1: Adds Generic Port Affinity Structure sub-tables to the SRAT. For
each CXL Host Bridge (HB) a GPAS entry is created with a unique proximity
domain. For example, if the system is created with 4 proximity domains (PXM) for
system memory, then the next GPAS will get PXM 4 and so on.

Patch2: Add the json support for generic port. Split out because
clang-format really clobbers the json files.

Patch3: Add a generic port object. The intention here is to allow setup of
numa nodes, add hmat-lb data and node distance for the generic targets. I had to
add a hack in qemu_create_cli_devices() to realize the genport objects. I need
guidance on where and how to do this properly so the genport objects
realize at the correct place and time.

Example of genport setup:
-object genport,id=$X -numa node,genport=genport$X,nodeid=$Y,initiator=$Z
-numa hmat-lb,initiator=$Z,target=$X,hierarchy=memory,data-type=access-latency,latency=$latency
-numa hmat-lb,initiator=$Z,target=$X,hierarchy=memory,data-type=access-bandwidth,bandwidth=$bandwidthM
for ((i = 0; i < total_nodes; i++)); do
        for ((j = 0; j < cxl_hbs; j++ )); do        # 2 CXL HBs
                -numa dist,src=$i,dst=$X,val=$dist
        done
done
Linux kernel support:
https://lore.kernel.org/linux-cxl/168088732996.1441063.10107817505475386072.stgit@djiang5-mobl3/T/#t

---

Dave Jiang (3):
      hw/acpi: Add support for Generic Port Affinity Structure to SRAT
      genport: Add json support for generic port
      acpi: add generic port device object


 hw/acpi/aml-build.c         | 21 +++++++++++++
 hw/acpi/genport.c           | 61 +++++++++++++++++++++++++++++++++++++
 hw/acpi/meson.build         |  1 +
 hw/i386/acpi-build.c        | 45 +++++++++++++++++++++++++++
 include/hw/acpi/aml-build.h | 27 ++++++++++++++++
 qapi/machine.json           |  3 +-
 qapi/qom.json               | 12 ++++++++
 softmmu/vl.c                | 26 ++++++++++++++++
 8 files changed, 195 insertions(+), 1 deletion(-)
 create mode 100644 hw/acpi/genport.c

--

[RFC PATCH 1/3] hw/acpi: Add support for Generic Port Affinity Structure to SRAT

[Dave Jiang]

The Generic Port Affinity Structure is added for the System Resource
Affinity Table in ACPI r6.4. It provides information on the proximity
domain that's associated with a device handle. This information in
combination with HMAT can be used by the CXL driver to calculate the
bandwidth and latency information between the CPU and the CXL Host Bridge
(HB).

Add a list to account for the ACPI0016 (CXL HB ACPI devices) being
created. Create GAPS entries equivalent to the number of HB devices
constructed by qemu using the list and inject the relevant device handle.

The proximity domain will be set to 0 for simplicity to enable Linux kernel
side debugging and usage of the new SRAT sub-tables.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 hw/acpi/aml-build.c         |   21 +++++++++++++++++++++
 hw/i386/acpi-build.c        |   27 +++++++++++++++++++++++++++
 include/hw/acpi/aml-build.h |   27 +++++++++++++++++++++++++++
 3 files changed, 75 insertions(+)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index ea331a20d131..949759efc0a7 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -1938,6 +1938,27 @@ void build_srat_memory(GArray *table_data, uint64_t base,
     build_append_int_noprefix(table_data, 0, 8); /* Reserved */
 }
 
+/*
+ * ACPI spec, Revision 6.5
+ * 5.2.16.7 Generic Port Affinity Structure
+ */
+void build_srat_generic_port_affinity(GArray *table_data, uint8_t htype,
+                                      int node, ACPIDeviceHandle *handle,
+                                      GenericAffinityFlags flags)
+{
+    build_append_int_noprefix(table_data, 6, 1);     /* Type */
+    build_append_int_noprefix(table_data, 32, 1);    /* Length */
+    build_append_int_noprefix(table_data, 0, 1);     /* Reserved */
+    build_append_int_noprefix(table_data, htype, 1); /* Device Handle Type */
+    build_append_int_noprefix(table_data, node, 4);  /* Proximity Domain */
+    build_append_int_noprefix(table_data, handle->raw[0],
+                              8); /* Device Handle part 1 */
+    build_append_int_noprefix(table_data, handle->raw[1],
+                              8);                    /* Device Handle part 2 */
+    build_append_int_noprefix(table_data, flags, 4); /* Flags */
+    build_append_int_noprefix(table_data, 0, 4);     /* Reserved */
+}
+
 /*
  * ACPI spec 5.2.17 System Locality Distance Information Table
  * (Revision 2.0 or later)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index d449e5b76f30..0d9e610af12b 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -132,6 +132,13 @@ const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio = {
     .bit_width = NVDIMM_ACPI_IO_LEN << 3
 };
 
+typedef struct CxlHBDev {
+    uint32_t uid;
+    QSLIST_ENTRY(CxlHBDev) entry;
+} CxlHBDev;
+
+static QSLIST_HEAD(, CxlHBDev) cxl_hb_list_head;
+
 static void init_common_fadt_data(MachineState *ms, Object *o,
                                   AcpiFadtData *data)
 {
@@ -1507,8 +1514,13 @@ 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)) {
+                CxlHBDev *hb_entry;
                 struct Aml *pkg = aml_package(2);
 
+                hb_entry = g_malloc0(sizeof(*hb_entry));
+                hb_entry->uid = bus_num;
+                QSLIST_INSERT_HEAD(&cxl_hb_list_head, hb_entry, entry);
+
                 aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0016")));
                 aml_append(pkg, aml_eisaid("PNP0A08"));
                 aml_append(pkg, aml_eisaid("PNP0A03"));
@@ -1866,6 +1878,7 @@ static void
 build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
 {
     int i;
+    CxlHBDev *hb_entry;
     int numa_mem_start, slots;
     uint64_t mem_len, mem_base, next_base;
     MachineClass *mc = MACHINE_GET_CLASS(machine);
@@ -1973,6 +1986,18 @@ build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
 
     sgx_epc_build_srat(table_data);
 
+    QSLIST_FOREACH(hb_entry, &cxl_hb_list_head, entry)
+    {
+        ACPIDeviceHandle handle = {
+            .hid = "ACPI0016",
+            .uid = hb_entry->uid,
+        };
+        uint32_t flags = GEN_AFFINITY_ENABLED;
+
+        build_srat_generic_port_affinity(table_data, 0, nb_numa_nodes,
+                                         &handle, flags);
+    }
+
     /*
      * TODO: this part is not in ACPI spec and current linux kernel boots fine
      * without these entries. But I recall there were issues the last time I
@@ -2728,6 +2753,8 @@ void acpi_setup(void)
         return;
     }
 
+    QSLIST_INIT(&cxl_hb_list_head);
+
     build_state = g_malloc0(sizeof *build_state);
 
     acpi_build_tables_init(&tables);
diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index d1fb08514bfa..32a4f574abaa 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -204,6 +204,10 @@ typedef enum {
     AML_PULL_NONE = 3,
 } AmlPinConfig;
 
+/*
+ * ACPI 6.5: Table 5-68 Flags - Generic Initiator/Port Affinity Structure
+ * Flags field definition
+ */
 typedef enum {
     MEM_AFFINITY_NOFLAGS      = 0,
     MEM_AFFINITY_ENABLED      = (1 << 0),
@@ -211,6 +215,25 @@ typedef enum {
     MEM_AFFINITY_NON_VOLATILE = (1 << 2),
 } MemoryAffinityFlags;
 
+/*
+ * ACPI 6.5: Table 5-65 Device Handle - ACPI
+ * Device Handle definition
+ */
+typedef union ACPIDeviceHandle {
+    struct {
+        uint8_t hid[8];
+        uint32_t uid;
+        uint32_t reserved;
+    };
+    uint64_t raw[2];
+} ACPIDeviceHandle;
+
+typedef enum {
+    GEN_AFFINITY_NOFLAGS = 0,
+    GEN_AFFINITY_ENABLED = (1 << 0),
+    GEN_AFFINITY_ARCH_TRANS = (2 << 0),
+} GenericAffinityFlags;
+
 typedef
 struct AcpiBuildTables {
     GArray *table_data;
@@ -486,6 +509,10 @@ Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set, uint32_t io_offset,
 void build_srat_memory(GArray *table_data, uint64_t base,
                        uint64_t len, int node, MemoryAffinityFlags flags);
 
+void build_srat_generic_port_affinity(GArray *table_data, uint8_t htype,
+                                      int node, ACPIDeviceHandle *handle,
+                                      GenericAffinityFlags flags);
+
 void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms,
                 const char *oem_id, const char *oem_table_id);
 

[RFC PATCH 2/3] genport: Add json support for generic port

[Dave Jiang]

Add QOM json update for ACPI generic port object to support HMAT
enumeration.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 qapi/machine.json |    3 ++-
 qapi/qom.json     |   12 ++++++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/qapi/machine.json b/qapi/machine.json
index 068427b8feb8..39cb5bd713f6 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -479,7 +479,8 @@
    '*cpus':   ['uint16'],
    '*mem':    'size',
    '*memdev': 'str',
-   '*initiator': 'uint16' }}
+   '*initiator': 'uint16',
+   '*genport': 'str' }}
 
 ##
 # @NumaDistOptions:
diff --git a/qapi/qom.json b/qapi/qom.json
index 30e76653ad28..8f5faff49114 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -444,6 +444,16 @@
   'base': 'NetfilterProperties',
   'data': { '*vnet_hdr_support': 'bool' } }
 
+##
+# @GenericPortDeviceProperties:
+#
+# Properties for generic port devices.
+#
+# Since: 7.2
+##
+{ 'struct': 'GenericPortDeviceProperties',
+  'data': {} }
+
 ##
 # @InputBarrierProperties:
 #
@@ -886,6 +896,7 @@
     'filter-redirector',
     'filter-replay',
     'filter-rewriter',
+    'genport',
     'input-barrier',
     { 'name': 'input-linux',
       'if': 'CONFIG_LINUX' },
@@ -955,6 +966,7 @@
       'filter-redirector':          'FilterRedirectorProperties',
       'filter-replay':              'NetfilterProperties',
       'filter-rewriter':            'FilterRewriterProperties',
+      'genport':                    'GenericPortDeviceProperties',
       'input-barrier':              'InputBarrierProperties',
       'input-linux':                { 'type': 'InputLinuxProperties',
                                       'if': 'CONFIG_LINUX' },

[RFC PATCH 3/3] acpi: add generic port device object

[Dave Jiang]

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 hw/acpi/genport.c           |   61 +++++++++++++++++++++++++++++++++++++++++++
 hw/acpi/meson.build         |    1 +
 hw/i386/acpi-build.c        |   32 ++++++++++++++++++-----
 include/hw/acpi/aml-build.h |    4 +--
 softmmu/vl.c                |   26 ++++++++++++++++++
 5 files changed, 115 insertions(+), 9 deletions(-)
 create mode 100644 hw/acpi/genport.c

diff --git a/hw/acpi/genport.c b/hw/acpi/genport.c
new file mode 100644
index 000000000000..5738730323c2
--- /dev/null
+++ b/hw/acpi/genport.c
@@ -0,0 +1,61 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Generic Port device implementation
+ *
+ * Copyright (C) 2023 Intel Corporation
+ */
+#include "qemu/osdep.h"
+#include "hw/qdev-properties.h"
+#include "qapi/error.h"
+#include "qapi/visitor.h"
+#include "qom/object_interfaces.h"
+#include "hw/qdev-core.h"
+
+#define TYPE_GENERIC_PORT_DEVICE "genport"
+
+#define GENPORT_NUMA_NODE_PROP "node"
+#define GENPORT_DEV_PROP       "genport"
+
+typedef struct GenericPortDevice {
+    /* private */
+    DeviceState parent_obj;
+
+    /* public */
+    uint32_t node;
+} GenericPortDevice;
+
+typedef struct GenericPortDeviceClass {
+        DeviceClass parent_class;
+} GenericPortDeviceClass;
+
+static Property genport_properties[] = {
+    DEFINE_PROP_UINT32(GENPORT_NUMA_NODE_PROP, GenericPortDevice, node, 0),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+OBJECT_DEFINE_TYPE_WITH_INTERFACES(GenericPortDevice, genport_device,
+                                   GENERIC_PORT_DEVICE, DEVICE,
+                                   { TYPE_USER_CREATABLE },
+                                   { NULL })
+
+static void genport_device_init(Object *obj)
+{
+}
+
+static void genport_device_finalize(Object *obj)
+{
+}
+
+static void genport_device_realize(DeviceState *dev, Error **errp)
+{
+}
+
+static void genport_device_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+
+    dc->realize = genport_device_realize;
+    dc->desc = "Generic Port";
+    device_class_set_props(dc, genport_properties);
+}
+
diff --git a/hw/acpi/meson.build b/hw/acpi/meson.build
index e0bf39bf4cd6..5247554998b0 100644
--- a/hw/acpi/meson.build
+++ b/hw/acpi/meson.build
@@ -5,6 +5,7 @@ acpi_ss.add(files(
   'bios-linker-loader.c',
   'core.c',
   'utils.c',
+  'genport.c',
 ))
 acpi_ss.add(when: 'CONFIG_ACPI_CPU_HOTPLUG', if_true: files('cpu.c', 'cpu_hotplug.c'))
 acpi_ss.add(when: 'CONFIG_ACPI_CPU_HOTPLUG', if_false: files('acpi-cpu-hotplug-stub.c'))
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 0d9e610af12b..db850bfd170d 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1514,12 +1514,22 @@ 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)) {
-                CxlHBDev *hb_entry;
+                CxlHBDev *hb_entry, *match;
+                bool found = false;
                 struct Aml *pkg = aml_package(2);
 
-                hb_entry = g_malloc0(sizeof(*hb_entry));
-                hb_entry->uid = bus_num;
-                QSLIST_INSERT_HEAD(&cxl_hb_list_head, hb_entry, entry);
+                QSLIST_FOREACH(match, &cxl_hb_list_head, entry)
+                {
+                    if (match->uid == bus_num) {
+                        found = true;
+                        break;
+                    }
+                }
+                if (!found) {
+                    hb_entry = g_malloc0(sizeof(*hb_entry));
+                    hb_entry->uid = bus_num;
+                    QSLIST_INSERT_HEAD(&cxl_hb_list_head, hb_entry, entry);
+                }
 
                 aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0016")));
                 aml_append(pkg, aml_eisaid("PNP0A08"));
@@ -1892,6 +1902,7 @@ build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
                                 NULL);
     AcpiTable table = { .sig = "SRAT", .rev = 1, .oem_id = x86ms->oem_id,
                         .oem_table_id = x86ms->oem_table_id };
+    int pxm_domain;
 
     acpi_table_begin(&table, table_data);
     build_append_int_noprefix(table_data, 1, 4); /* Reserved */
@@ -1986,16 +1997,23 @@ build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
 
     sgx_epc_build_srat(table_data);
 
+    /* FIXME: this is a hack, need a node property for genport */
+    pxm_domain = 6;
     QSLIST_FOREACH(hb_entry, &cxl_hb_list_head, entry)
     {
         ACPIDeviceHandle handle = {
             .hid = "ACPI0016",
-            .uid = hb_entry->uid,
+            .reserved = { 0 },
         };
+        char uid_str[5];
         uint32_t flags = GEN_AFFINITY_ENABLED;
 
-        build_srat_generic_port_affinity(table_data, 0, nb_numa_nodes,
-                                         &handle, flags);
+        snprintf(uid_str, 4, "%u", hb_entry->uid);
+        memcpy(handle.uid, uid_str, 4);
+
+        build_srat_generic_port_affinity(table_data, 0, pxm_domain, &handle,
+                                         flags);
+        pxm_domain++;
     }
 
     /*
diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index 32a4f574abaa..4ad7f0268f65 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -222,8 +222,8 @@ typedef enum {
 typedef union ACPIDeviceHandle {
     struct {
         uint8_t hid[8];
-        uint32_t uid;
-        uint32_t reserved;
+        uint8_t uid[4];
+        uint8_t reserved[4];
     };
     uint64_t raw[2];
 } ACPIDeviceHandle;
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 459588aa7d14..1ef1d9e5f383 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2523,6 +2523,31 @@ static void qemu_init_board(void)
     }
 }
 
+static int genport_realize_cb(Object *obj, void *opaque)
+{
+    DeviceState *dev = DEVICE(object_dynamic_cast(obj, TYPE_DEVICE));
+    ObjectClass *klass = object_get_class(obj);
+    ObjectClass *type_klass = object_class_by_name("genport");
+
+    if (!dev)
+            return 0;
+
+    if (dev->realized)
+            return 0;
+
+    if (klass != type_klass)
+            return 0;
+
+    qdev_realize(dev, NULL, &error_fatal);
+    return 0;
+}
+
+static void genports_realize(void)
+{
+        object_child_foreach_recursive(object_get_root(),
+                        genport_realize_cb, NULL);
+}
+
 static void qemu_create_cli_devices(void)
 {
     DeviceOption *opt;
@@ -2557,6 +2582,7 @@ static void qemu_create_cli_devices(void)
         loc_pop(&opt->loc);
     }
     rom_reset_order_override();
+    genports_realize();
 }
 
 static void qemu_machine_creation_done(void)

Re: [RFC PATCH 1/3] hw/acpi: Add support for Generic Port Affinity Structure to SRAT

[Jonathan Cameron via]

On Tue, 18 Apr 2023 15:21:42 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> The Generic Port Affinity Structure is added for the System Resource
> Affinity Table in ACPI r6.4. It provides information on the proximity
> domain that's associated with a device handle. This information in
> combination with HMAT can be used by the CXL driver to calculate the
> bandwidth and latency information between the CPU and the CXL Host Bridge
> (HB).
> 
> Add a list to account for the ACPI0016 (CXL HB ACPI devices) being
> created. Create GAPS entries equivalent to the number of HB devices
> constructed by qemu using the list and inject the relevant device handle.
> 
> The proximity domain will be set to 0 for simplicity to enable Linux kernel
> side debugging and usage of the new SRAT sub-tables.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

Hi Dave,

Some general comments inline.

> ---
>  hw/acpi/aml-build.c         |   21 +++++++++++++++++++++
>  hw/i386/acpi-build.c        |   27 +++++++++++++++++++++++++++
>  include/hw/acpi/aml-build.h |   27 +++++++++++++++++++++++++++
>  3 files changed, 75 insertions(+)
> 
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index ea331a20d131..949759efc0a7 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -1938,6 +1938,27 @@ void build_srat_memory(GArray *table_data, uint64_t base,
>      build_append_int_noprefix(table_data, 0, 8); /* Reserved */
>  }
>  
> +/*
> + * ACPI spec, Revision 6.5
> + * 5.2.16.7 Generic Port Affinity Structure
> + */
> +void build_srat_generic_port_affinity(GArray *table_data, uint8_t htype,
> +                                      int node, ACPIDeviceHandle *handle,
> +                                      GenericAffinityFlags flags)
> +{
> +    build_append_int_noprefix(table_data, 6, 1);     /* Type */
> +    build_append_int_noprefix(table_data, 32, 1);    /* Length */
> +    build_append_int_noprefix(table_data, 0, 1);     /* Reserved */
> +    build_append_int_noprefix(table_data, htype, 1); /* Device Handle Type */
> +    build_append_int_noprefix(table_data, node, 4);  /* Proximity Domain */
> +    build_append_int_noprefix(table_data, handle->raw[0],
> +                              8); /* Device Handle part 1 */
> +    build_append_int_noprefix(table_data, handle->raw[1],
> +                              8);                    /* Device Handle part 2 */
> +    build_append_int_noprefix(table_data, flags, 4); /* Flags */
> +    build_append_int_noprefix(table_data, 0, 4);     /* Reserved */
> +}
> +
>  /*
>   * ACPI spec 5.2.17 System Locality Distance Information Table
>   * (Revision 2.0 or later)
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index d449e5b76f30..0d9e610af12b 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -132,6 +132,13 @@ const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio = {
>      .bit_width = NVDIMM_ACPI_IO_LEN << 3
>  };
>  
> +typedef struct CxlHBDev {
> +    uint32_t uid;
> +    QSLIST_ENTRY(CxlHBDev) entry;
> +} CxlHBDev;
> +
> +static QSLIST_HEAD(, CxlHBDev) cxl_hb_list_head;
> +
>  static void init_common_fadt_data(MachineState *ms, Object *o,
>                                    AcpiFadtData *data)
>  {
> @@ -1507,8 +1514,13 @@ 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)) {
> +                CxlHBDev *hb_entry;
>                  struct Aml *pkg = aml_package(2);
>  
> +                hb_entry = g_malloc0(sizeof(*hb_entry));
> +                hb_entry->uid = bus_num;
> +                QSLIST_INSERT_HEAD(&cxl_hb_list_head, hb_entry, entry);

Who cleans this up?  Need to deal with the no numa nodes case where SRAT isn't
built, but otherwise I think you can clean it up immediately after that.


> +
>                  aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0016")));
>                  aml_append(pkg, aml_eisaid("PNP0A08"));
>                  aml_append(pkg, aml_eisaid("PNP0A03"));
> @@ -1866,6 +1878,7 @@ static void
>  build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
>  {
>      int i;
> +    CxlHBDev *hb_entry;
>      int numa_mem_start, slots;
>      uint64_t mem_len, mem_base, next_base;
>      MachineClass *mc = MACHINE_GET_CLASS(machine);
> @@ -1973,6 +1986,18 @@ build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
>  
>      sgx_epc_build_srat(table_data);
>  
> +    QSLIST_FOREACH(hb_entry, &cxl_hb_list_head, entry)
> +    {

Bracket on previous line looks to be standard in this file.  I'm too lazy
to check for the QEMU coding style guideline on this ;)

> +        ACPIDeviceHandle handle = {
> +            .hid = "ACPI0016",
> +            .uid = hb_entry->uid,
> +        };
> +        uint32_t flags = GEN_AFFINITY_ENABLED;
> +
> +        build_srat_generic_port_affinity(table_data, 0, nb_numa_nodes,
Comment needed on why that nb_numa_nodes value. 

No obvious reason all Generic Ports should be in the same node?  In general
I'd say they shouldn't be.  Typically on different sockets etc.

> +                                         &handle, flags);
> +    }
> +
>      /*
>       * TODO: this part is not in ACPI spec and current linux kernel boots fine
>       * without these entries. But I recall there were issues the last time I
> @@ -2728,6 +2753,8 @@ void acpi_setup(void)
>          return;
>      }
>  
> +    QSLIST_INIT(&cxl_hb_list_head);
> +
>      build_state = g_malloc0(sizeof *build_state);
>  
>      acpi_build_tables_init(&tables);
> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index d1fb08514bfa..32a4f574abaa 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -204,6 +204,10 @@ typedef enum {
>      AML_PULL_NONE = 3,
>  } AmlPinConfig;
>  
> +/*
> + * ACPI 6.5: Table 5-68 Flags - Generic Initiator/Port Affinity Structure
> + * Flags field definition
Nope. Comment on the wrong enum.  This looks to be the 5-59 one


> + */
>  typedef enum {
>      MEM_AFFINITY_NOFLAGS      = 0,
>      MEM_AFFINITY_ENABLED      = (1 << 0),
> @@ -211,6 +215,25 @@ typedef enum {
>      MEM_AFFINITY_NON_VOLATILE = (1 << 2),
>  } MemoryAffinityFlags;
>  
> +/*
> + * ACPI 6.5: Table 5-65 Device Handle - ACPI
> + * Device Handle definition
> + */
> +typedef union ACPIDeviceHandle {
> +    struct {
> +        uint8_t hid[8];
> +        uint32_t uid;
> +        uint32_t reserved;
> +    };
> +    uint64_t raw[2];

Does the PCI version of this also make sense here?  Defining it might make it
more obvious why this needs to be handled as a union.
There are a bunch of complexities with the PCI form, though not sure the affect
the RP cases we care about here.  They may be an issue if someone has
an embedded switch so we are actually pointing to a DSP here. (is that allowed?)

I'd gotten it into my head that this would point to the RP, but actually it's
the host bridge (via the e.g. in the ACPI spec)  SO ACPI ID is only option for
this.  I'd love to follow up with Generic Initiators, but can solve that at the time.


> +} ACPIDeviceHandle;
> +

Above comment belongs down here.

> +typedef enum {
> +    GEN_AFFINITY_NOFLAGS = 0,
> +    GEN_AFFINITY_ENABLED = (1 << 0),
> +    GEN_AFFINITY_ARCH_TRANS = (2 << 0),

Odd way to define this.  (1 << 1)?
 
> +} GenericAffinityFlags;
> +
>  typedef
>  struct AcpiBuildTables {
>      GArray *table_data;
> @@ -486,6 +509,10 @@ Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set, uint32_t io_offset,
>  void build_srat_memory(GArray *table_data, uint64_t base,
>                         uint64_t len, int node, MemoryAffinityFlags flags);
>  
> +void build_srat_generic_port_affinity(GArray *table_data, uint8_t htype,
> +                                      int node, ACPIDeviceHandle *handle,
> +                                      GenericAffinityFlags flags);
> +
>  void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms,
>                  const char *oem_id, const char *oem_table_id);
>  
> 
> 

Re: [RFC PATCH 0/3] QEMU ACPI generic port support

[Jonathan Cameron via]

On Tue, 18 Apr 2023 15:21:36 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> s small RFC patch series is really a hack on what I need from qemu rather
> than a proper implementation. I'm hoping to get some guidance from the list on
> how to implement this correctly for qemu upstream. Thank you!
> 
> The patch series provides support for the ACPI Generic Port support that's
> defined by ACPI spec 6.5 5.2.16.7 (Generic Port Affinity Structure). The
> series also adds a genport object that allows locality data to be injected via
> qemu commandline to the HMAT tables. The generic port support is to allow a hot
> plugged CXL memory device to calculate the locality data from the CPU to
> the CXL device. The generic port related data provides the locality data from
> the CPU to the CXL host bridge (latency and bandwidth). These data in
> addition to the PCIe link data, CDAT from device, and CXL switch CDAT if switch
> exist, provides the locality data for the entire path.
> 
> Patch1: Adds Generic Port Affinity Structure sub-tables to the SRAT. For
> each CXL Host Bridge (HB) a GPAS entry is created with a unique proximity
> domain. For example, if the system is created with 4 proximity domains (PXM) for
> system memory, then the next GPAS will get PXM 4 and so on.

I may be going crazy but I'm not seeing an increment on the numa node. So I think
they all get 4 at the moment. Found it increment in patch 3.

> 
> Patch2: Add the json support for generic port. Split out because
> clang-format really clobbers the json files.
> 
> Patch3: Add a generic port object. The intention here is to allow setup of
> numa nodes, add hmat-lb data and node distance for the generic targets. I had to
> add a hack in qemu_create_cli_devices() to realize the genport objects. I need
> guidance on where and how to do this properly so the genport objects
> realize at the correct place and time.
> 
> Example of genport setup:
> -object genport,id=$X -numa node,genport=genport$X,nodeid=$Y,initiator=$Z
> -numa hmat-lb,initiator=$Z,target=$X,hierarchy=memory,data-type=access-latency,latency=$latency
> -numa hmat-lb,initiator=$Z,target=$X,hierarchy=memory,data-type=access-bandwidth,bandwidth=$bandwidthM

I think we should be using some links to the host bridges in here.
So I don't think there should be an explicit genport object at all.
Instead we should be able to point at the pxb itself.  Perhaps also
allowing other port types in future.

Something like

-device pxb-cxl,id=cxl1.1
-numa node,genport=cxl1.1,nodeid=$X
-numa hmat-lb,initiator=$Z,target=$X,...
-numa hmat-lb,initiator=$X,target=$Y,...
//as generic port goes both ways.

As we are currently using bus_nr for UID (which is admittedly a somewhat dirty hack that
just happened to be convenient) the ACPI building code can use that to fill in the SRAT
entry at appropriate point.

I haven't tried implementing it so there may well be some ordering issues that
require some late binding etc, but it should be possible to make it work.

> for ((i = 0; i < total_nodes; i++)); do
>         for ((j = 0; j < cxl_hbs; j++ )); do        # 2 CXL HBs
>                 -numa dist,src=$i,dst=$X,val=$dist
>         done
> done
> Linux kernel support:
> https://lore.kernel.org/linux-cxl/168088732996.1441063.10107817505475386072.stgit@djiang5-mobl3/T/#t
> 
> ---
> 
> Dave Jiang (3):
>       hw/acpi: Add support for Generic Port Affinity Structure to SRAT
>       genport: Add json support for generic port
>       acpi: add generic port device object
> 
> 
>  hw/acpi/aml-build.c         | 21 +++++++++++++
>  hw/acpi/genport.c           | 61 +++++++++++++++++++++++++++++++++++++
>  hw/acpi/meson.build         |  1 +
>  hw/i386/acpi-build.c        | 45 +++++++++++++++++++++++++++
>  include/hw/acpi/aml-build.h | 27 ++++++++++++++++
>  qapi/machine.json           |  3 +-
>  qapi/qom.json               | 12 ++++++++
>  softmmu/vl.c                | 26 ++++++++++++++++
>  8 files changed, 195 insertions(+), 1 deletion(-)
>  create mode 100644 hw/acpi/genport.c
> 
> --
> 
> 

Re: [RFC PATCH 0/3] QEMU ACPI generic port support

[Dave Jiang]

On 5/3/23 3:42 AM, Jonathan Cameron wrote:
> On Tue, 18 Apr 2023 15:21:36 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> s small RFC patch series is really a hack on what I need from qemu rather
>> than a proper implementation. I'm hoping to get some guidance from the list on
>> how to implement this correctly for qemu upstream. Thank you!
>>
>> The patch series provides support for the ACPI Generic Port support that's
>> defined by ACPI spec 6.5 5.2.16.7 (Generic Port Affinity Structure). The
>> series also adds a genport object that allows locality data to be injected via
>> qemu commandline to the HMAT tables. The generic port support is to allow a hot
>> plugged CXL memory device to calculate the locality data from the CPU to
>> the CXL device. The generic port related data provides the locality data from
>> the CPU to the CXL host bridge (latency and bandwidth). These data in
>> addition to the PCIe link data, CDAT from device, and CXL switch CDAT if switch
>> exist, provides the locality data for the entire path.
>>
>> Patch1: Adds Generic Port Affinity Structure sub-tables to the SRAT. For
>> each CXL Host Bridge (HB) a GPAS entry is created with a unique proximity
>> domain. For example, if the system is created with 4 proximity domains (PXM) for
>> system memory, then the next GPAS will get PXM 4 and so on.
> 
> I may be going crazy but I'm not seeing an increment on the numa node. So I think
> they all get 4 at the moment. Found it increment in patch 3.
> 
>>
>> Patch2: Add the json support for generic port. Split out because
>> clang-format really clobbers the json files.
>>
>> Patch3: Add a generic port object. The intention here is to allow setup of
>> numa nodes, add hmat-lb data and node distance for the generic targets. I had to
>> add a hack in qemu_create_cli_devices() to realize the genport objects. I need
>> guidance on where and how to do this properly so the genport objects
>> realize at the correct place and time.
>>
>> Example of genport setup:
>> -object genport,id=$X -numa node,genport=genport$X,nodeid=$Y,initiator=$Z
>> -numa hmat-lb,initiator=$Z,target=$X,hierarchy=memory,data-type=access-latency,latency=$latency
>> -numa hmat-lb,initiator=$Z,target=$X,hierarchy=memory,data-type=access-bandwidth,bandwidth=$bandwidthM
> 
> I think we should be using some links to the host bridges in here.
> So I don't think there should be an explicit genport object at all.
> Instead we should be able to point at the pxb itself.  Perhaps also
> allowing other port types in future.
> 
> Something like
> 
> -device pxb-cxl,id=cxl1.1
> -numa node,genport=cxl1.1,nodeid=$X
> -numa hmat-lb,initiator=$Z,target=$X,...
> -numa hmat-lb,initiator=$X,target=$Y,...
> //as generic port goes both ways.
> 
> As we are currently using bus_nr for UID (which is admittedly a somewhat dirty hack that
> just happened to be convenient) the ACPI building code can use that to fill in the SRAT
> entry at appropriate point.
> 
> I haven't tried implementing it so there may well be some ordering issues that
> require some late binding etc, but it should be possible to make it work.

Ok thanks for the suggestion Jonathan! I appreciate you looking over 
this. I'll take a look when I get a chance.

> 
>> for ((i = 0; i < total_nodes; i++)); do
>>          for ((j = 0; j < cxl_hbs; j++ )); do        # 2 CXL HBs
>>                  -numa dist,src=$i,dst=$X,val=$dist
>>          done
>> done
>> Linux kernel support:
>> https://lore.kernel.org/linux-cxl/168088732996.1441063.10107817505475386072.stgit@djiang5-mobl3/T/#t
>>
>> ---
>>
>> Dave Jiang (3):
>>        hw/acpi: Add support for Generic Port Affinity Structure to SRAT
>>        genport: Add json support for generic port
>>        acpi: add generic port device object
>>
>>
>>   hw/acpi/aml-build.c         | 21 +++++++++++++
>>   hw/acpi/genport.c           | 61 +++++++++++++++++++++++++++++++++++++
>>   hw/acpi/meson.build         |  1 +
>>   hw/i386/acpi-build.c        | 45 +++++++++++++++++++++++++++
>>   include/hw/acpi/aml-build.h | 27 ++++++++++++++++
>>   qapi/machine.json           |  3 +-
>>   qapi/qom.json               | 12 ++++++++
>>   softmmu/vl.c                | 26 ++++++++++++++++
>>   8 files changed, 195 insertions(+), 1 deletion(-)
>>   create mode 100644 hw/acpi/genport.c
>>
>> --
>>
>>
> 

Re: [RFC PATCH 0/3] QEMU ACPI generic port support

[Dave Jiang]

On 5/3/23 3:42 AM, Jonathan Cameron wrote:
> On Tue, 18 Apr 2023 15:21:36 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> s small RFC patch series is really a hack on what I need from qemu rather
>> than a proper implementation. I'm hoping to get some guidance from the list on
>> how to implement this correctly for qemu upstream. Thank you!
>>
>> The patch series provides support for the ACPI Generic Port support that's
>> defined by ACPI spec 6.5 5.2.16.7 (Generic Port Affinity Structure). The
>> series also adds a genport object that allows locality data to be injected via
>> qemu commandline to the HMAT tables. The generic port support is to allow a hot
>> plugged CXL memory device to calculate the locality data from the CPU to
>> the CXL device. The generic port related data provides the locality data from
>> the CPU to the CXL host bridge (latency and bandwidth). These data in
>> addition to the PCIe link data, CDAT from device, and CXL switch CDAT if switch
>> exist, provides the locality data for the entire path.
>>
>> Patch1: Adds Generic Port Affinity Structure sub-tables to the SRAT. For
>> each CXL Host Bridge (HB) a GPAS entry is created with a unique proximity
>> domain. For example, if the system is created with 4 proximity domains (PXM) for
>> system memory, then the next GPAS will get PXM 4 and so on.
> 
> I may be going crazy but I'm not seeing an increment on the numa node. So I think
> they all get 4 at the moment. Found it increment in patch 3.

Sorry about that. There are some changes for 1/3 strayed into 3/3.

> 
>>
>> Patch2: Add the json support for generic port. Split out because
>> clang-format really clobbers the json files.
>>
>> Patch3: Add a generic port object. The intention here is to allow setup of
>> numa nodes, add hmat-lb data and node distance for the generic targets. I had to
>> add a hack in qemu_create_cli_devices() to realize the genport objects. I need
>> guidance on where and how to do this properly so the genport objects
>> realize at the correct place and time.
>>
>> Example of genport setup:
>> -object genport,id=$X -numa node,genport=genport$X,nodeid=$Y,initiator=$Z
>> -numa hmat-lb,initiator=$Z,target=$X,hierarchy=memory,data-type=access-latency,latency=$latency
>> -numa hmat-lb,initiator=$Z,target=$X,hierarchy=memory,data-type=access-bandwidth,bandwidth=$bandwidthM
> 
> I think we should be using some links to the host bridges in here.
> So I don't think there should be an explicit genport object at all.
> Instead we should be able to point at the pxb itself.  Perhaps also
> allowing other port types in future.
> 
> Something like
> 
> -device pxb-cxl,id=cxl1.1
> -numa node,genport=cxl1.1,nodeid=$X

Ok I think that makes sense. So now there's a relation between genport 
being constructed and the passed in numa node. When we are building the 
SRAT, I assume there's a way to get hold of the parsed numa nodes 
attributes and iterate through to attempt a match?

> -numa hmat-lb,initiator=$Z,target=$X,...
> -numa hmat-lb,initiator=$X,target=$Y,...
> //as generic port goes both ways.
> 
> As we are currently using bus_nr for UID (which is admittedly a somewhat dirty hack that
> just happened to be convenient) the ACPI building code can use that to fill in the SRAT
> entry at appropriate point.
> 
> I haven't tried implementing it so there may well be some ordering issues that
> require some late binding etc, but it should be possible to make it work.
> 
>> for ((i = 0; i < total_nodes; i++)); do
>>          for ((j = 0; j < cxl_hbs; j++ )); do        # 2 CXL HBs
>>                  -numa dist,src=$i,dst=$X,val=$dist
>>          done
>> done
>> Linux kernel support:
>> https://lore.kernel.org/linux-cxl/168088732996.1441063.10107817505475386072.stgit@djiang5-mobl3/T/#t
>>
>> ---
>>
>> Dave Jiang (3):
>>        hw/acpi: Add support for Generic Port Affinity Structure to SRAT
>>        genport: Add json support for generic port
>>        acpi: add generic port device object
>>
>>
>>   hw/acpi/aml-build.c         | 21 +++++++++++++
>>   hw/acpi/genport.c           | 61 +++++++++++++++++++++++++++++++++++++
>>   hw/acpi/meson.build         |  1 +
>>   hw/i386/acpi-build.c        | 45 +++++++++++++++++++++++++++
>>   include/hw/acpi/aml-build.h | 27 ++++++++++++++++
>>   qapi/machine.json           |  3 +-
>>   qapi/qom.json               | 12 ++++++++
>>   softmmu/vl.c                | 26 ++++++++++++++++
>>   8 files changed, 195 insertions(+), 1 deletion(-)
>>   create mode 100644 hw/acpi/genport.c
>>
>> --
>>
>>
>