linux-kernel - Re: [PATCH 5/8] cxl/region: Add helper function to determine memory is online

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: <Z8tspBrjZ7MoSVwh@aschofie-mobl2.lan>
Date: Fri, 7 Mar 2025 14:01:08 -0800
From: Alison Schofield <alison.schofield@...el.com>
To: shiju.jose@...wei.com
Cc: linux-cxl@...r.kernel.org, dan.j.williams@...el.com, dave@...olabs.net,
	jonathan.cameron@...wei.com, dave.jiang@...el.com,
	vishal.l.verma@...el.com, ira.weiny@...el.com, david@...hat.com,
	Vilas.Sridharan@....com, linux-edac@...r.kernel.org,
	linux-acpi@...r.kernel.org, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org, bp@...en8.de, tony.luck@...el.com,
	rafael@...nel.org, lenb@...nel.org, mchehab@...nel.org,
	leo.duran@....com, Yazen.Ghannam@....com, rientjes@...gle.com,
	jiaqiyan@...gle.com, Jon.Grimm@....com, dave.hansen@...ux.intel.com,
	naoya.horiguchi@....com, james.morse@....com, jthoughton@...gle.com,
	somasundaram.a@....com, erdemaktas@...gle.com, pgonda@...gle.com,
	duenwen@...gle.com, gthelen@...gle.com,
	wschwartz@...erecomputing.com, dferguson@...erecomputing.com,
	wbs@...amperecomputing.com, nifan.cxl@...il.com,
	tanxiaofei@...wei.com, prime.zeng@...ilicon.com,
	roberto.sassu@...wei.com, kangkang.shen@...urewei.com,
	wanghuiqiang@...wei.com, linuxarm@...wei.com
Subject: Re: [PATCH 5/8] cxl/region: Add helper function to determine memory
 is online

On Thu, Feb 27, 2025 at 10:38:12PM +0000, shiju.jose@...wei.com wrote:
> From: Shiju Jose <shiju.jose@...wei.com>
> 
> Add helper function to determine a CXL memory is online.
> 
> Use case: certain memory operations are permitted when the memory
> is offline only, for eg. some memory repair operations.
> 

Hi Shiju,

This helper is basically wrapping the existing helper
cxl_num_decoders_committed(port) so that you can start from a cxlmd.
Other callers of cxl_num_decoders_committed() do similar work, ie 
find the port, confirm an endpoint, then call it. I don't think
it's done enough to warrant a helper for all callers. (but others
may disagree and ask for that)

This patchset does this work a few times, so a helper makes sense
here. The part I don't get is why it is placed in the region driver,
requiring it to be stubbed out when region driver is not present.

cxl_num_decoders_committed() is part of the cxl/core and is 
defined in port.c.

Long way of asking, would it work to just add a helper to 
core/memfeatures.c and use it within?

The name could use an update. cxl_are_decoders_committed(cxlmd) is
a stretch because a cxlmd does not have decoders. That naming made
sense for 'cxl_num_decoders_committed(port) because a port has
decoders, a memdev does not.

-- Alison


> Co-developed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> Signed-off-by: Shiju Jose <shiju.jose@...wei.com>
> ---
>  drivers/cxl/core/core.h   |  9 +++++++++
>  drivers/cxl/core/region.c | 10 ++++++++++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> index 6c83f6f18122..386e36b18c19 100644
> --- a/drivers/cxl/core/core.h
> +++ b/drivers/cxl/core/core.h
> @@ -32,8 +32,17 @@ int cxl_get_poison_by_endpoint(struct cxl_port *port);
>  struct cxl_region *cxl_dpa_to_region(const struct cxl_memdev *cxlmd, u64 dpa);
>  u64 cxl_dpa_to_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
>  		   u64 dpa);
> +bool cxl_are_decoders_committed(const struct cxl_memdev *cxlmd);
>  
>  #else
> +static inline bool cxl_are_decoders_committed(const struct cxl_memdev *cxlmd)
> +{
> +	/*
> +	 * If no driver, in absence of a way to check, assume decoders are committed.
> +	 */
> +	return true;
> +}
> +
>  static inline u64 cxl_dpa_to_hpa(struct cxl_region *cxlr,
>  				 const struct cxl_memdev *cxlmd, u64 dpa)
>  {
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 9e7b716296d7..3fd14cb0c470 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2864,6 +2864,16 @@ struct cxl_region *cxl_dpa_to_region(const struct cxl_memdev *cxlmd, u64 dpa)
>  	return ctx.cxlr;
>  }
>  
> +bool cxl_are_decoders_committed(const struct cxl_memdev *cxlmd)
> +{
> +	struct cxl_port *port = cxlmd->endpoint;
> +
> +	if (port && is_cxl_endpoint(port) && cxl_num_decoders_committed(port))
> +		return true;
> +
> +	return false;
> +}
> +
>  static bool cxl_is_hpa_in_chunk(u64 hpa, struct cxl_region *cxlr, int pos)
>  {
>  	struct cxl_region_params *p = &cxlr->params;
> -- 
> 2.43.0
> 
>