Re: [PATCH 3/5] cxl/acpi: Minimize granularity for x1 interleaves

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

On Fri, 22 Jul 2022 17:56:09 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> The kernel enforces that region granularity is >= to the top-level
> interleave-granularity for the given CXL window. However, when the CXL
> window interleave is x1, i.e. non-interleaved at the host bridge level,
> then the specified granularity does not matter. Override the window
> specified granularity to the CXL minimum so that any valid region
> granularity is >= to the root granularity.
> 
> Reported-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Hi Dan,

Debugging exactly why this is failing (from cxl.git/preview) for my test setup...
(1 hb, 8 rp, 8 direct connected devices)

If I set the interleave granularity of a region to 256, I end
up with 256 for the CFMWS which is fine, then 512 for the HB which
is not - EP interleave granularity is expected 256.

https://git.kernel.org/pub/scm/linux/kernel/git/cxl/cxl.git/tree/drivers/cxl/core/region.c?h=preview#n1070

Calculates the eig as address_bit - eiw + 1

iw = 8
eiw = 3
peig = 0 (pig = 256)
peiw = 0 (piw = 1)
(all as expected I think...)

So address_bit = s max(peig + peiw, eiw + peig) = max(0, 3)
and eig = 3 - 3 + 1 = 1 (ig = 512) which is wrong.

I'm not 100% sure on the logic behind this maths, but would expect eig = 0 as the output for this
setup..

Even with this hacked, qemu address decode is landing at wrong address in the backing files (but it
is at least landing in the right file!)
Curiously interleave ways = 1 for the EPs which is obviously wrong. (I'm not convinced the
qemu address logic is right but it'll never work with that value).  I'm struggling to figure
out where we actually set the interleave ways for an EP.

Also I'm not having much luck requesting a larger interleave granularity for the region (desirable perhaps
because the devices give better performance with 1024 byte sequential reads)

Clearly going to be one of those bugs all the way down days. 

Thanks,

Jonathan


> ---
>  drivers/cxl/acpi.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index eb436268b92c..67137e17b8c9 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -140,6 +140,12 @@ static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg,
>  		.end = res->end,
>  	};
>  	cxld->interleave_ways = ways;
> +	/*
> +	 * Minimize the x1 granularity to advertise support for any
> +	 * valid region granularity
> +	 */
> +	if (ways == 1)
> +		ig = 256;
>  	cxld->interleave_granularity = ig;
>  
>  	rc = cxl_decoder_add(cxld, target_map);
> 
>