Re: [PATCH v8 4/5] hw/mem/cxl-type3: Add CXL CDAT Data Object Exchange

[Gregory Price <gregory.price@memverge.com>]

Other than the nitpicks below, lgtm.  Not sure if you need a sign off
from me given the contributions:

Signed-off-by: Gregory Price <gregory.price@memverge.com>

> +/* If no cdat_table == NULL returns number of entries */
> +static int ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table,
> +                                         int dsmad_handle, MemoryRegion *mr)
> +{
> +    enum {
> +        DSMAS,
> +        DSLBIS0,
> +        DSLBIS1,
> +        DSLBIS2,
> +        DSLBIS3,
> +        DSEMTS,
> +        NUM_ENTRIES
> +    };
// ...
> +    if (!cdat_table) {
> +        return NUM_ENTRIES;
> +    }

the only thing that i would recommend is making this enum global (maybe
renaming them CT3_CDAT_[ENTRY_NAME]) and using CT3_CDAT_NUM_ENTRIES
directly in the function that allocates the cdat buffer itself. I do
understand the want to keep the enum and the code creating the entries
co-located though, so i'm just nitpicking here i guess.

Generally I dislike the pattern of passing a NULL into a function to get
configuration data, and then calling that function again.  This function
wants to be named...

ct3_build_cdat_entries_for_mr_or_get_table_size_if_cdat_is_null(...)

to accurately describe what it does.  Just kinda feels like an extra
function call for no reason.

But either way, it works, this is just a nitpick on my side.

> +static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, void *priv)
> +{
> +    g_autofree CDATSubHeader **table = NULL;
> +    CXLType3Dev *ct3d = priv;
> +    MemoryRegion *volatile_mr;
> +    /* ... snip ... */
> +}

s/volatile/nonvolatile