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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <875xqang3i.fsf@yhuang6-desk2.ccr.corp.intel.com>
Date: Thu, 03 Oct 2024 09:13:37 +0800
From: "Huang, Ying" <ying.huang@...el.com>
To: Ben Cheatham <benjamin.cheatham@....com>
Cc: <linux-cxl@...r.kernel.org>,  <linux-kernel@...r.kernel.org>,  "Jonathan
 Cameron" <Jonathan.Cameron@...wei.com>,  Davidlohr Bueso
 <dave@...olabs.net>,  Alison Schofield <alison.schofield@...el.com>,
  Vishal Verma <vishal.l.verma@...el.com>,  Ira Weiny
 <ira.weiny@...el.com>,  Alejandro Lucero <alucerop@....com>,  Dan Williams
 <dan.j.williams@...el.com>,  Dave Jiang <dave.jiang@...el.com>
Subject: Re: [RFC 3/5] cxl: Separate coherence from target type

Ben Cheatham <benjamin.cheatham@....com> writes:

> Hi Huang, quick comments in this patch and the next.
>
> On 9/24/24 9:46 PM, 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@...el.com>
>> Cc: Jonathan Cameron <Jonathan.Cameron@...wei.com>
>> Cc: Dan Williams <dan.j.williams@...el.com>
>> Cc: Davidlohr Bueso <dave@...olabs.net>
>> Cc: Jonathan Cameron <jonathan.cameron@...wei.com>
>> Cc: Dave Jiang <dave.jiang@...el.com>
>> Cc: Alison Schofield <alison.schofield@...el.com>
>> Cc: Vishal Verma <vishal.l.verma@...el.com>
>> Cc: Ira Weiny <ira.weiny@...el.com>
>> Cc: Alejandro Lucero <alucerop@....com>
>> ---
>>  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(-)
>> 
>
> [snip]
>
>>  
>> +/*
>> + * enum cxl_devcoherence - the coherence of the cxl device
>> + * @CXL_DEVCOH_DEV      - HDM-D or HDM-DB
>> + * @CXL_DEVCOH_HOSTONLY - HDM-H
>> + */
>
> Could I suggest mapping the coherence type to the expected device type(s) in this
> comment? My thinking here is that the coherence types aren't exactly straightforward
> and having the device types they correspond to would help ease any confusion, especially
> since it looks like we are expecting type 2 driver writers to fill this in manually. I'm
> thinking something along the lines of:
> /*
>  * enum cxl_devcoherence - the coherence of the cxl device
>  * @CXL_DEVCOH_DEV      - HDM-D (type 2) or HDM-DB (type 2/3)
>  * @CXL_DEVCOH_HOSTONLY - HDM-H (type 3)
>  */

This looks good to me!  Thanks!  Will do this in the next version.

>> +enum cxl_devcoherence {
>> +	CXL_DEVCOH_DEV,
>> +	CXL_DEVCOH_HOSTONLY,
>> +};
>> +
>>  /**
>>   * struct cxl_dpa_perf - DPA performance property entry
>>   * @dpa_range: range for DPA address
>> @@ -438,6 +448,7 @@ struct cxl_dev_state {
>>  	struct resource ram_res;
>>  	u64 serial;
>>  	enum cxl_devtype type;
>> +	enum cxl_devcoherence coherence;
>>  };
>>  
>>  /**

--
Best Regards,
Huang, Ying