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.pr...@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
