Hi Dan, Thanks for reviewing this patchset,
Dan Williams <[email protected]> writes: > On Tue, Mar 31, 2020 at 7:33 AM Vaibhav Jain <[email protected]> wrote: >> >> 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. > > s/wont/will not/ Will fix this in next version of this patchset. > >> * "save_fail" : Indicating that nvdimm contents couldn't be flushed >> during last shutdown event. > > In the nfit definition this description is "flush_fail". The > "save_fail" flag was specific to hybrid devices that don't have > persistent media and instead scuttle away data from DRAM to flash on > power-failure. > Agree, will change the flag string to "flush_fail" to better match nfit. >> * "restore_fail": Indicating that nvdimm contents couldn't be restored >> during dimm initialization. >> * "encrypted" : Dimm contents are encrypted. > > This does not seem like a health flag to me, have you considered the > libnvdimm security interface for this indicator? Right now PAPR scm spec doesnt allow in-band (guest kernel) control of nvdimm encryption/scrubbing. It only provides bit flags indicating encrypted status or scrubbed/locked status for a nvdimm. Hence right now just exposing them as dimm flags. Once the inband-support for encrypting nvdimms from guest kernels is in place we will implement nvdimm security ops for papr_scm. >> * "smart_notify": There is health event for the nvdimm. > > Are you also going to signal the sysfs attribute when this event > happens? In future yes but at present no, as there are no notifications from PowerVM Hypervisor to the kernel that indicate a change in health status of a nvdimm. Hence we have to rely on userspace polling machenism. A patch to introduce that machenism in ndctl was proposed with the patch "monitor: Add epoll timeout for forcing a full dimm health check" https://lore.kernel.org/linux-nvdimm/[email protected] > >> * "scrubbed" : Indicating that contents of the nvdimm have been >> scrubbed. > > This one seems odd to me what does it mean if it is not set? What does > it mean if a new scrub has been launched. Basically, is there value in > exposing this state? As with 'encrypted' flag scrubbing is right now not supported inband by the guest kernel running on top of PowerVM Hypervisor. Its just a status bit set by the hypervisor to indicate that a scrubbing operation was completed. Hence right now just exposing them as dimm flags. > >> * "locked" : Indicating that nvdimm contents cant be modified >> until next power cycle. > > There is the generic NDD_LOCKED flag, can you use that? ...and in > general I wonder if we should try to unify all the common papr_scm and > nfit health flags in a generic location. It will already be the case > the ndctl needs to look somewhere papr specific for this data maybe it > all should have been generic from the beginning. This flag is different from NDD_LOCKED flag as it indicates that hot(un)plug of new nvdimm regions to the guest wont be allowed until PowerVM hypervisor reboots. The flag description I have put looks misleading in that regard and I will get it fixed in next version. Unification of common nvdimm flags between nfit/papr_scm would be very good idea but would also need some way to update nvdimm flags after nvdimm_create() > > In any event, can you also add this content to a new > Documentation/ABI/testing/sysfs-bus-papr? See sysfs-bus-nfit for > comparison. Agreed, will document these flags in sysfs ABI document in next iteration of this patchset. > >> >> [1]: commit 58b278f568f0 ("powerpc: Provide initial documentation for >> PAPR hcalls") >> >> Signed-off-by: Vaibhav Jain <[email protected]> >> --- >> 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) >> +/* SCM device failed to persist memory contents */ >> +#define PAPR_SCM_DIMM_SHUTDOWN_DIRTY PPC_BIT(1) >> +/* SCM device contents are persisted from previous IPL */ >> +#define PAPR_SCM_DIMM_SHUTDOWN_CLEAN PPC_BIT(2) >> +/* SCM device contents are not persisted from previous IPL */ >> +#define PAPR_SCM_DIMM_EMPTY PPC_BIT(3) >> +/* SCM device memory life remaining is critically low */ >> +#define PAPR_SCM_DIMM_HEALTH_CRITICAL PPC_BIT(4) >> +/* SCM device will be garded off next IPL due to failure */ >> +#define PAPR_SCM_DIMM_HEALTH_FATAL PPC_BIT(5) >> +/* SCM contents cannot persist due to current platform health status */ >> +#define PAPR_SCM_DIMM_HEALTH_UNHEALTHY PPC_BIT(6) >> +/* SCM device is unable to persist memory contents in certain conditions */ >> +#define PAPR_SCM_DIMM_HEALTH_NON_CRITICAL PPC_BIT(7) >> +/* SCM device is encrypted */ >> +#define PAPR_SCM_DIMM_ENCRYPTED PPC_BIT(8) >> +/* SCM device has been scrubbed and locked */ >> +#define PAPR_SCM_DIMM_SCRUBBED_AND_LOCKED PPC_BIT(9) >> + >> +/* Bits status indicators for health bitmap indicating unarmed dimm */ >> +#define PAPR_SCM_DIMM_UNARMED_MASK (PAPR_SCM_DIMM_UNARMED | \ >> + PAPR_SCM_DIMM_HEALTH_UNHEALTHY) >> + >> +/* Bits status indicators for health bitmap indicating unflushed dimm */ >> +#define PAPR_SCM_DIMM_BAD_SHUTDOWN_MASK (PAPR_SCM_DIMM_SHUTDOWN_DIRTY) >> + >> +/* Bits status indicators for health bitmap indicating unrestored dimm */ >> +#define PAPR_SCM_DIMM_BAD_RESTORE_MASK (PAPR_SCM_DIMM_EMPTY) >> + >> +/* Bit status indicators for smart event notification */ >> +#define PAPR_SCM_DIMM_SMART_EVENT_MASK (PAPR_SCM_DIMM_HEALTH_CRITICAL | \ >> + PAPR_SCM_DIMM_HEALTH_FATAL | \ >> + PAPR_SCM_DIMM_HEALTH_UNHEALTHY) >> + >> +#endif >> 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; >> }; >> >> static int drc_pmem_bind(struct papr_scm_priv *p) >> @@ -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; >> + >> + 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; >> + int rc; >> + >> + rc = drc_pmem_query_health(p); >> + if (rc) >> + return rc; > > This attribute is user readable which means userspace can trigger and > ongoing stream of device requests. Is that desired? The nfit driver > caches this flag data, and limits attributes with device side-effects > to root-only. The expectation is that ndctl submits commands to > retrieve the up to date state especially because that payload has > other interesting data like temperature that can't be cached. Agree that this data can cached like nfit does. But wanted to keep this patch simple and the structure that holds the health data will change in patch-4 hence took a call to not include any caching machenism for now. > >> + >> + /* Protect against modifications to papr_scm_priv with the mutex */ > > What papr_scm_priv modifications are you worried about because none of > the below looks like it needs to be locked, and papr_scm_priv had > better be stable otherwise the above usages would have crashed. > >> + rc = mutex_lock_interruptible(&p->dimm_mutex); >> + if (rc) >> + return rc; >> + >> + health = p->health_bitmap & p->health_bitmap_valid; >> + >> + /* Check for various masks in bitmap and set the buffer */ >> + if (health & PAPR_SCM_DIMM_UNARMED_MASK) >> + rc += sprintf(buf, "not_armed "); >> + >> + 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 "); > > See above about whether we event need this here... > >> + >> + if (rc > 0) >> + rc += sprintf(buf + rc, "\n"); >> + >> + mutex_unlock(&p->dimm_mutex); >> + return rc; >> +} >> +DEVICE_ATTR_RO(papr_flags); > > Rather than name this attribute "papr_flags", I'd prefer "papr/flags". > I.e. create a "papr" sub-directory... > Agree will include this change in the next iteration of this patchset. >> + >> +/* papr_scm specific dimm attributes */ >> +static struct attribute *papr_scm_nd_attributes[] = { >> + &dev_attr_papr_flags.attr, >> + NULL, >> +}; >> + >> +static struct attribute_group papr_scm_nd_attribute_group = { > > ...here: > > .name = "papr", > >> + .attrs = papr_scm_nd_attributes, >> +}; >> + > > With the continued discussions this is going to need I hope you > understand that I consider this v5.8 material. > Agreed >> +static const struct attribute_group *papr_scm_dimm_attr_groups[] = { >> + &papr_scm_nd_attribute_group, >> + NULL, >> +}; >> + >> static int papr_scm_nvdimm_init(struct papr_scm_priv *p) >> { >> struct device *dev = &p->pdev->dev; >> @@ -330,8 +428,8 @@ static int papr_scm_nvdimm_init(struct papr_scm_priv *p) >> dimm_flags = 0; >> set_bit(NDD_ALIASING, &dimm_flags); >> >> - p->nvdimm = nvdimm_create(p->bus, p, NULL, dimm_flags, >> - PAPR_SCM_DIMM_CMD_MASK, 0, NULL); >> + p->nvdimm = nvdimm_create(p->bus, p, papr_scm_dimm_attr_groups, >> + dimm_flags, PAPR_SCM_DIMM_CMD_MASK, 0, >> NULL); >> if (!p->nvdimm) { >> dev_err(dev, "Error creating DIMM object for %pOF\n", p->dn); >> goto err; >> @@ -415,6 +513,9 @@ static int papr_scm_probe(struct platform_device *pdev) >> if (!p) >> return -ENOMEM; >> >> + /* Initialize the dimm mutex */ >> + mutex_init(&p->dimm_mutex); >> + >> /* optional DT properties */ >> of_property_read_u32(dn, "ibm,metadata-size", &metadata_size); >> >> -- >> 2.25.1 >> -- Vaibhav Jain <[email protected]> Linux Technology Center, IBM India Pvt. Ltd. _______________________________________________ Linux-nvdimm mailing list -- [email protected] To unsubscribe send an email to [email protected]
