diff mbox series

[5/5] hw/mem/cxl_type3: Refactor CDAT sub-table entry initialization into a function

Message ID 20221012182120.174142-6-gregory.price@memverge.com
State New
Headers show
Series [1/5] hw/mem/cxl_type3: fix checkpatch errors | expand

Commit Message

Gregory Price Oct. 12, 2022, 6:21 p.m. UTC 
The CDAT can contain multiple entries for multiple memory regions, this
will allow us to re-use the initialization code when volatile memory
region support is added.

Signed-off-by: Gregory Price <gregory.price@memverge.com>
---
 hw/mem/cxl_type3.c | 137 ++++++++++++++++++++++++---------------------
 1 file changed, 72 insertions(+), 65 deletions(-)

Comments

Gregory Price Oct. 13, 2022, 7:40 p.m. UTC | #1 
> >      /* For now, no memory side cache, plausiblish numbers */
> > -    *dslbis_nonvolatile1 = (CDATDslbis) {
> > +    *dslbis1 = (CDATDslbis) {
> >          .header = {
> >              .type = CDAT_TYPE_DSLBIS,
> > -            .length = sizeof(*dslbis_nonvolatile1),
> > +            .length = sizeof(*dslbis1),
> >          },
> > -        .handle = nonvolatile_dsmad,
> > +        .handle = dsmad_handle,
> >          .flags = HMAT_LB_MEM_MEMORY,
> >          .data_type = HMAT_LB_DATA_READ_LATENCY,
> >          .entry_base_unit = 10000, /* 10ns base */
> >          .entry[0] = 15, /* 150ns */
> 
> If we are going to wrap this up for volatile / non-volatile 
> we probably need to pass in a reasonable value for these.
> Whilst not technically always true, to test the Linux handling
> I'd want non-volatile to report as longer latency.
> 

Here's a good question

Do we want the base unit and entry to be adjustable for volatile and
nonvolatile regions for the purpose of testing?  Or should this simply
be a static value for each?

Since we need to pass in (is_pmem/is_nonvolatile) or whatever into the
cdat function, we could just use that to do one of a few options:
    1) Select from a static value
    2) Select a static value and apply a multiplier for nvmem
    3) Use a base/value provided by the use and apply a multiplier
    4) Make vmem and pmem have separately configurable latencies
diff mbox series

Patch

diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index 220b9f09a9..3c5485abd0 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -19,117 +19,93 @@ 
 #define DWORD_BYTE 4
 #define CT3_CDAT_SUBTABLE_SIZE 6
 
