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

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

On Thu, 13 Oct 2022 12:26:58 -0400
Gregory Price <gregory.price@memverge.com> wrote:

> 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(...)
> 

I agree it's a messy pattern, but as things become dynamic I'd expect
we'll have a bunch of checks that really need to be in that function
and would be replicated at the caller to calculate the size of the array.

I'm expecting this code to get more complex over time.

For example adding memory-side cache support based on commandline parameters.
Once we do that, the enum will almost certainly no longer be global as we'll have
a bunch of other entries (potentially different numbers for each region).
Whether that is done with calls to a separate function, or by changing this
one isn't clear to me yet.


> 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

Doh. I'll update the gitlab tree in a minute for this, but leave sending
out the updated version for a few days to let others take a look if they wish.

In meantime, regression in mainline kernel against CXL qemu emulation.
Bisection with 8 steps to go. *sigh*

Jonathan