On Fri, 2 May 2025 08:57:36 -0700
Fan Ni <nifan....@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....@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?
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 {
> > > >
> >