-static int ct3_build_cdat_table(CDATSubHeader ***cdat_table,
-                                void *priv)
+static int ct3_build_cdat_subtable(CDATSubHeader **cdat_table,
+        MemoryRegion *mr, int dsmad_handle)
 {
-    g_autofree CDATDsmas *dsmas_nonvolatile = NULL;
-    g_autofree CDATDslbis *dslbis_nonvolatile1 = NULL;
-    g_autofree CDATDslbis *dslbis_nonvolatile2 = NULL;
-    g_autofree CDATDslbis *dslbis_nonvolatile3 = NULL;
-    g_autofree CDATDslbis *dslbis_nonvolatile4 = NULL;
-    g_autofree CDATDsemts *dsemts_nonvolatile = NULL;
-    CXLType3Dev *ct3d = priv;
-    int next_dsmad_handle = 0;
-    int nonvolatile_dsmad = -1;
-    MemoryRegion *mr;
-
-    if (!ct3d->hostmem) {
-        return 0;
-    }
-
-    mr = host_memory_backend_get_memory(ct3d->hostmem);
-    if (!mr) {
-        return -EINVAL;
-    }
-
-    *cdat_table = g_malloc0(CT3_CDAT_SUBTABLE_SIZE * sizeof(*cdat_table));
-    if (!*cdat_table) {
-        return -ENOMEM;
-    }
-
-    /* Non volatile aspects */
-    dsmas_nonvolatile = g_malloc(sizeof(*dsmas_nonvolatile));
-    dslbis_nonvolatile1 = g_malloc(sizeof(*dslbis_nonvolatile1));
-    dslbis_nonvolatile2 = g_malloc(sizeof(*dslbis_nonvolatile2));
-    dslbis_nonvolatile3 = g_malloc(sizeof(*dslbis_nonvolatile3));
-    dslbis_nonvolatile4 = g_malloc(sizeof(*dslbis_nonvolatile4));
-    dsemts_nonvolatile = g_malloc(sizeof(*dsemts_nonvolatile));
-
-    if (!dsmas_nonvolatile || !dsemts_nonvolatile ||
-        !dslbis_nonvolatile1 || !dslbis_nonvolatile2 ||
-        !dslbis_nonvolatile3 || !dslbis_nonvolatile4) {
-        g_free(*cdat_table);
-        *cdat_table = NULL;
+    g_autofree CDATDsmas *dsmas = NULL;
+    g_autofree CDATDslbis *dslbis1 = NULL;
+    g_autofree CDATDslbis *dslbis2 = NULL;
+    g_autofree CDATDslbis *dslbis3 = NULL;
+    g_autofree CDATDslbis *dslbis4 = NULL;
+    g_autofree CDATDsemts *dsemts = NULL;
+
+    dsmas = g_malloc(sizeof(*dsmas));
+    dslbis1 = g_malloc(sizeof(*dslbis1));
+    dslbis2 = g_malloc(sizeof(*dslbis2));
+    dslbis3 = g_malloc(sizeof(*dslbis3));
+    dslbis4 = g_malloc(sizeof(*dslbis4));
+    dsemts = g_malloc(sizeof(*dsemts));
+
+    if (!dsmas || !dslbis1 || !dslbis2 || !dslbis3 || !dslbis4 || !dsemts) {
         return -ENOMEM;
     }
 
-    nonvolatile_dsmad = next_dsmad_handle++;
-    *dsmas_nonvolatile = (CDATDsmas) {
+    *dsmas = (CDATDsmas) {
         .header = {
             .type = CDAT_TYPE_DSMAS,
-            .length = sizeof(*dsmas_nonvolatile),
+            .length = sizeof(*dsmas),
         },
-        .DSMADhandle = nonvolatile_dsmad,
+        .DSMADhandle = dsmad_handle,
         .flags = CDAT_DSMAS_FLAG_NV,
         .DPA_base = 0,
         .DPA_length = int128_get64(mr->size),
     };
 
     /* For now, no memory side cache, plausiblish numbers */
-    *dslbis_nonvolatile1 = (CDATDslbis) {
+    *dslbis1 = (CDATDslbis) {
         .header = {
             .type = CDAT_TYPE_DSLBIS,
-            .length = sizeof(*dslbis_nonvolatile1),
+            .length = sizeof(*dslbis1),
         },
-        .handle = nonvolatile_dsmad,
+        .handle = dsmad_handle,
         .flags = HMAT_LB_MEM_MEMORY,
         .data_type = HMAT_LB_DATA_READ_LATENCY,
         .entry_base_unit = 10000, /* 10ns base */
         .entry[0] = 15, /* 150ns */
     };
 
-    *dslbis_nonvolatile2 = (CDATDslbis) {
+    *dslbis2 = (CDATDslbis) {
         .header = {
             .type = CDAT_TYPE_DSLBIS,
-            .length = sizeof(*dslbis_nonvolatile2),
+            .length = sizeof(*dslbis2),
         },
-        .handle = nonvolatile_dsmad,
+        .handle = dsmad_handle,
         .flags = HMAT_LB_MEM_MEMORY,
         .data_type = HMAT_LB_DATA_WRITE_LATENCY,
         .entry_base_unit = 10000,
         .entry[0] = 25, /* 250ns */
     };
 
-    *dslbis_nonvolatile3 = (CDATDslbis) {
+    *dslbis3 = (CDATDslbis) {
         .header = {
             .type = CDAT_TYPE_DSLBIS,
-            .length = sizeof(*dslbis_nonvolatile3),
+            .length = sizeof(*dslbis3),
         },
-        .handle = nonvolatile_dsmad,
+        .handle = dsmad_handle,
         .flags = HMAT_LB_MEM_MEMORY,
         .data_type = HMAT_LB_DATA_READ_BANDWIDTH,
         .entry_base_unit = 1000, /* GB/s */
         .entry[0] = 16,
     };
 
-    *dslbis_nonvolatile4 = (CDATDslbis) {
+    *dslbis4 = (CDATDslbis) {
         .header = {
             .type = CDAT_TYPE_DSLBIS,
-            .length = sizeof(*dslbis_nonvolatile4),
+            .length = sizeof(*dslbis4),
         },
-        .handle = nonvolatile_dsmad,
+        .handle = dsmad_handle,
         .flags = HMAT_LB_MEM_MEMORY,
         .data_type = HMAT_LB_DATA_WRITE_BANDWIDTH,
         .entry_base_unit = 1000, /* GB/s */
         .entry[0] = 16,
     };
 
-    *dsemts_nonvolatile = (CDATDsemts) {
+    *dsemts = (CDATDsemts) {
         .header = {
             .type = CDAT_TYPE_DSEMTS,
-            .length = sizeof(*dsemts_nonvolatile),
+            .length = sizeof(*dsemts),
         },
-        .DSMAS_handle = nonvolatile_dsmad,
+        .DSMAS_handle = dsmad_handle,
         /* Reserved - the non volatile from DSMAS matters */
         .EFI_memory_type_attr = 2,
         .DPA_offset = 0,
@@ -137,16 +113,47 @@  static int ct3_build_cdat_table(CDATSubHeader ***cdat_table,
     };
 
     /* Header always at start of structure */
-    (*cdat_table)[0] = g_steal_pointer(&dsmas_nonvolatile);
-    (*cdat_table)[1] = (CDATSubHeader *)g_steal_pointer(&dslbis_nonvolatile1);
-    (*cdat_table)[2] = (CDATSubHeader *)g_steal_pointer(&dslbis_nonvolatile2);
-    (*cdat_table)[3] = (CDATSubHeader *)g_steal_pointer(&dslbis_nonvolatile3);
-    (*cdat_table)[4] = (CDATSubHeader *)g_steal_pointer(&dslbis_nonvolatile4);
-    (*cdat_table)[5] = g_steal_pointer(&dsemts_nonvolatile);
+    cdat_table[0] = g_steal_pointer(&dsmas);
+    cdat_table[1] = (CDATSubHeader *)g_steal_pointer(&dslbis1);
+    cdat_table[2] = (CDATSubHeader *)g_steal_pointer(&dslbis2);
+    cdat_table[3] = (CDATSubHeader *)g_steal_pointer(&dslbis3);
+    cdat_table[4] = (CDATSubHeader *)g_steal_pointer(&dslbis4);
+    cdat_table[5] = g_steal_pointer(&dsemts);
 
     return CT3_CDAT_SUBTABLE_SIZE;
 }
 
+static int ct3_build_cdat_table(CDATSubHeader ***cdat_table,
+                                void *priv)
+{
+    CXLType3Dev *ct3d = priv;
+    MemoryRegion *mr;
+    int ret = 0;
+
+    if (!ct3d->hostmem) {
+        return 0;
+    }
+
+    mr = host_memory_backend_get_memory(ct3d->hostmem);
+    if (!mr) {
+        return -EINVAL;
+    }
+
+    *cdat_table = g_malloc0(CT3_CDAT_SUBTABLE_SIZE * sizeof(*cdat_table));
+    if (!*cdat_table) {
+        return -ENOMEM;
+    }
+
+    /* Non volatile aspects */
+    ret = ct3_build_cdat_subtable(*cdat_table, mr, 0);
+    if (ret < 0) {
+        g_free(*cdat_table);
+        *cdat_table = NULL;
+    }
+
+    return ret;
+}
+
 static void ct3_free_cdat_table(CDATSubHeader **cdat_table, int num, void *priv)
 {
     int i;