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

[Fan Ni <nifan.cxl@gmail.com>]

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"?
> 
> 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
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?
> 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?

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