* [PATCH v2 0/2] cxl: add interleave capability check @ 2024-04-03 2:17 Yao Xingtao 2024-04-03 2:17 ` [PATCH v2 1/2] cxl/core/hdm: rename interleave_mask to ig_cap_mask Yao Xingtao 2024-04-03 2:17 ` [PATCH v2 2/2] cxl/core/region: check interleave capability Yao Xingtao 0 siblings, 2 replies; 8+ messages in thread From: Yao Xingtao @ 2024-04-03 2:17 UTC (permalink / raw) To: dave, jonathan.cameron, dave.jiang, alison.schofield, vishal.l.verma, ira.weiny, dan.j.williams, jim.harris Cc: linux-cxl, Yao Xingtao Changes: v1 -> v2: 1. rename interleave_mask to ig_cap_mask 2. add a check for interleave granularity 3. update commit message of PATCH 2 Currently driver does not check the interleave capability of target, it can attach target to region even if target does not support the interleave granularity or interleave ways. Thus, applications access the memory will occur unexpected behavior, such as segmentation fault. Therefore, it is necessary to check the interleave capability of target before attaching it to region. If the check fails, the attachment operation should be stopped. Yao Xingtao (2): cxl/core/hdm: rename interleave_mask to ig_cap_mask cxl/core/region: check interleave capability drivers/cxl/core/hdm.c | 8 ++++++-- drivers/cxl/core/region.c | 37 +++++++++++++++++++++++++++++++++++++ drivers/cxl/cxl.h | 2 ++ drivers/cxl/cxlmem.h | 3 ++- 4 files changed, 47 insertions(+), 3 deletions(-) -- 2.37.3 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 1/2] cxl/core/hdm: rename interleave_mask to ig_cap_mask 2024-04-03 2:17 [PATCH v2 0/2] cxl: add interleave capability check Yao Xingtao @ 2024-04-03 2:17 ` Yao Xingtao 2024-04-03 14:06 ` Jonathan Cameron 2024-04-03 2:17 ` [PATCH v2 2/2] cxl/core/region: check interleave capability Yao Xingtao 1 sibling, 1 reply; 8+ messages in thread From: Yao Xingtao @ 2024-04-03 2:17 UTC (permalink / raw) To: dave, jonathan.cameron, dave.jiang, alison.schofield, vishal.l.verma, ira.weiny, dan.j.williams, jim.harris Cc: linux-cxl, Yao Xingtao Since interleave contains interleave ways and interleave granularity, using ig_cap_mask is better to describe this capability. Signed-off-by: Yao Xingtao <yaoxt.fnst@fujitsu.com> --- drivers/cxl/core/hdm.c | 4 ++-- drivers/cxl/cxlmem.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c index 7d97790b893d..9bb6a256cc6f 100644 --- a/drivers/cxl/core/hdm.c +++ b/drivers/cxl/core/hdm.c @@ -76,9 +76,9 @@ static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm) cxlhdm->target_count = FIELD_GET(CXL_HDM_DECODER_TARGET_COUNT_MASK, hdm_cap); if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_11_8, hdm_cap)) - cxlhdm->interleave_mask |= GENMASK(11, 8); + cxlhdm->ig_cap_mask |= GENMASK(11, 8); if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_14_12, hdm_cap)) - cxlhdm->interleave_mask |= GENMASK(14, 12); + cxlhdm->ig_cap_mask |= GENMASK(14, 12); } static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info) diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h index 20fb3b35e89e..b53f7ae0fdd6 100644 --- a/drivers/cxl/cxlmem.h +++ b/drivers/cxl/cxlmem.h @@ -852,7 +852,7 @@ struct cxl_hdm { struct cxl_component_regs regs; unsigned int decoder_count; unsigned int target_count; - unsigned int interleave_mask; + unsigned int ig_cap_mask; struct cxl_port *port; }; -- 2.37.3 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] cxl/core/hdm: rename interleave_mask to ig_cap_mask 2024-04-03 2:17 ` [PATCH v2 1/2] cxl/core/hdm: rename interleave_mask to ig_cap_mask Yao Xingtao @ 2024-04-03 14:06 ` Jonathan Cameron 2024-04-08 2:42 ` Xingtao Yao (Fujitsu) 0 siblings, 1 reply; 8+ messages in thread From: Jonathan Cameron @ 2024-04-03 14:06 UTC (permalink / raw) To: Yao Xingtao Cc: dave, dave.jiang, alison.schofield, vishal.l.verma, ira.weiny, dan.j.williams, jim.harris, linux-cxl On Tue, 2 Apr 2024 22:17:46 -0400 Yao Xingtao <yaoxt.fnst@fujitsu.com> wrote: > Since interleave contains interleave ways and interleave granularity, > using ig_cap_mask is better to describe this capability. Hi Yao, I don't follow the reasoning of this. ig suggests interleave granularity which whilst related to the address bits, they aren't masking the granularity as such. I agree current naming isn't particularly meaningful, but don't see the suggested change as an improvement. Jonathan > > Signed-off-by: Yao Xingtao <yaoxt.fnst@fujitsu.com> > --- > drivers/cxl/core/hdm.c | 4 ++-- > drivers/cxl/cxlmem.h | 2 +- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c > index 7d97790b893d..9bb6a256cc6f 100644 > --- a/drivers/cxl/core/hdm.c > +++ b/drivers/cxl/core/hdm.c > @@ -76,9 +76,9 @@ static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm) > cxlhdm->target_count = > FIELD_GET(CXL_HDM_DECODER_TARGET_COUNT_MASK, hdm_cap); > if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_11_8, hdm_cap)) > - cxlhdm->interleave_mask |= GENMASK(11, 8); > + cxlhdm->ig_cap_mask |= GENMASK(11, 8); > if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_14_12, hdm_cap)) > - cxlhdm->interleave_mask |= GENMASK(14, 12); > + cxlhdm->ig_cap_mask |= GENMASK(14, 12); > } > > static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info) > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h > index 20fb3b35e89e..b53f7ae0fdd6 100644 > --- a/drivers/cxl/cxlmem.h > +++ b/drivers/cxl/cxlmem.h > @@ -852,7 +852,7 @@ struct cxl_hdm { > struct cxl_component_regs regs; > unsigned int decoder_count; > unsigned int target_count; > - unsigned int interleave_mask; > + unsigned int ig_cap_mask; > struct cxl_port *port; > }; > ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH v2 1/2] cxl/core/hdm: rename interleave_mask to ig_cap_mask 2024-04-03 14:06 ` Jonathan Cameron @ 2024-04-08 2:42 ` Xingtao Yao (Fujitsu) 0 siblings, 0 replies; 8+ messages in thread From: Xingtao Yao (Fujitsu) @ 2024-04-08 2:42 UTC (permalink / raw) To: Jonathan Cameron Cc: dave@stgolabs.net, dave.jiang@intel.com, alison.schofield@intel.com, vishal.l.verma@intel.com, ira.weiny@intel.com, dan.j.williams@intel.com, jim.harris@samsung.com, linux-cxl@vger.kernel.org > -----Original Message----- > From: Jonathan Cameron <Jonathan.Cameron@Huawei.com> > Sent: Wednesday, April 3, 2024 10:06 PM > To: Yao, Xingtao/姚 幸涛 <yaoxt.fnst@fujitsu.com> > Cc: dave@stgolabs.net; dave.jiang@intel.com; alison.schofield@intel.com; > vishal.l.verma@intel.com; ira.weiny@intel.com; dan.j.williams@intel.com; > jim.harris@samsung.com; linux-cxl@vger.kernel.org > Subject: Re: [PATCH v2 1/2] cxl/core/hdm: rename interleave_mask to > ig_cap_mask > > On Tue, 2 Apr 2024 22:17:46 -0400 > Yao Xingtao <yaoxt.fnst@fujitsu.com> wrote: > > > Since interleave contains interleave ways and interleave granularity, > > using ig_cap_mask is better to describe this capability. > > Hi Yao, > > I don't follow the reasoning of this. > > ig suggests interleave granularity which whilst related to the address > bits, they aren't masking the granularity as such. > > I agree current naming isn't particularly meaningful, but don't see the > suggested change as an improvement. this is my misunderstanding of these bits, I will drop this modification. > > Jonathan > > > > Signed-off-by: Yao Xingtao <yaoxt.fnst@fujitsu.com> > > --- > > drivers/cxl/core/hdm.c | 4 ++-- > > drivers/cxl/cxlmem.h | 2 +- > > 2 files changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c > > index 7d97790b893d..9bb6a256cc6f 100644 > > --- a/drivers/cxl/core/hdm.c > > +++ b/drivers/cxl/core/hdm.c > > @@ -76,9 +76,9 @@ static void parse_hdm_decoder_caps(struct cxl_hdm > *cxlhdm) > > cxlhdm->target_count = > > FIELD_GET(CXL_HDM_DECODER_TARGET_COUNT_MASK, > hdm_cap); > > if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_11_8, hdm_cap)) > > - cxlhdm->interleave_mask |= GENMASK(11, 8); > > + cxlhdm->ig_cap_mask |= GENMASK(11, 8); > > if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_14_12, hdm_cap)) > > - cxlhdm->interleave_mask |= GENMASK(14, 12); > > + cxlhdm->ig_cap_mask |= GENMASK(14, 12); > > } > > > > static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info) > > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h > > index 20fb3b35e89e..b53f7ae0fdd6 100644 > > --- a/drivers/cxl/cxlmem.h > > +++ b/drivers/cxl/cxlmem.h > > @@ -852,7 +852,7 @@ struct cxl_hdm { > > struct cxl_component_regs regs; > > unsigned int decoder_count; > > unsigned int target_count; > > - unsigned int interleave_mask; > > + unsigned int ig_cap_mask; > > struct cxl_port *port; > > }; > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 2/2] cxl/core/region: check interleave capability 2024-04-03 2:17 [PATCH v2 0/2] cxl: add interleave capability check Yao Xingtao 2024-04-03 2:17 ` [PATCH v2 1/2] cxl/core/hdm: rename interleave_mask to ig_cap_mask Yao Xingtao @ 2024-04-03 2:17 ` Yao Xingtao 2024-04-03 14:27 ` Jonathan Cameron 1 sibling, 1 reply; 8+ messages in thread From: Yao Xingtao @ 2024-04-03 2:17 UTC (permalink / raw) To: dave, jonathan.cameron, dave.jiang, alison.schofield, vishal.l.verma, ira.weiny, dan.j.williams, jim.harris Cc: linux-cxl, Yao Xingtao Since interleave capability is not checked, a target can be attached to region successfully even it does not support the interleave ways or interleave granularity. When accessing the memory, unexpected behavior occurs due to converting HPA to an error DPA: $ numactl -m 2 ls Segmentation fault (core dumped) Link: https://lore.kernel.org/qemu-devel/20240327014653.26623-1-yaoxt.fnst@fujitsu.com Signed-off-by: Yao Xingtao <yaoxt.fnst@fujitsu.com> --- drivers/cxl/core/hdm.c | 4 ++++ drivers/cxl/core/region.c | 41 +++++++++++++++++++++++++++++++++++++++ drivers/cxl/cxl.h | 2 ++ drivers/cxl/cxlmem.h | 1 + 4 files changed, 48 insertions(+) diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c index 9bb6a256cc6f..1a99b138dbec 100644 --- a/drivers/cxl/core/hdm.c +++ b/drivers/cxl/core/hdm.c @@ -79,6 +79,10 @@ static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm) cxlhdm->ig_cap_mask |= GENMASK(11, 8); if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_14_12, hdm_cap)) cxlhdm->ig_cap_mask |= GENMASK(14, 12); + if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_3_6_12_WAY, hdm_cap)) + cxlhdm->iw_cap_mask |= BIT(3) | BIT(6) | BIT(12); + if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_16_WAY, hdm_cap)) + cxlhdm->iw_cap_mask |= BIT(16); } static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info) diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index 5c186e0a39b9..25d178e14ed1 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -1786,6 +1786,36 @@ static int cxl_region_sort_targets(struct cxl_region *cxlr) return rc; } +static int +check_interleave_cap(struct cxl_decoder *cxld, int iw, int ig) +{ + struct cxl_port *port = to_cxl_port(cxld->dev.parent); + struct cxl_hdm *cxlhdm = dev_get_drvdata(&port->dev); + u8 eiw; + u16 eig; + int rc; + + rc = ways_to_eiw(iw, &eiw); + if (rc) + return rc; + + if (eiw > 3 && !(cxlhdm->iw_cap_mask & BIT(iw))) { + dev_dbg(&cxld->dev, "iw: %d is not supported\n", iw); + return -EOPNOTSUPP; + } + + rc = granularity_to_eig(ig, &eig); + if (rc) + return rc; + + if (!(BIT(eig + 8) & cxlhdm->ig_cap_mask)) { + dev_dbg(&cxld->dev, "ig: %d is not supported\n", ig); + return -EOPNOTSUPP; + } + + return 0; +} + static int cxl_region_attach(struct cxl_region *cxlr, struct cxl_endpoint_decoder *cxled, int pos) { @@ -1796,6 +1826,17 @@ static int cxl_region_attach(struct cxl_region *cxlr, struct cxl_dport *dport; int rc = -ENXIO; + rc = check_interleave_cap(&cxled->cxld, p->interleave_ways, + p->interleave_granularity); + if (rc) { + dev_dbg(&cxlr->dev, + "%s with region iw: %d, ig: %d is not supported\n", + dev_name(&cxled->cxld.dev), + p->interleave_ways, + p->interleave_granularity); + return rc; + } + if (cxled->mode != cxlr->mode) { dev_dbg(&cxlr->dev, "%s region mode: %d mismatch: %d\n", dev_name(&cxled->cxld.dev), cxlr->mode, cxled->mode); diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index 534e25e2f0a4..da8a487ededa 100644 --- a/drivers/cxl/cxl.h +++ b/drivers/cxl/cxl.h @@ -45,6 +45,8 @@ #define CXL_HDM_DECODER_TARGET_COUNT_MASK GENMASK(7, 4) #define CXL_HDM_DECODER_INTERLEAVE_11_8 BIT(8) #define CXL_HDM_DECODER_INTERLEAVE_14_12 BIT(9) +#define CXL_HDM_DECODER_INTERLEAVE_3_6_12_WAY BIT(11) +#define CXL_HDM_DECODER_INTERLEAVE_16_WAY BIT(12) #define CXL_HDM_DECODER_CTRL_OFFSET 0x4 #define CXL_HDM_DECODER_ENABLE BIT(1) #define CXL_HDM_DECODER0_BASE_LOW_OFFSET(i) (0x20 * (i) + 0x10) diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h index b53f7ae0fdd6..979c22955246 100644 --- a/drivers/cxl/cxlmem.h +++ b/drivers/cxl/cxlmem.h @@ -853,6 +853,7 @@ struct cxl_hdm { unsigned int decoder_count; unsigned int target_count; unsigned int ig_cap_mask; + unsigned int iw_cap_mask; struct cxl_port *port; }; -- 2.37.3 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] cxl/core/region: check interleave capability 2024-04-03 2:17 ` [PATCH v2 2/2] cxl/core/region: check interleave capability Yao Xingtao @ 2024-04-03 14:27 ` Jonathan Cameron 2024-04-08 2:50 ` Xingtao Yao (Fujitsu) 0 siblings, 1 reply; 8+ messages in thread From: Jonathan Cameron @ 2024-04-03 14:27 UTC (permalink / raw) To: Yao Xingtao Cc: dave, dave.jiang, alison.schofield, vishal.l.verma, ira.weiny, dan.j.williams, jim.harris, linux-cxl On Tue, 2 Apr 2024 22:17:47 -0400 Yao Xingtao <yaoxt.fnst@fujitsu.com> wrote: > Since interleave capability is not checked, a target can be attached to > region successfully even it does not support the interleave ways or > interleave granularity. > > When accessing the memory, unexpected behavior occurs due to converting > HPA to an error DPA: > $ numactl -m 2 ls > Segmentation fault (core dumped) > > Link: https://lore.kernel.org/qemu-devel/20240327014653.26623-1-yaoxt.fnst@fujitsu.com > Signed-off-by: Yao Xingtao <yaoxt.fnst@fujitsu.com> I argued on the CXL opensource sync call last night that we'd get an hdm commit fail (on working hardware - unlike current qemu) if this check wasn't present. Having thought more I think I was wrong and this is a necessary fix because a device that doesn't support one of these ways treats the HDM Decoder n Control Register / Interleave Ways (IW) values as 'reserved'. Is it guaranteed to not just do that by fixing the higher bits to zero? If that's a possible implementation and the decoder was set to 6-way (0x9) maybe the device would interpret that as 2-way (0x1) and give us the wrong decode. With that in mind I think this is a fix and needs a Fixes tag. > --- > drivers/cxl/core/hdm.c | 4 ++++ > drivers/cxl/core/region.c | 41 +++++++++++++++++++++++++++++++++++++++ > drivers/cxl/cxl.h | 2 ++ > drivers/cxl/cxlmem.h | 1 + > 4 files changed, 48 insertions(+) > > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c > index 9bb6a256cc6f..1a99b138dbec 100644 > --- a/drivers/cxl/core/hdm.c > +++ b/drivers/cxl/core/hdm.c > @@ -79,6 +79,10 @@ static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm) > cxlhdm->ig_cap_mask |= GENMASK(11, 8); > if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_14_12, hdm_cap)) > cxlhdm->ig_cap_mask |= GENMASK(14, 12); > + if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_3_6_12_WAY, hdm_cap)) > + cxlhdm->iw_cap_mask |= BIT(3) | BIT(6) | BIT(12); > + if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_16_WAY, hdm_cap)) > + cxlhdm->iw_cap_mask |= BIT(16); Whilst it doesn't matter as such (because they are always valid) I think we should also st the bits for 1,2,4,8 so that all relevant bits are enabled. > } > > static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info) > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index 5c186e0a39b9..25d178e14ed1 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -1786,6 +1786,36 @@ static int cxl_region_sort_targets(struct cxl_region *cxlr) > return rc; > } > > +static int > +check_interleave_cap(struct cxl_decoder *cxld, int iw, int ig) > +{ > + struct cxl_port *port = to_cxl_port(cxld->dev.parent); > + struct cxl_hdm *cxlhdm = dev_get_drvdata(&port->dev); > + u8 eiw; > + u16 eig; > + int rc; > + > + rc = ways_to_eiw(iw, &eiw); > + if (rc) > + return rc; > + > + if (eiw > 3 && !(cxlhdm->iw_cap_mask & BIT(iw))) { If you have all the bits set then you can avoid using eiw > 3 just to check we aren't 1,2,4,8 > + dev_dbg(&cxld->dev, "iw: %d is not supported\n", iw); > + return -EOPNOTSUPP; > + } > + > + rc = granularity_to_eig(ig, &eig); > + if (rc) > + return rc; > + > + if (!(BIT(eig + 8) & cxlhdm->ig_cap_mask)) { This seems too simple. Need to look at the calculations in IMPLEMENTATON NOTE: CXL Host Bridge and Upstream Switch Port Decode Flow. For a decode of more than 2 ways you need more bits to be supported. > + dev_dbg(&cxld->dev, "ig: %d is not supported\n", ig); > + return -EOPNOTSUPP; > + } > + > + return 0; > +} > + > static int cxl_region_attach(struct cxl_region *cxlr, > struct cxl_endpoint_decoder *cxled, int pos) > { > @@ -1796,6 +1826,17 @@ static int cxl_region_attach(struct cxl_region *cxlr, > struct cxl_dport *dport; > int rc = -ENXIO; > > + rc = check_interleave_cap(&cxled->cxld, p->interleave_ways, > + p->interleave_granularity); > + if (rc) { > + dev_dbg(&cxlr->dev, > + "%s with region iw: %d, ig: %d is not supported\n", > + dev_name(&cxled->cxld.dev), > + p->interleave_ways, > + p->interleave_granularity); > + return rc; > + } > + > if (cxled->mode != cxlr->mode) { > dev_dbg(&cxlr->dev, "%s region mode: %d mismatch: %d\n", > dev_name(&cxled->cxld.dev), cxlr->mode, cxled->mode); > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > index 534e25e2f0a4..da8a487ededa 100644 > --- a/drivers/cxl/cxl.h > +++ b/drivers/cxl/cxl.h > @@ -45,6 +45,8 @@ > #define CXL_HDM_DECODER_TARGET_COUNT_MASK GENMASK(7, 4) > #define CXL_HDM_DECODER_INTERLEAVE_11_8 BIT(8) > #define CXL_HDM_DECODER_INTERLEAVE_14_12 BIT(9) > +#define CXL_HDM_DECODER_INTERLEAVE_3_6_12_WAY BIT(11) > +#define CXL_HDM_DECODER_INTERLEAVE_16_WAY BIT(12) > #define CXL_HDM_DECODER_CTRL_OFFSET 0x4 > #define CXL_HDM_DECODER_ENABLE BIT(1) > #define CXL_HDM_DECODER0_BASE_LOW_OFFSET(i) (0x20 * (i) + 0x10) > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h > index b53f7ae0fdd6..979c22955246 100644 > --- a/drivers/cxl/cxlmem.h > +++ b/drivers/cxl/cxlmem.h > @@ -853,6 +853,7 @@ struct cxl_hdm { > unsigned int decoder_count; > unsigned int target_count; > unsigned int ig_cap_mask; > + unsigned int iw_cap_mask; > struct cxl_port *port; > }; > ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH v2 2/2] cxl/core/region: check interleave capability 2024-04-03 14:27 ` Jonathan Cameron @ 2024-04-08 2:50 ` Xingtao Yao (Fujitsu) 2024-04-08 23:10 ` Dan Williams 0 siblings, 1 reply; 8+ messages in thread From: Xingtao Yao (Fujitsu) @ 2024-04-08 2:50 UTC (permalink / raw) To: Jonathan Cameron Cc: dave@stgolabs.net, dave.jiang@intel.com, alison.schofield@intel.com, vishal.l.verma@intel.com, ira.weiny@intel.com, dan.j.williams@intel.com, jim.harris@samsung.com, linux-cxl@vger.kernel.org > -----Original Message----- > From: Jonathan Cameron <Jonathan.Cameron@Huawei.com> > Sent: Wednesday, April 3, 2024 10:28 PM > To: Yao, Xingtao/姚 幸涛 <yaoxt.fnst@fujitsu.com> > Cc: dave@stgolabs.net; dave.jiang@intel.com; alison.schofield@intel.com; > vishal.l.verma@intel.com; ira.weiny@intel.com; dan.j.williams@intel.com; > jim.harris@samsung.com; linux-cxl@vger.kernel.org > Subject: Re: [PATCH v2 2/2] cxl/core/region: check interleave capability > > On Tue, 2 Apr 2024 22:17:47 -0400 > Yao Xingtao <yaoxt.fnst@fujitsu.com> wrote: > > > Since interleave capability is not checked, a target can be attached to > > region successfully even it does not support the interleave ways or > > interleave granularity. > > > > When accessing the memory, unexpected behavior occurs due to converting > > HPA to an error DPA: > > $ numactl -m 2 ls > > Segmentation fault (core dumped) > > > > Link: > https://lore.kernel.org/qemu-devel/20240327014653.26623-1-yaoxt.fnst@fujitsu.c > om > > Signed-off-by: Yao Xingtao <yaoxt.fnst@fujitsu.com> > > I argued on the CXL opensource sync call last night that we'd get an > hdm commit fail (on working hardware - unlike current qemu) if this check > wasn't present. Having thought more I think I was wrong and this is a > necessary fix because a device that doesn't support one of these ways > treats the HDM Decoder n Control Register / Interleave Ways (IW) values > as 'reserved'. Is it guaranteed to not just do that by fixing the higher > bits to zero? > > If that's a possible implementation and the decoder was set to 6-way (0x9) > maybe the device would interpret that as 2-way (0x1) and give us the wrong > decode. > > With that in mind I think this is a fix and needs a Fixes tag. > > > > --- > > drivers/cxl/core/hdm.c | 4 ++++ > > drivers/cxl/core/region.c | 41 > +++++++++++++++++++++++++++++++++++++++ > > drivers/cxl/cxl.h | 2 ++ > > drivers/cxl/cxlmem.h | 1 + > > 4 files changed, 48 insertions(+) > > > > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c > > index 9bb6a256cc6f..1a99b138dbec 100644 > > --- a/drivers/cxl/core/hdm.c > > +++ b/drivers/cxl/core/hdm.c > > @@ -79,6 +79,10 @@ static void parse_hdm_decoder_caps(struct cxl_hdm > *cxlhdm) > > cxlhdm->ig_cap_mask |= GENMASK(11, 8); > > if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_14_12, hdm_cap)) > > cxlhdm->ig_cap_mask |= GENMASK(14, 12); > > + if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_3_6_12_WAY, > hdm_cap)) > > + cxlhdm->iw_cap_mask |= BIT(3) | BIT(6) | BIT(12); > > + if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_16_WAY, hdm_cap)) > > + cxlhdm->iw_cap_mask |= BIT(16); > > Whilst it doesn't matter as such (because they are always valid) I think > we should also st the bits for 1,2,4,8 so that all relevant bits are > enabled. good idea! > > > } > > > > static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info) > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > > index 5c186e0a39b9..25d178e14ed1 100644 > > --- a/drivers/cxl/core/region.c > > +++ b/drivers/cxl/core/region.c > > @@ -1786,6 +1786,36 @@ static int cxl_region_sort_targets(struct cxl_region > *cxlr) > > return rc; > > } > > > > +static int > > +check_interleave_cap(struct cxl_decoder *cxld, int iw, int ig) > > +{ > > + struct cxl_port *port = to_cxl_port(cxld->dev.parent); > > + struct cxl_hdm *cxlhdm = dev_get_drvdata(&port->dev); > > + u8 eiw; > > + u16 eig; > > + int rc; > > + > > + rc = ways_to_eiw(iw, &eiw); > > + if (rc) > > + return rc; > > + > > + if (eiw > 3 && !(cxlhdm->iw_cap_mask & BIT(iw))) { > If you have all the bits set then you can avoid using eiw > 3 just > to check we aren't 1,2,4,8 > > > + dev_dbg(&cxld->dev, "iw: %d is not supported\n", iw); > > + return -EOPNOTSUPP; > > + } > > + > > + rc = granularity_to_eig(ig, &eig); > > + if (rc) > > + return rc; > > + > > + if (!(BIT(eig + 8) & cxlhdm->ig_cap_mask)) { > > This seems too simple. Need to look at the calculations in > IMPLEMENTATON NOTE: CXL Host Bridge and Upstream Switch Port Decode > Flow. > > For a decode of more than 2 ways you need more bits to be supported. this is my misunderstanding of interleave bits, I will change this logical. > > > + dev_dbg(&cxld->dev, "ig: %d is not supported\n", ig); > > + return -EOPNOTSUPP; > > + } > > + > > + return 0; > > +} > > + > > static int cxl_region_attach(struct cxl_region *cxlr, > > struct cxl_endpoint_decoder *cxled, int pos) > > { > > @@ -1796,6 +1826,17 @@ static int cxl_region_attach(struct cxl_region *cxlr, > > struct cxl_dport *dport; > > int rc = -ENXIO; > > > > + rc = check_interleave_cap(&cxled->cxld, p->interleave_ways, > > + p->interleave_granularity); > > + if (rc) { > > + dev_dbg(&cxlr->dev, > > + "%s with region iw: %d, ig: %d is not supported\n", > > + dev_name(&cxled->cxld.dev), > > + p->interleave_ways, > > + p->interleave_granularity); > > + return rc; > > + } > > + > > if (cxled->mode != cxlr->mode) { > > dev_dbg(&cxlr->dev, "%s region mode: %d mismatch: %d\n", > > dev_name(&cxled->cxld.dev), cxlr->mode, > cxled->mode); > > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > > index 534e25e2f0a4..da8a487ededa 100644 > > --- a/drivers/cxl/cxl.h > > +++ b/drivers/cxl/cxl.h > > @@ -45,6 +45,8 @@ > > #define CXL_HDM_DECODER_TARGET_COUNT_MASK GENMASK(7, 4) > > #define CXL_HDM_DECODER_INTERLEAVE_11_8 BIT(8) > > #define CXL_HDM_DECODER_INTERLEAVE_14_12 BIT(9) > > +#define CXL_HDM_DECODER_INTERLEAVE_3_6_12_WAY BIT(11) > > +#define CXL_HDM_DECODER_INTERLEAVE_16_WAY BIT(12) > > #define CXL_HDM_DECODER_CTRL_OFFSET 0x4 > > #define CXL_HDM_DECODER_ENABLE BIT(1) > > #define CXL_HDM_DECODER0_BASE_LOW_OFFSET(i) (0x20 * (i) + 0x10) > > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h > > index b53f7ae0fdd6..979c22955246 100644 > > --- a/drivers/cxl/cxlmem.h > > +++ b/drivers/cxl/cxlmem.h > > @@ -853,6 +853,7 @@ struct cxl_hdm { > > unsigned int decoder_count; > > unsigned int target_count; > > unsigned int ig_cap_mask; > > + unsigned int iw_cap_mask; > > struct cxl_port *port; > > }; > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH v2 2/2] cxl/core/region: check interleave capability 2024-04-08 2:50 ` Xingtao Yao (Fujitsu) @ 2024-04-08 23:10 ` Dan Williams 0 siblings, 0 replies; 8+ messages in thread From: Dan Williams @ 2024-04-08 23:10 UTC (permalink / raw) To: Xingtao Yao (Fujitsu), Jonathan Cameron Cc: dave@stgolabs.net, dave.jiang@intel.com, alison.schofield@intel.com, vishal.l.verma@intel.com, ira.weiny@intel.com, dan.j.williams@intel.com, jim.harris@samsung.com, linux-cxl@vger.kernel.org Xingtao Yao (Fujitsu) wrote: [..] > > > + dev_dbg(&cxld->dev, "iw: %d is not supported\n", iw); > > > + return -EOPNOTSUPP; > > > + } > > > + > > > + rc = granularity_to_eig(ig, &eig); > > > + if (rc) > > > + return rc; > > > + > > > + if (!(BIT(eig + 8) & cxlhdm->ig_cap_mask)) { > > > > This seems too simple. Need to look at the calculations in > > IMPLEMENTATON NOTE: CXL Host Bridge and Upstream Switch Port Decode > > Flow. > > > > For a decode of more than 2 ways you need more bits to be supported. > this is my misunderstanding of interleave bits, I will change this logical. My expectation is that interleave_mask is an address mask and can only be validated by multiplying it against interleave_ways. For example, for power-of-2 interleave_ways: region_interleave_mask = GENMASK(eiw - 1 + eig + 8, eig + 8); ...that is the address bits required to decode the interleave, and if that mask is not fully supported by the device-address mask then the device can not be attached to that region. However, above is untested, please double check my math. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-04-08 23:10 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-04-03 2:17 [PATCH v2 0/2] cxl: add interleave capability check Yao Xingtao 2024-04-03 2:17 ` [PATCH v2 1/2] cxl/core/hdm: rename interleave_mask to ig_cap_mask Yao Xingtao 2024-04-03 14:06 ` Jonathan Cameron 2024-04-08 2:42 ` Xingtao Yao (Fujitsu) 2024-04-03 2:17 ` [PATCH v2 2/2] cxl/core/region: check interleave capability Yao Xingtao 2024-04-03 14:27 ` Jonathan Cameron 2024-04-08 2:50 ` Xingtao Yao (Fujitsu) 2024-04-08 23:10 ` Dan Williams
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.