[PATCH v5 0/4] powerpc/papr_scm: Add support for reporting nvdimm health

[PATCH v5 0/4] powerpc/papr_scm: Add support for reporting nvdimm health

[Vaibhav Jain]

The PAPR standard[1][3] provides mechanisms to query the health and
performance stats of an NVDIMM via various hcalls as described in Ref[2].
Until now these stats were never available nor exposed to the user-space
tools like 'ndctl'. This is partly due to PAPR platform not having support
for ACPI and NFIT. Hence 'ndctl' is unable to query and report the dimm
health status and a user had no way to determine the current health status
of a NDVIMM.

To overcome this limitation, this patch-set updates papr_scm kernel module
to query and fetch nvdimm health stats using hcalls described in Ref[2].
This health and performance stats are then exposed to userspace via syfs
and Dimm-Specific-Methods(DSM) issued by libndctl.

These changes coupled with proposed ndtcl changes located at Ref[4] should
provide a way for the user to retrieve NVDIMM health status using ndtcl.

Below is a sample output using proposed kernel + ndctl for PAPR NVDIMM in
a emulation environment:

 # ndctl list -DH
[
  {
    "dev":"nmem0",
    "health":{
      "health_state":"fatal",
      "shutdown_state":"dirty"
    }
  }
]

Dimm health report output on a pseries guest lpar with vPMEM or HMS
based nvdimms that are in perfectly healthy conditions:
	
 # ndctl list -d nmem0 -H
[
  {
    "dev":"nmem0",
    "health":{
      "health_state":"ok",
      "shutdown_state":"clean"
    }
  }
]

PAPR Dimm-Specific-Methods(DSM)
================================

As the name suggests DSMs are used by vendor specific code in libndctl to
execute certain operations or fetch certain information for NVDIMMS. DSMs
can be sent to papr_scm module via libndctl (userspace) and libnvdimm
(kernel) using the ND_CMD_CALL ioctl which can be handled in the dimm
control function papr_scm_ndctl(). For PAPR this patchset proposes a single
DSM to retrieve DIMM health, defined in the newly introduced uapi header
named 'papr_scm_dsm.h'. Support for more DSMs will be added in future.

Structure of the patch-set
==========================

The patchset starts with implementing support for fetching nvdimm health
information from PHYP and partially exposing it to user-space via nvdimm
flags.

Second & Third patches deal with implementing support for servicing DSM
commands papr_scm.

Finally the Fourth patch implements support for servicing DSM
'DSM_PAPR_SCM_HEALTH' that returns the nvdimm health information to
libndctl.

Changelog:
==========

v4..v5:

* Fixed a bug in new implementation of papr_scm_ndctl() that was triggering
  a false error condition.

v3..v4:

* Restructured papr_scm_ndctl() to dispatch ND_CMD_CALL commands to a new
  function named papr_scm_service_dsm() to serivice DSM requests. [Aneesh]

v2..v3:

* Updated the papr_scm_dsm.h header to be more confimant general kernel
  guidelines for UAPI headers. [Aneesh]

* Changed the definition of macro PAPR_SCM_DIMM_UNARMED_MASK to not
  include case when the nvdimm is unarmed because its a vPMEM
  nvdimm. [Aneesh]

v1..v2:

* Restructured the patch-set based on review comments on V1 patch-set to
simplify the patch review. Multiple small patches have been combined into
single patches to reduce cross referencing that was needed in earlier
patch-set. Hence most of the patches in this patch-set as now new. [Aneesh]

* Removed the initial work done for fetch nvdimm performance statistics.
These changes will be re-proposed in a separate patch-set. [Aneesh]

* Simplified handling of versioning of 'struct
nd_papr_scm_dimm_health_stat_v1' as only one version of the structure is
currently in existence.

References:
[1]: "Power Architecture Platform Reference"
      https://en.wikipedia.org/wiki/Power_Architecture_Platform_Reference
[2]: commit 58b278f568f0
     ("powerpc: Provide initial documentation for PAPR hcalls")
[3]: "Linux on Power Architecture Platform Reference"
     https://members.openpowerfoundation.org/document/dl/469
[4]: https://patchwork.kernel.org/project/linux-nvdimm/list/?series=244625

Vaibhav Jain (4):
  powerpc/papr_scm: Fetch nvdimm health information from PHYP
  ndctl/uapi: Introduce NVDIMM_FAMILY_PAPR_SCM as a new NVDIMM DSM
    family
  powerpc/papr_scm,uapi: Add support for handling PAPR DSM commands
  powerpc/papr_scm: Implement support for DSM_PAPR_SCM_HEALTH

 arch/powerpc/include/asm/papr_scm.h          |  48 ++++
 arch/powerpc/include/uapi/asm/papr_scm_dsm.h | 201 ++++++++++++++
 arch/powerpc/platforms/pseries/papr_scm.c    | 277 ++++++++++++++++++-
 include/uapi/linux/ndctl.h                   |   1 +
 4 files changed, 519 insertions(+), 8 deletions(-)
 create mode 100644 arch/powerpc/include/asm/papr_scm.h
 create mode 100644 arch/powerpc/include/uapi/asm/papr_scm_dsm.h

-- 
2.25.1
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

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

[Vaibhav Jain]

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)
+/* 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;
+
+	/* 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;
+
+	/* 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 ");
+
+	if (rc > 0)
+		rc += sprintf(buf + rc, "\n");
+
+	mutex_unlock(&p->dimm_mutex);
+	return rc;
+}
+DEVICE_ATTR_RO(papr_flags);
+
+/* 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 = {
+	.attrs = papr_scm_nd_attributes,
+};
+
+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
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

[PATCH v5 2/4] ndctl/uapi: Introduce NVDIMM_FAMILY_PAPR_SCM as a new NVDIMM DSM family

[Vaibhav Jain]

Add PAPR-scm family of DSM command-set to the white list of NVDIMM
command sets.

Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
---
Changelog:

v4..v5 : None

v3..v4 : None

v2..v3 : Updated the patch prefix to 'ndctl/uapi' [Aneesh]

v1..v2 : None
---
 include/uapi/linux/ndctl.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h
index de5d90212409..99fb60600ef8 100644
--- a/include/uapi/linux/ndctl.h
+++ b/include/uapi/linux/ndctl.h
@@ -244,6 +244,7 @@ struct nd_cmd_pkg {
 #define NVDIMM_FAMILY_HPE2 2
 #define NVDIMM_FAMILY_MSFT 3
 #define NVDIMM_FAMILY_HYPERV 4
+#define NVDIMM_FAMILY_PAPR_SCM 5
 
 #define ND_IOCTL_CALL			_IOWR(ND_IOCTL, ND_CMD_CALL,\
 					struct nd_cmd_pkg)
-- 
2.25.1
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

[PATCH v5 3/4] powerpc/papr_scm,uapi: Add support for handling PAPR DSM commands

[Vaibhav Jain]

Implement support for handling PAPR DSM commands in papr_scm
module. We advertise support for ND_CMD_CALL for the dimm command mask
and implement necessary scaffolding in the module to handle ND_CMD_CALL
ioctl and DSM commands that we receive.

The layout of the DSM commands as we expect from libnvdimm/libndctl is
described in newly introduced uapi header 'papr_scm_dsm.h' which
defines a new 'struct nd_papr_scm_cmd_pkg' header. This header is used
to communicate the DSM command via 'nd_pkg_papr_scm->nd_command' and
size of payload that need to be sent/received for servicing the DSM.

The PAPR DSM commands are assigned indexes started from 0x10000 to
prevent them from overlapping ND_CMD_* values and also makes handling
dimm commands in papr_scm_ndctl(). A new function cmd_to_func() is
implemented that reads the args to papr_scm_ndctl() and performs
sanity tests on them. In case of a DSM command being sent via
ND_CMD_CALL a newly introduced function papr_scm_service_dsm() is
called to handle the request.

Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>

---
Changelog:

v4..v5: Fixed a bug in new implementation of papr_scm_ndctl().

v3..v4: Updated papr_scm_ndctl() to delegate DSM command handling to a
	different function papr_scm_service_dsm(). [Aneesh]

v2..v3: Updated the nd_papr_scm_cmd_pkg to use __xx types as its
	exported to the userspace [Aneesh]

v1..v2: New patch in the series.
---
 arch/powerpc/include/uapi/asm/papr_scm_dsm.h | 161 +++++++++++++++++++
 arch/powerpc/platforms/pseries/papr_scm.c    |  97 ++++++++++-
 2 files changed, 252 insertions(+), 6 deletions(-)
 create mode 100644 arch/powerpc/include/uapi/asm/papr_scm_dsm.h

diff --git a/arch/powerpc/include/uapi/asm/papr_scm_dsm.h b/arch/powerpc/include/uapi/asm/papr_scm_dsm.h
new file mode 100644
index 000000000000..c039a49b41b4
--- /dev/null
+++ b/arch/powerpc/include/uapi/asm/papr_scm_dsm.h
@@ -0,0 +1,161 @@
+/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
+/*
+ * PAPR SCM Device specific methods and struct for libndctl and ndctl
+ *
+ * (C) Copyright IBM 2020
+ *
+ * Author: Vaibhav Jain <vaibhav at linux.ibm.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2, or (at your option)
+ * any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef _UAPI_ASM_POWERPC_PAPR_SCM_DSM_H_
+#define _UAPI_ASM_POWERPC_PAPR_SCM_DSM_H_
+
+#include <linux/types.h>
+
+#ifdef __KERNEL__
+#include <linux/ndctl.h>
+#else
+#include <ndctl.h>
+#endif
+
+/*
+ * DSM Envelope:
+ *
+ * The ioctl ND_CMD_CALL transfers data between user-space and kernel via
+ * 'envelopes' which consists of a header and user-defined payload sections.
+ * The header is described by 'struct nd_papr_scm_cmd_pkg' which expects a
+ * payload following it and offset of which relative to the struct is provided
+ * by 'nd_papr_scm_cmd_pkg.payload_offset'. *
+ *
+ *  +-------------+---------------------+---------------------------+
+ *  |   64-Bytes  |       8-Bytes       |       Max 184-Bytes       |
+ *  +-------------+---------------------+---------------------------+
+ *  |               nd_papr_scm_cmd_pkg |                           |
+ *  |-------------+                     |                           |
+ *  |  nd_cmd_pkg |                     |                           |
+ *  +-------------+---------------------+---------------------------+
+ *  | nd_family   |			|			    |
+ *  | nd_size_out | cmd_status          |			    |
+ *  | nd_size_in  | payload_version     |      PAYLOAD		    |
+ *  | nd_command  | payload_offset ----->			    |
+ *  | nd_fw_size  |                     |			    |
+ *  +-------------+---------------------+---------------------------+
+ *
+ * DSM Header:
+ *
+ * The header is defined as 'struct nd_papr_scm_cmd_pkg' which embeds a
+ * 'struct nd_cmd_pkg' instance. The DSM command is assigned to member
+ * 'nd_cmd_pkg.nd_command'. Apart from size information of the envelop which is
+ * contained in 'struct nd_cmd_pkg', the header also has members following
+ * members:
+ *
+ * 'cmd_status'		: (Out) Errors if any encountered while servicing DSM.
+ * 'payload_version'	: (In/Out) Version number associated with the payload.
+ * 'payload_offset'	: (In)Relative offset of payload from start of envelope.
+ *
+ * DSM Payload:
+ *
+ * The layout of the DSM Payload is defined by various structs shared between
+ * papr_scm and libndctl so that contents of payload can be interpreted. During
+ * servicing of a DSM the papr_scm module will read input args from the payload
+ * field by casting its contents to an appropriate struct pointer based on the
+ * DSM command. Similarly the output of servicing the DSM command will be copied
+ * to the payload field using the same struct.
+ *
+ * 'libnvdimm' enforces a hard limit of 256 bytes on the envelope size, which
+ * leaves around 184 bytes for the envelope payload (ignoring any padding that
+ * the compiler may silently introduce).
+ *
+ * Payload Version:
+ *
+ * A 'payload_version' field is present in DSM header that indicates a specific
+ * version of the structure present in DSM Payload for a given DSM command. This
+ * provides backward compatibility in case the DSM Payload structure evolves
+ * and different structures are supported by 'papr_scm' and 'libndctl'.
+ *
+ * When sending a DSM Payload to 'papr_scm', 'libndctl' should send the version
+ * of the payload struct it supports via 'payload_version' field. The 'papr_scm'
+ * module when servicing the DSM envelop checks the 'payload_version' and then
+ * uses 'payload struct version' == MIN('payload_version field',
+ * 'max payload-struct-version supported by papr_scm') to service the DSM. After
+ * servicing the DSM, 'papr_scm' put the negotiated version of payload struct in
+ * returned 'payload_version' field.
+ *
+ * Libndctl on receiving the envelop back from papr_scm again checks the
+ * 'payload_version' field and based on it use the appropriate version dsm
+ * struct to parse the results.
+ *
+ * Backward Compatibility:
+ *
+ * Above scheme of exchanging different versioned DSM struct between libndctl
+ * and papr_scm should provide backward compatibility until following two
+ * assumptions/conditions when defining new DSM structs hold:
+ *
+ * Let T(X) = { set of attributes in DSM struct 'T' versioned X }
+ *
+ * 1. T(X) is a proper subset of T(Y) if X > Y.
+ *    i.e Each new version of DSM struct should retain existing struct
+ *    attributes from previous version
+ *
+ * 2. If an entity (libndctl or papr_scm) supports a DSM struct T(X) then
+ *    it should also support T(1), T(2)...T(X - 1).
+ *    i.e When adding support for new version of a DSM struct, libndctl
+ *    and papr_scm should retain support of the existing DSM struct
+ *    version they support.
+ */
+
+/* Papr-scm-header + payload expected with ND_CMD_CALL ioctl from libnvdimm */
+struct nd_papr_scm_cmd_pkg {
+	struct nd_cmd_pkg hdr;		/* Package header containing sub-cmd */
+	__s32 cmd_status;		/* Out: Sub-cmd status returned back */
+	__u16 payload_offset;	/* In: offset from start of struct */
+	__u16 payload_version;	/* In/Out: version of the payload */
+	__u8 payload[];		/* In/Out: Sub-cmd data buffer */
+};
+
+/*
+ * Sub commands for ND_CMD_CALL. To prevent overlap from ND_CMD_*, values for
+ * these enums start at 0x10000. These values are then returned from
+ * cmd_to_func() making it easy to implement the switch-case block in
+ * papr_scm_ndctl(). These commands are sent to the kernel via
+ * 'nd_papr_scm_cmd_pkg.hdr.nd_command'
+ */
+enum dsm_papr_scm {
+	DSM_PAPR_SCM_MIN =  0x10000,
+	DSM_PAPR_SCM_MAX,
+};
+
+/* Helpers to evaluate the size of PAPR_SCM envelope */
+/* Calculate the papr_scm-header size */
+#define ND_PAPR_SCM_ENVELOPE_CONTENT_HDR_SIZE \
+	(sizeof(struct nd_papr_scm_cmd_pkg) - sizeof(struct nd_cmd_pkg))
+
+/* Given a type calculate envelope-content size (papr_scm-header + payload) */
+#define ND_PAPR_SCM_ENVELOPE_CONTENT_SIZE(_type_)	\
+	(sizeof(_type_) + ND_PAPR_SCM_ENVELOPE_CONTENT_HDR_SIZE)
+
+/* Convert a libnvdimm nd_cmd_pkg to papr_scm specific pkg */
+static struct nd_papr_scm_cmd_pkg *nd_to_papr_cmd_pkg(struct nd_cmd_pkg *cmd)
+{
+	return (struct nd_papr_scm_cmd_pkg *) cmd;
+}
+
+/* Return the payload pointer for a given pcmd */
+static void *papr_scm_pcmd_to_payload(struct nd_papr_scm_cmd_pkg *pcmd)
+{
+	if (pcmd->hdr.nd_size_in == 0 && pcmd->hdr.nd_size_out == 0)
+		return NULL;
+	else
+		return (void *)((__u8 *) pcmd + pcmd->payload_offset);
+}
+#endif /* _UAPI_ASM_POWERPC_PAPR_SCM_DSM_H_ */
diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
index aaf2e4ab1f75..e8ce96d2249e 100644
--- a/arch/powerpc/platforms/pseries/papr_scm.c
+++ b/arch/powerpc/platforms/pseries/papr_scm.c
@@ -15,13 +15,15 @@
 
 #include <asm/plpar_wrappers.h>
 #include <asm/papr_scm.h>
