| Message ID | 20221012182120.174142-6-gregory.price@memverge.com |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/5] hw/mem/cxl_type3: fix checkpatch errors | expand |
On Wed, 12 Oct 2022 14:21:20 -0400 Gregory Price <gourry.memverge@gmail.com> wrote: > 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> I'm in two minds about this... We could integrate it in the original series, but at that time the change is justified. Or we could leave it as a first patch in your follow on series. Anyhow, I went with a similar refactor inspired by this. > --- > hw/mem/cxl_type3.c | 137 ++++++++++++++++++++++++--------------------- > 1 file changed, 72 insertions(+), 65 deletions(-) > > 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) subtable is particularly well defined. Maybe ct3_build_cdat_entries_for_mr()? > { > - 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 */ 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. > }; > > - *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)); This bakes in assumptions at the wrong layer in the code. Out here we should not know how big the table is - that is a job just for the ct3_build_cdat_subtable() part. Various options come to mind.. 1) Two pass approach. First call ct3_build_cdat_subtable() with NULL pointer passed in. For that all it does it return the number of elements. The the caller calls it again providing suitable storage. 2) Allocate in ct3_build_cdat_subtable() then copy in the caller or use directly if only one type of memory present. I've gone with the 2 pass approach. Let me know what you think of it once I send the patches out in a few mins. Thanks, Jonathan > + 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;
> > /* 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
On Thu, 13 Oct 2022 15:40:47 -0400 Gregory Price <gregory.price@memverge.com> wrote: > > > /* 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? We definitely want a 'default' value if nothing is provided. It might be useful to allow it to be adjusted, but lets add that when we have a use for it (perhaps testing some stuff in kernel where the values matter enough to make them controllable). > > 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 For now 1 is fine I think. I've just pushed out a doe-v9 tag and cxl-2022-10-14 branch to gitlab.com/jic23/qemu Also advanced the base tree to current QEMU mainline. Note that if anyone is playing with the switch cci device and mainline kernel you'll currently need to revert https://lore.kernel.org/linux-pci/20220905080232.36087-5-mika.westerberg@linux.intel.com/ Jonathan
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;
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(-)