Re: [PATCH V2 03/20] nvdimm/namespace_label: Add namespace label changes as per CXL LSA v2.1

[Neeraj Kumar <s.neeraj@samsung.com>]

[-- Attachment #1: Type: text/plain, Size: 5221 bytes --]

On 19/08/25 10:57AM, Ira Weiny wrote:
>Neeraj Kumar wrote:
>> CXL 3.2 Spec mentions CXL LSA 2.1 Namespace Labels at section 9.13.2.5
>> Modified __pmem_label_update function using setter functions to update
>> namespace label as per CXL LSA 2.1
>
>But why?  And didn't we just remove nd_namespace_label in patch 2?
>
>Why are we now defining accessor functions for it?
>

Hi Ira,

No we haven't removed nd_namespace_label in patch 2. In patch 2, we have
introduced
lsa_label which contains nd_namespace_label as well as cxl_region_label
under union.

Actually, LSA 2.1 spec introduced new region label along with existing
(v1.1 & v1.2) namespace label
But in v2.1 namespace label members are also modified compared to v1.1 &
v1.2.

New members introduced in namespace label are following

	struct nvdimm_cxl_label {
		u8 type[NSLABEL_UUID_LEN];	--> Filling it with nsl_set_type()
		u8 uuid[NSLABEL_UUID_LEN];
		u8 name[NSLABEL_NAME_LEN];
		__le32 flags;
		__le16 nrange;
		__le16 position;
		__le64 dpa;
		__le64 rawsize;
		__le32 slot;
		__le32 align;			--> Filling it with
		nsl_set_alignment()
		u8 region_uuid[16];		--> Filling it with
		nsl_set_region_uuid()
		u8 abstraction_uuid[16];
		__le16 lbasize;
		u8 reserved[0x56];
		__le64 checksum;
	};

Therefore this patch-set address this modification in namespace label as per v2.1

>>
>> Signed-off-by: Neeraj Kumar <s.neeraj@samsung.com>
>> ---
>>  drivers/nvdimm/label.c |  3 +++
>>  drivers/nvdimm/nd.h    | 27 +++++++++++++++++++++++++++
>>  2 files changed, 30 insertions(+)
>>
>> diff --git a/drivers/nvdimm/label.c b/drivers/nvdimm/label.c
>> index 75bc11da4c11..3f8a6bdb77c7 100644
>> --- a/drivers/nvdimm/label.c
>> +++ b/drivers/nvdimm/label.c
>> @@ -933,6 +933,7 @@ static int __pmem_label_update(struct nd_region *nd_region,
>>  	memset(lsa_label, 0, sizeof_namespace_label(ndd));
>>
>>  	nd_label = &lsa_label->ns_label;
>> +	nsl_set_type(ndd, nd_label);
>>  	nsl_set_uuid(ndd, nd_label, nspm->uuid);
>>  	nsl_set_name(ndd, nd_label, nspm->alt_name);
>>  	nsl_set_flags(ndd, nd_label, flags);
>> @@ -944,7 +945,9 @@ static int __pmem_label_update(struct nd_region *nd_region,
>>  	nsl_set_lbasize(ndd, nd_label, nspm->lbasize);
>>  	nsl_set_dpa(ndd, nd_label, res->start);
>>  	nsl_set_slot(ndd, nd_label, slot);
>> +	nsl_set_alignment(ndd, nd_label, 0);
>>  	nsl_set_type_guid(ndd, nd_label, &nd_set->type_guid);
>> +	nsl_set_region_uuid(ndd, nd_label, NULL);
>>  	nsl_set_claim_class(ndd, nd_label, ndns->claim_class);
>>  	nsl_calculate_checksum(ndd, nd_label);
>>  	nd_dbg_dpa(nd_region, ndd, res, "\n");
>> diff --git a/drivers/nvdimm/nd.h b/drivers/nvdimm/nd.h
>> index 61348dee687d..651847f1bbf9 100644
>> --- a/drivers/nvdimm/nd.h
>> +++ b/drivers/nvdimm/nd.h
>> @@ -295,6 +295,33 @@ static inline const u8 *nsl_uuid_raw(struct nvdimm_drvdata *ndd,
>>  	return nd_label->efi.uuid;
>>  }
>>
>> +static inline void nsl_set_type(struct nvdimm_drvdata *ndd,
>> +				struct nd_namespace_label *ns_label)
>
>Set type to what?
>

LSA 2.1 spec mentions seperate UUID for namespace label and region
label.

#define CXL_REGION_UUID "529d7c61-da07-47c4-a93f-ecdf2c06f444"
#define CXL_NAMESPACE_UUID "68bb2c0a-5a77-4937-9f85-3caf41a0f93c"
Here we are setting label->type with CXL_NAMESPACE_UUID

In following patch, accordingly we are setting region label->type with
CXL_REGION_UUID

May be I will rename it to nsl_set_type_uuid() in next patch-set.

>Why is driver data passed here?

ndd->cxl is used to segregate between EFI labels (v1.1 & v1.2) and CXL
Labels (v2.1). It was introduced in 5af96835e4daf

>
>This reads as an accessor function for some sort of label class but seems
>to do some back checking into ndd to set the uuid of the label?
>
>At a minimum this should be *_set_uuid(..., uuid_t uuid)  But I'm not
>following this chunk of changes so don't just change it without more
>explaination.

I have created setter function taking inspiration from other members
setter helpers introduced in 8176f14789125

>
>> +{
>> +	uuid_t tmp;
>> +
>> +	if (ndd->cxl) {
>> +		uuid_parse(CXL_NAMESPACE_UUID, &tmp);
>> +		export_uuid(ns_label->cxl.type, &tmp);
>> +	}
>> +}
>> +
>> +static inline void nsl_set_alignment(struct nvdimm_drvdata *ndd,
>> +				     struct nd_namespace_label *ns_label,
>> +				     u32 align)
>
>Why is this needed?

As per CXL spec 3.2 Table - 9-11. Namespace Label Layout
The desired region alignment in multiples of 256 MB:
• 0 = No desired alignment
• 1 = 256-MB desired alignment
• 2 = 512-MB desired alignment
• etc.

In this patch-set we are using 0.

>
>> +{
>> +	if (ndd->cxl)
>> +		ns_label->cxl.align = __cpu_to_le16(align);
>> +}
>> +
>> +static inline void nsl_set_region_uuid(struct nvdimm_drvdata *ndd,
>> +				       struct nd_namespace_label *ns_label,
>> +				       const uuid_t *uuid)
>
>Again why?

This field is used to track namespace label associated with perticular
region label. It stores the region label's UUID

>
>> +{
>> +	if (ndd->cxl)
>> +		export_uuid(ns_label->cxl.region_uuid, uuid);
>
>export does a memcpy() and you are passing it NULL.  Is that safe?
>
>Ira

Thanks Ira for pointing this, Yes it will not be safe with NULL.
Sure I will fix this in next patch-set.


Regards,
Neeraj

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]