+#include <asm/papr_scm_dsm.h>
 
 #define BIND_ANY_ADDR (~0ul)
 
 #define PAPR_SCM_DIMM_CMD_MASK \
 	((1ul << ND_CMD_GET_CONFIG_SIZE) | \
 	 (1ul << ND_CMD_GET_CONFIG_DATA) | \
-	 (1ul << ND_CMD_SET_CONFIG_DATA))
+	 (1ul << ND_CMD_SET_CONFIG_DATA) | \
+	 (1ul << ND_CMD_CALL))
 
 struct papr_scm_priv {
 	struct platform_device *pdev;
@@ -283,15 +285,92 @@ static int papr_scm_meta_set(struct papr_scm_priv *p,
 	return 0;
 }
 
+/*
+ * Validate the inputs args to dimm-control function and return '0' if valid.
+ * This also does initial sanity validation to ND_CMD_CALL sub-command packages.
+ */
+static int is_cmd_valid(struct nvdimm *nvdimm, unsigned int cmd, void *buf,
+		       unsigned int buf_len)
+{
+	unsigned long cmd_mask = PAPR_SCM_DIMM_CMD_MASK;
+	struct nd_papr_scm_cmd_pkg *pkg = nd_to_papr_cmd_pkg(buf);
+
+	/* Only dimm-specific calls are supported atm */
+	if (!nvdimm)
+		return -EINVAL;
+
+	if (!test_bit(cmd, &cmd_mask)) {
+		pr_debug("%s: Unsupported cmd=%u\n", __func__, cmd);
+		return -EINVAL;
+	} else if (cmd == ND_CMD_CALL) {
+
+		/* Verify the envelop package */
+		if (!buf || buf_len < sizeof(struct nd_papr_scm_cmd_pkg)) {
+			pr_debug("%s: Invalid pkg size=%u\n", __func__,
+				 buf_len);
+			return -EINVAL;
+		}
+
+		/* Verify that the DSM command family is valid */
+		if (pkg->hdr.nd_family != NVDIMM_FAMILY_PAPR_SCM) {
+			pr_debug("%s: Invalid pkg family=0x%llx\n", __func__,
+				 pkg->hdr.nd_family);
+			return -EINVAL;
+
+		}
+
+		/* We except a payload with all DSM commands */
+		if (papr_scm_pcmd_to_payload(pkg) == NULL) {
+			pr_debug("%s: Empty payload for sub-command=0x%llx\n",
+				 __func__, pkg->hdr.nd_command);
+			return -EINVAL;
+		}
+	}
+
+	/* Command looks valid */
+	return 0;
+}
+
+static int papr_scm_service_dsm(struct papr_scm_priv *p,
+				struct nd_papr_scm_cmd_pkg *call_pkg)
+{
+	/* unknown subcommands return error in packages */
+	if (call_pkg->hdr.nd_command <= DSM_PAPR_SCM_MIN ||
+	    call_pkg->hdr.nd_command >= DSM_PAPR_SCM_MAX) {
+		pr_debug("Invalid DSM command 0x%llx\n",
+			 call_pkg->hdr.nd_command);
+		call_pkg->cmd_status = -EINVAL;
+		return 0;
+	}
+
+	/* Depending on the DSM command call appropriate service routine */
+	switch (call_pkg->hdr.nd_command) {
+	default:
+		pr_debug("Unsupported DSM command 0x%llx\n",
+			 call_pkg->hdr.nd_command);
+		call_pkg->cmd_status = -ENOENT;
+		return 0;
+	}
+}
+
 int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
 		unsigned int cmd, void *buf, unsigned int buf_len, int *cmd_rc)
 {
 	struct nd_cmd_get_config_size *get_size_hdr;
 	struct papr_scm_priv *p;
+	struct nd_papr_scm_cmd_pkg *call_pkg = NULL;
+	int rc;
 
-	/* Only dimm-specific calls are supported atm */
-	if (!nvdimm)
-		return -EINVAL;
+	/* Use a local variable in case cmd_rc pointer is NULL */
+	if (cmd_rc == NULL)
+		cmd_rc = &rc;
+
+	*cmd_rc = is_cmd_valid(nvdimm, cmd, buf, buf_len);
+	if (*cmd_rc) {
+		pr_debug("%s: Invalid cmd=0x%x. Err=%d\n", __func__,
+			 cmd, *cmd_rc);
+		return *cmd_rc;
+	}
 
 	p = nvdimm_provider_data(nvdimm);
 
@@ -313,13 +392,19 @@ int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
 		*cmd_rc = papr_scm_meta_set(p, buf);
 		break;
 
+	case ND_CMD_CALL:
+		call_pkg = nd_to_papr_cmd_pkg(buf);
+		*cmd_rc = papr_scm_service_dsm(p, call_pkg);
+		break;
+
 	default:
-		return -EINVAL;
+		dev_dbg(&p->pdev->dev, "Unknown command = %d\n", cmd);
+		*cmd_rc = -EINVAL;
 	}
 
 	dev_dbg(&p->pdev->dev, "returned with cmd_rc = %d\n", *cmd_rc);
 
-	return 0;
+	return *cmd_rc;
 }
 
 static inline int papr_scm_node(int node)
-- 
2.25.1
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

[PATCH v5 4/4] powerpc/papr_scm: Implement support for DSM_PAPR_SCM_HEALTH

[Vaibhav Jain]

This patch implements support for papr_scm command
'DSM_PAPR_SCM_HEALTH' that returns a newly introduced 'struct
nd_papr_scm_dimm_health_stat' instance containing dimm health
information back to user space in response to ND_CMD_CALL. This
functionality is implemented in newly introduced papr_scm_get_health()
that queries the scm-dimm health information and then copies these bitmaps
to the package payload whose layout is defined by 'struct
papr_scm_ndctl_health'.

The patch also introduces a new member a new member 'struct
papr_scm_priv.health' thats an instance of 'struct
nd_papr_scm_dimm_health_stat' to cache the health information of a
scm-dimm. As a result functions drc_pmem_query_health() and
papr_flags_show() are updated to populate and use this new struct
instead of two be64 integers that we earlier used.

Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
---
Changelog:

v4..v5: None

v3..v4: Call the DSM_PAPR_SCM_HEALTH service function from
	papr_scm_service_dsm() instead of papr_scm_ndctl(). [Aneesh]

v2..v3: Updated struct nd_papr_scm_dimm_health_stat_v1 to use '__xx'
	types as its exported to the userspace [Aneesh]
	Changed the constants DSM_PAPR_SCM_DIMM_XX indicating dimm
	health from enum to #defines [Aneesh]

v1..v2: New patch in the series
---
 arch/powerpc/include/uapi/asm/papr_scm_dsm.h |  40 +++++++
 arch/powerpc/platforms/pseries/papr_scm.c    | 109 ++++++++++++++++---
 2 files changed, 132 insertions(+), 17 deletions(-)

diff --git a/arch/powerpc/include/uapi/asm/papr_scm_dsm.h b/arch/powerpc/include/uapi/asm/papr_scm_dsm.h
index c039a49b41b4..8265125304ca 100644
--- a/arch/powerpc/include/uapi/asm/papr_scm_dsm.h
+++ b/arch/powerpc/include/uapi/asm/papr_scm_dsm.h
@@ -132,6 +132,7 @@ struct nd_papr_scm_cmd_pkg {
  */
 enum dsm_papr_scm {
 	DSM_PAPR_SCM_MIN =  0x10000,
+	DSM_PAPR_SCM_HEALTH,
 	DSM_PAPR_SCM_MAX,
 };
 
@@ -158,4 +159,43 @@ static void *papr_scm_pcmd_to_payload(struct nd_papr_scm_cmd_pkg *pcmd)
 	else
 		return (void *)((__u8 *) pcmd + pcmd->payload_offset);
 }
+
+/* Various scm-dimm health indicators */
+#define DSM_PAPR_SCM_DIMM_HEALTHY       0
+#define DSM_PAPR_SCM_DIMM_UNHEALTHY     1
+#define DSM_PAPR_SCM_DIMM_CRITICAL      2
+#define DSM_PAPR_SCM_DIMM_FATAL         3
+
+/*
+ * Struct exchanged between kernel & ndctl in for PAPR_DSM_PAPR_SMART_HEALTH
+ * Various bitflags indicate the health status of the dimm.
+ *
+ * dimm_unarmed		: Dimm not armed. So contents wont persist.
+ * dimm_bad_shutdown	: Previous shutdown did not persist contents.
+ * dimm_bad_restore	: Contents from previous shutdown werent restored.
+ * dimm_scrubbed	: Contents of the dimm have been scrubbed.
+ * dimm_locked		: Contents of the dimm cant be modified until CEC reboot
+ * dimm_encrypted	: Contents of dimm are encrypted.
+ * dimm_health		: Dimm health indicator.
+ */
+struct nd_papr_scm_dimm_health_stat_v1 {
+	__u8 dimm_unarmed;
+	__u8 dimm_bad_shutdown;
+	__u8 dimm_bad_restore;
+	__u8 dimm_scrubbed;
+	__u8 dimm_locked;
+	__u8 dimm_encrypted;
+	__u16 dimm_health;
+};
+
+/*
+ * Typedef the current struct for dimm_health so that any application
+ * or kernel recompiled after introducing a new version automatically
+ * supports the new version.
+ */
+#define nd_papr_scm_dimm_health_stat nd_papr_scm_dimm_health_stat_v1
+
+/* Current version number for the dimm health struct */
+#define ND_PAPR_SCM_DIMM_HEALTH_VERSION 1
+
 #endif /* _UAPI_ASM_POWERPC_PAPR_SCM_DSM_H_ */
diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
index e8ce96d2249e..ce94762954e0 100644
--- a/arch/powerpc/platforms/pseries/papr_scm.c
+++ b/arch/powerpc/platforms/pseries/papr_scm.c
@@ -47,8 +47,7 @@ struct papr_scm_priv {
 	struct mutex dimm_mutex;
 
 	/* Health information for the dimm */
-	__be64 health_bitmap;
-	__be64 health_bitmap_valid;
+	struct nd_papr_scm_dimm_health_stat health;
 };
 
 static int drc_pmem_bind(struct papr_scm_priv *p)
@@ -158,6 +157,7 @@ static int drc_pmem_query_health(struct papr_scm_priv *p)
 {
 	unsigned long ret[PLPAR_HCALL_BUFSIZE];
 	int64_t rc;
+	__be64 health;
 
 	rc = plpar_hcall(H_SCM_HEALTH, ret, p->drc_index);
 	if (rc != H_SUCCESS) {
@@ -172,13 +172,41 @@ static int drc_pmem_query_health(struct papr_scm_priv *p)
 		return rc;
 
 	/* Store the retrieved health information in dimm platform data */
-	p->health_bitmap = ret[0];
-	p->health_bitmap_valid = ret[1];
+	health = ret[0] & 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));
+		be64_to_cpu(ret[0]),
+		be64_to_cpu(ret[1]));
+
+	memset(&p->health, 0, sizeof(p->health));
+
+	/* Check for various masks in bitmap and set the buffer */
+	if (health & PAPR_SCM_DIMM_UNARMED_MASK)
+		p->health.dimm_unarmed = true;
+
+	if (health & PAPR_SCM_DIMM_BAD_SHUTDOWN_MASK)
+		p->health.dimm_bad_shutdown = true;
+
+	if (health & PAPR_SCM_DIMM_BAD_RESTORE_MASK)
+		p->health.dimm_bad_restore = true;
+
+	if (health & PAPR_SCM_DIMM_ENCRYPTED)
+		p->health.dimm_encrypted = true;
+
+	if (health & PAPR_SCM_DIMM_SCRUBBED_AND_LOCKED) {
+		p->health.dimm_locked = true;
+		p->health.dimm_scrubbed = true;
+	}
+
+	if (health & PAPR_SCM_DIMM_HEALTH_UNHEALTHY)
+		p->health.dimm_health = DSM_PAPR_SCM_DIMM_UNHEALTHY;
+
+	if (health & PAPR_SCM_DIMM_HEALTH_CRITICAL)
+		p->health.dimm_health = DSM_PAPR_SCM_DIMM_CRITICAL;
+
+	if (health & PAPR_SCM_DIMM_HEALTH_FATAL)
+		p->health.dimm_health = DSM_PAPR_SCM_DIMM_FATAL;
 
 	mutex_unlock(&p->dimm_mutex);
 	return 0;
@@ -331,6 +359,51 @@ static int is_cmd_valid(struct nvdimm *nvdimm, unsigned int cmd, void *buf,
 	return 0;
 }
 
