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

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

On Fri, 2 May 2025 08:57:36 -0700
Fan Ni <nifan.cxl@gmail.com> wrote:

> On Fri, May 02, 2025 at 10:01:55AM +0100, Jonathan Cameron wrote:
> > 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.  
> 
> OK. Since some flags are not support yet, can we hard coded them for now?

Sure. Add some breadcrumbs / comments for later though if that makes sense.

> 
> Fan
> > 
> >   
> > > > 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 {    
> > > >     
> >