[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v8 4/5] hw/mem/cxl-type3: Add CXL CDAT Data Object Exchange
From: |
Jonathan Cameron |
Subject: |
Re: [PATCH v8 4/5] hw/mem/cxl-type3: Add CXL CDAT Data Object Exchange |
Date: |
Thu, 13 Oct 2022 18:40:47 +0100 |
On Thu, 13 Oct 2022 13:32:26 -0400
Gregory Price <gregory.price@memverge.com> wrote:
> If you're expecting this to be dynamic in size in the future, then maybe
> what we really want here is a function
>
> static int ct3_get_cdat_size(CXLType3Dev ct3d) {
> /* TODO: Dynamic sizing calculations */
> return CT3_CDAT_NUM_ENTRIES;
> }
Perhaps, though that suffers from lack of locality and clearly
flagging that the enum is definitely not global.
Anyhow, lets argue that mechanism when it's needed :)
Jonathan
>
>
> On Thu, Oct 13, 2022 at 06:21:44PM +0100, Jonathan Cameron wrote:
> > On Thu, 13 Oct 2022 13:09:26 -0400
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> > > 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.
> >
> > Ok, seems a consensus against having this local.
> >
> > I can do this for now and then revisit if / when things become more complex
> > and this becomes not global. I guess a potential case of premature
> > flexibility.
> >
> > Jonathan
> >
> > >
> > > > 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
> > >
> >
[PATCH v8 5/5] hw/pci-bridge/cxl-upstream: Add a CDAT table access DOE, Jonathan Cameron, 2022/10/13