Re: [PATCH v2 3/3] cxl/region: Disallow region granularity != window granularity

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

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);
>