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: <e0f08438-69cf-4e79-abf2-f5f3240d9517@amd.com>
Date: Wed, 2 Oct 2024 16:15:35 -0500
From: Ben Cheatham <benjamin.cheatham@....com>
To: Huang Ying <ying.huang@...el.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

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)
 */

> +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;
>  };
>  
>  /**