| Message ID | 165973127171.1526540.9923273539049172976.stgit@dwillia2-xfh.jf.intel.com |
|---|---|
| State | Accepted |
| Commit | 4d8e4ea5bb396897111e8a740201bfd3c5926170 |
| Headers | show |
| Series | CXL Region Provisioning Fixes | expand |
On Fri, Aug 05, 2022 at 01:27:51PM -0700, Dan Williams wrote: > The endpoint decode granularity must be <= the window granularity > otherwise capacity in the endpoints is lost in the decode. Consider an > attempt to have a region granularity of 512 with 4 devices within a > window that maps 2 host bridges at a granularity of 256 bytes: > > HPA DPA Offset HB Port EP > 0x0 0x0 0 0 0 > 0x100 0x0 1 0 2 > 0x200 0x100 0 0 0 > 0x300 0x100 1 0 2 > 0x400 0x200 0 1 1 > 0x500 0x200 1 1 3 > 0x600 0x300 0 1 1 > 0x700 0x300 1 1 3 > 0x800 0x400 0 0 0 > 0x900 0x400 1 0 2 > 0xA00 0x500 0 0 0 > 0xB00 0x500 1 0 2 > > Notice how endpoint0 maps HPA 0x0 and 0x200 correctly, but then HPA > 0x800 results in DPA 0x200 to 0x400 on endpoint0 being not skipped. > > Fix this by restricing the region granularity to be equal to the window > granularity resulting in the following for a x4 region under a x2 window > at a granularity of 256. > > HPA DPA Offset HB Port EP > 0x0 0x0 0 0 0 > 0x100 0x0 1 0 2 > 0x200 0x0 0 1 1 > 0x300 0x0 1 1 3 > 0x400 0x100 0 0 0 > 0x500 0x100 1 0 2 > 0x600 0x100 0 1 1 > 0x700 0x100 1 1 3 > > Not that it ever made practical sense to support region granularity > > window granularity. The window rotates host bridges causing endpoints to > never see a consecutive stream of requests at the desired granularity > without breaks to issue cycles to the other host bridge. > > Fixes: 80d10a6cee05 ("cxl/region: Add interleave geometry attributes") > Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Reviewed-by: Vishal Verma <vishal.l.verma@intel.com> Reviewed-by: Ira Weiny <ira.weiny@intel.com> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > --- > drivers/cxl/core/region.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index 641bc6344a4a..401148016978 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -395,13 +395,14 @@ static ssize_t interleave_granularity_store(struct device *dev, > return rc; > > /* > - * Disallow region granularity less than root granularity to > - * simplify the implementation. Otherwise, region's with a > - * granularity less than the root interleave result in needing > - * multiple endpoints to support a single slot in the > - * interleave. > + * When the host-bridge is interleaved, disallow region granularity != > + * root granularity. Regions with a granularity less than the root > + * interleave result in needing multiple endpoints to support a single > + * slot in the interleave (possible to suport in the future). Regions > + * with a granularity greater than the root interleave result in invalid > + * DPA translations (invalid to support). > */ > - if (val < cxld->interleave_granularity) > + if (cxld->interleave_ways > 1 && val != cxld->interleave_granularity) > return -EINVAL; > > rc = down_write_killable(&cxl_region_rwsem); >
On Fri, 05 Aug 2022 13:27:51 -0700 Dan Williams <dan.j.williams@intel.com> wrote: > The endpoint decode granularity must be <= the window granularity > otherwise capacity in the endpoints is lost in the decode. Consider an > attempt to have a region granularity of 512 with 4 devices within a > window that maps 2 host bridges at a granularity of 256 bytes: > > HPA DPA Offset HB Port EP > 0x0 0x0 0 0 0 > 0x100 0x0 1 0 2 > 0x200 0x100 0 0 0 > 0x300 0x100 1 0 2 > 0x400 0x200 0 1 1 > 0x500 0x200 1 1 3 > 0x600 0x300 0 1 1 > 0x700 0x300 1 1 3 > 0x800 0x400 0 0 0 > 0x900 0x400 1 0 2 > 0xA00 0x500 0 0 0 > 0xB00 0x500 1 0 2 > > Notice how endpoint0 maps HPA 0x0 and 0x200 correctly, but then HPA > 0x800 results in DPA 0x200 to 0x400 on endpoint0 being not skipped. > > Fix this by restricing the region granularity to be equal to the window restricting > granularity resulting in the following for a x4 region under a x2 window > at a granularity of 256. > > HPA DPA Offset HB Port EP > 0x0 0x0 0 0 0 > 0x100 0x0 1 0 2 > 0x200 0x0 0 1 1 > 0x300 0x0 1 1 3 > 0x400 0x100 0 0 0 > 0x500 0x100 1 0 2 > 0x600 0x100 0 1 1 > 0x700 0x100 1 1 3 > > Not that it ever made practical sense to support region granularity > > window granularity. The window rotates host bridges causing endpoints to > never see a consecutive stream of requests at the desired granularity > without breaks to issue cycles to the other host bridge. Possibly worth description mentioning the (possible to support in the future) case you have in the comment in the code. I'm not that fussed though either way. > (Regions with a granularity less than the root > + * interleave result in needing multiple endpoints to support a single > + * slot in the interleave) Last bit is interesting and based on region granularity is always constant across different EPs (currently interface enforces that - not as far as I can tell the spec) I think the spec would allow: HB0: 2 EP, HB1 1EP with HB interleave at 256 bytes (interleave irrelevant for HB1 as it only has one port) and CFMWS at 512. HPA DPA Offset HB Port EP 0x0 0x0 0 0 0 0x100 0x0 0 1 1 0x200 0x0 1 0 2 0x300 0x100 1 0 2 0x400 0x100 0 0 0 0x500 0x100 0 1 1 0x600 0x200 1 0 2 0x700 0x300 1 0 2 0x800 0x200 0 0 0 0x900 0x200 0 1 1 0xa00 0x400 1 0 2 0xb00 0x500 1 0 2 Fun bit is region ends up having granularity 256 on some devices and 512 on others. Perhaps better to not mention this (assuming I haven't made an error in the maths) :) So I'm fine with leaving the comment as is given it's correct for current combination of software model and what the spec allows. > > Fixes: 80d10a6cee05 ("cxl/region: Add interleave geometry attributes") > Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Reviewed-by: Vishal Verma <vishal.l.verma@intel.com> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/cxl/core/region.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index 641bc6344a4a..401148016978 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -395,13 +395,14 @@ static ssize_t interleave_granularity_store(struct device *dev, > return rc; > > /* > - * Disallow region granularity less than root granularity to > - * simplify the implementation. Otherwise, region's with a > - * granularity less than the root interleave result in needing > - * multiple endpoints to support a single slot in the > - * interleave. > + * When the host-bridge is interleaved, disallow region granularity != > + * root granularity. Regions with a granularity less than the root > + * interleave result in needing multiple endpoints to support a single > + * slot in the interleave (possible to suport in the future). Regions > + * with a granularity greater than the root interleave result in invalid > + * DPA translations (invalid to support). > */ > - if (val < cxld->interleave_granularity) > + if (cxld->interleave_ways > 1 && val != cxld->interleave_granularity) > return -EINVAL; > > rc = down_write_killable(&cxl_region_rwsem); >
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index 641bc6344a4a..401148016978 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -395,13 +395,14 @@ static ssize_t interleave_granularity_store(struct device *dev, return rc; /* - * Disallow region granularity less than root granularity to - * simplify the implementation. Otherwise, region's with a - * granularity less than the root interleave result in needing - * multiple endpoints to support a single slot in the - * interleave. + * When the host-bridge is interleaved, disallow region granularity != + * root granularity. Regions with a granularity less than the root + * interleave result in needing multiple endpoints to support a single + * slot in the interleave (possible to suport in the future). Regions + * with a granularity greater than the root interleave result in invalid + * DPA translations (invalid to support). */ - if (val < cxld->interleave_granularity) + if (cxld->interleave_ways > 1 && val != cxld->interleave_granularity) return -EINVAL; rc = down_write_killable(&cxl_region_rwsem);
The endpoint decode granularity must be <= the window granularity otherwise capacity in the endpoints is lost in the decode. Consider an attempt to have a region granularity of 512 with 4 devices within a window that maps 2 host bridges at a granularity of 256 bytes: HPA DPA Offset HB Port EP 0x0 0x0 0 0 0 0x100 0x0 1 0 2 0x200 0x100 0 0 0 0x300 0x100 1 0 2 0x400 0x200 0 1 1 0x500 0x200 1 1 3 0x600 0x300 0 1 1 0x700 0x300 1 1 3 0x800 0x400 0 0 0 0x900 0x400 1 0 2 0xA00 0x500 0 0 0 0xB00 0x500 1 0 2 Notice how endpoint0 maps HPA 0x0 and 0x200 correctly, but then HPA 0x800 results in DPA 0x200 to 0x400 on endpoint0 being not skipped. Fix this by restricing the region granularity to be equal to the window granularity resulting in the following for a x4 region under a x2 window at a granularity of 256. HPA DPA Offset HB Port EP 0x0 0x0 0 0 0 0x100 0x0 1 0 2 0x200 0x0 0 1 1 0x300 0x0 1 1 3 0x400 0x100 0 0 0 0x500 0x100 1 0 2 0x600 0x100 0 1 1 0x700 0x100 1 1 3 Not that it ever made practical sense to support region granularity > window granularity. The window rotates host bridges causing endpoints to never see a consecutive stream of requests at the desired granularity without breaks to issue cycles to the other host bridge. Fixes: 80d10a6cee05 ("cxl/region: Add interleave geometry attributes") Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com> Reviewed-by: Vishal Verma <vishal.l.verma@intel.com> Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- drivers/cxl/core/region.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)