Re: [PATCH v5 1/4] powerpc/papr_scm: Fetch nvdimm health information from PHYP

[Vaibhav Jain <vajain21@vajain21.in.ibm.com.in.ibm.com>]

Thanks for reviewing this patch Mpe,

Michael Ellerman <ellerman@au1.ibm.com> writes:

> Vaibhav Jain <vaibhav@linux.ibm.com> writes:
>
>> Implement support for fetching nvdimm health information via
>> H_SCM_HEALTH hcall as documented in Ref[1]. The hcall returns a pair
>> of 64-bit big-endian integers which are then stored in 'struct
>> papr_scm_priv' and subsequently partially exposed to user-space via
>> newly introduced dimm specific attribute 'papr_flags'. Also a new asm
>> header named 'papr-scm.h' is added that describes the interface
>> between PHYP and guest kernel.
>>
>> Following flags are reported via 'papr_flags' sysfs attribute contents
>> of which are space separated string flags indicating various nvdimm
>> states:
>>
>>  * "not_armed" 	: Indicating that nvdimm contents wont survive a power
>> 		   cycle.
>>  * "save_fail" 	: Indicating that nvdimm contents couldn't be flushed
>> 		   during last shutdown event.
>>  * "restore_fail": Indicating that nvdimm contents couldn't be restored
>> 		   during dimm initialization.
>>  * "encrypted" 	: Dimm contents are encrypted.
>>  * "smart_notify": There is health event for the nvdimm.
>>  * "scrubbed" 	: Indicating that contents of the nvdimm have been
>> 		   scrubbed.
>>  * "locked"	: Indicating that nvdimm contents cant be modified
>> 		   until next power cycle.
>>
>> [1]: commit 58b278f568f0 ("powerpc: Provide initial documentation for
>> PAPR hcalls")
>>
>> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
>> ---
>> Changelog:
>>
>> v4..v5 : None
>>
>> v3..v4 : None
>>
>> v2..v3 : Removed PAPR_SCM_DIMM_HEALTH_NON_CRITICAL as a condition for
>>        	 NVDIMM unarmed [Aneesh]
>>
>> v1..v2 : New patch in the series.
>> ---
>>  arch/powerpc/include/asm/papr_scm.h       |  48 ++++++++++
>>  arch/powerpc/platforms/pseries/papr_scm.c | 105 +++++++++++++++++++++-
>>  2 files changed, 151 insertions(+), 2 deletions(-)
>>  create mode 100644 arch/powerpc/include/asm/papr_scm.h
>>
>> diff --git a/arch/powerpc/include/asm/papr_scm.h b/arch/powerpc/include/asm/papr_scm.h
>> new file mode 100644
>> index 000000000000..868d3360f56a
>> --- /dev/null
>> +++ b/arch/powerpc/include/asm/papr_scm.h
>> @@ -0,0 +1,48 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Structures and defines needed to manage nvdimms for spapr guests.
>> + */
>> +#ifndef _ASM_POWERPC_PAPR_SCM_H_
>> +#define _ASM_POWERPC_PAPR_SCM_H_
>> +
>> +#include <linux/types.h>
>> +#include <asm/bitsperlong.h>
>> +
>> +/* DIMM health bitmap bitmap indicators */
>> +/* SCM device is unable to persist memory contents */
>> +#define PAPR_SCM_DIMM_UNARMED			PPC_BIT(0)
>
> Please don't use PPC_BIT, it's just unncessary obfuscation for folks
> who are reading the code without access to the docs (ie. more or less
> everyone other than you :)
Sure, will get that replaced with int literals.
>
>> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
>> index 0b4467e378e5..aaf2e4ab1f75 100644
>> --- a/arch/powerpc/platforms/pseries/papr_scm.c
>> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
>> @@ -14,6 +14,7 @@
>>  #include <linux/delay.h>
>>  
>>  #include <asm/plpar_wrappers.h>
>> +#include <asm/papr_scm.h>
>>  
>>  #define BIND_ANY_ADDR (~0ul)
>>  
>> @@ -39,6 +40,13 @@ struct papr_scm_priv {
>>  	struct resource res;
>>  	struct nd_region *region;
>>  	struct nd_interleave_set nd_set;
>> +
>> +	/* Protect dimm data from concurrent access */
>> +	struct mutex dimm_mutex;
>> +
>> +	/* Health information for the dimm */
>> +	__be64 health_bitmap;
>> +	__be64 health_bitmap_valid;
>
> It's much less error prone to store the data in CPU endian and do the
> endian conversion only at the point where the data either comes from or
> goes to firmware.
>
> That would also mean you can define flags above without needing PPC_BIT
> because they'll be in CPU endian too.
Fair suggestion, will update this to u64 types in next iteration.

>
>> @@ -144,6 +152,35 @@ static int drc_pmem_query_n_bind(struct papr_scm_priv *p)
>>  	return drc_pmem_bind(p);
>>  }
>>  
>> +static int drc_pmem_query_health(struct papr_scm_priv *p)
>> +{
>> +	unsigned long ret[PLPAR_HCALL_BUFSIZE];
>> +	int64_t rc;
>
> Use kernel types please, ie. s64, or just long.
Agree, will get it fixed in next iteration.

>
>> +	rc = plpar_hcall(H_SCM_HEALTH, ret, p->drc_index);
>> +	if (rc != H_SUCCESS) {
>> +		dev_err(&p->pdev->dev,
>> +			 "Failed to query health information, Err:%lld\n", rc);
>> +		return -ENXIO;
>> +	}
>> +
>> +	/* Protect modifications to papr_scm_priv with the mutex */
>> +	rc = mutex_lock_interruptible(&p->dimm_mutex);
>> +	if (rc)
>> +		return rc;
>> +
>> +	/* Store the retrieved health information in dimm platform data */
>> +	p->health_bitmap = ret[0];
>> +	p->health_bitmap_valid = ret[1];
>> +
>> +	dev_dbg(&p->pdev->dev,
>> +		"Queried dimm health info. Bitmap:0x%016llx Mask:0x%016llx\n",
>> +		be64_to_cpu(p->health_bitmap),
>> +		be64_to_cpu(p->health_bitmap_valid));
>> +
>> +	mutex_unlock(&p->dimm_mutex);
>> +	return 0;
>> +}
>>  
>>  static int papr_scm_meta_get(struct papr_scm_priv *p,
>>  			     struct nd_cmd_get_config_data_hdr *hdr)
>> @@ -304,6 +341,67 @@ static inline int papr_scm_node(int node)
>>  	return min_node;
>>  }
>>  
>> +static ssize_t papr_flags_show(struct device *dev,
>> +				struct device_attribute *attr, char *buf)
>> +{
>> +	struct nvdimm *dimm = to_nvdimm(dev);
>> +	struct papr_scm_priv *p = nvdimm_provider_data(dimm);
>> +	__be64 health;
>
> No need for __be64 here if health_bitmap was stored in CPU endian.
Right, will change this to u64
>
>> +	int rc;
>> +
>> +	rc = drc_pmem_query_health(p);
>> +	if (rc)
>> +		return rc;
>> +
>> +	/* Protect against modifications to papr_scm_priv with the mutex */
>> +	rc = mutex_lock_interruptible(&p->dimm_mutex);
>> +	if (rc)
>> +		return rc;
>> +
>> +	health = p->health_bitmap & p->health_bitmap_valid;
>
> This is all you ever do with the health_bitmap? In which case why not
> just do the masking before storing it into priv and save yourself 8
> bytes?
Fair suggestion. will address this in next iteration.

>
>> +	/* Check for various masks in bitmap and set the buffer */
>> +	if (health & PAPR_SCM_DIMM_UNARMED_MASK)
>> +		rc += sprintf(buf, "not_armed ");
>
> I know buf is "big enough" but using sprintf() in 2020 is a bit ... :)
>
> seq_buf is a pretty thin wrapper over a buffer you can use to make this
> cleaner and also handles overflow for you.
>
> See eg. show_user_instructions() for an example.
Unfortunatly seq_buf_printf() is still not an exported symbol hence not
usable in external modules.

>
>> +
>> +	if (health & PAPR_SCM_DIMM_BAD_SHUTDOWN_MASK)
>> +		rc += sprintf(buf + rc, "save_fail ");
>> +
>> +	if (health & PAPR_SCM_DIMM_BAD_RESTORE_MASK)
>> +		rc += sprintf(buf + rc, "restore_fail ");
>> +
>> +	if (health & PAPR_SCM_DIMM_ENCRYPTED)
>> +		rc += sprintf(buf + rc, "encrypted ");
>> +
>> +	if (health & PAPR_SCM_DIMM_SMART_EVENT_MASK)
>> +		rc += sprintf(buf + rc, "smart_notify ");
>> +
>> +	if (health & PAPR_SCM_DIMM_SCRUBBED_AND_LOCKED)
>> +		rc += sprintf(buf + rc, "scrubbed locked ");
>> +
>> +	if (rc > 0)
>> +		rc += sprintf(buf + rc, "\n");
>> +
>> +	mutex_unlock(&p->dimm_mutex);
>> +	return rc;
>> +}
>> +DEVICE_ATTR_RO(papr_flags);
>
> cheers

-- 
Vaibhav Jain <vaibhav@linux.ibm.com>
Linux Technology Center, IBM India Pvt. Ltd.
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org