Re: [PATCH RFC 11/15] cxl/pmem: Add "Unlock" security command support

[Dave Jiang <dave.jiang@intel.com>]

On 8/4/2022 6:19 AM, Jonathan Cameron wrote:
> On Fri, 15 Jul 2022 14:09:36 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
>
>> Create callback function to support the nvdimm_security_ops() ->unlock()
>> callback. Translate the operation to send "Unlock" security command for CXL
>> mem device.
>>
>> When the mem device is unlocked, arch_invalidate_nvdimm_cache() is called
>> in order to invalidate all CPU caches before attempting to access the mem
>> device.
>>
>> See CXL 2.0 spec section 8.2.9.5.6.4 for reference.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> Hi Dave,
>
> One trivial thing inline.
>
> Thanks,
>
> Jonathan
>
>> ---
>>   drivers/cxl/cxlmem.h   |    1 +
>>   drivers/cxl/security.c |   21 +++++++++++++++++++++
>>   2 files changed, 22 insertions(+)
>>
>> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
>> index ced85be291f3..ae8ccd484491 100644
>> --- a/drivers/cxl/cxlmem.h
>> +++ b/drivers/cxl/cxlmem.h
>> @@ -253,6 +253,7 @@ enum cxl_opcode {
>>   	CXL_MBOX_OP_GET_SECURITY_STATE	= 0x4500,
>>   	CXL_MBOX_OP_SET_PASSPHRASE	= 0x4501,
>>   	CXL_MBOX_OP_DISABLE_PASSPHRASE	= 0x4502,
>> +	CXL_MBOX_OP_UNLOCK		= 0x4503,
>>   	CXL_MBOX_OP_FREEZE_SECURITY	= 0x4504,
>>   	CXL_MBOX_OP_MAX			= 0x10000
>>   };
>> diff --git a/drivers/cxl/security.c b/drivers/cxl/security.c
>> index 6399266a5908..d15520f280f0 100644
>> --- a/drivers/cxl/security.c
>> +++ b/drivers/cxl/security.c
>> @@ -114,11 +114,32 @@ static int cxl_pmem_security_freeze(struct nvdimm *nvdimm)
>>   	return cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_FREEZE_SECURITY, NULL, 0, NULL, 0);
>>   }
>>   
>> +static int cxl_pmem_security_unlock(struct nvdimm *nvdimm,
>> +				    const struct nvdimm_key_data *key_data)
>> +{
>> +	struct cxl_nvdimm *cxl_nvd = nvdimm_provider_data(nvdimm);
>> +	struct cxl_memdev *cxlmd = cxl_nvd->cxlmd;
>> +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>> +	u8 pass[NVDIMM_PASSPHRASE_LEN];
>> +	int rc;
>> +
>> +	memcpy(pass, key_data->data, NVDIMM_PASSPHRASE_LEN);
> Why do we need a local copy?  I'd have thought we could just
> pass keydata->data in as the payload for cxl_mbox_send_cmd()
> There might be some value in making it easier to check by
> having a structure defined for this payload (obviously trivial)
> but given we are using an array of length defined by a non CXL
> define, I'm not sure there is any point in the copy.

We end up hitting a compile warning if we just directly pass in because key_data->data has const qualifier.

tools/testing/cxl/../../../drivers/cxl/security.c: In function ‘cxl_pmem_security_unlock’:
tools/testing/cxl/../../../drivers/cxl/security.c:116:40: warning: passing argument 3 of ‘cxl_mbox_send_cmd’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
   116 |                                key_data->data, NVDIMM_PASSPHRASE_LEN, NULL, 0);
       |                                ~~~~~~~~^~~~~~
In file included from tools/testing/cxl/../../../drivers/cxl/security.c:8:
tools/testing/cxl/../../../drivers/cxl/cxlmem.h:408:70: note: expected ‘void *’ but argument is of type ‘const u8 *’ {aka ‘const unsigned char *’}
   408 | int cxl_mbox_send_cmd(struct cxl_dev_state *cxlds, u16 opcode, void *in,
       |                                                                ~~~~~~^~


>
>> +	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_UNLOCK,
>> +			       pass, NVDIMM_PASSPHRASE_LEN, NULL, 0);
>> +	if (rc < 0)
>> +		return rc;
>> +
>> +	/* DIMM unlocked, invalidate all CPU caches before we read it */
>> +	arch_invalidate_nvdimm_cache();
>> +	return 0;
>> +}
>> +
>>   static const struct nvdimm_security_ops __cxl_security_ops = {
>>   	.get_flags = cxl_pmem_get_security_flags,
>>   	.change_key = cxl_pmem_security_change_key,
>>   	.disable = cxl_pmem_security_disable,
>>   	.freeze = cxl_pmem_security_freeze,
>> +	.unlock = cxl_pmem_security_unlock,
>>   };
>>   
>>   const struct nvdimm_security_ops *cxl_security_ops = &__cxl_security_ops;
>>
>>