On Thu, 6 Jan 2022 16:37:52 -0800
Ben Widawsky <ben.widawsky@xxxxxxxxx> wrote:
> Cross host bridge verification primarily determines if the requested
> interleave ordering can be achieved by the root decoder, which isn't as
> programmable as other decoders.
>
> The algorithm implemented here is based on the CXL Type 3 Memory Device
> Software Guide, chapter 2.13.14
>
> Signed-off-by: Ben Widawsky <ben.widawsky@xxxxxxxxx>
Hi Ben,
A few things I'm carrying 'fixes' for in here.
Jonathan
> ---
> .clang-format | 2 +
> drivers/cxl/region.c | 89 +++++++++++++++++++++++++++++++++++++++++++-
> drivers/cxl/trace.h | 3 ++
> 3 files changed, 93 insertions(+), 1 deletion(-)
>
> diff --git a/.clang-format b/.clang-format
> index 15d4eaabc6b5..55f628f21722 100644
> --- a/.clang-format
> +++ b/.clang-format
> @@ -169,6 +169,8 @@ ForEachMacros:
> - 'for_each_cpu_and'
> - 'for_each_cpu_not'
> - 'for_each_cpu_wrap'
> + - 'for_each_cxl_decoder_target'
> + - 'for_each_cxl_endpoint'
> - 'for_each_dapm_widgets'
> - 'for_each_dev_addr'
> - 'for_each_dev_scope'
> diff --git a/drivers/cxl/region.c b/drivers/cxl/region.c
> index c8e3c48dfbb9..ca559a4b5347 100644
> --- a/drivers/cxl/region.c
> +++ b/drivers/cxl/region.c
> @@ -28,6 +28,17 @@
> */
>
> #define region_ways(region) ((region)->config.eniw)
> +#define region_ig(region) (ilog2((region)->config.ig))
> +
> +#define for_each_cxl_endpoint(ep, region, idx) \
> + for (idx = 0, ep = (region)->config.targets[idx]; \
> + idx < region_ways(region); \
> + idx++, ep = (region)->config.targets[idx])
> +
> +#define for_each_cxl_decoder_target(target, decoder, idx) \
> + for (idx = 0, target = (decoder)->target[idx]; \
> + idx < (decoder)->nr_targets; \
> + idx++, target++)
>
> static struct cxl_decoder *rootd_from_region(struct cxl_region *r)
> {
> @@ -175,6 +186,30 @@ static bool qtg_match(const struct cxl_decoder *rootd,
> return true;
> }
>
> +static int get_unique_hostbridges(const struct cxl_region *region,
> + struct cxl_port **hbs)
> +{
> + struct cxl_memdev *ep;
> + int i, hb_count = 0;
> +
> + for_each_cxl_endpoint(ep, region, i) {
> + struct cxl_port *hb = get_hostbridge(ep);
> + bool found = false;
> + int j;
> +
> + BUG_ON(!hb);
> +
> + for (j = 0; j < hb_count; j++) {
> + if (hbs[j] == hb)
> + found = true;
> + }
> + if (!found)
> + hbs[hb_count++] = hb;
> + }
> +
> + return hb_count;
> +}
> +
> /**
> * region_xhb_config_valid() - determine cross host bridge validity
> * @rootd: The root decoder to check against
> @@ -188,7 +223,59 @@ static bool qtg_match(const struct cxl_decoder *rootd,
> static bool region_xhb_config_valid(const struct cxl_region *region,
> const struct cxl_decoder *rootd)
> {
> - /* TODO: */
> + struct cxl_port *hbs[CXL_DECODER_MAX_INTERLEAVE];
> + int rootd_ig, i;
> + struct cxl_dport *target;
> +
> + /* Are all devices in this region on the same CXL host bridge */
> + if (get_unique_hostbridges(region, hbs) == 1)
> + return true;
> +
> + rootd_ig = rootd->interleave_granularity;
> +
> + /* CFMWS.HBIG >= Device.Label.IG */
> + if (rootd_ig < region_ig(region)) {
> + trace_xhb_valid(region,
> + "granularity does not support the region interleave granularity\n");
> + return false;
> + }
> +
> + /* ((2^(CFMWS.HBIG - Device.RLabel.IG) * (2^CFMWS.ENIW)) > Device.RLabel.NLabel) */
> + if (1 << (rootd_ig - region_ig(region)) * (1 << rootd->interleave_ways) >
This maths isn't what the comment says it is.
((1 << (rootd_ig - region_ig(region))) * rootd->interleaveways)
so brackets needed to avoid 2^( all the rest) and rootd->interleave_ways seems to the
actual number of ways not the log2 of it.
That feeds through below.
> + region_ways(region)) {
> + trace_xhb_valid(region,
> + "granularity to device granularity ratio requires a larger number of devices than currently configured");
> + return false;
> + }
> +
> + /* Check that endpoints are hooked up in the correct order */
> + for_each_cxl_decoder_target(target, rootd, i) {
> + struct cxl_memdev *endpoint = region->config.targets[i];
> +
> + if (get_hostbridge(endpoint) != target->port) {
I think this should be
get_hostbridge(endpoint)->uport != target->dport
As it stands you are comparing the host bridge with the root object.
> + trace_xhb_valid(region, "device ordering bad\n");
> + return false;
> + }
> + }
> +
> + /*
> + * CFMWS.InterleaveTargetList[n] must contain all devices, x where:
> + * (Device[x],RegionLabel.Position >> (CFMWS.HBIG -
> + * Device[x].RegionLabel.InterleaveGranularity)) &
> + * ((2^CFMWS.ENIW) - 1) = n
> + *
> + * Linux notes: All devices are known to have the same interleave
> + * granularity at this point.
> + */
> + for_each_cxl_decoder_target(target, rootd, i) {
> + if (((i >> (rootd_ig - region_ig(region)))) &
> + (((1 << rootd->interleave_ways) - 1) != target->port_id)) {
> + trace_xhb_valid(region,
> + "One or more devices are not connected to the correct hostbridge.");
> + return false;
> + }
> + }
> +
> return true;
> }
>
> diff --git a/drivers/cxl/trace.h b/drivers/cxl/trace.h
> index a53f00ba5d0e..4de47d1111ac 100644
> --- a/drivers/cxl/trace.h
> +++ b/drivers/cxl/trace.h
> @@ -38,6 +38,9 @@ DEFINE_EVENT(cxl_region_template, sanitize_failed,
> DEFINE_EVENT(cxl_region_template, allocation_failed,
> TP_PROTO(const struct cxl_region *region, char *status),
> TP_ARGS(region, status));
> +DEFINE_EVENT(cxl_region_template, xhb_valid,
> + TP_PROTO(const struct cxl_region *region, char *status),
> + TP_ARGS(region, status));
>
> #endif /* if !defined (__CXL_TRACE_H__) || defined(TRACE_HEADER_MULTI_READ) */
>
