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;
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 {
--
2.47.2On 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;
This is relying to much on the ordering of creating fields in
ct3_build_cdat_entries_for_mr().
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
configuration in 6.2 spec. I've sent a mail to see if we can clean that
'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.
>
> 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 {
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 {
>
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 {
> >
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?
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 {
> > >
>
© 2016 - 2026 Red Hat, Inc.