+/* Fetch the DIMM health info and populate it in provided package. */
+static int papr_scm_get_health(struct papr_scm_priv *p,
+			       struct nd_papr_scm_cmd_pkg *pkg)
+{
+	int rc;
+	size_t copysize = sizeof(p->health);
+
+	rc = drc_pmem_query_health(p);
+	if (rc)
+		goto out;
+	/*
+	 * If the requested payload version is greater than one we know
+	 * about, return the payload version we know about and let
+	 * caller/userspace handle.
+	 */
+	if (pkg->payload_version > ND_PAPR_SCM_DIMM_HEALTH_VERSION)
+		pkg->payload_version = ND_PAPR_SCM_DIMM_HEALTH_VERSION;
+
+	if (pkg->hdr.nd_size_out < copysize) {
+		dev_dbg(&p->pdev->dev, "%s Payload not large enough\n",
+			__func__);
+		dev_dbg(&p->pdev->dev, "%s Expected %lu, available %u\n",
+			__func__, copysize, pkg->hdr.nd_size_out);
+		rc = -ENOSPC;
+		goto out;
+	}
+
+	dev_dbg(&p->pdev->dev, "%s Copying payload size=%lu version=0x%x\n",
+		__func__, copysize, pkg->payload_version);
+
+	/* Copy a subset of health struct based on copysize */
+	memcpy(papr_scm_pcmd_to_payload(pkg), &p->health, copysize);
+	pkg->hdr.nd_fw_size = copysize;
+
+out:
+	/*
+	 * Put the error in out package and return success from function
+	 * so that errors if any are propogated back to userspace.
+	 */
+	pkg->cmd_status = rc;
+	dev_dbg(&p->pdev->dev, "%s completion code = %d\n", __func__, rc);
+
+	return 0;
+}
+
 static int papr_scm_service_dsm(struct papr_scm_priv *p,
 				struct nd_papr_scm_cmd_pkg *call_pkg)
 {
@@ -345,6 +418,9 @@ static int papr_scm_service_dsm(struct papr_scm_priv *p,
 
 	/* Depending on the DSM command call appropriate service routine */
 	switch (call_pkg->hdr.nd_command) {
+	case DSM_PAPR_SCM_HEALTH:
+		return papr_scm_get_health(p, call_pkg);
+
 	default:
 		pr_debug("Unsupported DSM command 0x%llx\n",
 			 call_pkg->hdr.nd_command);
@@ -431,7 +507,6 @@ static ssize_t papr_flags_show(struct device *dev,
 {
 	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);
@@ -443,26 +518,26 @@ static ssize_t papr_flags_show(struct device *dev,
 	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)
+	if (p->health.dimm_unarmed)
 		rc += sprintf(buf, "not_armed ");
 
-	if (health & PAPR_SCM_DIMM_BAD_SHUTDOWN_MASK)
+	if (p->health.dimm_bad_shutdown)
 		rc += sprintf(buf + rc, "save_fail ");
 
-	if (health & PAPR_SCM_DIMM_BAD_RESTORE_MASK)
+	if (p->health.dimm_bad_restore)
 		rc += sprintf(buf + rc, "restore_fail ");
 
-	if (health & PAPR_SCM_DIMM_ENCRYPTED)
+	if (p->health.dimm_encrypted)
 		rc += sprintf(buf + rc, "encrypted ");
 
-	if (health & PAPR_SCM_DIMM_SMART_EVENT_MASK)
+	if (p->health.dimm_health)
 		rc += sprintf(buf + rc, "smart_notify ");
 
-	if (health & PAPR_SCM_DIMM_SCRUBBED_AND_LOCKED)
-		rc += sprintf(buf + rc, "scrubbed locked ");
+	if (p->health.dimm_scrubbed)
+		rc += sprintf(buf + rc, "scrubbed ");
+
+	if (p->health.dimm_locked)
+		rc += sprintf(buf + rc, "locked ");
 
 	if (rc > 0)
 		rc += sprintf(buf + rc, "\n");
-- 
2.25.1
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

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

[Aneesh Kumar K.V]

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")
>

Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>

> 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)
> +/* 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;
> +
> +	/* 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;
> +
> +	/* 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 ");
> +
> +	if (rc > 0)
> +		rc += sprintf(buf + rc, "\n");
> +
> +	mutex_unlock(&p->dimm_mutex);
> +	return rc;
> +}
> +DEVICE_ATTR_RO(papr_flags);
> +
> +/* 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 = {
> +	.attrs = papr_scm_nd_attributes,
> +};
> +
> +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
> _______________________________________________
> Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
> To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [PATCH v5 2/4] ndctl/uapi: Introduce NVDIMM_FAMILY_PAPR_SCM as a new NVDIMM DSM family

[Aneesh Kumar K.V]

Vaibhav Jain <vaibhav@linux.ibm.com> writes:

> Add PAPR-scm family of DSM command-set to the white list of NVDIMM
> command sets.
>

Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>

> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> ---
> Changelog:
>
> v4..v5 : None
>
> v3..v4 : None
>
> v2..v3 : Updated the patch prefix to 'ndctl/uapi' [Aneesh]
>
> v1..v2 : None
> ---
>  include/uapi/linux/ndctl.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h
> index de5d90212409..99fb60600ef8 100644
> --- a/include/uapi/linux/ndctl.h
> +++ b/include/uapi/linux/ndctl.h
> @@ -244,6 +244,7 @@ struct nd_cmd_pkg {
>  #define NVDIMM_FAMILY_HPE2 2
>  #define NVDIMM_FAMILY_MSFT 3
>  #define NVDIMM_FAMILY_HYPERV 4
> +#define NVDIMM_FAMILY_PAPR_SCM 5
>  
>  #define ND_IOCTL_CALL			_IOWR(ND_IOCTL, ND_CMD_CALL,\
>  					struct nd_cmd_pkg)
> -- 
> 2.25.1
> _______________________________________________
> Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
> To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [PATCH v5 3/4] powerpc/papr_scm,uapi: Add support for handling PAPR DSM commands

[Aneesh Kumar K.V]

Vaibhav Jain <vaibhav@linux.ibm.com> writes:

> Implement support for handling PAPR DSM commands in papr_scm
> module. We advertise support for ND_CMD_CALL for the dimm command mask
> and implement necessary scaffolding in the module to handle ND_CMD_CALL
> ioctl and DSM commands that we receive.
>
> The layout of the DSM commands as we expect from libnvdimm/libndctl is
> described in newly introduced uapi header 'papr_scm_dsm.h' which
> defines a new 'struct nd_papr_scm_cmd_pkg' header. This header is used
> to communicate the DSM command via 'nd_pkg_papr_scm->nd_command' and
> size of payload that need to be sent/received for servicing the DSM.
>
> The PAPR DSM commands are assigned indexes started from 0x10000 to
> prevent them from overlapping ND_CMD_* values and also makes handling
> dimm commands in papr_scm_ndctl(). A new function cmd_to_func() is
> implemented that reads the args to papr_scm_ndctl() and performs
> sanity tests on them. In case of a DSM command being sent via
> ND_CMD_CALL a newly introduced function papr_scm_service_dsm() is
> called to handle the request.
>

Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>

> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
>
> ---
> Changelog:
>
> v4..v5: Fixed a bug in new implementation of papr_scm_ndctl().
>
> v3..v4: Updated papr_scm_ndctl() to delegate DSM command handling to a
> 	different function papr_scm_service_dsm(). [Aneesh]
>
> v2..v3: Updated the nd_papr_scm_cmd_pkg to use __xx types as its
> 	exported to the userspace [Aneesh]
>
> v1..v2: New patch in the series.
> ---
>  arch/powerpc/include/uapi/asm/papr_scm_dsm.h | 161 +++++++++++++++++++
>  arch/powerpc/platforms/pseries/papr_scm.c    |  97 ++++++++++-
>  2 files changed, 252 insertions(+), 6 deletions(-)
>  create mode 100644 arch/powerpc/include/uapi/asm/papr_scm_dsm.h
>
> diff --git a/arch/powerpc/include/uapi/asm/papr_scm_dsm.h b/arch/powerpc/include/uapi/asm/papr_scm_dsm.h
> new file mode 100644
> index 000000000000..c039a49b41b4
> --- /dev/null
> +++ b/arch/powerpc/include/uapi/asm/papr_scm_dsm.h
> @@ -0,0 +1,161 @@
> +/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
> +/*
> + * PAPR SCM Device specific methods and struct for libndctl and ndctl
> + *
> + * (C) Copyright IBM 2020
> + *
> + * Author: Vaibhav Jain <vaibhav at linux.ibm.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2, or (at your option)
> + * any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#ifndef _UAPI_ASM_POWERPC_PAPR_SCM_DSM_H_
> +#define _UAPI_ASM_POWERPC_PAPR_SCM_DSM_H_
> +
> +#include <linux/types.h>
> +
> +#ifdef __KERNEL__
> +#include <linux/ndctl.h>
> +#else
> +#include <ndctl.h>
> +#endif
> +
> +/*
> + * DSM Envelope:
> + *
> + * The ioctl ND_CMD_CALL transfers data between user-space and kernel via
> + * 'envelopes' which consists of a header and user-defined payload sections.
> + * The header is described by 'struct nd_papr_scm_cmd_pkg' which expects a
> + * payload following it and offset of which relative to the struct is provided
> + * by 'nd_papr_scm_cmd_pkg.payload_offset'. *
> + *
> + *  +-------------+---------------------+---------------------------+
> + *  |   64-Bytes  |       8-Bytes       |       Max 184-Bytes       |
> + *  +-------------+---------------------+---------------------------+
> + *  |               nd_papr_scm_cmd_pkg |                           |
> + *  |-------------+                     |                           |
> + *  |  nd_cmd_pkg |                     |                           |
> + *  +-------------+---------------------+---------------------------+
> + *  | nd_family   |			|			    |
> + *  | nd_size_out | cmd_status          |			    |
> + *  | nd_size_in  | payload_version     |      PAYLOAD		    |
> + *  | nd_command  | payload_offset ----->			    |
> + *  | nd_fw_size  |                     |			    |
> + *  +-------------+---------------------+---------------------------+
> + *
> + * DSM Header:
> + *
> + * The header is defined as 'struct nd_papr_scm_cmd_pkg' which embeds a
> + * 'struct nd_cmd_pkg' instance. The DSM command is assigned to member
> + * 'nd_cmd_pkg.nd_command'. Apart from size information of the envelop which is
> + * contained in 'struct nd_cmd_pkg', the header also has members following
> + * members:
> + *
> + * 'cmd_status'		: (Out) Errors if any encountered while servicing DSM.
> + * 'payload_version'	: (In/Out) Version number associated with the payload.
> + * 'payload_offset'	: (In)Relative offset of payload from start of envelope.
> + *
> + * DSM Payload:
> + *
> + * The layout of the DSM Payload is defined by various structs shared between
> + * papr_scm and libndctl so that contents of payload can be interpreted. During
> + * servicing of a DSM the papr_scm module will read input args from the payload
> + * field by casting its contents to an appropriate struct pointer based on the
> + * DSM command. Similarly the output of servicing the DSM command will be copied
> + * to the payload field using the same struct.
> + *
> + * 'libnvdimm' enforces a hard limit of 256 bytes on the envelope size, which
> + * leaves around 184 bytes for the envelope payload (ignoring any padding that
> + * the compiler may silently introduce).
> + *
> + * Payload Version:
> + *
> + * A 'payload_version' field is present in DSM header that indicates a specific
> + * version of the structure present in DSM Payload for a given DSM command. This
> + * provides backward compatibility in case the DSM Payload structure evolves
> + * and different structures are supported by 'papr_scm' and 'libndctl'.
> + *
> + * When sending a DSM Payload to 'papr_scm', 'libndctl' should send the version
> + * of the payload struct it supports via 'payload_version' field. The 'papr_scm'
> + * module when servicing the DSM envelop checks the 'payload_version' and then
> + * uses 'payload struct version' == MIN('payload_version field',
> + * 'max payload-struct-version supported by papr_scm') to service the DSM. After
> + * servicing the DSM, 'papr_scm' put the negotiated version of payload struct in
> + * returned 'payload_version' field.
> + *
> + * Libndctl on receiving the envelop back from papr_scm again checks the
> + * 'payload_version' field and based on it use the appropriate version dsm
> + * struct to parse the results.
> + *
> + * Backward Compatibility:
> + *
> + * Above scheme of exchanging different versioned DSM struct between libndctl
> + * and papr_scm should provide backward compatibility until following two
> + * assumptions/conditions when defining new DSM structs hold:
> + *
> + * Let T(X) = { set of attributes in DSM struct 'T' versioned X }
> + *
> + * 1. T(X) is a proper subset of T(Y) if X > Y.
> + *    i.e Each new version of DSM struct should retain existing struct
> + *    attributes from previous version
> + *
> + * 2. If an entity (libndctl or papr_scm) supports a DSM struct T(X) then
> + *    it should also support T(1), T(2)...T(X - 1).
> + *    i.e When adding support for new version of a DSM struct, libndctl
> + *    and papr_scm should retain support of the existing DSM struct
> + *    version they support.
> + */
> +
> +/* Papr-scm-header + payload expected with ND_CMD_CALL ioctl from libnvdimm */
> +struct nd_papr_scm_cmd_pkg {
> +	struct nd_cmd_pkg hdr;		/* Package header containing sub-cmd */
> +	__s32 cmd_status;		/* Out: Sub-cmd status returned back */
> +	__u16 payload_offset;	/* In: offset from start of struct */
> +	__u16 payload_version;	/* In/Out: version of the payload */
> +	__u8 payload[];		/* In/Out: Sub-cmd data buffer */
> +};
> +
> +/*
> + * Sub commands for ND_CMD_CALL. To prevent overlap from ND_CMD_*, values for
> + * these enums start at 0x10000. These values are then returned from
> + * cmd_to_func() making it easy to implement the switch-case block in
> + * papr_scm_ndctl(). These commands are sent to the kernel via
> + * 'nd_papr_scm_cmd_pkg.hdr.nd_command'
> + */
> +enum dsm_papr_scm {
> +	DSM_PAPR_SCM_MIN =  0x10000,
> +	DSM_PAPR_SCM_MAX,
> +};
> +
> +/* Helpers to evaluate the size of PAPR_SCM envelope */
> +/* Calculate the papr_scm-header size */
> +#define ND_PAPR_SCM_ENVELOPE_CONTENT_HDR_SIZE \
> +	(sizeof(struct nd_papr_scm_cmd_pkg) - sizeof(struct nd_cmd_pkg))
> +
> +/* Given a type calculate envelope-content size (papr_scm-header + payload) */
> +#define ND_PAPR_SCM_ENVELOPE_CONTENT_SIZE(_type_)	\
> +	(sizeof(_type_) + ND_PAPR_SCM_ENVELOPE_CONTENT_HDR_SIZE)
> +
> +/* Convert a libnvdimm nd_cmd_pkg to papr_scm specific pkg */
> +static struct nd_papr_scm_cmd_pkg *nd_to_papr_cmd_pkg(struct nd_cmd_pkg *cmd)
> +{
> +	return (struct nd_papr_scm_cmd_pkg *) cmd;
> +}
> +
> +/* Return the payload pointer for a given pcmd */
> +static void *papr_scm_pcmd_to_payload(struct nd_papr_scm_cmd_pkg *pcmd)
> +{
> +	if (pcmd->hdr.nd_size_in == 0 && pcmd->hdr.nd_size_out == 0)
> +		return NULL;
> +	else
> +		return (void *)((__u8 *) pcmd + pcmd->payload_offset);
> +}
> +#endif /* _UAPI_ASM_POWERPC_PAPR_SCM_DSM_H_ */
> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> index aaf2e4ab1f75..e8ce96d2249e 100644
> --- a/arch/powerpc/platforms/pseries/papr_scm.c
> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> @@ -15,13 +15,15 @@
>  
>  #include <asm/plpar_wrappers.h>
>  #include <asm/papr_scm.h>
> +#include <asm/papr_scm_dsm.h>
>  
>  #define BIND_ANY_ADDR (~0ul)
>  
>  #define PAPR_SCM_DIMM_CMD_MASK \
>  	((1ul << ND_CMD_GET_CONFIG_SIZE) | \
>  	 (1ul << ND_CMD_GET_CONFIG_DATA) | \
> -	 (1ul << ND_CMD_SET_CONFIG_DATA))
> +	 (1ul << ND_CMD_SET_CONFIG_DATA) | \
> +	 (1ul << ND_CMD_CALL))
>  
>  struct papr_scm_priv {
>  	struct platform_device *pdev;
> @@ -283,15 +285,92 @@ static int papr_scm_meta_set(struct papr_scm_priv *p,
>  	return 0;
>  }
>  
> +/*
> + * Validate the inputs args to dimm-control function and return '0' if valid.
> + * This also does initial sanity validation to ND_CMD_CALL sub-command packages.
> + */
> +static int is_cmd_valid(struct nvdimm *nvdimm, unsigned int cmd, void *buf,
> +		       unsigned int buf_len)
> +{
> +	unsigned long cmd_mask = PAPR_SCM_DIMM_CMD_MASK;
> +	struct nd_papr_scm_cmd_pkg *pkg = nd_to_papr_cmd_pkg(buf);
> +
> +	/* Only dimm-specific calls are supported atm */
> +	if (!nvdimm)
> +		return -EINVAL;
> +
> +	if (!test_bit(cmd, &cmd_mask)) {
> +		pr_debug("%s: Unsupported cmd=%u\n", __func__, cmd);
> +		return -EINVAL;
> +	} else if (cmd == ND_CMD_CALL) {
> +
> +		/* Verify the envelop package */
> +		if (!buf || buf_len < sizeof(struct nd_papr_scm_cmd_pkg)) {
> +			pr_debug("%s: Invalid pkg size=%u\n", __func__,
> +				 buf_len);
> +			return -EINVAL;
> +		}
> +
> +		/* Verify that the DSM command family is valid */
> +		if (pkg->hdr.nd_family != NVDIMM_FAMILY_PAPR_SCM) {
> +			pr_debug("%s: Invalid pkg family=0x%llx\n", __func__,
> +				 pkg->hdr.nd_family);
> +			return -EINVAL;
> +
> +		}
> +
> +		/* We except a payload with all DSM commands */
> +		if (papr_scm_pcmd_to_payload(pkg) == NULL) {
> +			pr_debug("%s: Empty payload for sub-command=0x%llx\n",
> +				 __func__, pkg->hdr.nd_command);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	/* Command looks valid */
> +	return 0;
> +}
> +
> +static int papr_scm_service_dsm(struct papr_scm_priv *p,
> +				struct nd_papr_scm_cmd_pkg *call_pkg)
> +{
> +	/* unknown subcommands return error in packages */
> +	if (call_pkg->hdr.nd_command <= DSM_PAPR_SCM_MIN ||
> +	    call_pkg->hdr.nd_command >= DSM_PAPR_SCM_MAX) {
> +		pr_debug("Invalid DSM command 0x%llx\n",
> +			 call_pkg->hdr.nd_command);
> +		call_pkg->cmd_status = -EINVAL;
> +		return 0;
> +	}
> +
> +	/* Depending on the DSM command call appropriate service routine */
> +	switch (call_pkg->hdr.nd_command) {
> +	default:
> +		pr_debug("Unsupported DSM command 0x%llx\n",
> +			 call_pkg->hdr.nd_command);
> +		call_pkg->cmd_status = -ENOENT;
> +		return 0;
> +	}
> +}
> +
>  int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
>  		unsigned int cmd, void *buf, unsigned int buf_len, int *cmd_rc)
>  {
>  	struct nd_cmd_get_config_size *get_size_hdr;
>  	struct papr_scm_priv *p;
> +	struct nd_papr_scm_cmd_pkg *call_pkg = NULL;
> +	int rc;
>  
> -	/* Only dimm-specific calls are supported atm */
> -	if (!nvdimm)
> -		return -EINVAL;
> +	/* Use a local variable in case cmd_rc pointer is NULL */
> +	if (cmd_rc == NULL)
> +		cmd_rc = &rc;
> +
> +	*cmd_rc = is_cmd_valid(nvdimm, cmd, buf, buf_len);
> +	if (*cmd_rc) {
> +		pr_debug("%s: Invalid cmd=0x%x. Err=%d\n", __func__,
> +			 cmd, *cmd_rc);
> +		return *cmd_rc;
> +	}
>  
>  	p = nvdimm_provider_data(nvdimm);
>  
> @@ -313,13 +392,19 @@ int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
>  		*cmd_rc = papr_scm_meta_set(p, buf);
>  		break;
>  
> +	case ND_CMD_CALL:
> +		call_pkg = nd_to_papr_cmd_pkg(buf);
> +		*cmd_rc = papr_scm_service_dsm(p, call_pkg);
> +		break;
> +
>  	default:
> -		return -EINVAL;
> +		dev_dbg(&p->pdev->dev, "Unknown command = %d\n", cmd);
> +		*cmd_rc = -EINVAL;
>  	}
>  
>  	dev_dbg(&p->pdev->dev, "returned with cmd_rc = %d\n", *cmd_rc);
>  
> -	return 0;
> +	return *cmd_rc;
>  }
>  
>  static inline int papr_scm_node(int node)
> -- 
> 2.25.1
> _______________________________________________
> Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
> To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [PATCH v5 4/4] powerpc/papr_scm: Implement support for DSM_PAPR_SCM_HEALTH

[Aneesh Kumar K.V]

Vaibhav Jain <vaibhav@linux.ibm.com> writes:

> This patch implements support for papr_scm command
> 'DSM_PAPR_SCM_HEALTH' that returns a newly introduced 'struct
> nd_papr_scm_dimm_health_stat' instance containing dimm health
> information back to user space in response to ND_CMD_CALL. This
> functionality is implemented in newly introduced papr_scm_get_health()
> that queries the scm-dimm health information and then copies these bitmaps
> to the package payload whose layout is defined by 'struct
> papr_scm_ndctl_health'.
>
> The patch also introduces a new member a new member 'struct
> papr_scm_priv.health' thats an instance of 'struct
> nd_papr_scm_dimm_health_stat' to cache the health information of a
> scm-dimm. As a result functions drc_pmem_query_health() and
> papr_flags_show() are updated to populate and use this new struct
> instead of two be64 integers that we earlier used.
>

Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>

> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> ---
> Changelog:
>
> v4..v5: None
>
> v3..v4: Call the DSM_PAPR_SCM_HEALTH service function from
> 	papr_scm_service_dsm() instead of papr_scm_ndctl(). [Aneesh]
>
> v2..v3: Updated struct nd_papr_scm_dimm_health_stat_v1 to use '__xx'
> 	types as its exported to the userspace [Aneesh]
> 	Changed the constants DSM_PAPR_SCM_DIMM_XX indicating dimm
> 	health from enum to #defines [Aneesh]
>
> v1..v2: New patch in the series
> ---
>  arch/powerpc/include/uapi/asm/papr_scm_dsm.h |  40 +++++++
>  arch/powerpc/platforms/pseries/papr_scm.c    | 109 ++++++++++++++++---
>  2 files changed, 132 insertions(+), 17 deletions(-)
>
> diff --git a/arch/powerpc/include/uapi/asm/papr_scm_dsm.h b/arch/powerpc/include/uapi/asm/papr_scm_dsm.h
> index c039a49b41b4..8265125304ca 100644
> --- a/arch/powerpc/include/uapi/asm/papr_scm_dsm.h
> +++ b/arch/powerpc/include/uapi/asm/papr_scm_dsm.h
> @@ -132,6 +132,7 @@ struct nd_papr_scm_cmd_pkg {
>   */
>  enum dsm_papr_scm {
>  	DSM_PAPR_SCM_MIN =  0x10000,
> +	DSM_PAPR_SCM_HEALTH,
>  	DSM_PAPR_SCM_MAX,
>  };
>  
> @@ -158,4 +159,43 @@ static void *papr_scm_pcmd_to_payload(struct nd_papr_scm_cmd_pkg *pcmd)
>  	else
>  		return (void *)((__u8 *) pcmd + pcmd->payload_offset);
>  }
> +
> +/* Various scm-dimm health indicators */
> +#define DSM_PAPR_SCM_DIMM_HEALTHY       0
> +#define DSM_PAPR_SCM_DIMM_UNHEALTHY     1
> +#define DSM_PAPR_SCM_DIMM_CRITICAL      2
> +#define DSM_PAPR_SCM_DIMM_FATAL         3
> +
> +/*
> + * Struct exchanged between kernel & ndctl in for PAPR_DSM_PAPR_SMART_HEALTH
> + * Various bitflags indicate the health status of the dimm.
> + *
> + * dimm_unarmed		: Dimm not armed. So contents wont persist.
> + * dimm_bad_shutdown	: Previous shutdown did not persist contents.
> + * dimm_bad_restore	: Contents from previous shutdown werent restored.
> + * dimm_scrubbed	: Contents of the dimm have been scrubbed.
> + * dimm_locked		: Contents of the dimm cant be modified until CEC reboot
> + * dimm_encrypted	: Contents of dimm are encrypted.
> + * dimm_health		: Dimm health indicator.
> + */
> +struct nd_papr_scm_dimm_health_stat_v1 {
> +	__u8 dimm_unarmed;
> +	__u8 dimm_bad_shutdown;
> +	__u8 dimm_bad_restore;
> +	__u8 dimm_scrubbed;
> +	__u8 dimm_locked;
> +	__u8 dimm_encrypted;
> +	__u16 dimm_health;
> +};
> +
> +/*
> + * Typedef the current struct for dimm_health so that any application
> + * or kernel recompiled after introducing a new version automatically
> + * supports the new version.
> + */
> +#define nd_papr_scm_dimm_health_stat nd_papr_scm_dimm_health_stat_v1
> +
> +/* Current version number for the dimm health struct */
> +#define ND_PAPR_SCM_DIMM_HEALTH_VERSION 1
> +
>  #endif /* _UAPI_ASM_POWERPC_PAPR_SCM_DSM_H_ */
> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> index e8ce96d2249e..ce94762954e0 100644
> --- a/arch/powerpc/platforms/pseries/papr_scm.c
> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> @@ -47,8 +47,7 @@ struct papr_scm_priv {
>  	struct mutex dimm_mutex;
>  
>  	/* Health information for the dimm */
> -	__be64 health_bitmap;
> -	__be64 health_bitmap_valid;
> +	struct nd_papr_scm_dimm_health_stat health;
>  };
>  
>  static int drc_pmem_bind(struct papr_scm_priv *p)
> @@ -158,6 +157,7 @@ static int drc_pmem_query_health(struct papr_scm_priv *p)
>  {
>  	unsigned long ret[PLPAR_HCALL_BUFSIZE];
>  	int64_t rc;
> +	__be64 health;
>  
>  	rc = plpar_hcall(H_SCM_HEALTH, ret, p->drc_index);
>  	if (rc != H_SUCCESS) {
> @@ -172,13 +172,41 @@ static int drc_pmem_query_health(struct papr_scm_priv *p)
>  		return rc;
>  
>  	/* Store the retrieved health information in dimm platform data */
> -	p->health_bitmap = ret[0];
> -	p->health_bitmap_valid = ret[1];
> +	health = ret[0] & 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));
> +		be64_to_cpu(ret[0]),
> +		be64_to_cpu(ret[1]));
> +
> +	memset(&p->health, 0, sizeof(p->health));
> +
> +	/* Check for various masks in bitmap and set the buffer */
> +	if (health & PAPR_SCM_DIMM_UNARMED_MASK)
> +		p->health.dimm_unarmed = true;
> +
> +	if (health & PAPR_SCM_DIMM_BAD_SHUTDOWN_MASK)
> +		p->health.dimm_bad_shutdown = true;
> +
> +	if (health & PAPR_SCM_DIMM_BAD_RESTORE_MASK)
> +		p->health.dimm_bad_restore = true;
> +
> +	if (health & PAPR_SCM_DIMM_ENCRYPTED)
> +		p->health.dimm_encrypted = true;
> +
> +	if (health & PAPR_SCM_DIMM_SCRUBBED_AND_LOCKED) {
> +		p->health.dimm_locked = true;
> +		p->health.dimm_scrubbed = true;
> +	}
> +
> +	if (health & PAPR_SCM_DIMM_HEALTH_UNHEALTHY)
> +		p->health.dimm_health = DSM_PAPR_SCM_DIMM_UNHEALTHY;
> +
> +	if (health & PAPR_SCM_DIMM_HEALTH_CRITICAL)
> +		p->health.dimm_health = DSM_PAPR_SCM_DIMM_CRITICAL;
> +
> +	if (health & PAPR_SCM_DIMM_HEALTH_FATAL)
> +		p->health.dimm_health = DSM_PAPR_SCM_DIMM_FATAL;
>  
>  	mutex_unlock(&p->dimm_mutex);
>  	return 0;
> @@ -331,6 +359,51 @@ static int is_cmd_valid(struct nvdimm *nvdimm, unsigned int cmd, void *buf,
>  	return 0;
>  }
>  
> +/* Fetch the DIMM health info and populate it in provided package. */
> +static int papr_scm_get_health(struct papr_scm_priv *p,
> +			       struct nd_papr_scm_cmd_pkg *pkg)
> +{
> +	int rc;
> +	size_t copysize = sizeof(p->health);
> +
> +	rc = drc_pmem_query_health(p);
> +	if (rc)
> +		goto out;
> +	/*
> +	 * If the requested payload version is greater than one we know
> +	 * about, return the payload version we know about and let
> +	 * caller/userspace handle.
> +	 */
> +	if (pkg->payload_version > ND_PAPR_SCM_DIMM_HEALTH_VERSION)
> +		pkg->payload_version = ND_PAPR_SCM_DIMM_HEALTH_VERSION;
> +
> +	if (pkg->hdr.nd_size_out < copysize) {
> +		dev_dbg(&p->pdev->dev, "%s Payload not large enough\n",
> +			__func__);
> +		dev_dbg(&p->pdev->dev, "%s Expected %lu, available %u\n",
> +			__func__, copysize, pkg->hdr.nd_size_out);
> +		rc = -ENOSPC;
> +		goto out;
> +	}
> +
> +	dev_dbg(&p->pdev->dev, "%s Copying payload size=%lu version=0x%x\n",
> +		__func__, copysize, pkg->payload_version);
> +
> +	/* Copy a subset of health struct based on copysize */
> +	memcpy(papr_scm_pcmd_to_payload(pkg), &p->health, copysize);
> +	pkg->hdr.nd_fw_size = copysize;
> +
> +out:
> +	/*
> +	 * Put the error in out package and return success from function
> +	 * so that errors if any are propogated back to userspace.
> +	 */
> +	pkg->cmd_status = rc;
> +	dev_dbg(&p->pdev->dev, "%s completion code = %d\n", __func__, rc);
> +
> +	return 0;
> +}
> +
>  static int papr_scm_service_dsm(struct papr_scm_priv *p,
>  				struct nd_papr_scm_cmd_pkg *call_pkg)
>  {
> @@ -345,6 +418,9 @@ static int papr_scm_service_dsm(struct papr_scm_priv *p,
>  
>  	/* Depending on the DSM command call appropriate service routine */
>  	switch (call_pkg->hdr.nd_command) {
> +	case DSM_PAPR_SCM_HEALTH:
> +		return papr_scm_get_health(p, call_pkg);
> +
>  	default:
>  		pr_debug("Unsupported DSM command 0x%llx\n",
>  			 call_pkg->hdr.nd_command);
> @@ -431,7 +507,6 @@ static ssize_t papr_flags_show(struct device *dev,
>  {
>  	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);
> @@ -443,26 +518,26 @@ static ssize_t papr_flags_show(struct device *dev,
>  	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)
> +	if (p->health.dimm_unarmed)
>  		rc += sprintf(buf, "not_armed ");
>  
> -	if (health & PAPR_SCM_DIMM_BAD_SHUTDOWN_MASK)
> +	if (p->health.dimm_bad_shutdown)
>  		rc += sprintf(buf + rc, "save_fail ");
>  
> -	if (health & PAPR_SCM_DIMM_BAD_RESTORE_MASK)
> +	if (p->health.dimm_bad_restore)
>  		rc += sprintf(buf + rc, "restore_fail ");
>  
> -	if (health & PAPR_SCM_DIMM_ENCRYPTED)
> +	if (p->health.dimm_encrypted)
>  		rc += sprintf(buf + rc, "encrypted ");
>  
> -	if (health & PAPR_SCM_DIMM_SMART_EVENT_MASK)
> +	if (p->health.dimm_health)
>  		rc += sprintf(buf + rc, "smart_notify ");
>  
> -	if (health & PAPR_SCM_DIMM_SCRUBBED_AND_LOCKED)
> -		rc += sprintf(buf + rc, "scrubbed locked ");
> +	if (p->health.dimm_scrubbed)
> +		rc += sprintf(buf + rc, "scrubbed ");
> +
> +	if (p->health.dimm_locked)
> +		rc += sprintf(buf + rc, "locked ");
>  
>  	if (rc > 0)
>  		rc += sprintf(buf + rc, "\n");
> -- 
> 2.25.1
> _______________________________________________
> Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
> To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

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

[Dan Williams]

On Tue, Mar 31, 2020 at 7:33 AM Vaibhav Jain <vaibhav@linux.ibm.com> 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/

>  * "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.

>  * "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?

>  * "smart_notify": There is health event for the nvdimm.

Are you also going to signal the sysfs attribute when this event happens?

>  * "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?

>  * "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.


In any event, can you also add this content to a new
Documentation/ABI/testing/sysfs-bus-papr? See sysfs-bus-nfit for
comparison.

>
> [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)
> +/* 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.

> +
> +       /* 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...

> +
> +/* 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.

> +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
>
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

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

[Vaibhav Jain]

Hi Dan,

Thanks for reviewing this patchset,

Dan Williams <dan.j.williams@intel.com> writes:

> On Tue, Mar 31, 2020 at 7:33 AM Vaibhav Jain <vaibhav@linux.ibm.com> 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/20200221175459.255556-1-vaibhav@linux.ibm.com

>
>>  * "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 <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)
>> +/* 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 <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

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

[Michael Ellerman]

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 :)

> 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.

> @@ -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.

> +	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.

> +	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?

> +	/* 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.

> +
> +	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
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

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

[Vaibhav Jain]

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

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

[Michael Ellerman]

Vaibhav Jain <vajain21@vajain21.in.ibm.com.in.ibm.com> writes:
> Thanks for reviewing this patch Mpe,
> Michael Ellerman <ellerman@au1.ibm.com> writes:
>> Vaibhav Jain <vaibhav@linux.ibm.com> writes:
...
>>
>>> +	/* 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.

Send a patch? :)

cheers
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

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

[Dan Williams]

On Wed, Apr 1, 2020 at 8:08 PM Dan Williams <dan.j.williams@intel.com> wrote:
[..]
> >  * "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.

The more I think about this more I think this would be a good time to
introduce a common "health/" attribute group under the generic nmemX
sysfs, and then have one flag per-file / attribute. Not only does that
match the recommended sysfs ABI better, but it allows ndctl to
enumerate which flags are supported in addition to their state.

> In any event, can you also add this content to a new
> Documentation/ABI/testing/sysfs-bus-papr? See sysfs-bus-nfit for
> comparison.
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [PATCH v5 2/4] ndctl/uapi: Introduce NVDIMM_FAMILY_PAPR_SCM as a new NVDIMM DSM family

[Dan Williams]

On Tue, Mar 31, 2020 at 7:33 AM Vaibhav Jain <vaibhav@linux.ibm.com> wrote:
>
> Add PAPR-scm family of DSM command-set to the white list of NVDIMM
> command sets.
>
> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> ---
> Changelog:
>
> v4..v5 : None
>
> v3..v4 : None
>
> v2..v3 : Updated the patch prefix to 'ndctl/uapi' [Aneesh]
>
> v1..v2 : None
> ---
>  include/uapi/linux/ndctl.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h
> index de5d90212409..99fb60600ef8 100644
> --- a/include/uapi/linux/ndctl.h
> +++ b/include/uapi/linux/ndctl.h
> @@ -244,6 +244,7 @@ struct nd_cmd_pkg {
>  #define NVDIMM_FAMILY_HPE2 2
>  #define NVDIMM_FAMILY_MSFT 3
>  #define NVDIMM_FAMILY_HYPERV 4
> +#define NVDIMM_FAMILY_PAPR_SCM 5

Looks good, but please squash it with patch 3.
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [PATCH v5 3/4] powerpc/papr_scm,uapi: Add support for handling PAPR DSM commands

[Dan Williams]

On Tue, Mar 31, 2020 at 7:33 AM Vaibhav Jain <vaibhav@linux.ibm.com> wrote:
>
> Implement support for handling PAPR DSM commands in papr_scm
> module. We advertise support for ND_CMD_CALL for the dimm command mask
> and implement necessary scaffolding in the module to handle ND_CMD_CALL
> ioctl and DSM commands that we receive.

They aren't ACPI Device Specific Methods in the papr_scm case, right?
I'd call them what the papr_scm specification calls them and replace
"DSM" throughout.

> The layout of the DSM commands as we expect from libnvdimm/libndctl is
> described in newly introduced uapi header 'papr_scm_dsm.h' which
> defines a new 'struct nd_papr_scm_cmd_pkg' header. This header is used
> to communicate the DSM command via 'nd_pkg_papr_scm->nd_command' and
> size of payload that need to be sent/received for servicing the DSM.
>
> The PAPR DSM commands are assigned indexes started from 0x10000 to
> prevent them from overlapping ND_CMD_* values and also makes handling
> dimm commands in papr_scm_ndctl().

You don't necessarily need to have command number separation like
that. The function number spaces are unique per family.

> A new function cmd_to_func() is
> implemented that reads the args to papr_scm_ndctl() and performs
> sanity tests on them. In case of a DSM command being sent via
> ND_CMD_CALL a newly introduced function papr_scm_service_dsm() is
> called to handle the request.
>
> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
>
> ---
> Changelog:
>
> v4..v5: Fixed a bug in new implementation of papr_scm_ndctl().
>
> v3..v4: Updated papr_scm_ndctl() to delegate DSM command handling to a
>         different function papr_scm_service_dsm(). [Aneesh]
>
> v2..v3: Updated the nd_papr_scm_cmd_pkg to use __xx types as its
>         exported to the userspace [Aneesh]
>
> v1..v2: New patch in the series.
> ---
>  arch/powerpc/include/uapi/asm/papr_scm_dsm.h | 161 +++++++++++++++++++
>  arch/powerpc/platforms/pseries/papr_scm.c    |  97 ++++++++++-
>  2 files changed, 252 insertions(+), 6 deletions(-)
>  create mode 100644 arch/powerpc/include/uapi/asm/papr_scm_dsm.h
>
> diff --git a/arch/powerpc/include/uapi/asm/papr_scm_dsm.h b/arch/powerpc/include/uapi/asm/papr_scm_dsm.h
> new file mode 100644
> index 000000000000..c039a49b41b4
> --- /dev/null
> +++ b/arch/powerpc/include/uapi/asm/papr_scm_dsm.h
> @@ -0,0 +1,161 @@
> +/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
> +/*
> + * PAPR SCM Device specific methods and struct for libndctl and ndctl
> + *
> + * (C) Copyright IBM 2020
> + *
> + * Author: Vaibhav Jain <vaibhav at linux.ibm.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2, or (at your option)
> + * any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.

These 2 paragraphs of redundant license text can be dropped. The SPDX
line is sufficient.

> + */
> +
> +#ifndef _UAPI_ASM_POWERPC_PAPR_SCM_DSM_H_
> +#define _UAPI_ASM_POWERPC_PAPR_SCM_DSM_H_
> +
> +#include <linux/types.h>
> +
> +#ifdef __KERNEL__
> +#include <linux/ndctl.h>
> +#else
> +#include <ndctl.h>
> +#endif
> +
> +/*
> + * DSM Envelope:
> + *
> + * The ioctl ND_CMD_CALL transfers data between user-space and kernel via
> + * 'envelopes' which consists of a header and user-defined payload sections.
> + * The header is described by 'struct nd_papr_scm_cmd_pkg' which expects a
> + * payload following it and offset of which relative to the struct is provided
> + * by 'nd_papr_scm_cmd_pkg.payload_offset'. *
> + *
> + *  +-------------+---------------------+---------------------------+
> + *  |   64-Bytes  |       8-Bytes       |       Max 184-Bytes       |
> + *  +-------------+---------------------+---------------------------+
> + *  |               nd_papr_scm_cmd_pkg |                           |
> + *  |-------------+                     |                           |
> + *  |  nd_cmd_pkg |                     |                           |
> + *  +-------------+---------------------+---------------------------+
> + *  | nd_family   |                    |                           |
> + *  | nd_size_out | cmd_status          |                          |
> + *  | nd_size_in  | payload_version     |      PAYLOAD             |
> + *  | nd_command  | payload_offset ----->                          |
> + *  | nd_fw_size  |                     |                          |
> + *  +-------------+---------------------+---------------------------+
> + *
> + * DSM Header:
> + *
> + * The header is defined as 'struct nd_papr_scm_cmd_pkg' which embeds a
> + * 'struct nd_cmd_pkg' instance. The DSM command is assigned to member
> + * 'nd_cmd_pkg.nd_command'. Apart from size information of the envelop which is

 s/envelop/envelope/

There's a difference between commands and functions in libnvdimm.
Commands are the top-level ioctl methods where ND_CMD_CALL is the
generic payload command and functions are communicated inside of that
wrapper. Otherwise if the command does not wrap other commands like
ND_CMD_{GET,SET}_CONFIG_DATA then the individual bus providers
translate them to the bus-local functionality. For example
ND_CMD_{GET,SET}_CONFIG_DATA correspond to either the _LSx ACPI
methods, or the Intel DSM label methods. The details of those
implementations are hidden from the caller, the caller just sees the
kernel's ND_CMD_{GET,SET}_CONFIG_DATA interface.

> + * contained in 'struct nd_cmd_pkg', the header also has members following
> + * members:
> + *
> + * 'cmd_status'                : (Out) Errors if any encountered while servicing DSM.
> + * 'payload_version'   : (In/Out) Version number associated with the payload.
> + * 'payload_offset'    : (In)Relative offset of payload from start of envelope.
> + *
> + * DSM Payload:
> + *
> + * The layout of the DSM Payload is defined by various structs shared between
> + * papr_scm and libndctl so that contents of payload can be interpreted. During
> + * servicing of a DSM the papr_scm module will read input args from the payload
> + * field by casting its contents to an appropriate struct pointer based on the
> + * DSM command. Similarly the output of servicing the DSM command will be copied
> + * to the payload field using the same struct.
> + *
> + * 'libnvdimm' enforces a hard limit of 256 bytes on the envelope size, which
> + * leaves around 184 bytes for the envelope payload (ignoring any padding that
> + * the compiler may silently introduce).
> + *
> + * Payload Version:
> + *
> + * A 'payload_version' field is present in DSM header that indicates a specific
> + * version of the structure present in DSM Payload for a given DSM command. This
> + * provides backward compatibility in case the DSM Payload structure evolves
> + * and different structures are supported by 'papr_scm' and 'libndctl'.
> + *
> + * When sending a DSM Payload to 'papr_scm', 'libndctl' should send the version
> + * of the payload struct it supports via 'payload_version' field. The 'papr_scm'
> + * module when servicing the DSM envelop checks the 'payload_version' and then

s/envelop/envelope/

> + * uses 'payload struct version' == MIN('payload_version field',
> + * 'max payload-struct-version supported by papr_scm') to service the DSM. After
> + * servicing the DSM, 'papr_scm' put the negotiated version of payload struct in
> + * returned 'payload_version' field.
> + *
> + * Libndctl on receiving the envelop back from papr_scm again checks the

s/envelop/envelope/

> + * 'payload_version' field and based on it use the appropriate version dsm
> + * struct to parse the results.
> + *
> + * Backward Compatibility:
> + *
> + * Above scheme of exchanging different versioned DSM struct between libndctl
> + * and papr_scm should provide backward compatibility until following two
> + * assumptions/conditions when defining new DSM structs hold:
> + *
> + * Let T(X) = { set of attributes in DSM struct 'T' versioned X }
> + *
> + * 1. T(X) is a proper subset of T(Y) if X > Y.
> + *    i.e Each new version of DSM struct should retain existing struct
> + *    attributes from previous version
> + *
> + * 2. If an entity (libndctl or papr_scm) supports a DSM struct T(X) then
> + *    it should also support T(1), T(2)...T(X - 1).
> + *    i.e When adding support for new version of a DSM struct, libndctl
> + *    and papr_scm should retain support of the existing DSM struct
> + *    version they support.
> + */
> +
> +/* Papr-scm-header + payload expected with ND_CMD_CALL ioctl from libnvdimm */
> +struct nd_papr_scm_cmd_pkg {
> +       struct nd_cmd_pkg hdr;          /* Package header containing sub-cmd */
> +       __s32 cmd_status;               /* Out: Sub-cmd status returned back */
> +       __u16 payload_offset;   /* In: offset from start of struct */
> +       __u16 payload_version;  /* In/Out: version of the payload */
> +       __u8 payload[];         /* In/Out: Sub-cmd data buffer */

Does this structure pack the same on all compilers?

> +};
> +
> +/*
> + * Sub commands for ND_CMD_CALL. To prevent overlap from ND_CMD_*, values for
> + * these enums start at 0x10000. These values are then returned from
> + * cmd_to_func() making it easy to implement the switch-case block in
> + * papr_scm_ndctl(). These commands are sent to the kernel via
> + * 'nd_papr_scm_cmd_pkg.hdr.nd_command'
> + */

With nd_family set to NVDIMM_FAMILY_PAPR_SCM the function number space
can overlap the function numbers from other families.

> +enum dsm_papr_scm {
> +       DSM_PAPR_SCM_MIN =  0x10000,
> +       DSM_PAPR_SCM_MAX,
> +};
> +
> +/* Helpers to evaluate the size of PAPR_SCM envelope */
> +/* Calculate the papr_scm-header size */
> +#define ND_PAPR_SCM_ENVELOPE_CONTENT_HDR_SIZE \
> +       (sizeof(struct nd_papr_scm_cmd_pkg) - sizeof(struct nd_cmd_pkg))
> +
> +/* Given a type calculate envelope-content size (papr_scm-header + payload) */
> +#define ND_PAPR_SCM_ENVELOPE_CONTENT_SIZE(_type_)      \
> +       (sizeof(_type_) + ND_PAPR_SCM_ENVELOPE_CONTENT_HDR_SIZE)
> +
> +/* Convert a libnvdimm nd_cmd_pkg to papr_scm specific pkg */
> +static struct nd_papr_scm_cmd_pkg *nd_to_papr_cmd_pkg(struct nd_cmd_pkg *cmd)
> +{
> +       return (struct nd_papr_scm_cmd_pkg *) cmd;
> +}
> +
> +/* Return the payload pointer for a given pcmd */
> +static void *papr_scm_pcmd_to_payload(struct nd_papr_scm_cmd_pkg *pcmd)
> +{
> +       if (pcmd->hdr.nd_size_in == 0 && pcmd->hdr.nd_size_out == 0)
> +               return NULL;
> +       else
> +               return (void *)((__u8 *) pcmd + pcmd->payload_offset);
> +}
> +#endif /* _UAPI_ASM_POWERPC_PAPR_SCM_DSM_H_ */
> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> index aaf2e4ab1f75..e8ce96d2249e 100644
> --- a/arch/powerpc/platforms/pseries/papr_scm.c
> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> @@ -15,13 +15,15 @@
>
>  #include <asm/plpar_wrappers.h>
>  #include <asm/papr_scm.h>
> +#include <asm/papr_scm_dsm.h>
>
>  #define BIND_ANY_ADDR (~0ul)
>
>  #define PAPR_SCM_DIMM_CMD_MASK \
>         ((1ul << ND_CMD_GET_CONFIG_SIZE) | \
>          (1ul << ND_CMD_GET_CONFIG_DATA) | \
> -        (1ul << ND_CMD_SET_CONFIG_DATA))
> +        (1ul << ND_CMD_SET_CONFIG_DATA) | \
> +        (1ul << ND_CMD_CALL))
>
>  struct papr_scm_priv {
>         struct platform_device *pdev;
> @@ -283,15 +285,92 @@ static int papr_scm_meta_set(struct papr_scm_priv *p,
>         return 0;
>  }
>
> +/*
> + * Validate the inputs args to dimm-control function and return '0' if valid.
> + * This also does initial sanity validation to ND_CMD_CALL sub-command packages.
> + */
> +static int is_cmd_valid(struct nvdimm *nvdimm, unsigned int cmd, void *buf,
> +                      unsigned int buf_len)
> +{
> +       unsigned long cmd_mask = PAPR_SCM_DIMM_CMD_MASK;
> +       struct nd_papr_scm_cmd_pkg *pkg = nd_to_papr_cmd_pkg(buf);
> +
> +       /* Only dimm-specific calls are supported atm */
> +       if (!nvdimm)
> +               return -EINVAL;
> +
> +       if (!test_bit(cmd, &cmd_mask)) {
> +               pr_debug("%s: Unsupported cmd=%u\n", __func__, cmd);

Please convert all the pr_debug() statements in this file to
dev_dbg(). Also you don't need to put the __func__ in the print
statement because you can enable that via dynamic_debug_control see
the "f" flag in addition to the "p" flag for dynamic debug enabling.

> +               return -EINVAL;
> +       } else if (cmd == ND_CMD_CALL) {
> +
> +               /* Verify the envelop package */

s/envelop/envelope/

> +               if (!buf || buf_len < sizeof(struct nd_papr_scm_cmd_pkg)) {
> +                       pr_debug("%s: Invalid pkg size=%u\n", __func__,
> +                                buf_len);
> +                       return -EINVAL;
> +               }
> +
> +               /* Verify that the DSM command family is valid */
> +               if (pkg->hdr.nd_family != NVDIMM_FAMILY_PAPR_SCM) {
> +                       pr_debug("%s: Invalid pkg family=0x%llx\n", __func__,
> +                                pkg->hdr.nd_family);
> +                       return -EINVAL;
> +
> +               }
> +
> +               /* We except a payload with all DSM commands */
> +               if (papr_scm_pcmd_to_payload(pkg) == NULL) {
> +                       pr_debug("%s: Empty payload for sub-command=0x%llx\n",
> +                                __func__, pkg->hdr.nd_command);
> +                       return -EINVAL;
> +               }
> +       }
> +
> +       /* Command looks valid */
> +       return 0;
> +}
> +
> +static int papr_scm_service_dsm(struct papr_scm_priv *p,
> +                               struct nd_papr_scm_cmd_pkg *call_pkg)
> +{
> +       /* unknown subcommands return error in packages */
> +       if (call_pkg->hdr.nd_command <= DSM_PAPR_SCM_MIN ||
> +           call_pkg->hdr.nd_command >= DSM_PAPR_SCM_MAX) {
> +               pr_debug("Invalid DSM command 0x%llx\n",
> +                        call_pkg->hdr.nd_command);
> +               call_pkg->cmd_status = -EINVAL;
> +               return 0;
> +       }
> +
> +       /* Depending on the DSM command call appropriate service routine */
> +       switch (call_pkg->hdr.nd_command) {
> +       default:
> +               pr_debug("Unsupported DSM command 0x%llx\n",
> +                        call_pkg->hdr.nd_command);
> +               call_pkg->cmd_status = -ENOENT;
> +               return 0;
> +       }
> +}
> +
>  int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
>                 unsigned int cmd, void *buf, unsigned int buf_len, int *cmd_rc)
>  {
>         struct nd_cmd_get_config_size *get_size_hdr;
>         struct papr_scm_priv *p;
> +       struct nd_papr_scm_cmd_pkg *call_pkg = NULL;
> +       int rc;
>
> -       /* Only dimm-specific calls are supported atm */
> -       if (!nvdimm)
> -               return -EINVAL;
> +       /* Use a local variable in case cmd_rc pointer is NULL */
> +       if (cmd_rc == NULL)
> +               cmd_rc = &rc;
> +
> +       *cmd_rc = is_cmd_valid(nvdimm, cmd, buf, buf_len);
> +       if (*cmd_rc) {
> +               pr_debug("%s: Invalid cmd=0x%x. Err=%d\n", __func__,
> +                        cmd, *cmd_rc);
> +               return *cmd_rc;
> +       }
>
>         p = nvdimm_provider_data(nvdimm);
>
> @@ -313,13 +392,19 @@ int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
>                 *cmd_rc = papr_scm_meta_set(p, buf);
>                 break;
>
> +       case ND_CMD_CALL:
> +               call_pkg = nd_to_papr_cmd_pkg(buf);
> +               *cmd_rc = papr_scm_service_dsm(p, call_pkg);
> +               break;
> +
>         default:
> -               return -EINVAL;
> +               dev_dbg(&p->pdev->dev, "Unknown command = %d\n", cmd);
> +               *cmd_rc = -EINVAL;
>         }
>
>         dev_dbg(&p->pdev->dev, "returned with cmd_rc = %d\n", *cmd_rc);
>
> -       return 0;
> +       return *cmd_rc;
>  }
>
>  static inline int papr_scm_node(int node)
> --
> 2.25.1
>
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [PATCH v5 4/4] powerpc/papr_scm: Implement support for DSM_PAPR_SCM_HEALTH

[Dan Williams]

On Tue, Mar 31, 2020 at 7:33 AM Vaibhav Jain <vaibhav@linux.ibm.com> wrote:
>
> This patch implements support for papr_scm command
> 'DSM_PAPR_SCM_HEALTH' that returns a newly introduced 'struct
> nd_papr_scm_dimm_health_stat' instance containing dimm health
> information back to user space in response to ND_CMD_CALL. This
> functionality is implemented in newly introduced papr_scm_get_health()
> that queries the scm-dimm health information and then copies these bitmaps
> to the package payload whose layout is defined by 'struct
> papr_scm_ndctl_health'.
>
> The patch also introduces a new member a new member 'struct
> papr_scm_priv.health' thats an instance of 'struct
> nd_papr_scm_dimm_health_stat' to cache the health information of a
> scm-dimm. As a result functions drc_pmem_query_health() and
> papr_flags_show() are updated to populate and use this new struct
> instead of two be64 integers that we earlier used.

Link to HCALL specification?

>
> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> ---
> Changelog:
>
> v4..v5: None
>
> v3..v4: Call the DSM_PAPR_SCM_HEALTH service function from
>         papr_scm_service_dsm() instead of papr_scm_ndctl(). [Aneesh]
>
> v2..v3: Updated struct nd_papr_scm_dimm_health_stat_v1 to use '__xx'
>         types as its exported to the userspace [Aneesh]
>         Changed the constants DSM_PAPR_SCM_DIMM_XX indicating dimm
>         health from enum to #defines [Aneesh]
>
> v1..v2: New patch in the series
> ---
>  arch/powerpc/include/uapi/asm/papr_scm_dsm.h |  40 +++++++
>  arch/powerpc/platforms/pseries/papr_scm.c    | 109 ++++++++++++++++---
>  2 files changed, 132 insertions(+), 17 deletions(-)
>
> diff --git a/arch/powerpc/include/uapi/asm/papr_scm_dsm.h b/arch/powerpc/include/uapi/asm/papr_scm_dsm.h
> index c039a49b41b4..8265125304ca 100644
> --- a/arch/powerpc/include/uapi/asm/papr_scm_dsm.h
> +++ b/arch/powerpc/include/uapi/asm/papr_scm_dsm.h
> @@ -132,6 +132,7 @@ struct nd_papr_scm_cmd_pkg {
>   */
>  enum dsm_papr_scm {
>         DSM_PAPR_SCM_MIN =  0x10000,
> +       DSM_PAPR_SCM_HEALTH,
>         DSM_PAPR_SCM_MAX,
>  };
>
> @@ -158,4 +159,43 @@ static void *papr_scm_pcmd_to_payload(struct nd_papr_scm_cmd_pkg *pcmd)
>         else
>                 return (void *)((__u8 *) pcmd + pcmd->payload_offset);
>  }
> +
> +/* Various scm-dimm health indicators */
> +#define DSM_PAPR_SCM_DIMM_HEALTHY       0
> +#define DSM_PAPR_SCM_DIMM_UNHEALTHY     1
> +#define DSM_PAPR_SCM_DIMM_CRITICAL      2
> +#define DSM_PAPR_SCM_DIMM_FATAL         3
> +
> +/*
> + * Struct exchanged between kernel & ndctl in for PAPR_DSM_PAPR_SMART_HEALTH
> + * Various bitflags indicate the health status of the dimm.
> + *
> + * dimm_unarmed                : Dimm not armed. So contents wont persist.
> + * dimm_bad_shutdown   : Previous shutdown did not persist contents.
> + * dimm_bad_restore    : Contents from previous shutdown werent restored.
> + * dimm_scrubbed       : Contents of the dimm have been scrubbed.
> + * dimm_locked         : Contents of the dimm cant be modified until CEC reboot
> + * dimm_encrypted      : Contents of dimm are encrypted.
> + * dimm_health         : Dimm health indicator.
> + */
> +struct nd_papr_scm_dimm_health_stat_v1 {
> +       __u8 dimm_unarmed;
> +       __u8 dimm_bad_shutdown;
> +       __u8 dimm_bad_restore;
> +       __u8 dimm_scrubbed;
> +       __u8 dimm_locked;
> +       __u8 dimm_encrypted;
> +       __u16 dimm_health;
> +};

Does the structure pack the same across different compilers and configurations?

> +
> +/*
> + * Typedef the current struct for dimm_health so that any application
> + * or kernel recompiled after introducing a new version automatically
> + * supports the new version.
> + */
> +#define nd_papr_scm_dimm_health_stat nd_papr_scm_dimm_health_stat_v1
> +
> +/* Current version number for the dimm health struct */
> +#define ND_PAPR_SCM_DIMM_HEALTH_VERSION 1
> +
>  #endif /* _UAPI_ASM_POWERPC_PAPR_SCM_DSM_H_ */
> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> index e8ce96d2249e..ce94762954e0 100644
> --- a/arch/powerpc/platforms/pseries/papr_scm.c
> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> @@ -47,8 +47,7 @@ struct papr_scm_priv {
>         struct mutex dimm_mutex;
>
>         /* Health information for the dimm */
> -       __be64 health_bitmap;
> -       __be64 health_bitmap_valid;
> +       struct nd_papr_scm_dimm_health_stat health;
>  };
>
>  static int drc_pmem_bind(struct papr_scm_priv *p)
> @@ -158,6 +157,7 @@ static int drc_pmem_query_health(struct papr_scm_priv *p)
>  {
>         unsigned long ret[PLPAR_HCALL_BUFSIZE];
>         int64_t rc;
> +       __be64 health;
>
>         rc = plpar_hcall(H_SCM_HEALTH, ret, p->drc_index);
>         if (rc != H_SUCCESS) {
> @@ -172,13 +172,41 @@ static int drc_pmem_query_health(struct papr_scm_priv *p)
>                 return rc;
>
>         /* Store the retrieved health information in dimm platform data */
> -       p->health_bitmap = ret[0];
> -       p->health_bitmap_valid = ret[1];
> +       health = ret[0] & 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));
> +               be64_to_cpu(ret[0]),
> +               be64_to_cpu(ret[1]));
> +
> +       memset(&p->health, 0, sizeof(p->health));
> +
> +       /* Check for various masks in bitmap and set the buffer */
> +       if (health & PAPR_SCM_DIMM_UNARMED_MASK)
> +               p->health.dimm_unarmed = true;
> +
> +       if (health & PAPR_SCM_DIMM_BAD_SHUTDOWN_MASK)
> +               p->health.dimm_bad_shutdown = true;
> +
> +       if (health & PAPR_SCM_DIMM_BAD_RESTORE_MASK)
> +               p->health.dimm_bad_restore = true;
> +
> +       if (health & PAPR_SCM_DIMM_ENCRYPTED)
> +               p->health.dimm_encrypted = true;
> +
> +       if (health & PAPR_SCM_DIMM_SCRUBBED_AND_LOCKED) {
> +               p->health.dimm_locked = true;
> +               p->health.dimm_scrubbed = true;
> +       }

I don't think bool is suitable for ioctl ABI. For example the true
value may be positive, or negative depending on the arch. This should
be using explicit integer values.

I assume the reason you are translating this rather than transmitting
that raw 64-bit health value is for future compatibility if the HCALL
format changes / is extended?

> +
> +       if (health & PAPR_SCM_DIMM_HEALTH_UNHEALTHY)
> +               p->health.dimm_health = DSM_PAPR_SCM_DIMM_UNHEALTHY;
> +
> +       if (health & PAPR_SCM_DIMM_HEALTH_CRITICAL)
> +               p->health.dimm_health = DSM_PAPR_SCM_DIMM_CRITICAL;
> +
> +       if (health & PAPR_SCM_DIMM_HEALTH_FATAL)
> +               p->health.dimm_health = DSM_PAPR_SCM_DIMM_FATAL;

>
>         mutex_unlock(&p->dimm_mutex);
>         return 0;
> @@ -331,6 +359,51 @@ static int is_cmd_valid(struct nvdimm *nvdimm, unsigned int cmd, void *buf,
>         return 0;
>  }
>
> +/* Fetch the DIMM health info and populate it in provided package. */
> +static int papr_scm_get_health(struct papr_scm_priv *p,
> +                              struct nd_papr_scm_cmd_pkg *pkg)
> +{
> +       int rc;
> +       size_t copysize = sizeof(p->health);
> +
> +       rc = drc_pmem_query_health(p);
> +       if (rc)
> +               goto out;
> +       /*
> +        * If the requested payload version is greater than one we know
> +        * about, return the payload version we know about and let
> +        * caller/userspace handle.
> +        */
> +       if (pkg->payload_version > ND_PAPR_SCM_DIMM_HEALTH_VERSION)
> +               pkg->payload_version = ND_PAPR_SCM_DIMM_HEALTH_VERSION;
> +
> +       if (pkg->hdr.nd_size_out < copysize) {
> +               dev_dbg(&p->pdev->dev, "%s Payload not large enough\n",
> +                       __func__);
> +               dev_dbg(&p->pdev->dev, "%s Expected %lu, available %u\n",
> +                       __func__, copysize, pkg->hdr.nd_size_out);
> +               rc = -ENOSPC;
> +               goto out;
> +       }
> +
> +       dev_dbg(&p->pdev->dev, "%s Copying payload size=%lu version=0x%x\n",
> +               __func__, copysize, pkg->payload_version);
> +
> +       /* Copy a subset of health struct based on copysize */
> +       memcpy(papr_scm_pcmd_to_payload(pkg), &p->health, copysize);
> +       pkg->hdr.nd_fw_size = copysize;
> +
> +out:
> +       /*
> +        * Put the error in out package and return success from function
> +        * so that errors if any are propogated back to userspace.
> +        */
> +       pkg->cmd_status = rc;
> +       dev_dbg(&p->pdev->dev, "%s completion code = %d\n", __func__, rc);
> +
> +       return 0;
> +}
> +
>  static int papr_scm_service_dsm(struct papr_scm_priv *p,
>                                 struct nd_papr_scm_cmd_pkg *call_pkg)
>  {
> @@ -345,6 +418,9 @@ static int papr_scm_service_dsm(struct papr_scm_priv *p,
>
>         /* Depending on the DSM command call appropriate service routine */
>         switch (call_pkg->hdr.nd_command) {
> +       case DSM_PAPR_SCM_HEALTH:
> +               return papr_scm_get_health(p, call_pkg);
> +
>         default:
>                 pr_debug("Unsupported DSM command 0x%llx\n",
>                          call_pkg->hdr.nd_command);
> @@ -431,7 +507,6 @@ static ssize_t papr_flags_show(struct device *dev,
>  {
>         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);
> @@ -443,26 +518,26 @@ static ssize_t papr_flags_show(struct device *dev,
>         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)
> +       if (p->health.dimm_unarmed)
>                 rc += sprintf(buf, "not_armed ");
>
> -       if (health & PAPR_SCM_DIMM_BAD_SHUTDOWN_MASK)
> +       if (p->health.dimm_bad_shutdown)
>                 rc += sprintf(buf + rc, "save_fail ");

Per my patch1 comment is this "save_fail" or "flush_fail"?

>
> -       if (health & PAPR_SCM_DIMM_BAD_RESTORE_MASK)
> +       if (p->health.dimm_bad_restore)
>                 rc += sprintf(buf + rc, "restore_fail ");
>
> -       if (health & PAPR_SCM_DIMM_ENCRYPTED)
> +       if (p->health.dimm_encrypted)
>                 rc += sprintf(buf + rc, "encrypted ");
>
> -       if (health & PAPR_SCM_DIMM_SMART_EVENT_MASK)
> +       if (p->health.dimm_health)
>                 rc += sprintf(buf + rc, "smart_notify ");
>
> -       if (health & PAPR_SCM_DIMM_SCRUBBED_AND_LOCKED)
> -               rc += sprintf(buf + rc, "scrubbed locked ");
> +       if (p->health.dimm_scrubbed)
> +               rc += sprintf(buf + rc, "scrubbed ");
> +
> +       if (p->health.dimm_locked)
> +               rc += sprintf(buf + rc, "locked ");
>
>         if (rc > 0)
>                 rc += sprintf(buf + rc, "\n");
> --
> 2.25.1
>
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [PATCH v5 3/4] powerpc/papr_scm,uapi: Add support for handling PAPR DSM commands

[Vaibhav Jain]

Dan Williams <dan.j.williams@intel.com> writes:

> On Tue, Mar 31, 2020 at 7:33 AM Vaibhav Jain <vaibhav@linux.ibm.com> wrote:
>>
>> Implement support for handling PAPR DSM commands in papr_scm
>> module. We advertise support for ND_CMD_CALL for the dimm command mask
>> and implement necessary scaffolding in the module to handle ND_CMD_CALL
>> ioctl and DSM commands that we receive.
>
> They aren't ACPI Device Specific Methods in the papr_scm case, right?
> I'd call them what the papr_scm specification calls them and replace
> "DSM" throughout.
Right, papr_scm doesnt use ACPI. However there is no pre-existing term
for these methods as they are defined by us. Hence we reused the "DSM"
to indicate meaning similar to what ACPI does.

The closest term in papr_scm spec is Hcall thats a system-call to
hypervisor to fetch some data. But there is no 1:1 mapping between HCall
and these "DSMs". 

>
>> The layout of the DSM commands as we expect from libnvdimm/libndctl is
>> described in newly introduced uapi header 'papr_scm_dsm.h' which
>> defines a new 'struct nd_papr_scm_cmd_pkg' header. This header is used
>> to communicate the DSM command via 'nd_pkg_papr_scm->nd_command' and
>> size of payload that need to be sent/received for servicing the DSM.
>>
>> The PAPR DSM commands are assigned indexes started from 0x10000 to
>> prevent them from overlapping ND_CMD_* values and also makes handling
>> dimm commands in papr_scm_ndctl().
>
> You don't necessarily need to have command number separation like
> that. The function number spaces are unique per family.
>
Right. This is a carry over from previous version of this patchset that
tried using ND_CMD_& and PAPR_DSM_* in a single switch-case
block. However not needed anymore, hence I will switch to values
starting from 0x0 in next iteration.

>> A new function cmd_to_func() is
>> implemented that reads the args to papr_scm_ndctl() and performs
>> sanity tests on them. In case of a DSM command being sent via
>> ND_CMD_CALL a newly introduced function papr_scm_service_dsm() is
>> called to handle the request.
>>
>> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
>>
>> ---
>> Changelog:
>>
>> v4..v5: Fixed a bug in new implementation of papr_scm_ndctl().
>>
>> v3..v4: Updated papr_scm_ndctl() to delegate DSM command handling to a
>>         different function papr_scm_service_dsm(). [Aneesh]
>>
>> v2..v3: Updated the nd_papr_scm_cmd_pkg to use __xx types as its
>>         exported to the userspace [Aneesh]
>>
>> v1..v2: New patch in the series.
>> ---
>>  arch/powerpc/include/uapi/asm/papr_scm_dsm.h | 161 +++++++++++++++++++
>>  arch/powerpc/platforms/pseries/papr_scm.c    |  97 ++++++++++-
>>  2 files changed, 252 insertions(+), 6 deletions(-)
>>  create mode 100644 arch/powerpc/include/uapi/asm/papr_scm_dsm.h
>>
>> diff --git a/arch/powerpc/include/uapi/asm/papr_scm_dsm.h b/arch/powerpc/include/uapi/asm/papr_scm_dsm.h
>> new file mode 100644
>> index 000000000000..c039a49b41b4
>> --- /dev/null
>> +++ b/arch/powerpc/include/uapi/asm/papr_scm_dsm.h
>> @@ -0,0 +1,161 @@
>> +/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
>> +/*
>> + * PAPR SCM Device specific methods and struct for libndctl and ndctl
>> + *
>> + * (C) Copyright IBM 2020
>> + *
>> + * Author: Vaibhav Jain <vaibhav at linux.ibm.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2, or (at your option)
>> + * any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>
> These 2 paragraphs of redundant license text can be dropped. The SPDX
> line is sufficient.
Agree, will address this in next iteration.

>
>> + */
>> +
>> +#ifndef _UAPI_ASM_POWERPC_PAPR_SCM_DSM_H_
>> +#define _UAPI_ASM_POWERPC_PAPR_SCM_DSM_H_
>> +
>> +#include <linux/types.h>
>> +
>> +#ifdef __KERNEL__
>> +#include <linux/ndctl.h>
>> +#else
>> +#include <ndctl.h>
>> +#endif
>> +
>> +/*
>> + * DSM Envelope:
>> + *
>> + * The ioctl ND_CMD_CALL transfers data between user-space and kernel via
>> + * 'envelopes' which consists of a header and user-defined payload sections.
>> + * The header is described by 'struct nd_papr_scm_cmd_pkg' which expects a
>> + * payload following it and offset of which relative to the struct is provided
>> + * by 'nd_papr_scm_cmd_pkg.payload_offset'. *
>> + *
>> + *  +-------------+---------------------+---------------------------+
>> + *  |   64-Bytes  |       8-Bytes       |       Max 184-Bytes       |
>> + *  +-------------+---------------------+---------------------------+
>> + *  |               nd_papr_scm_cmd_pkg |                           |
>> + *  |-------------+                     |                           |
>> + *  |  nd_cmd_pkg |                     |                           |
>> + *  +-------------+---------------------+---------------------------+
>> + *  | nd_family   |                    |                           |
>> + *  | nd_size_out | cmd_status          |                          |
>> + *  | nd_size_in  | payload_version     |      PAYLOAD             |
>> + *  | nd_command  | payload_offset ----->                          |
>> + *  | nd_fw_size  |                     |                          |
>> + *  +-------------+---------------------+---------------------------+
>> + *
>> + * DSM Header:
>> + *
>> + * The header is defined as 'struct nd_papr_scm_cmd_pkg' which embeds a
>> + * 'struct nd_cmd_pkg' instance. The DSM command is assigned to member
>> + * 'nd_cmd_pkg.nd_command'. Apart from size information of the envelop which is
>
>  s/envelop/envelope/
Agree, will fix this and other instances of the same error

>
> There's a difference between commands and functions in libnvdimm.
> Commands are the top-level ioctl methods where ND_CMD_CALL is the
> generic payload command and functions are communicated inside of that
> wrapper. Otherwise if the command does not wrap other commands like
> ND_CMD_{GET,SET}_CONFIG_DATA then the individual bus providers
> translate them to the bus-local functionality. For example
> ND_CMD_{GET,SET}_CONFIG_DATA correspond to either the _LSx ACPI
> methods, or the Intel DSM label methods. The details of those
> implementations are hidden from the caller, the caller just sees the
> kernel's ND_CMD_{GET,SET}_CONFIG_DATA interface.
>
>> + * contained in 'struct nd_cmd_pkg', the header also has members following
>> + * members:
>> + *
>> + * 'cmd_status'                : (Out) Errors if any encountered while servicing DSM.
>> + * 'payload_version'   : (In/Out) Version number associated with the payload.
>> + * 'payload_offset'    : (In)Relative offset of payload from start of envelope.
>> + *
>> + * DSM Payload:
>> + *
>> + * The layout of the DSM Payload is defined by various structs shared between
>> + * papr_scm and libndctl so that contents of payload can be interpreted. During
>> + * servicing of a DSM the papr_scm module will read input args from the payload
>> + * field by casting its contents to an appropriate struct pointer based on the
>> + * DSM command. Similarly the output of servicing the DSM command will be copied
>> + * to the payload field using the same struct.
>> + *
>> + * 'libnvdimm' enforces a hard limit of 256 bytes on the envelope size, which
>> + * leaves around 184 bytes for the envelope payload (ignoring any padding that
>> + * the compiler may silently introduce).
>> + *
>> + * Payload Version:
>> + *
>> + * A 'payload_version' field is present in DSM header that indicates a specific
>> + * version of the structure present in DSM Payload for a given DSM command. This
>> + * provides backward compatibility in case the DSM Payload structure evolves
>> + * and different structures are supported by 'papr_scm' and 'libndctl'.
>> + *
>> + * When sending a DSM Payload to 'papr_scm', 'libndctl' should send the version
>> + * of the payload struct it supports via 'payload_version' field. The 'papr_scm'
>> + * module when servicing the DSM envelop checks the 'payload_version' and then
>
> s/envelop/envelope/
>
>> + * uses 'payload struct version' == MIN('payload_version field',
>> + * 'max payload-struct-version supported by papr_scm') to service the DSM. After
>> + * servicing the DSM, 'papr_scm' put the negotiated version of payload struct in
>> + * returned 'payload_version' field.
>> + *
>> + * Libndctl on receiving the envelop back from papr_scm again checks the
>
> s/envelop/envelope/
>
>> + * 'payload_version' field and based on it use the appropriate version dsm
>> + * struct to parse the results.
>> + *
>> + * Backward Compatibility:
>> + *
>> + * Above scheme of exchanging different versioned DSM struct between libndctl
>> + * and papr_scm should provide backward compatibility until following two
>> + * assumptions/conditions when defining new DSM structs hold:
>> + *
>> + * Let T(X) = { set of attributes in DSM struct 'T' versioned X }
>> + *
>> + * 1. T(X) is a proper subset of T(Y) if X > Y.
>> + *    i.e Each new version of DSM struct should retain existing struct
>> + *    attributes from previous version
>> + *
>> + * 2. If an entity (libndctl or papr_scm) supports a DSM struct T(X) then
>> + *    it should also support T(1), T(2)...T(X - 1).
>> + *    i.e When adding support for new version of a DSM struct, libndctl
>> + *    and papr_scm should retain support of the existing DSM struct
>> + *    version they support.
>> + */
>> +
>> +/* Papr-scm-header + payload expected with ND_CMD_CALL ioctl from libnvdimm */
>> +struct nd_papr_scm_cmd_pkg {
>> +       struct nd_cmd_pkg hdr;          /* Package header containing sub-cmd */
>> +       __s32 cmd_status;               /* Out: Sub-cmd status returned back */
>> +       __u16 payload_offset;   /* In: offset from start of struct */
>> +       __u16 payload_version;  /* In/Out: version of the payload */
>> +       __u8 payload[];         /* In/Out: Sub-cmd data buffer */
>
> Does this structure pack the same on all compilers?
I tried with both gcc-8.2 and clang-9.0 and pahole reported the same
struct layout. 

>
>> +};
>> +
>> +/*
>> + * Sub commands for ND_CMD_CALL. To prevent overlap from ND_CMD_*, values for
>> + * these enums start at 0x10000. These values are then returned from
>> + * cmd_to_func() making it easy to implement the switch-case block in
>> + * papr_scm_ndctl(). These commands are sent to the kernel via
>> + * 'nd_papr_scm_cmd_pkg.hdr.nd_command'
>> + */
>
> With nd_family set to NVDIMM_FAMILY_PAPR_SCM the function number space
> can overlap the function numbers from other families.
Agree, will reset DSM_PAPR_SCM_MIN = 0 in next iteration.
>
>> +enum dsm_papr_scm {
>> +       DSM_PAPR_SCM_MIN =  0x10000,
>> +       DSM_PAPR_SCM_MAX,
>> +};
>> +
>> +/* Helpers to evaluate the size of PAPR_SCM envelope */
>> +/* Calculate the papr_scm-header size */
>> +#define ND_PAPR_SCM_ENVELOPE_CONTENT_HDR_SIZE \
>> +       (sizeof(struct nd_papr_scm_cmd_pkg) - sizeof(struct nd_cmd_pkg))
>> +
>> +/* Given a type calculate envelope-content size (papr_scm-header + payload) */
>> +#define ND_PAPR_SCM_ENVELOPE_CONTENT_SIZE(_type_)      \
>> +       (sizeof(_type_) + ND_PAPR_SCM_ENVELOPE_CONTENT_HDR_SIZE)
>> +
>> +/* Convert a libnvdimm nd_cmd_pkg to papr_scm specific pkg */
>> +static struct nd_papr_scm_cmd_pkg *nd_to_papr_cmd_pkg(struct nd_cmd_pkg *cmd)
>> +{
>> +       return (struct nd_papr_scm_cmd_pkg *) cmd;
>> +}
>> +
>> +/* Return the payload pointer for a given pcmd */
>> +static void *papr_scm_pcmd_to_payload(struct nd_papr_scm_cmd_pkg *pcmd)
>> +{
>> +       if (pcmd->hdr.nd_size_in == 0 && pcmd->hdr.nd_size_out == 0)
>> +               return NULL;
>> +       else
>> +               return (void *)((__u8 *) pcmd + pcmd->payload_offset);
>> +}
>> +#endif /* _UAPI_ASM_POWERPC_PAPR_SCM_DSM_H_ */
>> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
>> index aaf2e4ab1f75..e8ce96d2249e 100644
>> --- a/arch/powerpc/platforms/pseries/papr_scm.c
>> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
>> @@ -15,13 +15,15 @@
>>
>>  #include <asm/plpar_wrappers.h>
>>  #include <asm/papr_scm.h>
>> +#include <asm/papr_scm_dsm.h>
>>
>>  #define BIND_ANY_ADDR (~0ul)
>>
>>  #define PAPR_SCM_DIMM_CMD_MASK \
>>         ((1ul << ND_CMD_GET_CONFIG_SIZE) | \
>>          (1ul << ND_CMD_GET_CONFIG_DATA) | \
>> -        (1ul << ND_CMD_SET_CONFIG_DATA))
>> +        (1ul << ND_CMD_SET_CONFIG_DATA) | \
>> +        (1ul << ND_CMD_CALL))
>>
>>  struct papr_scm_priv {
>>         struct platform_device *pdev;
>> @@ -283,15 +285,92 @@ static int papr_scm_meta_set(struct papr_scm_priv *p,
>>         return 0;
>>  }
>>
>> +/*
>> + * Validate the inputs args to dimm-control function and return '0' if valid.
>> + * This also does initial sanity validation to ND_CMD_CALL sub-command packages.
>> + */
>> +static int is_cmd_valid(struct nvdimm *nvdimm, unsigned int cmd, void *buf,
>> +                      unsigned int buf_len)
>> +{
>> +       unsigned long cmd_mask = PAPR_SCM_DIMM_CMD_MASK;
>> +       struct nd_papr_scm_cmd_pkg *pkg = nd_to_papr_cmd_pkg(buf);
>> +
>> +       /* Only dimm-specific calls are supported atm */
>> +       if (!nvdimm)
>> +               return -EINVAL;
>> +
>> +       if (!test_bit(cmd, &cmd_mask)) {
>> +               pr_debug("%s: Unsupported cmd=%u\n", __func__, cmd);
>
> Please convert all the pr_debug() statements in this file to
> dev_dbg(). Also you don't need to put the __func__ in the print
> statement because you can enable that via dynamic_debug_control see
> the "f" flag in addition to the "p" flag for dynamic debug enabling.
>
Fair suggestion. Will update in next iteration.

>> +               return -EINVAL;
>> +       } else if (cmd == ND_CMD_CALL) {
>> +
>> +               /* Verify the envelop package */
>
> s/envelop/envelope/
>
>> +               if (!buf || buf_len < sizeof(struct nd_papr_scm_cmd_pkg)) {
>> +                       pr_debug("%s: Invalid pkg size=%u\n", __func__,
>> +                                buf_len);
>> +                       return -EINVAL;
>> +               }
>> +
>> +               /* Verify that the DSM command family is valid */
>> +               if (pkg->hdr.nd_family != NVDIMM_FAMILY_PAPR_SCM) {
>> +                       pr_debug("%s: Invalid pkg family=0x%llx\n", __func__,
>> +                                pkg->hdr.nd_family);
>> +                       return -EINVAL;
>> +
>> +               }
>> +
>> +               /* We except a payload with all DSM commands */
>> +               if (papr_scm_pcmd_to_payload(pkg) == NULL) {
>> +                       pr_debug("%s: Empty payload for sub-command=0x%llx\n",
>> +                                __func__, pkg->hdr.nd_command);
>> +                       return -EINVAL;
>> +               }
>> +       }
>> +
>> +       /* Command looks valid */
>> +       return 0;
>> +}
>> +
>> +static int papr_scm_service_dsm(struct papr_scm_priv *p,
>> +                               struct nd_papr_scm_cmd_pkg *call_pkg)
>> +{
>> +       /* unknown subcommands return error in packages */
>> +       if (call_pkg->hdr.nd_command <= DSM_PAPR_SCM_MIN ||
>> +           call_pkg->hdr.nd_command >= DSM_PAPR_SCM_MAX) {
>> +               pr_debug("Invalid DSM command 0x%llx\n",
>> +                        call_pkg->hdr.nd_command);
>> +               call_pkg->cmd_status = -EINVAL;
>> +               return 0;
>> +       }
>> +
>> +       /* Depending on the DSM command call appropriate service routine */
>> +       switch (call_pkg->hdr.nd_command) {
>> +       default:
>> +               pr_debug("Unsupported DSM command 0x%llx\n",
>> +                        call_pkg->hdr.nd_command);
>> +               call_pkg->cmd_status = -ENOENT;
>> +               return 0;
>> +       }
>> +}
>> +
>>  int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
>>                 unsigned int cmd, void *buf, unsigned int buf_len, int *cmd_rc)
>>  {
>>         struct nd_cmd_get_config_size *get_size_hdr;
>>         struct papr_scm_priv *p;
>> +       struct nd_papr_scm_cmd_pkg *call_pkg = NULL;
>> +       int rc;
>>
>> -       /* Only dimm-specific calls are supported atm */
>> -       if (!nvdimm)
>> -               return -EINVAL;
>> +       /* Use a local variable in case cmd_rc pointer is NULL */
>> +       if (cmd_rc == NULL)
>> +               cmd_rc = &rc;
>> +
>> +       *cmd_rc = is_cmd_valid(nvdimm, cmd, buf, buf_len);
>> +       if (*cmd_rc) {
>> +               pr_debug("%s: Invalid cmd=0x%x. Err=%d\n", __func__,
>> +                        cmd, *cmd_rc);
>> +               return *cmd_rc;
>> +       }
>>
>>         p = nvdimm_provider_data(nvdimm);
>>
>> @@ -313,13 +392,19 @@ int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
>>                 *cmd_rc = papr_scm_meta_set(p, buf);
>>                 break;
>>
>> +       case ND_CMD_CALL:
>> +               call_pkg = nd_to_papr_cmd_pkg(buf);
>> +               *cmd_rc = papr_scm_service_dsm(p, call_pkg);
>> +               break;
>> +
>>         default:
>> -               return -EINVAL;
>> +               dev_dbg(&p->pdev->dev, "Unknown command = %d\n", cmd);
>> +               *cmd_rc = -EINVAL;
>>         }
>>
>>         dev_dbg(&p->pdev->dev, "returned with cmd_rc = %d\n", *cmd_rc);
>>
>> -       return 0;
>> +       return *cmd_rc;
>>  }
>>
>>  static inline int papr_scm_node(int node)
>> --
>> 2.25.1
>>

-- 
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

Re: [PATCH v5 4/4] powerpc/papr_scm: Implement support for DSM_PAPR_SCM_HEALTH

[Vaibhav Jain]

Hi Dan / Mpe,

I have sent out a v6 of this patch set that addresses your review
comments so far. Also I have added a new doc patch in the patchset that
adds documentation for PAPR_SCM_HEALTH hcall specification.

Requesting you to please review the new patchset at
https://lore.kernel.org/linux-nvdimm/20200420070711.223545-1-vaibhav@linux.ibm.com

Thanks,
~ Vaibhav Jain

Dan Williams <dan.j.williams@intel.com> writes:

> On Tue, Mar 31, 2020 at 7:33 AM Vaibhav Jain <vaibhav@linux.ibm.com> wrote:
>>
>> This patch implements support for papr_scm command
>> 'DSM_PAPR_SCM_HEALTH' that returns a newly introduced 'struct
>> nd_papr_scm_dimm_health_stat' instance containing dimm health
>> information back to user space in response to ND_CMD_CALL. This
>> functionality is implemented in newly introduced papr_scm_get_health()
>> that queries the scm-dimm health information and then copies these bitmaps
>> to the package payload whose layout is defined by 'struct
>> papr_scm_ndctl_health'.
>>
>> The patch also introduces a new member a new member 'struct
>> papr_scm_priv.health' thats an instance of 'struct
>> nd_papr_scm_dimm_health_stat' to cache the health information of a
>> scm-dimm. As a result functions drc_pmem_query_health() and
>> papr_flags_show() are updated to populate and use this new struct
>> instead of two be64 integers that we earlier used.
>
> Link to HCALL specification?
>
>>
>> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
>> ---
>> Changelog:
>>
>> v4..v5: None
>>
>> v3..v4: Call the DSM_PAPR_SCM_HEALTH service function from
>>         papr_scm_service_dsm() instead of papr_scm_ndctl(). [Aneesh]
>>
>> v2..v3: Updated struct nd_papr_scm_dimm_health_stat_v1 to use '__xx'
>>         types as its exported to the userspace [Aneesh]
>>         Changed the constants DSM_PAPR_SCM_DIMM_XX indicating dimm
>>         health from enum to #defines [Aneesh]
>>
>> v1..v2: New patch in the series
>> ---
>>  arch/powerpc/include/uapi/asm/papr_scm_dsm.h |  40 +++++++
>>  arch/powerpc/platforms/pseries/papr_scm.c    | 109 ++++++++++++++++---
>>  2 files changed, 132 insertions(+), 17 deletions(-)
>>
>> diff --git a/arch/powerpc/include/uapi/asm/papr_scm_dsm.h b/arch/powerpc/include/uapi/asm/papr_scm_dsm.h
>> index c039a49b41b4..8265125304ca 100644
>> --- a/arch/powerpc/include/uapi/asm/papr_scm_dsm.h
>> +++ b/arch/powerpc/include/uapi/asm/papr_scm_dsm.h
>> @@ -132,6 +132,7 @@ struct nd_papr_scm_cmd_pkg {
>>   */
>>  enum dsm_papr_scm {
>>         DSM_PAPR_SCM_MIN =  0x10000,
>> +       DSM_PAPR_SCM_HEALTH,
>>         DSM_PAPR_SCM_MAX,
>>  };
>>
>> @@ -158,4 +159,43 @@ static void *papr_scm_pcmd_to_payload(struct nd_papr_scm_cmd_pkg *pcmd)
>>         else
>>                 return (void *)((__u8 *) pcmd + pcmd->payload_offset);
>>  }
>> +
>> +/* Various scm-dimm health indicators */
>> +#define DSM_PAPR_SCM_DIMM_HEALTHY       0
>> +#define DSM_PAPR_SCM_DIMM_UNHEALTHY     1
>> +#define DSM_PAPR_SCM_DIMM_CRITICAL      2
>> +#define DSM_PAPR_SCM_DIMM_FATAL         3
>> +
>> +/*
>> + * Struct exchanged between kernel & ndctl in for PAPR_DSM_PAPR_SMART_HEALTH
>> + * Various bitflags indicate the health status of the dimm.
>> + *
>> + * dimm_unarmed                : Dimm not armed. So contents wont persist.
>> + * dimm_bad_shutdown   : Previous shutdown did not persist contents.
>> + * dimm_bad_restore    : Contents from previous shutdown werent restored.
>> + * dimm_scrubbed       : Contents of the dimm have been scrubbed.
>> + * dimm_locked         : Contents of the dimm cant be modified until CEC reboot
>> + * dimm_encrypted      : Contents of dimm are encrypted.
>> + * dimm_health         : Dimm health indicator.
>> + */
>> +struct nd_papr_scm_dimm_health_stat_v1 {
>> +       __u8 dimm_unarmed;
>> +       __u8 dimm_bad_shutdown;
>> +       __u8 dimm_bad_restore;
>> +       __u8 dimm_scrubbed;
>> +       __u8 dimm_locked;
>> +       __u8 dimm_encrypted;
>> +       __u16 dimm_health;
>> +};
>
> Does the structure pack the same across different compilers and configurations?
>
>> +
>> +/*
>> + * Typedef the current struct for dimm_health so that any application
>> + * or kernel recompiled after introducing a new version automatically
>> + * supports the new version.
>> + */
>> +#define nd_papr_scm_dimm_health_stat nd_papr_scm_dimm_health_stat_v1
>> +
>> +/* Current version number for the dimm health struct */
>> +#define ND_PAPR_SCM_DIMM_HEALTH_VERSION 1
>> +
>>  #endif /* _UAPI_ASM_POWERPC_PAPR_SCM_DSM_H_ */
>> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
>> index e8ce96d2249e..ce94762954e0 100644
>> --- a/arch/powerpc/platforms/pseries/papr_scm.c
>> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
>> @@ -47,8 +47,7 @@ struct papr_scm_priv {
>>         struct mutex dimm_mutex;
>>
>>         /* Health information for the dimm */
>> -       __be64 health_bitmap;
>> -       __be64 health_bitmap_valid;
>> +       struct nd_papr_scm_dimm_health_stat health;
>>  };
>>
>>  static int drc_pmem_bind(struct papr_scm_priv *p)
>> @@ -158,6 +157,7 @@ static int drc_pmem_query_health(struct papr_scm_priv *p)
>>  {
>>         unsigned long ret[PLPAR_HCALL_BUFSIZE];
>>         int64_t rc;
>> +       __be64 health;
>>
>>         rc = plpar_hcall(H_SCM_HEALTH, ret, p->drc_index);
>>         if (rc != H_SUCCESS) {
>> @@ -172,13 +172,41 @@ static int drc_pmem_query_health(struct papr_scm_priv *p)
>>                 return rc;
>>
>>         /* Store the retrieved health information in dimm platform data */
>> -       p->health_bitmap = ret[0];
>> -       p->health_bitmap_valid = ret[1];
>> +       health = ret[0] & 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));
>> +               be64_to_cpu(ret[0]),
>> +               be64_to_cpu(ret[1]));
>> +
>> +       memset(&p->health, 0, sizeof(p->health));
>> +
>> +       /* Check for various masks in bitmap and set the buffer */
>> +       if (health & PAPR_SCM_DIMM_UNARMED_MASK)
>> +               p->health.dimm_unarmed = true;
>> +
>> +       if (health & PAPR_SCM_DIMM_BAD_SHUTDOWN_MASK)
>> +               p->health.dimm_bad_shutdown = true;
>> +
>> +       if (health & PAPR_SCM_DIMM_BAD_RESTORE_MASK)
>> +               p->health.dimm_bad_restore = true;
>> +
>> +       if (health & PAPR_SCM_DIMM_ENCRYPTED)
>> +               p->health.dimm_encrypted = true;
>> +
>> +       if (health & PAPR_SCM_DIMM_SCRUBBED_AND_LOCKED) {
>> +               p->health.dimm_locked = true;
>> +               p->health.dimm_scrubbed = true;
>> +       }
>
> I don't think bool is suitable for ioctl ABI. For example the true
> value may be positive, or negative depending on the arch. This should
> be using explicit integer values.
>
> I assume the reason you are translating this rather than transmitting
> that raw 64-bit health value is for future compatibility if the HCALL
> format changes / is extended?
>
>> +
>> +       if (health & PAPR_SCM_DIMM_HEALTH_UNHEALTHY)
>> +               p->health.dimm_health = DSM_PAPR_SCM_DIMM_UNHEALTHY;
>> +
>> +       if (health & PAPR_SCM_DIMM_HEALTH_CRITICAL)
>> +               p->health.dimm_health = DSM_PAPR_SCM_DIMM_CRITICAL;
>> +
>> +       if (health & PAPR_SCM_DIMM_HEALTH_FATAL)
>> +               p->health.dimm_health = DSM_PAPR_SCM_DIMM_FATAL;
>
>>
>>         mutex_unlock(&p->dimm_mutex);
>>         return 0;
>> @@ -331,6 +359,51 @@ static int is_cmd_valid(struct nvdimm *nvdimm, unsigned int cmd, void *buf,
>>         return 0;
>>  }
>>
>> +/* Fetch the DIMM health info and populate it in provided package. */
>> +static int papr_scm_get_health(struct papr_scm_priv *p,
>> +                              struct nd_papr_scm_cmd_pkg *pkg)
>> +{
>> +       int rc;
>> +       size_t copysize = sizeof(p->health);
>> +
>> +       rc = drc_pmem_query_health(p);
>> +       if (rc)
>> +               goto out;
>> +       /*
>> +        * If the requested payload version is greater than one we know
>> +        * about, return the payload version we know about and let
>> +        * caller/userspace handle.
>> +        */
>> +       if (pkg->payload_version > ND_PAPR_SCM_DIMM_HEALTH_VERSION)
>> +               pkg->payload_version = ND_PAPR_SCM_DIMM_HEALTH_VERSION;
>> +
>> +       if (pkg->hdr.nd_size_out < copysize) {
>> +               dev_dbg(&p->pdev->dev, "%s Payload not large enough\n",
>> +                       __func__);
>> +               dev_dbg(&p->pdev->dev, "%s Expected %lu, available %u\n",
>> +                       __func__, copysize, pkg->hdr.nd_size_out);
>> +               rc = -ENOSPC;
>> +               goto out;
>> +       }
>> +
>> +       dev_dbg(&p->pdev->dev, "%s Copying payload size=%lu version=0x%x\n",
>> +               __func__, copysize, pkg->payload_version);
>> +
>> +       /* Copy a subset of health struct based on copysize */
>> +       memcpy(papr_scm_pcmd_to_payload(pkg), &p->health, copysize);
>> +       pkg->hdr.nd_fw_size = copysize;
>> +
>> +out:
>> +       /*
>> +        * Put the error in out package and return success from function
>> +        * so that errors if any are propogated back to userspace.
>> +        */
>> +       pkg->cmd_status = rc;
>> +       dev_dbg(&p->pdev->dev, "%s completion code = %d\n", __func__, rc);
>> +
>> +       return 0;
>> +}
>> +
>>  static int papr_scm_service_dsm(struct papr_scm_priv *p,
>>                                 struct nd_papr_scm_cmd_pkg *call_pkg)
>>  {
>> @@ -345,6 +418,9 @@ static int papr_scm_service_dsm(struct papr_scm_priv *p,
>>
>>         /* Depending on the DSM command call appropriate service routine */
>>         switch (call_pkg->hdr.nd_command) {
>> +       case DSM_PAPR_SCM_HEALTH:
>> +               return papr_scm_get_health(p, call_pkg);
>> +
>>         default:
>>                 pr_debug("Unsupported DSM command 0x%llx\n",
>>                          call_pkg->hdr.nd_command);
>> @@ -431,7 +507,6 @@ static ssize_t papr_flags_show(struct device *dev,
>>  {
>>         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);
>> @@ -443,26 +518,26 @@ static ssize_t papr_flags_show(struct device *dev,
>>         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)
>> +       if (p->health.dimm_unarmed)
>>                 rc += sprintf(buf, "not_armed ");
>>
>> -       if (health & PAPR_SCM_DIMM_BAD_SHUTDOWN_MASK)
>> +       if (p->health.dimm_bad_shutdown)
>>                 rc += sprintf(buf + rc, "save_fail ");
>
> Per my patch1 comment is this "save_fail" or "flush_fail"?
>
>>
>> -       if (health & PAPR_SCM_DIMM_BAD_RESTORE_MASK)
>> +       if (p->health.dimm_bad_restore)
>>                 rc += sprintf(buf + rc, "restore_fail ");
>>
>> -       if (health & PAPR_SCM_DIMM_ENCRYPTED)
>> +       if (p->health.dimm_encrypted)
>>                 rc += sprintf(buf + rc, "encrypted ");
>>
>> -       if (health & PAPR_SCM_DIMM_SMART_EVENT_MASK)
>> +       if (p->health.dimm_health)
>>                 rc += sprintf(buf + rc, "smart_notify ");
>>
>> -       if (health & PAPR_SCM_DIMM_SCRUBBED_AND_LOCKED)
>> -               rc += sprintf(buf + rc, "scrubbed locked ");
>> +       if (p->health.dimm_scrubbed)
>> +               rc += sprintf(buf + rc, "scrubbed ");
>> +
>> +       if (p->health.dimm_locked)
>> +               rc += sprintf(buf + rc, "locked ");
>>
>>         if (rc > 0)
>>                 rc += sprintf(buf + rc, "\n");
>> --
>> 2.25.1
>>

-- 
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org