| Message ID | 165999281717.493131.1159254270127915425.stgit@djiang5-desk4.jf.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add sanity check for interleave setup | expand |
Dave Jiang wrote: > Add a helper function to check the combination of interleave ways and > interleave granularity together is sane against the interleave mask from > the HDM decoder. Add the check to cxl_region_attach() to make sure the > region config is sane. Add the check to cxl_port_setup_targets() to make > sure the port setup config is also sane. > > Calculation refers to CXL spec v3 8.2.4.19.13 implementation note #3. > > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > --- > drivers/cxl/core/region.c | 17 ++++++++++++++++- > drivers/cxl/cxl.h | 11 +++++++++++ > drivers/cxl/cxlmem.h | 31 +++++++++++++++++++++++++++++++ > 3 files changed, 58 insertions(+), 1 deletion(-) > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index cf5d5811fe4c..a209a8de31fd 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -1081,6 +1081,13 @@ static int cxl_port_setup_targets(struct cxl_port *port, > return rc; > } > > + rc = cxl_interleave_verify(port, iw, ig); There are multiple "interleave verify" actions, this function is just handling the interleave address bit capability, so how about: s/cxl_interleave_verify/cxl_interleave_capable/ > + if (rc) { > + dev_dbg(&cxlr->dev, "%s:%s: invalid interleave & granularity combo: %d\n", If this fires I would want to know the iw and ig settings, something like: "%s:%s interleave (ig: %d iw: %d mask: %#x) exceeds capability (mask: %#x)\n" Likely that message would need to move internal to cxl_interleave_capable() where you have the address masks available. > + dev_name(port->uport), dev_name(&port->dev), rc); > + return rc; > + } > + > cxld->interleave_ways = iw; > cxld->interleave_granularity = ig; > dev_dbg(&cxlr->dev, "%s:%s iw: %d ig: %d\n", dev_name(port->uport), > @@ -1218,6 +1225,15 @@ static int cxl_region_attach(struct cxl_region *cxlr, > return -EBUSY; > } > > + ep_port = cxled_to_port(cxled); > + rc = cxl_interleave_verify(ep_port, p->interleave_ways, > + p->interleave_granularity); > + if (rc) { > + dev_dbg(&cxlr->dev, "%s: invalid interleave & granularity combo: %d\n", > + dev_name(&cxlmd->dev), rc); ...and then you don't need to duplicate the message if it is internal to cxl_interleave_capable(). > + return rc; > + } > + > for (i = 0; i < p->interleave_ways; i++) { > struct cxl_endpoint_decoder *cxled_target; > struct cxl_memdev *cxlmd_target; > @@ -1236,7 +1252,6 @@ static int cxl_region_attach(struct cxl_region *cxlr, > } > } > > - ep_port = cxled_to_port(cxled); > root_port = cxlrd_to_port(cxlrd); > dport = cxl_find_dport_by_dev(root_port, ep_port->host_bridge); > if (!dport) { > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > index bc604b7e44fb..275979fbd15a 100644 > --- a/drivers/cxl/cxl.h > +++ b/drivers/cxl/cxl.h > @@ -61,6 +61,17 @@ > #define CXL_HDM_DECODER0_SKIP_LOW(i) CXL_HDM_DECODER0_TL_LOW(i) > #define CXL_HDM_DECODER0_SKIP_HIGH(i) CXL_HDM_DECODER0_TL_HIGH(i) > > +enum { > + CXL_INTERLEAVE_1_WAY = 0, > + CXL_INTERLEAVE_2_WAY, > + CXL_INTERLEAVE_4_WAY, > + CXL_INTERLEAVE_8_WAY, > + CXL_INTERLEAVE_16_WAY, > + CXL_INTERLEAVE_3_WAY = 8, > + CXL_INTERLEAVE_6_WAY, > + CXL_INTERLEAVE_12_WAY I'm not sure this new enum is worth it given only one of these will ever be used. > +}; > + > static inline int cxl_hdm_decoder_count(u32 cap_hdr) > { > int val = FIELD_GET(CXL_HDM_DECODER_COUNT_MASK, cap_hdr); > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h > index 88e3a8e54b6a..d5f872ca62f9 100644 > --- a/drivers/cxl/cxlmem.h > +++ b/drivers/cxl/cxlmem.h > @@ -401,6 +401,37 @@ struct cxl_hdm { > struct cxl_port *port; > }; > > +static inline int cxl_interleave_verify(struct cxl_port *port, > + int ways, int granularity) > +{ > + struct cxl_hdm *cxlhdm = dev_get_drvdata(&port->dev); > + unsigned int addr_mask; > + u16 ig; > + u8 iw; > + int rc; > + > + rc = granularity_to_cxl(granularity, &ig); > + if (rc) > + return rc; > + > + rc = ways_to_cxl(ways, &iw); > + if (rc) > + return rc; > + > + if (iw == 0) > + return 0; > + > + if (iw < CXL_INTERLEAVE_3_WAY) ...just do "is_power_of_2(iw)" here instead. > + addr_mask = GENMASK(ig + 8 + iw - 1, ig + 8); > + else > + addr_mask = GENMASK((ig + iw) / 3 - 1, ig + 8);
Dave - Haven't reviewed yet, but wanted to drop this tidbit - On Mon, Aug 08, 2022 at 02:06:57PM -0700, Dave Jiang wrote: > Add a helper function to check the combination of interleave ways and > interleave granularity together is sane against the interleave mask from > the HDM decoder. Add the check to cxl_region_attach() to make sure the > region config is sane. Add the check to cxl_port_setup_targets() to make > sure the port setup config is also sane. > > Calculation refers to CXL spec v3 8.2.4.19.13 implementation note #3. > > Signed-off-by: Dave Jiang <dave.jiang@intel.com> snip > +static inline int cxl_interleave_verify(struct cxl_port *port, > + int ways, int granularity) > +{ > + struct cxl_hdm *cxlhdm = dev_get_drvdata(&port->dev); > + unsigned int addr_mask; > + u16 ig; > + u8 iw; s/ig/eig s/iw/eiw For consistency, let's use the "e" version of these names to mean encoded value. > + int rc; > + > + rc = granularity_to_cxl(granularity, &ig); > + if (rc) > + return rc; > + > + rc = ways_to_cxl(ways, &iw); > + if (rc) > + return rc; > + > + if (iw == 0) > + return 0; > + > + if (iw < CXL_INTERLEAVE_3_WAY) > + addr_mask = GENMASK(ig + 8 + iw - 1, ig + 8); > + else > + addr_mask = GENMASK((ig + iw) / 3 - 1, ig + 8); > + > + if (~cxlhdm->interleave_mask & addr_mask) > + return -EINVAL; > + > + return 0; > +} > + > struct seq_file; > struct dentry *cxl_debugfs_create_dir(const char *dir); > void cxl_dpa_debug(struct seq_file *file, struct cxl_dev_state *cxlds); > >
On 8/9/2022 9:18 AM, Dan Williams wrote: > Dave Jiang wrote: >> Add a helper function to check the combination of interleave ways and >> interleave granularity together is sane against the interleave mask from >> the HDM decoder. Add the check to cxl_region_attach() to make sure the >> region config is sane. Add the check to cxl_port_setup_targets() to make >> sure the port setup config is also sane. >> >> Calculation refers to CXL spec v3 8.2.4.19.13 implementation note #3. >> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com> >> --- >> drivers/cxl/core/region.c | 17 ++++++++++++++++- >> drivers/cxl/cxl.h | 11 +++++++++++ >> drivers/cxl/cxlmem.h | 31 +++++++++++++++++++++++++++++++ >> 3 files changed, 58 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c >> index cf5d5811fe4c..a209a8de31fd 100644 >> --- a/drivers/cxl/core/region.c >> +++ b/drivers/cxl/core/region.c >> @@ -1081,6 +1081,13 @@ static int cxl_port_setup_targets(struct cxl_port *port, >> return rc; >> } >> >> + rc = cxl_interleave_verify(port, iw, ig); > There are multiple "interleave verify" actions, this function is just > handling the interleave address bit capability, so how about: > > s/cxl_interleave_verify/cxl_interleave_capable/ ok > >> + if (rc) { >> + dev_dbg(&cxlr->dev, "%s:%s: invalid interleave & granularity combo: %d\n", > If this fires I would want to know the iw and ig settings, something > like: > > "%s:%s interleave (ig: %d iw: %d mask: %#x) exceeds capability (mask: %#x)\n" > > Likely that message would need to move internal to > cxl_interleave_capable() where you have the address masks available. Will move it internally > > >> + dev_name(port->uport), dev_name(&port->dev), rc); >> + return rc; >> + } >> + >> cxld->interleave_ways = iw; >> cxld->interleave_granularity = ig; >> dev_dbg(&cxlr->dev, "%s:%s iw: %d ig: %d\n", dev_name(port->uport), >> @@ -1218,6 +1225,15 @@ static int cxl_region_attach(struct cxl_region *cxlr, >> return -EBUSY; >> } >> >> + ep_port = cxled_to_port(cxled); >> + rc = cxl_interleave_verify(ep_port, p->interleave_ways, >> + p->interleave_granularity); >> + if (rc) { >> + dev_dbg(&cxlr->dev, "%s: invalid interleave & granularity combo: %d\n", >> + dev_name(&cxlmd->dev), rc); > ...and then you don't need to duplicate the message if it is internal to > cxl_interleave_capable(). > >> + return rc; >> + } >> + >> for (i = 0; i < p->interleave_ways; i++) { >> struct cxl_endpoint_decoder *cxled_target; >> struct cxl_memdev *cxlmd_target; >> @@ -1236,7 +1252,6 @@ static int cxl_region_attach(struct cxl_region *cxlr, >> } >> } >> >> - ep_port = cxled_to_port(cxled); >> root_port = cxlrd_to_port(cxlrd); >> dport = cxl_find_dport_by_dev(root_port, ep_port->host_bridge); >> if (!dport) { >> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h >> index bc604b7e44fb..275979fbd15a 100644 >> --- a/drivers/cxl/cxl.h >> +++ b/drivers/cxl/cxl.h >> @@ -61,6 +61,17 @@ >> #define CXL_HDM_DECODER0_SKIP_LOW(i) CXL_HDM_DECODER0_TL_LOW(i) >> #define CXL_HDM_DECODER0_SKIP_HIGH(i) CXL_HDM_DECODER0_TL_HIGH(i) >> >> +enum { >> + CXL_INTERLEAVE_1_WAY = 0, >> + CXL_INTERLEAVE_2_WAY, >> + CXL_INTERLEAVE_4_WAY, >> + CXL_INTERLEAVE_8_WAY, >> + CXL_INTERLEAVE_16_WAY, >> + CXL_INTERLEAVE_3_WAY = 8, >> + CXL_INTERLEAVE_6_WAY, >> + CXL_INTERLEAVE_12_WAY > I'm not sure this new enum is worth it given only one of these will ever > be used. Will remove > >> +}; >> + >> static inline int cxl_hdm_decoder_count(u32 cap_hdr) >> { >> int val = FIELD_GET(CXL_HDM_DECODER_COUNT_MASK, cap_hdr); >> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h >> index 88e3a8e54b6a..d5f872ca62f9 100644 >> --- a/drivers/cxl/cxlmem.h >> +++ b/drivers/cxl/cxlmem.h >> @@ -401,6 +401,37 @@ struct cxl_hdm { >> struct cxl_port *port; >> }; >> >> +static inline int cxl_interleave_verify(struct cxl_port *port, >> + int ways, int granularity) >> +{ >> + struct cxl_hdm *cxlhdm = dev_get_drvdata(&port->dev); >> + unsigned int addr_mask; >> + u16 ig; >> + u8 iw; >> + int rc; >> + >> + rc = granularity_to_cxl(granularity, &ig); >> + if (rc) >> + return rc; >> + >> + rc = ways_to_cxl(ways, &iw); >> + if (rc) >> + return rc; >> + >> + if (iw == 0) >> + return 0; >> + >> + if (iw < CXL_INTERLEAVE_3_WAY) > ...just do "is_power_of_2(iw)" here instead. ok > >> + addr_mask = GENMASK(ig + 8 + iw - 1, ig + 8); >> + else >> + addr_mask = GENMASK((ig + iw) / 3 - 1, ig + 8);
On 8/9/2022 10:06 AM, Alison Schofield wrote: > Dave - Haven't reviewed yet, but wanted to drop this tidbit - > > On Mon, Aug 08, 2022 at 02:06:57PM -0700, Dave Jiang wrote: >> Add a helper function to check the combination of interleave ways and >> interleave granularity together is sane against the interleave mask from >> the HDM decoder. Add the check to cxl_region_attach() to make sure the >> region config is sane. Add the check to cxl_port_setup_targets() to make >> sure the port setup config is also sane. >> >> Calculation refers to CXL spec v3 8.2.4.19.13 implementation note #3. >> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com> > snip > >> +static inline int cxl_interleave_verify(struct cxl_port *port, >> + int ways, int granularity) >> +{ >> + struct cxl_hdm *cxlhdm = dev_get_drvdata(&port->dev); >> + unsigned int addr_mask; >> + u16 ig; >> + u8 iw; > s/ig/eig > s/iw/eiw > For consistency, let's use the "e" version of these names to mean > encoded value. Ok. Good suggestion. > > >> + int rc; >> + >> + rc = granularity_to_cxl(granularity, &ig); >> + if (rc) >> + return rc; >> + >> + rc = ways_to_cxl(ways, &iw); >> + if (rc) >> + return rc; >> + >> + if (iw == 0) >> + return 0; >> + >> + if (iw < CXL_INTERLEAVE_3_WAY) >> + addr_mask = GENMASK(ig + 8 + iw - 1, ig + 8); >> + else >> + addr_mask = GENMASK((ig + iw) / 3 - 1, ig + 8); >> + >> + if (~cxlhdm->interleave_mask & addr_mask) >> + return -EINVAL; >> + >> + return 0; >> +} >> + >> struct seq_file; >> struct dentry *cxl_debugfs_create_dir(const char *dir); >> void cxl_dpa_debug(struct seq_file *file, struct cxl_dev_state *cxlds); >> >>
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index cf5d5811fe4c..a209a8de31fd 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -1081,6 +1081,13 @@ static int cxl_port_setup_targets(struct cxl_port *port, return rc; } + rc = cxl_interleave_verify(port, iw, ig); + if (rc) { + dev_dbg(&cxlr->dev, "%s:%s: invalid interleave & granularity combo: %d\n", + dev_name(port->uport), dev_name(&port->dev), rc); + return rc; + } + cxld->interleave_ways = iw; cxld->interleave_granularity = ig; dev_dbg(&cxlr->dev, "%s:%s iw: %d ig: %d\n", dev_name(port->uport), @@ -1218,6 +1225,15 @@ static int cxl_region_attach(struct cxl_region *cxlr, return -EBUSY; } + ep_port = cxled_to_port(cxled); + rc = cxl_interleave_verify(ep_port, p->interleave_ways, + p->interleave_granularity); + if (rc) { + dev_dbg(&cxlr->dev, "%s: invalid interleave & granularity combo: %d\n", + dev_name(&cxlmd->dev), rc); + return rc; + } + for (i = 0; i < p->interleave_ways; i++) { struct cxl_endpoint_decoder *cxled_target; struct cxl_memdev *cxlmd_target; @@ -1236,7 +1252,6 @@ static int cxl_region_attach(struct cxl_region *cxlr, } } - ep_port = cxled_to_port(cxled); root_port = cxlrd_to_port(cxlrd); dport = cxl_find_dport_by_dev(root_port, ep_port->host_bridge); if (!dport) { diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index bc604b7e44fb..275979fbd15a 100644 --- a/drivers/cxl/cxl.h +++ b/drivers/cxl/cxl.h @@ -61,6 +61,17 @@ #define CXL_HDM_DECODER0_SKIP_LOW(i) CXL_HDM_DECODER0_TL_LOW(i) #define CXL_HDM_DECODER0_SKIP_HIGH(i) CXL_HDM_DECODER0_TL_HIGH(i) +enum { + CXL_INTERLEAVE_1_WAY = 0, + CXL_INTERLEAVE_2_WAY, + CXL_INTERLEAVE_4_WAY, + CXL_INTERLEAVE_8_WAY, + CXL_INTERLEAVE_16_WAY, + CXL_INTERLEAVE_3_WAY = 8, + CXL_INTERLEAVE_6_WAY, + CXL_INTERLEAVE_12_WAY +}; + static inline int cxl_hdm_decoder_count(u32 cap_hdr) { int val = FIELD_GET(CXL_HDM_DECODER_COUNT_MASK, cap_hdr); diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h index 88e3a8e54b6a..d5f872ca62f9 100644 --- a/drivers/cxl/cxlmem.h +++ b/drivers/cxl/cxlmem.h @@ -401,6 +401,37 @@ struct cxl_hdm { struct cxl_port *port; }; +static inline int cxl_interleave_verify(struct cxl_port *port, + int ways, int granularity) +{ + struct cxl_hdm *cxlhdm = dev_get_drvdata(&port->dev); + unsigned int addr_mask; + u16 ig; + u8 iw; + int rc; + + rc = granularity_to_cxl(granularity, &ig); + if (rc) + return rc; + + rc = ways_to_cxl(ways, &iw); + if (rc) + return rc; + + if (iw == 0) + return 0; + + if (iw < CXL_INTERLEAVE_3_WAY) + addr_mask = GENMASK(ig + 8 + iw - 1, ig + 8); + else + addr_mask = GENMASK((ig + iw) / 3 - 1, ig + 8); + + if (~cxlhdm->interleave_mask & addr_mask) + return -EINVAL; + + return 0; +} + struct seq_file; struct dentry *cxl_debugfs_create_dir(const char *dir); void cxl_dpa_debug(struct seq_file *file, struct cxl_dev_state *cxlds);
Add a helper function to check the combination of interleave ways and interleave granularity together is sane against the interleave mask from the HDM decoder. Add the check to cxl_region_attach() to make sure the region config is sane. Add the check to cxl_port_setup_targets() to make sure the port setup config is also sane. Calculation refers to CXL spec v3 8.2.4.19.13 implementation note #3. Signed-off-by: Dave Jiang <dave.jiang@intel.com> --- drivers/cxl/core/region.c | 17 ++++++++++++++++- drivers/cxl/cxl.h | 11 +++++++++++ drivers/cxl/cxlmem.h | 31 +++++++++++++++++++++++++++++++ 3 files changed, 58 insertions(+), 1 deletion(-)