Re: [PATCH 3/9] cxl/type3: Add dsmas_flags to CXLDCRegion struct

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Thu, 1 May 2025 20:21:56 +0000
Fan Ni <nifan.cxl@gmail.com> wrote:

> On Thu, Apr 24, 2025 at 11:42:59AM +0100, Jonathan Cameron wrote:
> > On Mon, 17 Mar 2025 16:31:30 +0000
> > anisa.su887@gmail.com wrote:
> >   
> > > From: Anisa Su <anisa.su@samsung.com>
> > > 
> > > Add dsmas_flags field to DC Region struct in preparation for next
> > > command, which returns the dsmas flags in the response.
> > > 
> > > Signed-off-by: Anisa Su <anisa.su@samsung.com>
> > > ---
> > >  hw/mem/cxl_type3.c          | 2 ++
> > >  include/hw/cxl/cxl_device.h | 1 +
> > >  2 files changed, 3 insertions(+)
> > > 
> > > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> > > index 731497ebda..452a0c101a 100644
> > > --- a/hw/mem/cxl_type3.c
> > > +++ b/hw/mem/cxl_type3.c
> > > @@ -237,6 +237,8 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, void *priv)
> > >                                            ct3d->dc.regions[i].len,
> > >                                            false, true, region_base);
> > >              ct3d->dc.regions[i].dsmadhandle = dsmad_handle - 1;
> > > +            CDATDsmas *dsmas = (CDATDsmas *) table[cur_ent + CT3_CDAT_DSMAS];
> > > +            ct3d->dc.regions[i].dsmas_flags = dsmas->flags;  
> >   
> Hi Jonathan,
> Thanks for the feedback.
> > This is relying to much on the ordering of creating fields in
> > ct3_build_cdat_entries_for_mr().  
> I am not sure whether I understand this clearly.
> In current qemu implemtation, each mr (ram,pmem or dc region) will have the
> whole set of cdat table entries (dsmas, dslbis0-3, etc), so as long as we point
> to the right table entry, we can get the table correctly.
> What do you mean "the ordering of creating fields"?

It is an implementation detail only that the first bit of that table is
the DSMAS entry.  I think we shouldn't rely on that.

> > 
> > I'd rather you just stored the information flags is built from in CXLDCRegion
> > and then built the field that is wonderfully called 'Note' in the DC region  
I got distracted by the spec oddity :)

> This sentence is kind of broken for me, not totally clear what you are
> suggesting :-(. Can you explain more?
> Are you suggesting not directly take dsmas->flags as dsmas_flags, but
> use bit op to generate the value used in Table 7-66 in cxl spec 3.2?

No. Just store the various  bools etc that become dsmas->flags in the
CXLDCRegion structure directly rather than reading back from dsmas->flags.
Probably as explicit bools etc not a single value.

Then pass those in to  ct3_build_cdat_entries_for_mr() .  Mostly they overlap
with current true / false parameters that are hard coded.


> > configuration in 6.2 spec.   I've sent a mail to see if we can clean that  
> 6.2 spec???
> > 'what is the field called' question for future spec releases.
> > 
> > Whilst the flag definitions cross refer the CDAT spec, the actual locations
> > of those flags matches, but doesn't cross refer so maybe in the future
> > we will have other flags in here and locations might not match.  
> For the flags stored in dsmas table, do we expect there can be more than those
> defined in Table 7-66 in spec 3.2?

Not for now. Though I'm sure something will come along at some point.
The comment is about there being particular reason the flag locations should match
between CDAT and what we report via the commands being added here.  The definitions
of individual bits cross refer between specs, the register as a whole does not.

Jonathan

> 
> Fan
> 
> >   
> > >  
> > >              cur_ent += CT3_CDAT_NUM_ENTRIES;
> > >              region_base += ct3d->dc.regions[i].len;
> > > diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> > > index bebed04085..81b826f570 100644
> > > --- a/include/hw/cxl/cxl_device.h
> > > +++ b/include/hw/cxl/cxl_device.h
> > > @@ -609,6 +609,7 @@ typedef struct CXLDCRegion {
> > >      uint8_t flags;
> > >      unsigned long *blk_bitmap;
> > >      uint64_t supported_blk_size_bitmask;
> > > +    uint8_t dsmas_flags;
> > >  } CXLDCRegion;
> > >  
> > >  typedef struct CXLSetFeatureInfo {  
> >