Re: [RFC 3/5] cxl: Separate coherence from target type

[Huang, Ying]

Gregory Price <gourry@xxxxxxxxxx> writes:

> On Wed, Sep 25, 2024 at 10:46:45AM +0800, Huang Ying wrote:
>> Previously, target type (expander or accelerator) and
>> coherence (HOSTONLY (HDM-H) or DEV (HDM-D/DB)) are synonym.  So target
>> type is used to designate coherence too.  However, it's possible for
>> expanders to use HDM-DB now.  So, we need to separate coherence from
>> target type.
>> 
>> Accordingly, the HOSTONLY field of decoder ctrl
>> register (CXL_HDM_DECODER0_CTRL_HOSTONLY) need to be set according to
>> coherence.
>> 
>> The coherence of decoders can not be determined via target type too.
>> So, accelerator/expander device drivers need to specify coherence
>> explicitly via newly added coherence field in struct cxl_dev_state.
>> 
>> The coherence of each end points in a region need to be same.  So, the
>> coherence of the first end point is recorded in struct region.  Which
>> will be checked against the coherence of all other end points.
>> 
>> Signed-off-by: "Huang, Ying" <ying.huang@xxxxxxxxx>
>> Cc: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
>> Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
>> Cc: Davidlohr Bueso <dave@xxxxxxxxxxxx>
>> Cc: Jonathan Cameron <jonathan.cameron@xxxxxxxxxx>
>> Cc: Dave Jiang <dave.jiang@xxxxxxxxx>
>> Cc: Alison Schofield <alison.schofield@xxxxxxxxx>
>> Cc: Vishal Verma <vishal.l.verma@xxxxxxxxx>
>> Cc: Ira Weiny <ira.weiny@xxxxxxxxx>
>> Cc: Alejandro Lucero <alucerop@xxxxxxx>
>> ---
>>  drivers/cxl/core/hdm.c    | 22 +++++++++++++++-------
>>  drivers/cxl/core/mbox.c   |  1 +
>>  drivers/cxl/core/port.c   |  1 +
>>  drivers/cxl/core/region.c | 37 ++++++++++++++++++++++++++++++++++---
>>  drivers/cxl/cxl.h         |  9 +++++++++
>>  drivers/cxl/cxlmem.h      | 11 +++++++++++
>>  6 files changed, 71 insertions(+), 10 deletions(-)
>> 
>
> Reviewed-by: Gregory Price <gourry@xxxxxxxxxx>

Thanks!

>> @@ -1925,6 +1933,29 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>>  		return -ENXIO;
>>  	}
>>  
>> +	/* Set the coherence of region to that of the first endpoint */
>> +	if (cxlr->coherence == CXL_DECODER_INVALIDCOH) {
>> +		unsigned long flags = cxlrd->cxlsd.cxld.flags;
>> +		enum cxl_decoder_coherence coherence = cxled->cxld.coherence;
>> +
>> +		cxlr->coherence = coherence;
>> +		if ((coherence == CXL_DECODER_HOSTONLYCOH &&
>> +		     !(flags & CXL_DECODER_F_HOSTONLYCOH)) ||
>> +		    (coherence == CXL_DECODER_DEVCOH &&
>> +		     !(flags & CXL_DECODER_F_DEVCOH))) {
>
> silly nit but my gut tells me we can make this less ugly.
> Not a blocker though.

Yes.  This looks urgly.  Will think about how to improve it.

>> +			dev_dbg(&cxlr->dev,
>> +"%s:%s endpoint coherence: %d isn't supported by root decoder: %#lx\n",
>> +				dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
>> +				coherence, flags);
>> +			return -ENXIO;
>> +		}

--
Best Regards,
Huang, Ying