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

[Jonathan Cameron via <qemu-devel@nongnu.org>]

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;