Re: [PATCH 1/3] cxl: Add check for result of interleave ways plus granularity combo

[Dave Jiang <dave.jiang@intel.com>]

On 8/9/2022 9:18 AM, Dan Williams wrote:
> Dave Jiang wrote:
>> Add a helper function to check the combination of interleave ways and
>> interleave granularity together is sane against the interleave mask from
>> the HDM decoder. Add the check to cxl_region_attach() to make sure the
>> region config is sane. Add the check to cxl_port_setup_targets() to make
>> sure the port setup config is also sane.
>>
>> Calculation refers to CXL spec v3 8.2.4.19.13 implementation note #3.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>   drivers/cxl/core/region.c |   17 ++++++++++++++++-
>>   drivers/cxl/cxl.h         |   11 +++++++++++
>>   drivers/cxl/cxlmem.h      |   31 +++++++++++++++++++++++++++++++
>>   3 files changed, 58 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>> index cf5d5811fe4c..a209a8de31fd 100644
>> --- a/drivers/cxl/core/region.c
>> +++ b/drivers/cxl/core/region.c
>> @@ -1081,6 +1081,13 @@ static int cxl_port_setup_targets(struct cxl_port *port,
>>   		return rc;
>>   	}
>>   
>> +	rc = cxl_interleave_verify(port, iw, ig);
> There are multiple "interleave verify" actions, this function is just
> handling the interleave address bit capability, so how about:
>
> s/cxl_interleave_verify/cxl_interleave_capable/
ok
>
>> +	if (rc) {
>> +		dev_dbg(&cxlr->dev, "%s:%s: invalid interleave & granularity combo: %d\n",
> If this fires I would want to know the iw and ig settings, something
> like:
>
>      "%s:%s interleave (ig: %d iw: %d mask: %#x) exceeds capability (mask: %#x)\n"
>
> Likely that message would need to move internal to
> cxl_interleave_capable() where you have the address masks available.
Will move it internally
>
>
>> +			dev_name(port->uport), dev_name(&port->dev), rc);
>> +		return rc;
>> +	}
>> +
>>   	cxld->interleave_ways = iw;
>>   	cxld->interleave_granularity = ig;
>>   	dev_dbg(&cxlr->dev, "%s:%s iw: %d ig: %d\n", dev_name(port->uport),
>> @@ -1218,6 +1225,15 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>>   		return -EBUSY;
>>   	}
>>   
>> +	ep_port = cxled_to_port(cxled);
>> +	rc = cxl_interleave_verify(ep_port, p->interleave_ways,
>> +				   p->interleave_granularity);
>> +	if (rc) {
>> +		dev_dbg(&cxlr->dev, "%s: invalid interleave & granularity combo: %d\n",
>> +			dev_name(&cxlmd->dev), rc);
> ...and then you don't need to duplicate the message if it is internal to
> cxl_interleave_capable().
>
>> +		return rc;
>> +	}
>> +
>>   	for (i = 0; i < p->interleave_ways; i++) {
>>   		struct cxl_endpoint_decoder *cxled_target;
>>   		struct cxl_memdev *cxlmd_target;
>> @@ -1236,7 +1252,6 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>>   		}
>>   	}
>>   
>> -	ep_port = cxled_to_port(cxled);
>>   	root_port = cxlrd_to_port(cxlrd);
>>   	dport = cxl_find_dport_by_dev(root_port, ep_port->host_bridge);
>>   	if (!dport) {
>> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
>> index bc604b7e44fb..275979fbd15a 100644
>> --- a/drivers/cxl/cxl.h
>> +++ b/drivers/cxl/cxl.h
>> @@ -61,6 +61,17 @@
>>   #define CXL_HDM_DECODER0_SKIP_LOW(i) CXL_HDM_DECODER0_TL_LOW(i)
>>   #define CXL_HDM_DECODER0_SKIP_HIGH(i) CXL_HDM_DECODER0_TL_HIGH(i)
>>   
>> +enum {
>> +	CXL_INTERLEAVE_1_WAY = 0,
>> +	CXL_INTERLEAVE_2_WAY,
>> +	CXL_INTERLEAVE_4_WAY,
>> +	CXL_INTERLEAVE_8_WAY,
>> +	CXL_INTERLEAVE_16_WAY,
>> +	CXL_INTERLEAVE_3_WAY = 8,
>> +	CXL_INTERLEAVE_6_WAY,
>> +	CXL_INTERLEAVE_12_WAY
> I'm not sure this new enum is worth it given only one of these will ever
> be used.
Will remove
>
>> +};
>> +
>>   static inline int cxl_hdm_decoder_count(u32 cap_hdr)
>>   {
>>   	int val = FIELD_GET(CXL_HDM_DECODER_COUNT_MASK, cap_hdr);
>> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
>> index 88e3a8e54b6a..d5f872ca62f9 100644
>> --- a/drivers/cxl/cxlmem.h
>> +++ b/drivers/cxl/cxlmem.h
>> @@ -401,6 +401,37 @@ struct cxl_hdm {
>>   	struct cxl_port *port;
>>   };
>>   
>> +static inline int cxl_interleave_verify(struct cxl_port *port,
>> +					int ways, int granularity)
>> +{
>> +	struct cxl_hdm *cxlhdm = dev_get_drvdata(&port->dev);
>> +	unsigned int addr_mask;
>> +	u16 ig;
>> +	u8 iw;
>> +	int rc;
>> +
>> +	rc = granularity_to_cxl(granularity, &ig);
>> +	if (rc)
>> +		return rc;
>> +
>> +	rc = ways_to_cxl(ways, &iw);
>> +	if (rc)
>> +		return rc;
>> +
>> +	if (iw == 0)
>> +		return 0;
>> +
>> +	if (iw < CXL_INTERLEAVE_3_WAY)
> ...just do "is_power_of_2(iw)" here instead.

ok

>
>> +		addr_mask = GENMASK(ig + 8 + iw - 1, ig + 8);
>> +	else
>> +		addr_mask = GENMASK((ig + iw) / 3 - 1, ig + 8);