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

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Oct 13, 2022 at 12:26:58PM -0400, Gregory Price 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.


Yes I think I agree here.

> 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