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....@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.su...@gmail.com wrote:
> > >
> > > > From: Anisa Su <anisa...@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...@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?
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 {
> > >
>
