linux-kernel - Re: [RFC PATCH 01/20] nvdimm/label: Introduce NDD_CXL

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] [day] [month] [year] [list]
Message-ID: <624c255d-d7e2-4ea3-9186-b435499838a7@intel.com>
Date: Wed, 9 Jul 2025 15:57:57 -0700
From: Dave Jiang <dave.jiang@...el.com>
To: Neeraj Kumar <s.neeraj@...sung.com>, dan.j.williams@...el.com,
 dave@...olabs.net, jonathan.cameron@...wei.com, alison.schofield@...el.com,
 vishal.l.verma@...el.com, ira.weiny@...el.com
Cc: a.manzanares@...sung.com, nifan.cxl@...il.com, anisa.su@...sung.com,
 vishak.g@...sung.com, krish.reddy@...sung.com, arun.george@...sung.com,
 alok.rathore@...sung.com, neeraj.kernel@...il.com,
 linux-kernel@...r.kernel.org, linux-cxl@...r.kernel.org,
 nvdimm@...ts.linux.dev, gost.dev@...sung.com, cpgs@...sung.com
Subject: Re: [RFC PATCH 01/20] nvdimm/label: Introduce NDD_CXL_LABEL flag to
 set cxl label format



On 6/17/25 5:39 AM, Neeraj Kumar wrote:
> NDD_CXL_LABEL is introduced to set cxl LSA 2.1 label format
> Accordingly updated label index version

Maybe add the spec reference that defines label 2.1 format
> 
> Signed-off-by: Neeraj Kumar <s.neeraj@...sung.com>
> ---
>  drivers/nvdimm/dimm.c      |  1 +
>  drivers/nvdimm/dimm_devs.c | 10 ++++++++++
>  drivers/nvdimm/label.c     | 16 ++++++++++++----
>  drivers/nvdimm/nd.h        |  1 +
>  include/linux/libnvdimm.h  |  3 +++
>  5 files changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/nvdimm/dimm.c b/drivers/nvdimm/dimm.c
> index 91d9163ee303..8753b5cd91cc 100644
> --- a/drivers/nvdimm/dimm.c
> +++ b/drivers/nvdimm/dimm.c
> @@ -62,6 +62,7 @@ static int nvdimm_probe(struct device *dev)
>  	if (rc < 0)
>  		dev_dbg(dev, "failed to unlock dimm: %d\n", rc);
>  
> +	ndd->cxl = nvdimm_check_cxl_label_format(ndd->dev);
>  
>  	/*
>  	 * EACCES failures reading the namespace label-area-properties
> diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c
> index 21498d461fde..e8f545f889fd 100644
> --- a/drivers/nvdimm/dimm_devs.c
> +++ b/drivers/nvdimm/dimm_devs.c
> @@ -18,6 +18,16 @@
>  
>  static DEFINE_IDA(dimm_ida);
>  
> +bool nvdimm_check_cxl_label_format(struct device *dev)
> +{
> +	struct nvdimm *nvdimm = to_nvdimm(dev);
> +
> +	if (test_bit(NDD_CXL_LABEL, &nvdimm->flags))
> +		return true;
> +
> +	return false;
> +}

I think we may want to move the checking of the flag to where the patch also set the flag in order to provide a more coherent review experience. Given that I haven't read the rest of the patchset and don't know how NDD_CXL_LABEL is set, I really can't comment on whether there's a better way to detect LSA 2.1 labels. Is there a generic way to determine label versions without the implication of this is CXL device?

> +
>  /*
>   * Retrieve bus and dimm handle and return if this bus supports
>   * get_config_data commands
> diff --git a/drivers/nvdimm/label.c b/drivers/nvdimm/label.c
> index 082253a3a956..48b5ba90216d 100644
> --- a/drivers/nvdimm/label.c
> +++ b/drivers/nvdimm/label.c
> @@ -687,11 +687,19 @@ static int nd_label_write_index(struct nvdimm_drvdata *ndd, int index, u32 seq,
>  		- (unsigned long) to_namespace_index(ndd, 0);
>  	nsindex->labeloff = __cpu_to_le64(offset);
>  	nsindex->nslot = __cpu_to_le32(nslot);
> -	nsindex->major = __cpu_to_le16(1);
> -	if (sizeof_namespace_label(ndd) < 256)
> +
> +	/* Support CXL LSA 2.1 label format */
> +	if (ndd->cxl) {
> +		nsindex->major = __cpu_to_le16(2);
>  		nsindex->minor = __cpu_to_le16(1);
> -	else
> -		nsindex->minor = __cpu_to_le16(2);
> +	} else {
> +		nsindex->major = __cpu_to_le16(1);
> +		if (sizeof_namespace_label(ndd) < 256)
> +			nsindex->minor = __cpu_to_le16(1);
> +		else
> +			nsindex->minor = __cpu_to_le16(2);
> +	}

Would like to see a more coherent way of detecting label versioning. What happens when there are newer versions introduced later on? This currently feels very disjointed.

> +
>  	nsindex->checksum = __cpu_to_le64(0);
>  	if (flags & ND_NSINDEX_INIT) {
>  		unsigned long *free = (unsigned long *) nsindex->free;
> diff --git a/drivers/nvdimm/nd.h b/drivers/nvdimm/nd.h
> index 5ca06e9a2d29..304f0e9904f1 100644
> --- a/drivers/nvdimm/nd.h
> +++ b/drivers/nvdimm/nd.h
> @@ -522,6 +522,7 @@ void nvdimm_set_labeling(struct device *dev);
>  void nvdimm_set_locked(struct device *dev);
>  void nvdimm_clear_locked(struct device *dev);
>  int nvdimm_security_setup_events(struct device *dev);
> +bool nvdimm_check_cxl_label_format(struct device *dev);
>  #if IS_ENABLED(CONFIG_NVDIMM_KEYS)
>  int nvdimm_security_unlock(struct device *dev);
>  #else
> diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
> index e772aae71843..0a55900842c8 100644
> --- a/include/linux/libnvdimm.h
> +++ b/include/linux/libnvdimm.h
> @@ -44,6 +44,9 @@ enum {
>  	/* dimm provider wants synchronous registration by __nvdimm_create() */
>  	NDD_REGISTER_SYNC = 8,
>  
> +	/* dimm supports region labels (LSA Format 2.1) */
> +	NDD_CXL_LABEL = 9,

While 2.1 is defined by the CXL spec, is there anything declared by the CXL spec that makes 2.1 exclusive to CXL? Maybe the focus should be to support LSA 2.1 and avoid dragging naming CXL into the conversation at this point. It can be made more generic right? I'm concerned about dragging CXL into nvdimm when it isn't necessary. Maybe introduce a label cap field where when an nvdimm device is registered, the caller can pass in that it's capable of supporting up to a certain version of labeling? Just throwing ideas out to see if it's feasible.

> +
>  	/* need to set a limit somewhere, but yes, this is likely overkill */
>  	ND_IOCTL_MAX_BUFLEN = SZ_4M,
>  	ND_CMD_MAX_ELEM = 5,