Re: [PATCH 2/5] hw/mem/cxl_type3: Pull validation checks ahead of functional code

[Jonathan Cameron]

On Wed, 12 Oct 2022 14:21:17 -0400
Gregory Price <gourry.memverge@gmail.com> wrote:

> For style - pulling these validations ahead flattens the code.

True, but at the cost of separating the check from where it is
obvious why we have the check.  I'd prefer to see it next to the
use. 

Inverting the hostmem check is resonable so I'll make that change.

My original thinking is that doing so would make adding non volatile
support messier but given you plan to factor out most of this the
change won't be too bad anyway.


> 
> Signed-off-by: Gregory Price <gregory.price@memverge.com>
> ---
>  hw/mem/cxl_type3.c | 193 ++++++++++++++++++++++-----------------------
>  1 file changed, 96 insertions(+), 97 deletions(-)
> 
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index 94bc439d89..43b2b9e041 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -32,107 +32,106 @@ static int ct3_build_cdat_table(CDATSubHeader 
> ***cdat_table,
>      int dslbis_nonvolatile_num = 4;
>      MemoryRegion *mr;
>  
> +    if (!ct3d->hostmem) {
> +        return len;
> +    }
> +
> +    mr = host_memory_backend_get_memory(ct3d->hostmem);
> +    if (!mr) {
> +        return -EINVAL;
> +    }
> +
>      /* Non volatile aspects */
> -    if (ct3d->hostmem) {
> -        dsmas_nonvolatile = g_malloc(sizeof(*dsmas_nonvolatile));
> -        if (!dsmas_nonvolatile) {
> -            return -ENOMEM;
> -        }
> -        nonvolatile_dsmad = next_dsmad_handle++;
> -        mr = host_memory_backend_get_memory(ct3d->hostmem);
> -        if (!mr) {
> -            return -EINVAL;
> -        }
> -        *dsmas_nonvolatile = (CDATDsmas) {
> -            .header = {
> -                .type = CDAT_TYPE_DSMAS,
> -                .length = sizeof(*dsmas_nonvolatile),
> -            },
> -            .DSMADhandle = nonvolatile_dsmad,
> -            .flags = CDAT_DSMAS_FLAG_NV,
> -            .DPA_base = 0,
> -            .DPA_length = int128_get64(mr->size),
> -        };
> -        len++;
> -
> -        /* For now, no memory side cache, plausiblish numbers */
> -        dslbis_nonvolatile =
> -            g_malloc(sizeof(*dslbis_nonvolatile) * dslbis_nonvolatile_num);
> -        if (!dslbis_nonvolatile) {
> -            return -ENOMEM;
> -        }
> +    dsmas_nonvolatile = g_malloc(sizeof(*dsmas_nonvolatile));
> +    if (!dsmas_nonvolatile) {
> +        return -ENOMEM;
> +    }
> +    nonvolatile_dsmad = next_dsmad_handle++;
> +    *dsmas_nonvolatile = (CDATDsmas) {
> +        .header = {
> +            .type = CDAT_TYPE_DSMAS,
> +            .length = sizeof(*dsmas_nonvolatile),
> +        },
> +        .DSMADhandle = nonvolatile_dsmad,
> +        .flags = CDAT_DSMAS_FLAG_NV,
> +        .DPA_base = 0,
> +        .DPA_length = int128_get64(mr->size),
> +    };
> +    len++;
>  
> -        dslbis_nonvolatile[0] = (CDATDslbis) {
> -            .header = {
> -                .type = CDAT_TYPE_DSLBIS,
> -                .length = sizeof(*dslbis_nonvolatile),
> -            },
> -            .handle = nonvolatile_dsmad,
> -            .flags = HMAT_LB_MEM_MEMORY,
> -            .data_type = HMAT_LB_DATA_READ_LATENCY,
> -            .entry_base_unit = 10000, /* 10ns base */
> -            .entry[0] = 15, /* 150ns */
> -        };
> -        len++;
> -
> -        dslbis_nonvolatile[1] = (CDATDslbis) {
> -            .header = {
> -                .type = CDAT_TYPE_DSLBIS,
> -                .length = sizeof(*dslbis_nonvolatile),
> -            },
> -            .handle = nonvolatile_dsmad,
> -            .flags = HMAT_LB_MEM_MEMORY,
> -            .data_type = HMAT_LB_DATA_WRITE_LATENCY,
> -            .entry_base_unit = 10000,
> -            .entry[0] = 25, /* 250ns */
> -        };
> -        len++;
> -
> -        dslbis_nonvolatile[2] = (CDATDslbis) {
> -            .header = {
> -                .type = CDAT_TYPE_DSLBIS,
> -                .length = sizeof(*dslbis_nonvolatile),
> -            },
> -            .handle = nonvolatile_dsmad,
> -            .flags = HMAT_LB_MEM_MEMORY,
> -            .data_type = HMAT_LB_DATA_READ_BANDWIDTH,
> -            .entry_base_unit = 1000, /* GB/s */
> -            .entry[0] = 16,
> -        };
> -        len++;
> -
> -        dslbis_nonvolatile[3] = (CDATDslbis) {
> -            .header = {
> -                .type = CDAT_TYPE_DSLBIS,
> -                .length = sizeof(*dslbis_nonvolatile),
> -            },
> -            .handle = nonvolatile_dsmad,
> -            .flags = HMAT_LB_MEM_MEMORY,
> -            .data_type = HMAT_LB_DATA_WRITE_BANDWIDTH,
> -            .entry_base_unit = 1000, /* GB/s */
> -            .entry[0] = 16,
> -        };
> -        len++;
> -
> -        mr = host_memory_backend_get_memory(ct3d->hostmem);
> -        if (!mr) {
> -            return -EINVAL;
> -        }
> -        dsemts_nonvolatile = g_malloc(sizeof(*dsemts_nonvolatile));
> -        *dsemts_nonvolatile = (CDATDsemts) {
> -            .header = {
> -                .type = CDAT_TYPE_DSEMTS,
> -                .length = sizeof(*dsemts_nonvolatile),
> -            },
> -            .DSMAS_handle = nonvolatile_dsmad,
> -            /* Reserved - the non volatile from DSMAS matters */
> -            .EFI_memory_type_attr = 2,
> -            .DPA_offset = 0,
> -            .DPA_length = int128_get64(mr->size),
> -        };
> -        len++;
> +    /* For now, no memory side cache, plausiblish numbers */
> +    dslbis_nonvolatile =
> +        g_malloc(sizeof(*dslbis_nonvolatile) * dslbis_nonvolatile_num);
> +    if (!dslbis_nonvolatile) {
> +        return -ENOMEM;
>      }
>  
> +    dslbis_nonvolatile[0] = (CDATDslbis) {
> +        .header = {
> +            .type = CDAT_TYPE_DSLBIS,
> +            .length = sizeof(*dslbis_nonvolatile),
> +        },
> +        .handle = nonvolatile_dsmad,
> +        .flags = HMAT_LB_MEM_MEMORY,
> +        .data_type = HMAT_LB_DATA_READ_LATENCY,
> +        .entry_base_unit = 10000, /* 10ns base */
> +        .entry[0] = 15, /* 150ns */
> +    };
> +    len++;
> +
> +    dslbis_nonvolatile[1] = (CDATDslbis) {
> +        .header = {
> +            .type = CDAT_TYPE_DSLBIS,
> +            .length = sizeof(*dslbis_nonvolatile),
> +        },
> +        .handle = nonvolatile_dsmad,
> +        .flags = HMAT_LB_MEM_MEMORY,
> +        .data_type = HMAT_LB_DATA_WRITE_LATENCY,
> +        .entry_base_unit = 10000,
> +        .entry[0] = 25, /* 250ns */
> +    };
> +    len++;
> +
> +    dslbis_nonvolatile[2] = (CDATDslbis) {
> +        .header = {
> +            .type = CDAT_TYPE_DSLBIS,
> +            .length = sizeof(*dslbis_nonvolatile),
> +        },
> +        .handle = nonvolatile_dsmad,
> +        .flags = HMAT_LB_MEM_MEMORY,
> +        .data_type = HMAT_LB_DATA_READ_BANDWIDTH,
> +        .entry_base_unit = 1000, /* GB/s */
> +        .entry[0] = 16,
> +    };
> +    len++;
> +
> +    dslbis_nonvolatile[3] = (CDATDslbis) {
> +        .header = {
> +            .type = CDAT_TYPE_DSLBIS,
> +            .length = sizeof(*dslbis_nonvolatile),
> +        },
> +        .handle = nonvolatile_dsmad,
> +        .flags = HMAT_LB_MEM_MEMORY,
> +        .data_type = HMAT_LB_DATA_WRITE_BANDWIDTH,
> +        .entry_base_unit = 1000, /* GB/s */
> +        .entry[0] = 16,
> +    };
> +    len++;
> +
> +    dsemts_nonvolatile = g_malloc(sizeof(*dsemts_nonvolatile));
> +    *dsemts_nonvolatile = (CDATDsemts) {
> +        .header = {
> +            .type = CDAT_TYPE_DSEMTS,
> +            .length = sizeof(*dsemts_nonvolatile),
> +        },
> +        .DSMAS_handle = nonvolatile_dsmad,
> +        /* Reserved - the non volatile from DSMAS matters */
> +        .EFI_memory_type_attr = 2,
> +        .DPA_offset = 0,
> +        .DPA_length = int128_get64(mr->size),
> +    };
> +    len++;
> +
>      *cdat_table = g_malloc0(len * sizeof(*cdat_table));
>      /* Header always at start of structure */
>      if (dsmas_nonvolatile) {