Re: [PATCH v4 2/2] cxl/region: check interleave capability

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

On Mon, 22 Apr 2024 05:13:50 -0400
Yao Xingtao <yaoxt.fnst@fujitsu.com> wrote:

> Since interleave capability is not verified, a target can successfully
> attach to a region even if it lacks support for the specified interleave
> ways or granularity.
> 
> When attempting to access memory, unexpected behavior occurs due to the
> incorrect conversion of HPA to a faulty DPA, leading to a segmentation
> fault, as observed when executing the command:
> $ numactl -m 2 ls

Don't mention a segfault as in theory it should have been impossible to
set this decoder up (commit decoder should have failed at the device end).
So just say that it fails and leave the details unmentioned.

> 
> According to the CXL specification (section 8.2.4.20 CXL HDM Decoder
> Capability Structure), bits 11 and 12 within the 'CXL HDM Decoder
> Capability Register' indicate the capability to establish interleaving
> in 3, 6, 12, and 16 ways. If these bits are not set, the target cannot
> be attached to a region utilizing such interleave ways.
> 
> Additionally, bits 8 and 9 in the same register represent the capability
> of the bits used for interleaving in the address, commonly referred to as
> the interleave mask.
> 
> Regarding 'Decoder Protection':
> If IW is less than 8 (for interleave ways of 1, 2, 4, 8, 16), the
> interleave bits start at bit position IG + 8 and end at IG + IW + 8 - 1.
> 
> If the IW is greater than or equal to 8 (for interleave ways of 3, 6, 12),
> the interleave bits start at bit position IG + 8 and end at IG + IW - 1.
> 
> If the interleave mask is insufficient to cover the required interleave
> bits, the target cannot be attached to the region.
> 
> The above check does not apply to the host-bridges with single port and
> switches with single dport, because there does not have a instance of
> CXL HDM Decoder Capability Structure for them.

There needs to be clear separation between such cases that do not
have HDM decoders and the permissible case where there is one but
it can effectively be set to anything because no interleaving is
going on.

> 
> Fixes: 384e624bb211 ("cxl/region: Attach endpoint decoders")
> Signed-off-by: Yao Xingtao <yaoxt.fnst@fujitsu.com>

>  
>  static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 5c186e0a39b9..5c09652136c9 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1210,6 +1210,60 @@ static int check_last_peer(struct cxl_endpoint_decoder *cxled,
>  	return 0;
>  }
>  
> +static int check_interleave_cap(struct cxl_decoder *cxld, int iw, int ig)
> +{
> +	struct cxl_port *port = to_cxl_port(cxld->dev.parent);
> +	struct cxl_hdm *cxlhdm = dev_get_drvdata(&port->dev);
> +	struct cxl_switch_decoder *cxlsd;
> +	unsigned int interleave_mask;
> +	u8 eiw;
> +	u16 eig;
> +	int rc, high_pos, low_pos;
> +
> +	if (is_switch_decoder(&cxld->dev)) {
> +		cxlsd = to_cxl_switch_decoder(&cxld->dev);
> +		if (cxlsd->passthrough)
> +			return 0;
> +	}
> +
> +	rc = ways_to_eiw(iw, &eiw);
> +	if (rc)
> +		return rc;
> +
> +	if (!(cxlhdm->iw_cap_mask & BIT(iw)))
> +		return -EOPNOTSUPP;
> +
> +	rc = granularity_to_eig(ig, &eig);
> +	if (rc)
> +		return rc;
> +
> +	/*
> +	 * Per CXL specification (8.2.3.20.13 Decoder Protection in r3.1)
> +	 * if IW < 8, the interleave bits start at bit position IG + 8, and
> +	 * end at IG + IW + 8 - 1.
> +	 * if IW >= 8, the interleave bits start at bit position IG + 8, and
> +	 * end at IG + IW - 1.
> +	 */
> +	if (eiw >= 8)
> +		high_pos = eiw + eig - 1;
> +	else
> +		high_pos = eiw + eig + 7;
> +	low_pos = eig + 8;
> +	/*
> +	 * when the IW is 0 or 8 (interlave way is 1 or 3), the low_pos is
> +	 * larger than high_pos, since the target is in the target list of a
> +	 * passthrough decoder, the following check is ignored.

There may be a decoder even on 1 to 1 routing cases so is this correct?

> +	 */
> +	if (low_pos > high_pos)
> +		return 0;