2020-06-04 23:44:52

by Vaibhav Jain

[permalink] [raw]
Subject: [PATCH v10 0/6] powerpc/papr_scm: Add support for reporting nvdimm health

Changes since v9 [1]:
* Addressed review comments from Ira and Dan Williams.
* Removed the contentious 'payload_version' field from struct
nd_pdsm_cmd_pkg.
* Also removed code/defines related to handling of different version
of a pdsm payload struct.
* Consolidated validation checks for nd_pdsm_cmd_pkg in
is_cmd_valid().
* Added a check in is_cmd_valid() to ensure reserved fields in struct
nd_pdsm_cmd_pkg are set to '0'.
* Reworked papr_pdsm_health() to avoid removing code that was added in
initial part of this patch-series.
* Added a new patch to the series to move out some proposed changes to
papr_scm_ndctl() in an independent patch.
* Reworked papr_pdsm_health() to ensure correct payload_size in the
pdsm command package.

[1] https://lore.kernel.org/linux-nvdimm/[email protected]
---

The PAPR standard[2][4] provides mechanisms to query the health and
performance stats of an NVDIMM via various hcalls as described in
Ref[3]. 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[3]. This health and performance stats are then exposed to
userspace via sysfs and PAPR-NVDIMM-Specific-Methods(PDSM) issued by
libndctl.

These changes coupled with proposed ndtcl changes located at Ref[5]
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 NVDIMM-Specific-Methods(PDSM)
==================================

PDSM requests are issued by vendor specific code in libndctl to
execute certain operations or fetch information from NVDIMMS. PDSMs
requests can be sent to papr_scm module via libndctl(userspace) and
libnvdimm (kernel) using the ND_CMD_CALL ioctl command which can be
handled in the dimm control function papr_scm_ndctl(). Current
patchset proposes a single PDSM to retrieve NVDIMM health, defined in
the newly introduced uapi header named 'papr_pdsm.h'. Support for
more PDSMs will be added in future.

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

The patch-set starts with a doc patch documenting details of hcall
H_SCM_HEALTH. Second patch exports kernel symbol seq_buf_printf()
thats used in subsequent patches to generate sysfs attribute content.

Third patch implements support for fetching NVDIMM health information
from PHYP and partially exposing it to user-space via a NVDIMM sysfs
flag.

Fourth patch updates papr_scm_ndctl() to handle a possible error case
and also improve debug logging.

Fifth patch deals with implementing support for servicing PDSM
commands in papr_scm module.

Finally the last patch implements support for servicing PDSM
'PAPR_PDSM_HEALTH' that returns the NVDIMM health information to
libndctl.

References:
[2] "Power Architecture Platform Reference"
https://en.wikipedia.org/wiki/Power_Architecture_Platform_Reference
[3] commit 58b278f568f0
("powerpc: Provide initial documentation for PAPR hcalls")
[4] "Linux on Power Architecture Platform Reference"
https://members.openpowerfoundation.org/document/dl/469
[5] https://github.com/vaibhav92/ndctl/tree/papr_scm_health_v10

---

Vaibhav Jain (6):
powerpc: Document details on H_SCM_HEALTH hcall
seq_buf: Export seq_buf_printf
powerpc/papr_scm: Fetch nvdimm health information from PHYP
powerpc/papr_scm: Improve error logging and handling papr_scm_ndctl()
ndctl/papr_scm,uapi: Add support for PAPR nvdimm specific methods
powerpc/papr_scm: Implement support for PAPR_PDSM_HEALTH

Documentation/ABI/testing/sysfs-bus-papr-pmem | 27 ++
Documentation/powerpc/papr_hcalls.rst | 46 ++-
arch/powerpc/include/uapi/asm/papr_pdsm.h | 131 +++++++
arch/powerpc/platforms/pseries/papr_scm.c | 361 +++++++++++++++++-
include/uapi/linux/ndctl.h | 1 +
lib/seq_buf.c | 1 +
6 files changed, 554 insertions(+), 13 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-bus-papr-pmem
create mode 100644 arch/powerpc/include/uapi/asm/papr_pdsm.h

--
2.26.2


2020-06-04 23:44:59

by Vaibhav Jain

[permalink] [raw]
Subject: [PATCH v10 5/6] ndctl/papr_scm,uapi: Add support for PAPR nvdimm specific methods

Introduce support for PAPR NVDIMM Specific Methods (PDSM) in papr_scm
module and add the command family NVDIMM_FAMILY_PAPR to the white list
of NVDIMM command sets. Also advertise support for ND_CMD_CALL for the
nvdimm command mask and implement necessary scaffolding in the module
to handle ND_CMD_CALL ioctl and PDSM requests that we receive.

The layout of the PDSM request as we expect from libnvdimm/libndctl is
described in newly introduced uapi header 'papr_pdsm.h' which
defines a new 'struct nd_pdsm_cmd_pkg' header. This header is used
to communicate the PDSM request via member
'nd_cmd_pkg.nd_command' and size of payload that need to be
sent/received for servicing the PDSM.

A new function is_cmd_valid() is implemented that reads the args to
papr_scm_ndctl() and performs sanity tests on them. A new function
papr_scm_service_pdsm() is introduced and is called from
papr_scm_ndctl() in case of a PDSM request is received via ND_CMD_CALL
command from libnvdimm.

Cc: "Aneesh Kumar K . V" <[email protected]>
Cc: Dan Williams <[email protected]>
Cc: Michael Ellerman <[email protected]>
Cc: Ira Weiny <[email protected]>
Signed-off-by: Vaibhav Jain <[email protected]>
---
Changelog:

v9..v10:
* Simplified 'struct nd_pdsm_cmd_pkg' by removing the
'payload_version' field.
* Removed the corrosponding documentation on versioning and backward
compatibility from 'papr_pdsm.h'
* Reduced the size of reserved fields to 4-bytes making 'struct
nd_pdsm_cmd_pkg' 64 + 8 bytes long.
* Updated is_cmd_valid() to enforce validation checks on pdsm
commands. [ Dan Williams ]
* Added check for reserved fields being set to '0' in is_cmd_valid()
[ Ira ]
* Moved changes for checking cmd_rc == NULL and logging improvements
to a separate prelim patch [ Ira ].
* Moved pdsm package validation checks from papr_scm_service_pdsm()
to is_cmd_valid().
* Marked papr_scm_service_pdsm() return type as 'void' since errors
are reported in nd_pdsm_cmd_pkg.cmd_status field.

Resend:
* Added ack from Aneesh.

v8..v9:
* Reduced the usage of term SCM replacing it with appropriate
replacement [ Dan Williams, Aneesh ]
* Renamed 'papr_scm_pdsm.h' to 'papr_pdsm.h'
* s/PAPR_SCM_PDSM_*/PAPR_PDSM_*/g
* s/NVDIMM_FAMILY_PAPR_SCM/NVDIMM_FAMILY_PAPR/g
* Minor updates to 'papr_psdm.h' to replace usage of term 'SCM'.
* Minor update to patch description.

v7..v8:
* Removed the 'payload_offset' field from 'struct
nd_pdsm_cmd_pkg'. Instead command payload is always assumed to start
at 'nd_pdsm_cmd_pkg.payload'. [ Aneesh ]
* To enable introducing new fields to 'struct nd_pdsm_cmd_pkg',
'reserved' field of 10-bytes is introduced. [ Aneesh ]
* Fixed a typo in "Backward Compatibility" section of papr_scm_pdsm.h
[ Ira ]

Resend:
* None

v6..v7 :
* Removed the re-definitions of __packed macro from papr_scm_pdsm.h
[Mpe].
* Removed the usage of __KERNEL__ macros in papr_scm_pdsm.h [Mpe].
* Removed macros that were unused in papr_scm.c from papr_scm_pdsm.h
[Mpe].
* Made functions defined in papr_scm_pdsm.h as static inline. [Mpe]

v5..v6 :
* Changed the usage of the term DSM to PDSM to distinguish it from the
ACPI term [ Dan Williams ]
* Renamed papr_scm_dsm.h to papr_scm_pdsm.h and updated various struct
to reflect the new terminology.
* Updated the patch description and title to reflect the new terminology.
* Squashed patch to introduce new command family in 'ndctl.h' with
this patch [ Dan Williams ]
* Updated the papr_scm_pdsm method starting index from 0x10000 to 0x0
[ Dan Williams ]
* Removed redundant license text from the papr_scm_psdm.h file.
[ Dan Williams ]
* s/envelop/envelope/ at various places [ Dan Williams ]
* Added '__packed' attribute to command package header to gaurd
against different compiler adding paddings between the fields.
[ Dan Williams]
* Converted various pr_debug to dev_debug [ Dan Williams ]

v4..v5 :
* None

v3..v4 :
* None

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

v1..v2 :
* None
---
arch/powerpc/include/uapi/asm/papr_pdsm.h | 98 +++++++++++++++++++
arch/powerpc/platforms/pseries/papr_scm.c | 113 +++++++++++++++++++++-
include/uapi/linux/ndctl.h | 1 +
3 files changed, 207 insertions(+), 5 deletions(-)
create mode 100644 arch/powerpc/include/uapi/asm/papr_pdsm.h

diff --git a/arch/powerpc/include/uapi/asm/papr_pdsm.h b/arch/powerpc/include/uapi/asm/papr_pdsm.h
new file mode 100644
index 000000000000..8b1a4f8fa316
--- /dev/null
+++ b/arch/powerpc/include/uapi/asm/papr_pdsm.h
@@ -0,0 +1,98 @@
+/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
+/*
+ * PAPR nvDimm Specific Methods (PDSM) and structs for libndctl
+ *
+ * (C) Copyright IBM 2020
+ *
+ * Author: Vaibhav Jain <vaibhav at linux.ibm.com>
+ */
+
+#ifndef _UAPI_ASM_POWERPC_PAPR_PDSM_H_
+#define _UAPI_ASM_POWERPC_PAPR_PDSM_H_
+
+#include <linux/types.h>
+
+/*
+ * PDSM Envelope:
+ *
+ * The ioctl ND_CMD_CALL transfers data between user-space and kernel via
+ * envelope which consists of a header and user-defined payload sections.
+ * The header is described by 'struct nd_pdsm_cmd_pkg' which expects a
+ * payload following it and accessible via 'nd_pdsm_cmd_pkg.payload' field.
+ * There is reserved field that can used to introduce new fields to the
+ * structure in future. It also tries to ensure that 'nd_pdsm_cmd_pkg.payload'
+ * lies at a 8-byte boundary.
+ *
+ * +-------------+---------------------+---------------------------+
+ * | 64-Bytes | 8-Bytes | Max 184-Bytes |
+ * +-------------+---------------------+---------------------------+
+ * | nd_pdsm_cmd_pkg | |
+ * |-------------+ | |
+ * | nd_cmd_pkg | | |
+ * +-------------+---------------------+---------------------------+
+ * | nd_family | | |
+ * | nd_size_out | cmd_status | |
+ * | nd_size_in | reserved | payload |
+ * | nd_command | | |
+ * | nd_fw_size | | |
+ * +-------------+---------------------+---------------------------+
+ *
+ * PDSM Header:
+ *
+ * The header is defined as 'struct nd_pdsm_cmd_pkg' which embeds a
+ * 'struct nd_cmd_pkg' instance. The PDSM command is assigned to member
+ * 'nd_cmd_pkg.nd_command'. Apart from size information of the envelope which is
+ * contained in 'struct nd_cmd_pkg', the header also has members following
+ * members:
+ *
+ * 'cmd_status' : (Out) Errors if any encountered while servicing PDSM.
+ * 'reserved' : Not used, reserved for future and should be set to 0.
+ *
+ * PDSM Payload:
+ *
+ * The layout of the PDSM Payload is defined by various structs shared between
+ * papr_scm and libndctl so that contents of payload can be interpreted. During
+ * servicing of a PDSM the papr_scm module will read input args from the payload
+ * field by casting its contents to an appropriate struct pointer based on the
+ * PDSM command. Similarly the output of servicing the PDSM 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).
+ *
+ */
+
+/* PDSM-header + payload expected with ND_CMD_CALL ioctl from libnvdimm */
+struct nd_pdsm_cmd_pkg {
+ struct nd_cmd_pkg hdr; /* Package header containing sub-cmd */
+ __s32 cmd_status; /* Out: Sub-cmd status returned back */
+ __u16 reserved[2]; /* Ignored and to be used in future */
+ __u8 payload[]; /* In/Out: Sub-cmd data buffer */
+} __packed;
+
+/*
+ * Methods to be embedded in ND_CMD_CALL request. These are sent to the kernel
+ * via 'nd_pdsm_cmd_pkg.hdr.nd_command' member of the ioctl struct
+ */
+enum papr_pdsm {
+ PAPR_PDSM_MIN = 0x0,
+ PAPR_PDSM_MAX,
+};
+
+/* Convert a libnvdimm nd_cmd_pkg to pdsm specific pkg */
+static inline struct nd_pdsm_cmd_pkg *nd_to_pdsm_cmd_pkg(struct nd_cmd_pkg *cmd)
+{
+ return (struct nd_pdsm_cmd_pkg *)cmd;
+}
+
+/* Return the payload pointer for a given pcmd */
+static inline void *pdsm_cmd_to_payload(struct nd_pdsm_cmd_pkg *pcmd)
+{
+ if (pcmd->hdr.nd_size_in == 0 && pcmd->hdr.nd_size_out == 0)
+ return NULL;
+ else
+ return (void *)(pcmd->payload);
+}
+
+#endif /* _UAPI_ASM_POWERPC_PAPR_PDSM_H_ */
diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
index 6512fe6a2874..05eb56ecab5e 100644
--- a/arch/powerpc/platforms/pseries/papr_scm.c
+++ b/arch/powerpc/platforms/pseries/papr_scm.c
@@ -15,13 +15,15 @@
#include <linux/seq_buf.h>

#include <asm/plpar_wrappers.h>
+#include <asm/papr_pdsm.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))

/* DIMM health bitmap bitmap indicators */
/* SCM device is unable to persist memory contents */
@@ -349,22 +351,117 @@ static int papr_scm_meta_set(struct papr_scm_priv *p,
return 0;
}

+/*
+ * Do a sanity checks on the inputs args to dimm-control function and return
+ * '0' if valid. This also does validation on 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_pdsm_cmd_pkg *pkg;
+ struct nd_cmd_pkg *nd_cmd;
+ struct papr_scm_priv *p;
+ enum papr_pdsm pdsm;
+
+ /* Only dimm-specific calls are supported atm */
+ if (!nvdimm)
+ return -EINVAL;
+
+ /* get the provider data from struct nvdimm */
+ p = nvdimm_provider_data(nvdimm);
+
+ if (!test_bit(cmd, &cmd_mask)) {
+ dev_dbg(&p->pdev->dev, "Unsupported cmd=%u\n", cmd);
+ return -EINVAL;
+ }
+
+ /* For CMD_CALL verify pdsm request */
+ if (cmd == ND_CMD_CALL) {
+ /* Verify the envelope package */
+ if (!buf || buf_len < sizeof(struct nd_pdsm_cmd_pkg)) {
+ dev_dbg(&p->pdev->dev, "Invalid pkg size=%u\n",
+ buf_len);
+ return -EINVAL;
+ }
+
+ /* Verify that the nd_cmd_pkg.nd_family is correct */
+ nd_cmd = (struct nd_cmd_pkg *)buf;
+ if (nd_cmd->nd_family != NVDIMM_FAMILY_PAPR) {
+ dev_dbg(&p->pdev->dev, "Invalid pkg family=0x%llx\n",
+ nd_cmd->nd_family);
+ return -EINVAL;
+ }
+
+ /* Get the pdsm request package and the command */
+ pkg = nd_to_pdsm_cmd_pkg(nd_cmd);
+ pdsm = pkg->hdr.nd_command;
+
+ /* Verify if the psdm command is valid */
+ if (pdsm <= PAPR_PDSM_MIN || pdsm >= PAPR_PDSM_MAX) {
+ dev_dbg(&p->pdev->dev, "PDSM[0x%x]: Invalid PDSM\n", pdsm);
+ return -EINVAL;
+ }
+
+ /* We except a payload with all PDSM commands */
+ if (!pdsm_cmd_to_payload(pkg)) {
+ dev_dbg(&p->pdev->dev, "PDSM[0x%x]: Empty payload\n", pdsm);
+ return -EINVAL;
+ }
+
+ /* Ensure reserved fields of the pdsm header are set to 0 */
+ if (pkg->reserved[0] || pkg->reserved[1]) {
+ dev_dbg(&p->pdev->dev,
+ "PDSM[0x%x]: Invalid reserved field usage\n", pdsm);
+ return -EINVAL;
+ }
+ }
+
+ /* Let the command be further processed */
+ return 0;
+}
+
+/*
+ * For a given pdsm request call an appropriate service function.
+ * Note: Use 'nd_pdsm_cmd_pkg.cmd_status to report psdm servicing errors. Hence
+ * function marked as void.
+ */
+static void papr_scm_service_pdsm(struct papr_scm_priv *p,
+ struct nd_pdsm_cmd_pkg *pkg)
+{
+ const enum papr_pdsm pdsm = pkg->hdr.nd_command;
+
+ dev_dbg(&p->pdev->dev, "PDSM[0x%x]: Servicing..\n", pdsm);
+
+ /* Call pdsm service function */
+ switch (pdsm) {
+ default:
+ dev_dbg(&p->pdev->dev, "PDSM[0x%x]: Unsupported PDSM request\n",
+ pdsm);
+ pkg->cmd_status = -ENOENT;
+ break;
+ }
+}
+
static 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 nd_pdsm_cmd_pkg *call_pkg = NULL;
struct papr_scm_priv *p;
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)
cmd_rc = &rc;

+ *cmd_rc = is_cmd_valid(nvdimm, cmd, buf, buf_len);
+ if (*cmd_rc) {
+ pr_debug("Invalid cmd=0x%x. Err=%d\n", cmd, *cmd_rc);
+ return *cmd_rc;
+ }
+
p = nvdimm_provider_data(nvdimm);

switch (cmd) {
@@ -385,6 +482,12 @@ static int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc,
*cmd_rc = papr_scm_meta_set(p, buf);
break;

+ case ND_CMD_CALL:
+ call_pkg = nd_to_pdsm_cmd_pkg(buf);
+ papr_scm_service_pdsm(p, call_pkg);
+ *cmd_rc = 0;
+ break;
+
default:
dev_dbg(&p->pdev->dev, "Unknown command = %d\n", cmd);
*cmd_rc = -EINVAL;
diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h
index de5d90212409..0e09dc5cec19 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 5

#define ND_IOCTL_CALL _IOWR(ND_IOCTL, ND_CMD_CALL,\
struct nd_cmd_pkg)
--
2.26.2

2020-06-04 23:45:20

by Vaibhav Jain

[permalink] [raw]
Subject: [PATCH v10 3/6] powerpc/papr_scm: Fetch nvdimm health information from PHYP

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 bitmap, bitwise-and of which is then stored in
'struct papr_scm_priv' and subsequently partially exposed to
user-space via newly introduced dimm specific attribute
'papr/flags'. Since the hcall is costly, the health information is
cached and only re-queried, 60s after the previous successful hcall.

The patch also adds a documentation text describing flags reported by
the the new sysfs attribute 'papr/flags' is also introduced at
Documentation/ABI/testing/sysfs-bus-papr-pmem.

[1] commit 58b278f568f0 ("powerpc: Provide initial documentation for
PAPR hcalls")

Cc: "Aneesh Kumar K . V" <[email protected]>
Cc: Dan Williams <[email protected]>
Cc: Michael Ellerman <[email protected]>
Cc: Ira Weiny <[email protected]>
Signed-off-by: Vaibhav Jain <[email protected]>
---
Changelog:

v9..v10:
* Removed an avoidable 'goto' in __drc_pmem_query_health. [ Ira ].

Resend:
* Added ack from Aneesh.

v8..v9:
* Rename some variables and defines to reduce usage of term SCM
replacing it with PMEM [Dan Williams, Aneesh]
* s/PAPR_SCM_DIMM/PAPR_PMEM/g
* s/papr_scm_nd_attributes/papr_nd_attributes/g
* s/papr_scm_nd_attribute_group/papr_nd_attribute_group/g
* s/papr_scm_dimm_attr_groups/papr_nd_attribute_groups/g
* Renamed file sysfs-bus-papr-scm to sysfs-bus-papr-pmem

v7..v8:
* Update type of variable 'rc' in __drc_pmem_query_health() and
drc_pmem_query_health() to long and int respectively. [ Ira ]
* Updated the patch description to s/64 bit Big Endian Number/64-bit
bitmap/ [ Ira, Aneesh ].

Resend:
* None

v6..v7 :
* Used the exported buf_seq_printf() function to generate content for
'papr/flags'
* Moved the PAPR_SCM_DIMM_* bit-flags macro definitions to papr_scm.c
and removed the papr_scm.h file [Mpe]
* Some minor consistency issued in sysfs-bus-papr-scm
documentation. [Mpe]
* s/dimm_mutex/health_mutex/g [Mpe]
* Split drc_pmem_query_health() into two function one of which takes
care of caching and locking. [Mpe]
* Fixed a local copy creation of dimm health information using
READ_ONCE(). [Mpe]

v5..v6 :
* Change the flags sysfs attribute from 'papr_flags' to 'papr/flags'
[Dan Williams]
* Include documentation for 'papr/flags' attr [Dan Williams]
* Change flag 'save_fail' to 'flush_fail' [Dan Williams]
* Caching of health bitmap to reduce expensive hcalls [Dan Williams]
* Removed usage of PPC_BIT from 'papr-scm.h' header [Mpe]
* Replaced two __be64 integers from papr_scm_priv to a single u64
integer [Mpe]
* Updated patch description to reflect the changes made in this
version.
* Removed avoidable usage of 'papr_scm_priv.dimm_mutex' from
flags_show() [Dan Williams]

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.
---
Documentation/ABI/testing/sysfs-bus-papr-pmem | 27 +++
arch/powerpc/platforms/pseries/papr_scm.c | 168 +++++++++++++++++-
2 files changed, 193 insertions(+), 2 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-bus-papr-pmem

diff --git a/Documentation/ABI/testing/sysfs-bus-papr-pmem b/Documentation/ABI/testing/sysfs-bus-papr-pmem
new file mode 100644
index 000000000000..5b10d036a8d4
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-papr-pmem
@@ -0,0 +1,27 @@
+What: /sys/bus/nd/devices/nmemX/papr/flags
+Date: Apr, 2020
+KernelVersion: v5.8
+Contact: linuxppc-dev <[email protected]>, [email protected],
+Description:
+ (RO) Report flags indicating various states of a
+ papr-pmem NVDIMM device. Each flag maps to a one or
+ more bits set in the dimm-health-bitmap retrieved in
+ response to H_SCM_HEALTH hcall. The details of the bit
+ flags returned in response to this hcall is available
+ at 'Documentation/powerpc/papr_hcalls.rst' . Below are
+ the flags reported in this sysfs file:
+
+ * "not_armed" : Indicates that NVDIMM contents will not
+ survive a power cycle.
+ * "flush_fail" : Indicates that NVDIMM contents
+ couldn't be flushed during last
+ shut-down event.
+ * "restore_fail": Indicates that NVDIMM contents
+ couldn't be restored during NVDIMM
+ initialization.
+ * "encrypted" : NVDIMM 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.
diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
index f35592423380..0c091622b15e 100644
--- a/arch/powerpc/platforms/pseries/papr_scm.c
+++ b/arch/powerpc/platforms/pseries/papr_scm.c
@@ -12,6 +12,7 @@
#include <linux/libnvdimm.h>
#include <linux/platform_device.h>
#include <linux/delay.h>
+#include <linux/seq_buf.h>

#include <asm/plpar_wrappers.h>

@@ -22,6 +23,44 @@
(1ul << ND_CMD_GET_CONFIG_DATA) | \
(1ul << ND_CMD_SET_CONFIG_DATA))

+/* DIMM health bitmap bitmap indicators */
+/* SCM device is unable to persist memory contents */
+#define PAPR_PMEM_UNARMED (1ULL << (63 - 0))
+/* SCM device failed to persist memory contents */
+#define PAPR_PMEM_SHUTDOWN_DIRTY (1ULL << (63 - 1))
+/* SCM device contents are persisted from previous IPL */
+#define PAPR_PMEM_SHUTDOWN_CLEAN (1ULL << (63 - 2))
+/* SCM device contents are not persisted from previous IPL */
+#define PAPR_PMEM_EMPTY (1ULL << (63 - 3))
+/* SCM device memory life remaining is critically low */
+#define PAPR_PMEM_HEALTH_CRITICAL (1ULL << (63 - 4))
+/* SCM device will be garded off next IPL due to failure */
+#define PAPR_PMEM_HEALTH_FATAL (1ULL << (63 - 5))
+/* SCM contents cannot persist due to current platform health status */
+#define PAPR_PMEM_HEALTH_UNHEALTHY (1ULL << (63 - 6))
+/* SCM device is unable to persist memory contents in certain conditions */
+#define PAPR_PMEM_HEALTH_NON_CRITICAL (1ULL << (63 - 7))
+/* SCM device is encrypted */
+#define PAPR_PMEM_ENCRYPTED (1ULL << (63 - 8))
+/* SCM device has been scrubbed and locked */
+#define PAPR_PMEM_SCRUBBED_AND_LOCKED (1ULL << (63 - 9))
+
+/* Bits status indicators for health bitmap indicating unarmed dimm */
+#define PAPR_PMEM_UNARMED_MASK (PAPR_PMEM_UNARMED | \
+ PAPR_PMEM_HEALTH_UNHEALTHY)
+
+/* Bits status indicators for health bitmap indicating unflushed dimm */
+#define PAPR_PMEM_BAD_SHUTDOWN_MASK (PAPR_PMEM_SHUTDOWN_DIRTY)
+
+/* Bits status indicators for health bitmap indicating unrestored dimm */
+#define PAPR_PMEM_BAD_RESTORE_MASK (PAPR_PMEM_EMPTY)
+
+/* Bit status indicators for smart event notification */
+#define PAPR_PMEM_SMART_EVENT_MASK (PAPR_PMEM_HEALTH_CRITICAL | \
+ PAPR_PMEM_HEALTH_FATAL | \
+ PAPR_PMEM_HEALTH_UNHEALTHY)
+
+/* private struct associated with each region */
struct papr_scm_priv {
struct platform_device *pdev;
struct device_node *dn;
@@ -39,6 +78,15 @@ struct papr_scm_priv {
struct resource res;
struct nd_region *region;
struct nd_interleave_set nd_set;
+
+ /* Protect dimm health data from concurrent read/writes */
+ struct mutex health_mutex;
+
+ /* Last time the health information of the dimm was updated */
+ unsigned long lasthealth_jiffies;
+
+ /* Health information for the dimm */
+ u64 health_bitmap;
};

static int drc_pmem_bind(struct papr_scm_priv *p)
@@ -144,6 +192,61 @@ static int drc_pmem_query_n_bind(struct papr_scm_priv *p)
return drc_pmem_bind(p);
}

+/*
+ * Issue hcall to retrieve dimm health info and populate papr_scm_priv with the
+ * health information.
+ */
+static int __drc_pmem_query_health(struct papr_scm_priv *p)
+{
+ unsigned long ret[PLPAR_HCALL_BUFSIZE];
+ long rc;
+
+ /* issue the hcall */
+ 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:%ld\n", rc);
+ return -ENXIO;
+ }
+
+ p->lasthealth_jiffies = jiffies;
+ p->health_bitmap = ret[0] & ret[1];
+
+ dev_dbg(&p->pdev->dev,
+ "Queried dimm health info. Bitmap:0x%016lx Mask:0x%016lx\n",
+ ret[0], ret[1]);
+
+ return 0;
+}
+
+/* Min interval in seconds for assuming stable dimm health */
+#define MIN_HEALTH_QUERY_INTERVAL 60
+
+/* Query cached health info and if needed call drc_pmem_query_health */
+static int drc_pmem_query_health(struct papr_scm_priv *p)
+{
+ unsigned long cache_timeout;
+ int rc;
+
+ /* Protect concurrent modifications to papr_scm_priv */
+ rc = mutex_lock_interruptible(&p->health_mutex);
+ if (rc)
+ return rc;
+
+ /* Jiffies offset for which the health data is assumed to be same */
+ cache_timeout = p->lasthealth_jiffies +
+ msecs_to_jiffies(MIN_HEALTH_QUERY_INTERVAL * 1000);
+
+ /* Fetch new health info is its older than MIN_HEALTH_QUERY_INTERVAL */
+ if (time_after(jiffies, cache_timeout))
+ rc = __drc_pmem_query_health(p);
+ else
+ /* Assume cached health data is valid */
+ rc = 0;
+
+ mutex_unlock(&p->health_mutex);
+ return rc;
+}

static int papr_scm_meta_get(struct papr_scm_priv *p,
struct nd_cmd_get_config_data_hdr *hdr)
@@ -286,6 +389,64 @@ static int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc,
return 0;
}

+static ssize_t 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);
+ struct seq_buf s;
+ u64 health;
+ int rc;
+
+ rc = drc_pmem_query_health(p);
+ if (rc)
+ return rc;
+
+ /* Copy health_bitmap locally, check masks & update out buffer */
+ health = READ_ONCE(p->health_bitmap);
+
+ seq_buf_init(&s, buf, PAGE_SIZE);
+ if (health & PAPR_PMEM_UNARMED_MASK)
+ seq_buf_printf(&s, "not_armed ");
+
+ if (health & PAPR_PMEM_BAD_SHUTDOWN_MASK)
+ seq_buf_printf(&s, "flush_fail ");
+
+ if (health & PAPR_PMEM_BAD_RESTORE_MASK)
+ seq_buf_printf(&s, "restore_fail ");
+
+ if (health & PAPR_PMEM_ENCRYPTED)
+ seq_buf_printf(&s, "encrypted ");
+
+ if (health & PAPR_PMEM_SMART_EVENT_MASK)
+ seq_buf_printf(&s, "smart_notify ");
+
+ if (health & PAPR_PMEM_SCRUBBED_AND_LOCKED)
+ seq_buf_printf(&s, "scrubbed locked ");
+
+ if (seq_buf_used(&s))
+ seq_buf_printf(&s, "\n");
+
+ return seq_buf_used(&s);
+}
+DEVICE_ATTR_RO(flags);
+
+/* papr_scm specific dimm attributes */
+static struct attribute *papr_nd_attributes[] = {
+ &dev_attr_flags.attr,
+ NULL,
+};
+
+static struct attribute_group papr_nd_attribute_group = {
+ .name = "papr",
+ .attrs = papr_nd_attributes,
+};
+
+static const struct attribute_group *papr_nd_attr_groups[] = {
+ &papr_nd_attribute_group,
+ NULL,
+};
+
static int papr_scm_nvdimm_init(struct papr_scm_priv *p)
{
struct device *dev = &p->pdev->dev;
@@ -312,8 +473,8 @@ static int papr_scm_nvdimm_init(struct papr_scm_priv *p)
dimm_flags = 0;
set_bit(NDD_LABELING, &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_nd_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;
@@ -399,6 +560,9 @@ static int papr_scm_probe(struct platform_device *pdev)
if (!p)
return -ENOMEM;

+ /* Initialize the dimm mutex */
+ mutex_init(&p->health_mutex);
+
/* optional DT properties */
of_property_read_u32(dn, "ibm,metadata-size", &metadata_size);

--
2.26.2

2020-06-04 23:45:45

by Vaibhav Jain

[permalink] [raw]
Subject: [PATCH v10 2/6] seq_buf: Export seq_buf_printf

'seq_buf' provides a very useful abstraction for writing to a string
buffer without needing to worry about it over-flowing. However even
though the API has been stable for couple of years now its still not
exported to kernel loadable modules limiting its usage.

Hence this patch proposes update to 'seq_buf.c' to mark
seq_buf_printf() which is part of the seq_buf API to be exported to
kernel loadable GPL modules. This symbol will be used in later parts
of this patch-set to simplify content creation for a sysfs attribute.

Cc: Piotr Maziarz <[email protected]>
Cc: Cezary Rojewski <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Borislav Petkov <[email protected]>
Acked-by: Steven Rostedt (VMware) <[email protected]>
Signed-off-by: Vaibhav Jain <[email protected]>
---
Changelog:

v9..v10:
* None

Resend:
* Added ack from Steven Rostedt

v8..v9:
* None

v7..v8:
* Updated the patch title [ Christoph Hellwig ]
* Updated patch description to replace confusing term 'external kernel
modules' to 'kernel lodable modules'.

Resend:
* Added ack from Steven Rostedt

v6..v7:
* New patch in the series
---
lib/seq_buf.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/lib/seq_buf.c b/lib/seq_buf.c
index 4e865d42ab03..707453f5d58e 100644
--- a/lib/seq_buf.c
+++ b/lib/seq_buf.c
@@ -91,6 +91,7 @@ int seq_buf_printf(struct seq_buf *s, const char *fmt, ...)

return ret;
}
+EXPORT_SYMBOL_GPL(seq_buf_printf);

#ifdef CONFIG_BINARY_PRINTF
/**
--
2.26.2

2020-06-04 23:46:09

by Vaibhav Jain

[permalink] [raw]
Subject: [PATCH v10 4/6] powerpc/papr_scm: Improve error logging and handling papr_scm_ndctl()

Since papr_scm_ndctl() can be called from outside papr_scm, its
exposed to the possibility of receiving NULL as value of 'cmd_rc'
argument. This patch updates papr_scm_ndctl() to protect against such
possibility by assigning it pointer to a local variable in case cmd_rc
== NULL.

Finally the patch also updates the 'default' clause of the switch-case
block removing a 'return' statement thereby ensuring that value of
'cmd_rc' is always logged when papr_scm_ndctl() returns.

Cc: "Aneesh Kumar K . V" <[email protected]>
Cc: Dan Williams <[email protected]>
Cc: Michael Ellerman <[email protected]>
Cc: Ira Weiny <[email protected]>
Signed-off-by: Vaibhav Jain <[email protected]>
---
Changelog:

v9..v10
* New patch in the series
---
arch/powerpc/platforms/pseries/papr_scm.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
index 0c091622b15e..6512fe6a2874 100644
--- a/arch/powerpc/platforms/pseries/papr_scm.c
+++ b/arch/powerpc/platforms/pseries/papr_scm.c
@@ -355,11 +355,16 @@ static int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc,
{
struct nd_cmd_get_config_size *get_size_hdr;
struct papr_scm_priv *p;
+ 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)
+ cmd_rc = &rc;
+
p = nvdimm_provider_data(nvdimm);

switch (cmd) {
@@ -381,12 +386,13 @@ static int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc,
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 ssize_t flags_show(struct device *dev,
--
2.26.2

2020-06-04 23:46:22

by Vaibhav Jain

[permalink] [raw]
Subject: [PATCH v10 6/6] powerpc/papr_scm: Implement support for PAPR_PDSM_HEALTH

This patch implements support for PDSM request 'PAPR_PDSM_HEALTH'
that returns a newly introduced 'struct nd_papr_pdsm_health' instance
containing dimm health information back to user space in response to
ND_CMD_CALL. This functionality is implemented in newly introduced
papr_pdsm_health() that queries the nvdimm health information and
then copies this information to the package payload whose layout is
defined by 'struct nd_papr_pdsm_health'.

Cc: "Aneesh Kumar K . V" <[email protected]>
Cc: Dan Williams <[email protected]>
Cc: Michael Ellerman <[email protected]>
Cc: Ira Weiny <[email protected]>
Signed-off-by: Vaibhav Jain <[email protected]>
---
Changelog:

v9..v10:
* Removed code in papr_pdsm_health that performed validation on pdsm
payload version and corrosponding struct and defines used for
validation of payload version.
* Dropped usage of struct papr_pdsm_health in 'struct
papr_scm_priv'. Instead papr_psdm_health() now uses
'papr_scm_priv.health_bitmap' to populate the pdsm payload.
* Above change also fixes the problem where this patch was removing
the code that was previously introduced in this patch-series.
[ Ira ]
* Introduced a new def ND_PDSM_ENVELOPE_HDR_SIZE that indicates the
space allocated to 'struct nd_pdsm_cmd_pkg' fields except 'struct
nd_cmd_pkg'. This def is useful in validating payload sizes.
* Reworked papr_pdsm_health() to enforce a specific payload size for
'PAPR_PDSM_HEALTH' pdsm request.

Resend:
* Added ack from Aneesh.

v8..v9:
* s/PAPR_SCM_PDSM_HEALTH/PAPR_PDSM_HEALTH/g [ Dan , Aneesh ]
* s/PAPR_SCM_PSDM_DIMM_*/PAPR_PDSM_DIMM_*/g
* Renamed papr_scm_get_health() to papr_psdm_health()
* Updated patch description to replace papr-scm dimm with nvdimm.

v7..v8:
* None

Resend:
* None

v6..v7:
* Updated flags_show() to use seq_buf_printf(). [Mpe]
* Updated papr_scm_get_health() to use newly introduced
__drc_pmem_query_health() bypassing the cache [Mpe].

v5..v6:
* Added attribute '__packed' to 'struct nd_papr_pdsm_health_v1' to
gaurd against possibility of different compilers adding different
paddings to the struct [ Dan Williams ]

* Updated 'struct nd_papr_pdsm_health_v1' to use __u8 instead of
'bool' and also updated drc_pmem_query_health() to take this into
account. [ Dan Williams ]

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_pdsm.h | 33 +++++++++++
arch/powerpc/platforms/pseries/papr_scm.c | 70 +++++++++++++++++++++++
2 files changed, 103 insertions(+)

diff --git a/arch/powerpc/include/uapi/asm/papr_pdsm.h b/arch/powerpc/include/uapi/asm/papr_pdsm.h
index 8b1a4f8fa316..c4c990ede5d4 100644
--- a/arch/powerpc/include/uapi/asm/papr_pdsm.h
+++ b/arch/powerpc/include/uapi/asm/papr_pdsm.h
@@ -71,12 +71,17 @@ struct nd_pdsm_cmd_pkg {
__u8 payload[]; /* In/Out: Sub-cmd data buffer */
} __packed;

+/* Calculate size used by the pdsm header fields minus 'struct nd_cmd_pkg' */
+#define ND_PDSM_ENVELOPE_HDR_SIZE \
+ (sizeof(struct nd_pdsm_cmd_pkg) - sizeof(struct nd_cmd_pkg))
+
/*
* Methods to be embedded in ND_CMD_CALL request. These are sent to the kernel
* via 'nd_pdsm_cmd_pkg.hdr.nd_command' member of the ioctl struct
*/
enum papr_pdsm {
PAPR_PDSM_MIN = 0x0,
+ PAPR_PDSM_HEALTH,
PAPR_PDSM_MAX,
};

@@ -95,4 +100,32 @@ static inline void *pdsm_cmd_to_payload(struct nd_pdsm_cmd_pkg *pcmd)
return (void *)(pcmd->payload);
}

+/* Various nvdimm health indicators */
+#define PAPR_PDSM_DIMM_HEALTHY 0
+#define PAPR_PDSM_DIMM_UNHEALTHY 1
+#define PAPR_PDSM_DIMM_CRITICAL 2
+#define PAPR_PDSM_DIMM_FATAL 3
+
+/*
+ * Struct exchanged between kernel & ndctl in for PAPR_PDSM_HEALTH
+ * Various flags 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. One of PAPR_PDSM_DIMM_XXXX
+ */
+struct nd_papr_pdsm_health {
+ __u8 dimm_unarmed;
+ __u8 dimm_bad_shutdown;
+ __u8 dimm_bad_restore;
+ __u8 dimm_scrubbed;
+ __u8 dimm_locked;
+ __u8 dimm_encrypted;
+ __u16 dimm_health;
+} __packed;
+
#endif /* _UAPI_ASM_POWERPC_PAPR_PDSM_H_ */
diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
index 05eb56ecab5e..984942be24c1 100644
--- a/arch/powerpc/platforms/pseries/papr_scm.c
+++ b/arch/powerpc/platforms/pseries/papr_scm.c
@@ -421,6 +421,72 @@ 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_pdsm_health(struct papr_scm_priv *p,
+ struct nd_pdsm_cmd_pkg *pkg)
+{
+ int rc;
+ struct nd_papr_pdsm_health health = { 0 };
+ u16 copysize = sizeof(struct nd_papr_pdsm_health);
+ u16 payload_size = pkg->hdr.nd_size_out - ND_PDSM_ENVELOPE_HDR_SIZE;
+
+ /* Ensure correct payload size that can hold struct nd_papr_pdsm_health */
+ if (payload_size != copysize) {
+ dev_dbg(&p->pdev->dev,
+ "Unexpected payload-size (%u). Expected (%u)",
+ pkg->hdr.nd_size_out, copysize);
+ rc = -ENOSPC;
+ goto out;
+ }
+
+ /* Ensure dimm health mutex is taken preventing concurrent access */
+ rc = mutex_lock_interruptible(&p->health_mutex);
+ if (rc)
+ goto out;
+
+ /* Always fetch upto date dimm health data ignoring cached values */
+ rc = __drc_pmem_query_health(p);
+ if (rc) {
+ mutex_unlock(&p->health_mutex);
+ goto out;
+ }
+
+ /* update health struct with various flags derived from health bitmap */
+ health = (struct nd_papr_pdsm_health) {
+ .dimm_unarmed = p->health_bitmap & PAPR_PMEM_UNARMED_MASK,
+ .dimm_bad_shutdown = p->health_bitmap & PAPR_PMEM_BAD_SHUTDOWN_MASK,
+ .dimm_bad_restore = p->health_bitmap & PAPR_PMEM_BAD_RESTORE_MASK,
+ .dimm_encrypted = p->health_bitmap & PAPR_PMEM_ENCRYPTED,
+ .dimm_locked = p->health_bitmap & PAPR_PMEM_SCRUBBED_AND_LOCKED,
+ .dimm_scrubbed = p->health_bitmap & PAPR_PMEM_SCRUBBED_AND_LOCKED,
+ .dimm_health = PAPR_PDSM_DIMM_HEALTHY,
+ };
+
+ /* Update field dimm_health based on health_bitmap flags */
+ if (p->health_bitmap & PAPR_PMEM_HEALTH_FATAL)
+ health.dimm_health = PAPR_PDSM_DIMM_FATAL;
+ else if (p->health_bitmap & PAPR_PMEM_HEALTH_CRITICAL)
+ health.dimm_health = PAPR_PDSM_DIMM_CRITICAL;
+ else if (p->health_bitmap & PAPR_PMEM_HEALTH_UNHEALTHY)
+ health.dimm_health = PAPR_PDSM_DIMM_UNHEALTHY;
+
+ /* struct populated hence can release the mutex now */
+ mutex_unlock(&p->health_mutex);
+
+ dev_dbg(&p->pdev->dev, "Copying payload size=%u\n", copysize);
+
+ /* Copy the health struct to the payload */
+ memcpy(pdsm_cmd_to_payload(pkg), &health, copysize);
+
+ /* Update fw size including size of struct nd_pdsm_cmd_pkg fields */
+ pkg->hdr.nd_fw_size = copysize + ND_PDSM_ENVELOPE_HDR_SIZE;
+
+out:
+ dev_dbg(&p->pdev->dev, "completion code = %d\n", rc);
+
+ return rc;
+}
+
/*
* For a given pdsm request call an appropriate service function.
* Note: Use 'nd_pdsm_cmd_pkg.cmd_status to report psdm servicing errors. Hence
@@ -435,6 +501,10 @@ static void papr_scm_service_pdsm(struct papr_scm_priv *p,

/* Call pdsm service function */
switch (pdsm) {
+ case PAPR_PDSM_HEALTH:
+ pkg->cmd_status = papr_pdsm_health(p, pkg);
+ break;
+
default:
dev_dbg(&p->pdev->dev, "PDSM[0x%x]: Unsupported PDSM request\n",
pdsm);
--
2.26.2

2020-06-05 17:15:22

by Ira Weiny

[permalink] [raw]
Subject: Re: [PATCH v10 4/6] powerpc/papr_scm: Improve error logging and handling papr_scm_ndctl()

On Fri, Jun 05, 2020 at 05:11:34AM +0530, Vaibhav Jain wrote:
> Since papr_scm_ndctl() can be called from outside papr_scm, its
> exposed to the possibility of receiving NULL as value of 'cmd_rc'
> argument. This patch updates papr_scm_ndctl() to protect against such
> possibility by assigning it pointer to a local variable in case cmd_rc
> == NULL.
>
> Finally the patch also updates the 'default' clause of the switch-case
> block removing a 'return' statement thereby ensuring that value of
> 'cmd_rc' is always logged when papr_scm_ndctl() returns.
>
> Cc: "Aneesh Kumar K . V" <[email protected]>
> Cc: Dan Williams <[email protected]>
> Cc: Michael Ellerman <[email protected]>
> Cc: Ira Weiny <[email protected]>
> Signed-off-by: Vaibhav Jain <[email protected]>
> ---
> Changelog:
>
> v9..v10
> * New patch in the series

Thanks for making this a separate patch it is easier to see what is going on
here.

> ---
> arch/powerpc/platforms/pseries/papr_scm.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> index 0c091622b15e..6512fe6a2874 100644
> --- a/arch/powerpc/platforms/pseries/papr_scm.c
> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> @@ -355,11 +355,16 @@ static int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc,
> {
> struct nd_cmd_get_config_size *get_size_hdr;
> struct papr_scm_priv *p;
> + 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)
> + cmd_rc = &rc;
> +

This protects you from the NULL. However...

> p = nvdimm_provider_data(nvdimm);
>
> switch (cmd) {
> @@ -381,12 +386,13 @@ static int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc,
> break;
>
> default:
> - return -EINVAL;
> + dev_dbg(&p->pdev->dev, "Unknown command = %d\n", cmd);
> + *cmd_rc = -EINVAL;

... I think you are conflating rc and cmd_rc...

> }
>
> dev_dbg(&p->pdev->dev, "returned with cmd_rc = %d\n", *cmd_rc);
>
> - return 0;
> + return *cmd_rc;

... this changes the behavior of the current commands. Now if the underlying
papr_scm_meta_[get|set]() fails you return that failure as rc rather than 0.

Is that ok?

Also 'logging cmd_rc' in the invalid cmd case does not seem quite right unless
you really want rc to be cmd_rc.

The architecture is designed to separate errors which occur in the kernel vs
errors in the firmware/dimm. Are they always the same? The current code
differentiates them.

Ira

> }
>
> static ssize_t flags_show(struct device *dev,
> --
> 2.26.2
>

2020-06-05 18:40:21

by Ira Weiny

[permalink] [raw]
Subject: Re: [PATCH v10 6/6] powerpc/papr_scm: Implement support for PAPR_PDSM_HEALTH

On Fri, Jun 05, 2020 at 05:11:36AM +0530, Vaibhav Jain wrote:
> This patch implements support for PDSM request 'PAPR_PDSM_HEALTH'
> that returns a newly introduced 'struct nd_papr_pdsm_health' instance
> containing dimm health information back to user space in response to
> ND_CMD_CALL. This functionality is implemented in newly introduced
> papr_pdsm_health() that queries the nvdimm health information and
> then copies this information to the package payload whose layout is
> defined by 'struct nd_papr_pdsm_health'.
>
> Cc: "Aneesh Kumar K . V" <[email protected]>
> Cc: Dan Williams <[email protected]>
> Cc: Michael Ellerman <[email protected]>
> Cc: Ira Weiny <[email protected]>
> Signed-off-by: Vaibhav Jain <[email protected]>
> ---
> Changelog:
>
> v9..v10:
> * Removed code in papr_pdsm_health that performed validation on pdsm
> payload version and corrosponding struct and defines used for
> validation of payload version.
> * Dropped usage of struct papr_pdsm_health in 'struct
> papr_scm_priv'. Instead papr_psdm_health() now uses
> 'papr_scm_priv.health_bitmap' to populate the pdsm payload.
> * Above change also fixes the problem where this patch was removing
> the code that was previously introduced in this patch-series.
> [ Ira ]
> * Introduced a new def ND_PDSM_ENVELOPE_HDR_SIZE that indicates the
> space allocated to 'struct nd_pdsm_cmd_pkg' fields except 'struct
> nd_cmd_pkg'. This def is useful in validating payload sizes.
> * Reworked papr_pdsm_health() to enforce a specific payload size for
> 'PAPR_PDSM_HEALTH' pdsm request.
>
> Resend:
> * Added ack from Aneesh.
>
> v8..v9:
> * s/PAPR_SCM_PDSM_HEALTH/PAPR_PDSM_HEALTH/g [ Dan , Aneesh ]
> * s/PAPR_SCM_PSDM_DIMM_*/PAPR_PDSM_DIMM_*/g
> * Renamed papr_scm_get_health() to papr_psdm_health()
> * Updated patch description to replace papr-scm dimm with nvdimm.
>
> v7..v8:
> * None
>
> Resend:
> * None
>
> v6..v7:
> * Updated flags_show() to use seq_buf_printf(). [Mpe]
> * Updated papr_scm_get_health() to use newly introduced
> __drc_pmem_query_health() bypassing the cache [Mpe].
>
> v5..v6:
> * Added attribute '__packed' to 'struct nd_papr_pdsm_health_v1' to
> gaurd against possibility of different compilers adding different
> paddings to the struct [ Dan Williams ]
>
> * Updated 'struct nd_papr_pdsm_health_v1' to use __u8 instead of
> 'bool' and also updated drc_pmem_query_health() to take this into
> account. [ Dan Williams ]
>
> 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_pdsm.h | 33 +++++++++++
> arch/powerpc/platforms/pseries/papr_scm.c | 70 +++++++++++++++++++++++
> 2 files changed, 103 insertions(+)
>
> diff --git a/arch/powerpc/include/uapi/asm/papr_pdsm.h b/arch/powerpc/include/uapi/asm/papr_pdsm.h
> index 8b1a4f8fa316..c4c990ede5d4 100644
> --- a/arch/powerpc/include/uapi/asm/papr_pdsm.h
> +++ b/arch/powerpc/include/uapi/asm/papr_pdsm.h
> @@ -71,12 +71,17 @@ struct nd_pdsm_cmd_pkg {
> __u8 payload[]; /* In/Out: Sub-cmd data buffer */
> } __packed;
>
> +/* Calculate size used by the pdsm header fields minus 'struct nd_cmd_pkg' */
> +#define ND_PDSM_ENVELOPE_HDR_SIZE \
> + (sizeof(struct nd_pdsm_cmd_pkg) - sizeof(struct nd_cmd_pkg))
> +

This is kind of a weird name for this.

Isn't this just the ND PDSM header size? What is 'envelope' mean here?

> /*
> * Methods to be embedded in ND_CMD_CALL request. These are sent to the kernel
> * via 'nd_pdsm_cmd_pkg.hdr.nd_command' member of the ioctl struct
> */
> enum papr_pdsm {
> PAPR_PDSM_MIN = 0x0,
> + PAPR_PDSM_HEALTH,
> PAPR_PDSM_MAX,
> };
>
> @@ -95,4 +100,32 @@ static inline void *pdsm_cmd_to_payload(struct nd_pdsm_cmd_pkg *pcmd)
> return (void *)(pcmd->payload);
> }
>
> +/* Various nvdimm health indicators */
> +#define PAPR_PDSM_DIMM_HEALTHY 0
> +#define PAPR_PDSM_DIMM_UNHEALTHY 1
> +#define PAPR_PDSM_DIMM_CRITICAL 2
> +#define PAPR_PDSM_DIMM_FATAL 3
> +
> +/*
> + * Struct exchanged between kernel & ndctl in for PAPR_PDSM_HEALTH
> + * Various flags 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. One of PAPR_PDSM_DIMM_XXXX
> + */
> +struct nd_papr_pdsm_health {
> + __u8 dimm_unarmed;
> + __u8 dimm_bad_shutdown;
> + __u8 dimm_bad_restore;
> + __u8 dimm_scrubbed;
> + __u8 dimm_locked;
> + __u8 dimm_encrypted;
> + __u16 dimm_health;
> +} __packed;
> +
> #endif /* _UAPI_ASM_POWERPC_PAPR_PDSM_H_ */
> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> index 05eb56ecab5e..984942be24c1 100644
> --- a/arch/powerpc/platforms/pseries/papr_scm.c
> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> @@ -421,6 +421,72 @@ 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_pdsm_health(struct papr_scm_priv *p,
> + struct nd_pdsm_cmd_pkg *pkg)
> +{
> + int rc;
> + struct nd_papr_pdsm_health health = { 0 };
> + u16 copysize = sizeof(struct nd_papr_pdsm_health);
> + u16 payload_size = pkg->hdr.nd_size_out - ND_PDSM_ENVELOPE_HDR_SIZE;
> +
> + /* Ensure correct payload size that can hold struct nd_papr_pdsm_health */
> + if (payload_size != copysize) {
> + dev_dbg(&p->pdev->dev,
> + "Unexpected payload-size (%u). Expected (%u)",
> + pkg->hdr.nd_size_out, copysize);
> + rc = -ENOSPC;
> + goto out;
> + }
> +
> + /* Ensure dimm health mutex is taken preventing concurrent access */
> + rc = mutex_lock_interruptible(&p->health_mutex);
> + if (rc)
> + goto out;
> +
> + /* Always fetch upto date dimm health data ignoring cached values */
> + rc = __drc_pmem_query_health(p);
> + if (rc) {
> + mutex_unlock(&p->health_mutex);
> + goto out;
> + }
> +
> + /* update health struct with various flags derived from health bitmap */
> + health = (struct nd_papr_pdsm_health) {
> + .dimm_unarmed = p->health_bitmap & PAPR_PMEM_UNARMED_MASK,
> + .dimm_bad_shutdown = p->health_bitmap & PAPR_PMEM_BAD_SHUTDOWN_MASK,
> + .dimm_bad_restore = p->health_bitmap & PAPR_PMEM_BAD_RESTORE_MASK,
> + .dimm_encrypted = p->health_bitmap & PAPR_PMEM_ENCRYPTED,
> + .dimm_locked = p->health_bitmap & PAPR_PMEM_SCRUBBED_AND_LOCKED,
> + .dimm_scrubbed = p->health_bitmap & PAPR_PMEM_SCRUBBED_AND_LOCKED,

Are you sure these work? These are not assignments to a bool so I don't think
gcc will do what you want here.

Ira

> + .dimm_health = PAPR_PDSM_DIMM_HEALTHY,
> + };
> +
> + /* Update field dimm_health based on health_bitmap flags */
> + if (p->health_bitmap & PAPR_PMEM_HEALTH_FATAL)
> + health.dimm_health = PAPR_PDSM_DIMM_FATAL;
> + else if (p->health_bitmap & PAPR_PMEM_HEALTH_CRITICAL)
> + health.dimm_health = PAPR_PDSM_DIMM_CRITICAL;
> + else if (p->health_bitmap & PAPR_PMEM_HEALTH_UNHEALTHY)
> + health.dimm_health = PAPR_PDSM_DIMM_UNHEALTHY;
> +
> + /* struct populated hence can release the mutex now */
> + mutex_unlock(&p->health_mutex);
> +
> + dev_dbg(&p->pdev->dev, "Copying payload size=%u\n", copysize);
> +
> + /* Copy the health struct to the payload */
> + memcpy(pdsm_cmd_to_payload(pkg), &health, copysize);
> +
> + /* Update fw size including size of struct nd_pdsm_cmd_pkg fields */
> + pkg->hdr.nd_fw_size = copysize + ND_PDSM_ENVELOPE_HDR_SIZE;
> +
> +out:
> + dev_dbg(&p->pdev->dev, "completion code = %d\n", rc);
> +
> + return rc;
> +}
> +
> /*
> * For a given pdsm request call an appropriate service function.
> * Note: Use 'nd_pdsm_cmd_pkg.cmd_status to report psdm servicing errors. Hence
> @@ -435,6 +501,10 @@ static void papr_scm_service_pdsm(struct papr_scm_priv *p,
>
> /* Call pdsm service function */
> switch (pdsm) {
> + case PAPR_PDSM_HEALTH:
> + pkg->cmd_status = papr_pdsm_health(p, pkg);
> + break;
> +
> default:
> dev_dbg(&p->pdev->dev, "PDSM[0x%x]: Unsupported PDSM request\n",
> pdsm);
> --
> 2.26.2
>

2020-06-05 19:52:03

by Dan Williams

[permalink] [raw]
Subject: Re: [PATCH v10 4/6] powerpc/papr_scm: Improve error logging and handling papr_scm_ndctl()

On Fri, Jun 5, 2020 at 10:13 AM Ira Weiny <[email protected]> wrote:
>
> On Fri, Jun 05, 2020 at 05:11:34AM +0530, Vaibhav Jain wrote:
> > Since papr_scm_ndctl() can be called from outside papr_scm, its
> > exposed to the possibility of receiving NULL as value of 'cmd_rc'
> > argument. This patch updates papr_scm_ndctl() to protect against such
> > possibility by assigning it pointer to a local variable in case cmd_rc
> > == NULL.
> >
> > Finally the patch also updates the 'default' clause of the switch-case
> > block removing a 'return' statement thereby ensuring that value of
> > 'cmd_rc' is always logged when papr_scm_ndctl() returns.
> >
> > Cc: "Aneesh Kumar K . V" <[email protected]>
> > Cc: Dan Williams <[email protected]>
> > Cc: Michael Ellerman <[email protected]>
> > Cc: Ira Weiny <[email protected]>
> > Signed-off-by: Vaibhav Jain <[email protected]>
> > ---
> > Changelog:
> >
> > v9..v10
> > * New patch in the series
>
> Thanks for making this a separate patch it is easier to see what is going on
> here.
>
> > ---
> > arch/powerpc/platforms/pseries/papr_scm.c | 10 ++++++++--
> > 1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> > index 0c091622b15e..6512fe6a2874 100644
> > --- a/arch/powerpc/platforms/pseries/papr_scm.c
> > +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> > @@ -355,11 +355,16 @@ static int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc,
> > {
> > struct nd_cmd_get_config_size *get_size_hdr;
> > struct papr_scm_priv *p;
> > + 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)
> > + cmd_rc = &rc;
> > +
>
> This protects you from the NULL. However...
>
> > p = nvdimm_provider_data(nvdimm);
> >
> > switch (cmd) {
> > @@ -381,12 +386,13 @@ static int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc,
> > break;
> >
> > default:
> > - return -EINVAL;
> > + dev_dbg(&p->pdev->dev, "Unknown command = %d\n", cmd);
> > + *cmd_rc = -EINVAL;
>
> ... I think you are conflating rc and cmd_rc...
>
> > }
> >
> > dev_dbg(&p->pdev->dev, "returned with cmd_rc = %d\n", *cmd_rc);
> >
> > - return 0;
> > + return *cmd_rc;
>
> ... this changes the behavior of the current commands. Now if the underlying
> papr_scm_meta_[get|set]() fails you return that failure as rc rather than 0.
>
> Is that ok?

The expectation is that rc is "did the command get sent to the device,
or did it fail for 'transport' reasons". The role of cmd_rc is to
translate the specific status response of the command into a common
error code. The expectations are:

rc < 0: Error code, Linux terminated the ioctl before talking to hardware

rc == 0: Linux successfully submitted the command to hardware, cmd_rc
is valid for command specific response

rc > 0: Linux successfully submitted the command, but detected that
only a subset of the data was accepted for "write"-style commands, or
that only subset of data was returned for "read"-style commands. I.e.
short-write / short-read semantics. cmd_rc is valid in this case and
its up to userspace to determine if a short transfer is an error or
not.

> Also 'logging cmd_rc' in the invalid cmd case does not seem quite right unless
> you really want rc to be cmd_rc.
>
> The architecture is designed to separate errors which occur in the kernel vs
> errors in the firmware/dimm. Are they always the same? The current code
> differentiates them.

Yeah, they're distinct, transport vs end-point / command-specific
status returns.

2020-06-05 19:52:08

by Ira Weiny

[permalink] [raw]
Subject: Re: [PATCH v10 5/6] ndctl/papr_scm,uapi: Add support for PAPR nvdimm specific methods

On Fri, Jun 05, 2020 at 05:11:35AM +0530, Vaibhav Jain wrote:
> Introduce support for PAPR NVDIMM Specific Methods (PDSM) in papr_scm
> module and add the command family NVDIMM_FAMILY_PAPR to the white list
> of NVDIMM command sets. Also advertise support for ND_CMD_CALL for the
> nvdimm command mask and implement necessary scaffolding in the module
> to handle ND_CMD_CALL ioctl and PDSM requests that we receive.
>
> The layout of the PDSM request as we expect from libnvdimm/libndctl is
> described in newly introduced uapi header 'papr_pdsm.h' which
> defines a new 'struct nd_pdsm_cmd_pkg' header. This header is used
> to communicate the PDSM request via member
> 'nd_cmd_pkg.nd_command' and size of payload that need to be
> sent/received for servicing the PDSM.
>
> A new function is_cmd_valid() is implemented that reads the args to
> papr_scm_ndctl() and performs sanity tests on them. A new function
> papr_scm_service_pdsm() is introduced and is called from
> papr_scm_ndctl() in case of a PDSM request is received via ND_CMD_CALL
> command from libnvdimm.
>
> Cc: "Aneesh Kumar K . V" <[email protected]>
> Cc: Dan Williams <[email protected]>
> Cc: Michael Ellerman <[email protected]>
> Cc: Ira Weiny <[email protected]>
> Signed-off-by: Vaibhav Jain <[email protected]>
> ---
> Changelog:
>
> v9..v10:
> * Simplified 'struct nd_pdsm_cmd_pkg' by removing the
> 'payload_version' field.
> * Removed the corrosponding documentation on versioning and backward
> compatibility from 'papr_pdsm.h'
> * Reduced the size of reserved fields to 4-bytes making 'struct
> nd_pdsm_cmd_pkg' 64 + 8 bytes long.
> * Updated is_cmd_valid() to enforce validation checks on pdsm
> commands. [ Dan Williams ]
> * Added check for reserved fields being set to '0' in is_cmd_valid()
> [ Ira ]
> * Moved changes for checking cmd_rc == NULL and logging improvements
> to a separate prelim patch [ Ira ].
> * Moved pdsm package validation checks from papr_scm_service_pdsm()
> to is_cmd_valid().
> * Marked papr_scm_service_pdsm() return type as 'void' since errors
> are reported in nd_pdsm_cmd_pkg.cmd_status field.
>
> Resend:
> * Added ack from Aneesh.
>
> v8..v9:
> * Reduced the usage of term SCM replacing it with appropriate
> replacement [ Dan Williams, Aneesh ]
> * Renamed 'papr_scm_pdsm.h' to 'papr_pdsm.h'
> * s/PAPR_SCM_PDSM_*/PAPR_PDSM_*/g
> * s/NVDIMM_FAMILY_PAPR_SCM/NVDIMM_FAMILY_PAPR/g
> * Minor updates to 'papr_psdm.h' to replace usage of term 'SCM'.
> * Minor update to patch description.
>
> v7..v8:
> * Removed the 'payload_offset' field from 'struct
> nd_pdsm_cmd_pkg'. Instead command payload is always assumed to start
> at 'nd_pdsm_cmd_pkg.payload'. [ Aneesh ]
> * To enable introducing new fields to 'struct nd_pdsm_cmd_pkg',
> 'reserved' field of 10-bytes is introduced. [ Aneesh ]
> * Fixed a typo in "Backward Compatibility" section of papr_scm_pdsm.h
> [ Ira ]
>
> Resend:
> * None
>
> v6..v7 :
> * Removed the re-definitions of __packed macro from papr_scm_pdsm.h
> [Mpe].
> * Removed the usage of __KERNEL__ macros in papr_scm_pdsm.h [Mpe].
> * Removed macros that were unused in papr_scm.c from papr_scm_pdsm.h
> [Mpe].
> * Made functions defined in papr_scm_pdsm.h as static inline. [Mpe]
>
> v5..v6 :
> * Changed the usage of the term DSM to PDSM to distinguish it from the
> ACPI term [ Dan Williams ]
> * Renamed papr_scm_dsm.h to papr_scm_pdsm.h and updated various struct
> to reflect the new terminology.
> * Updated the patch description and title to reflect the new terminology.
> * Squashed patch to introduce new command family in 'ndctl.h' with
> this patch [ Dan Williams ]
> * Updated the papr_scm_pdsm method starting index from 0x10000 to 0x0
> [ Dan Williams ]
> * Removed redundant license text from the papr_scm_psdm.h file.
> [ Dan Williams ]
> * s/envelop/envelope/ at various places [ Dan Williams ]
> * Added '__packed' attribute to command package header to gaurd
> against different compiler adding paddings between the fields.
> [ Dan Williams]
> * Converted various pr_debug to dev_debug [ Dan Williams ]
>
> v4..v5 :
> * None
>
> v3..v4 :
> * None
>
> v2..v3 :
> * Updated the patch prefix to 'ndctl/uapi' [Aneesh]
>
> v1..v2 :
> * None
> ---
> arch/powerpc/include/uapi/asm/papr_pdsm.h | 98 +++++++++++++++++++
> arch/powerpc/platforms/pseries/papr_scm.c | 113 +++++++++++++++++++++-
> include/uapi/linux/ndctl.h | 1 +
> 3 files changed, 207 insertions(+), 5 deletions(-)
> create mode 100644 arch/powerpc/include/uapi/asm/papr_pdsm.h
>
> diff --git a/arch/powerpc/include/uapi/asm/papr_pdsm.h b/arch/powerpc/include/uapi/asm/papr_pdsm.h
> new file mode 100644
> index 000000000000..8b1a4f8fa316
> --- /dev/null
> +++ b/arch/powerpc/include/uapi/asm/papr_pdsm.h
> @@ -0,0 +1,98 @@
> +/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
> +/*
> + * PAPR nvDimm Specific Methods (PDSM) and structs for libndctl
> + *
> + * (C) Copyright IBM 2020
> + *
> + * Author: Vaibhav Jain <vaibhav at linux.ibm.com>
> + */
> +
> +#ifndef _UAPI_ASM_POWERPC_PAPR_PDSM_H_
> +#define _UAPI_ASM_POWERPC_PAPR_PDSM_H_
> +
> +#include <linux/types.h>
> +
> +/*
> + * PDSM Envelope:
> + *
> + * The ioctl ND_CMD_CALL transfers data between user-space and kernel via
> + * envelope which consists of a header and user-defined payload sections.
> + * The header is described by 'struct nd_pdsm_cmd_pkg' which expects a
> + * payload following it and accessible via 'nd_pdsm_cmd_pkg.payload' field.
> + * There is reserved field that can used to introduce new fields to the
> + * structure in future. It also tries to ensure that 'nd_pdsm_cmd_pkg.payload'
> + * lies at a 8-byte boundary.
> + *
> + * +-------------+---------------------+---------------------------+
> + * | 64-Bytes | 8-Bytes | Max 184-Bytes |
> + * +-------------+---------------------+---------------------------+
> + * | nd_pdsm_cmd_pkg | |
> + * |-------------+ | |
> + * | nd_cmd_pkg | | |
> + * +-------------+---------------------+---------------------------+
> + * | nd_family | | |
> + * | nd_size_out | cmd_status | |
> + * | nd_size_in | reserved | payload |
> + * | nd_command | | |
> + * | nd_fw_size | | |
> + * +-------------+---------------------+---------------------------+
> + *
> + * PDSM Header:
> + *
> + * The header is defined as 'struct nd_pdsm_cmd_pkg' which embeds a
> + * 'struct nd_cmd_pkg' instance. The PDSM command is assigned to member
> + * 'nd_cmd_pkg.nd_command'. Apart from size information of the envelope which is
> + * contained in 'struct nd_cmd_pkg', the header also has members following
> + * members:
> + *
> + * 'cmd_status' : (Out) Errors if any encountered while servicing PDSM.
> + * 'reserved' : Not used, reserved for future and should be set to 0.
> + *
> + * PDSM Payload:
> + *
> + * The layout of the PDSM Payload is defined by various structs shared between
> + * papr_scm and libndctl so that contents of payload can be interpreted. During
> + * servicing of a PDSM the papr_scm module will read input args from the payload
> + * field by casting its contents to an appropriate struct pointer based on the
> + * PDSM command. Similarly the output of servicing the PDSM 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).
> + *
> + */
> +
> +/* PDSM-header + payload expected with ND_CMD_CALL ioctl from libnvdimm */
> +struct nd_pdsm_cmd_pkg {
> + struct nd_cmd_pkg hdr; /* Package header containing sub-cmd */
> + __s32 cmd_status; /* Out: Sub-cmd status returned back */
> + __u16 reserved[2]; /* Ignored and to be used in future */
> + __u8 payload[]; /* In/Out: Sub-cmd data buffer */
> +} __packed;
> +
> +/*
> + * Methods to be embedded in ND_CMD_CALL request. These are sent to the kernel
> + * via 'nd_pdsm_cmd_pkg.hdr.nd_command' member of the ioctl struct
> + */
> +enum papr_pdsm {
> + PAPR_PDSM_MIN = 0x0,
> + PAPR_PDSM_MAX,
> +};
> +
> +/* Convert a libnvdimm nd_cmd_pkg to pdsm specific pkg */
> +static inline struct nd_pdsm_cmd_pkg *nd_to_pdsm_cmd_pkg(struct nd_cmd_pkg *cmd)
> +{
> + return (struct nd_pdsm_cmd_pkg *)cmd;
> +}
> +
> +/* Return the payload pointer for a given pcmd */
> +static inline void *pdsm_cmd_to_payload(struct nd_pdsm_cmd_pkg *pcmd)
> +{
> + if (pcmd->hdr.nd_size_in == 0 && pcmd->hdr.nd_size_out == 0)
> + return NULL;
> + else
> + return (void *)(pcmd->payload);
> +}

I just realized these were in the uapi header. You don't want to do this as
this code gets built into any user space program that uses it and there may be
license issues if the user space code is not a compatible license.

Ira

> +
> +#endif /* _UAPI_ASM_POWERPC_PAPR_PDSM_H_ */
> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> index 6512fe6a2874..05eb56ecab5e 100644
> --- a/arch/powerpc/platforms/pseries/papr_scm.c
> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> @@ -15,13 +15,15 @@
> #include <linux/seq_buf.h>
>
> #include <asm/plpar_wrappers.h>
> +#include <asm/papr_pdsm.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))
>
> /* DIMM health bitmap bitmap indicators */
> /* SCM device is unable to persist memory contents */
> @@ -349,22 +351,117 @@ static int papr_scm_meta_set(struct papr_scm_priv *p,
> return 0;
> }
>
> +/*
> + * Do a sanity checks on the inputs args to dimm-control function and return
> + * '0' if valid. This also does validation on 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_pdsm_cmd_pkg *pkg;
> + struct nd_cmd_pkg *nd_cmd;
> + struct papr_scm_priv *p;
> + enum papr_pdsm pdsm;
> +
> + /* Only dimm-specific calls are supported atm */
> + if (!nvdimm)
> + return -EINVAL;
> +
> + /* get the provider data from struct nvdimm */
> + p = nvdimm_provider_data(nvdimm);
> +
> + if (!test_bit(cmd, &cmd_mask)) {
> + dev_dbg(&p->pdev->dev, "Unsupported cmd=%u\n", cmd);
> + return -EINVAL;
> + }
> +
> + /* For CMD_CALL verify pdsm request */
> + if (cmd == ND_CMD_CALL) {
> + /* Verify the envelope package */
> + if (!buf || buf_len < sizeof(struct nd_pdsm_cmd_pkg)) {
> + dev_dbg(&p->pdev->dev, "Invalid pkg size=%u\n",
> + buf_len);
> + return -EINVAL;
> + }
> +
> + /* Verify that the nd_cmd_pkg.nd_family is correct */
> + nd_cmd = (struct nd_cmd_pkg *)buf;
> + if (nd_cmd->nd_family != NVDIMM_FAMILY_PAPR) {
> + dev_dbg(&p->pdev->dev, "Invalid pkg family=0x%llx\n",
> + nd_cmd->nd_family);
> + return -EINVAL;
> + }
> +
> + /* Get the pdsm request package and the command */
> + pkg = nd_to_pdsm_cmd_pkg(nd_cmd);
> + pdsm = pkg->hdr.nd_command;
> +
> + /* Verify if the psdm command is valid */
> + if (pdsm <= PAPR_PDSM_MIN || pdsm >= PAPR_PDSM_MAX) {
> + dev_dbg(&p->pdev->dev, "PDSM[0x%x]: Invalid PDSM\n", pdsm);
> + return -EINVAL;
> + }
> +
> + /* We except a payload with all PDSM commands */
> + if (!pdsm_cmd_to_payload(pkg)) {
> + dev_dbg(&p->pdev->dev, "PDSM[0x%x]: Empty payload\n", pdsm);
> + return -EINVAL;
> + }
> +
> + /* Ensure reserved fields of the pdsm header are set to 0 */
> + if (pkg->reserved[0] || pkg->reserved[1]) {
> + dev_dbg(&p->pdev->dev,
> + "PDSM[0x%x]: Invalid reserved field usage\n", pdsm);
> + return -EINVAL;
> + }
> + }
> +
> + /* Let the command be further processed */
> + return 0;
> +}
> +
> +/*
> + * For a given pdsm request call an appropriate service function.
> + * Note: Use 'nd_pdsm_cmd_pkg.cmd_status to report psdm servicing errors. Hence
> + * function marked as void.
> + */
> +static void papr_scm_service_pdsm(struct papr_scm_priv *p,
> + struct nd_pdsm_cmd_pkg *pkg)
> +{
> + const enum papr_pdsm pdsm = pkg->hdr.nd_command;
> +
> + dev_dbg(&p->pdev->dev, "PDSM[0x%x]: Servicing..\n", pdsm);
> +
> + /* Call pdsm service function */
> + switch (pdsm) {
> + default:
> + dev_dbg(&p->pdev->dev, "PDSM[0x%x]: Unsupported PDSM request\n",
> + pdsm);
> + pkg->cmd_status = -ENOENT;
> + break;
> + }
> +}
> +
> static 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 nd_pdsm_cmd_pkg *call_pkg = NULL;
> struct papr_scm_priv *p;
> 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)
> cmd_rc = &rc;
>
> + *cmd_rc = is_cmd_valid(nvdimm, cmd, buf, buf_len);
> + if (*cmd_rc) {
> + pr_debug("Invalid cmd=0x%x. Err=%d\n", cmd, *cmd_rc);
> + return *cmd_rc;
> + }
> +
> p = nvdimm_provider_data(nvdimm);
>
> switch (cmd) {
> @@ -385,6 +482,12 @@ static int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc,
> *cmd_rc = papr_scm_meta_set(p, buf);
> break;
>
> + case ND_CMD_CALL:
> + call_pkg = nd_to_pdsm_cmd_pkg(buf);
> + papr_scm_service_pdsm(p, call_pkg);
> + *cmd_rc = 0;
> + break;
> +
> default:
> dev_dbg(&p->pdev->dev, "Unknown command = %d\n", cmd);
> *cmd_rc = -EINVAL;
> diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h
> index de5d90212409..0e09dc5cec19 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 5
>
> #define ND_IOCTL_CALL _IOWR(ND_IOCTL, ND_CMD_CALL,\
> struct nd_cmd_pkg)
> --
> 2.26.2
>

2020-06-05 21:08:03

by Dan Williams

[permalink] [raw]
Subject: Re: [PATCH v10 5/6] ndctl/papr_scm,uapi: Add support for PAPR nvdimm specific methods

On Fri, Jun 5, 2020 at 12:50 PM Ira Weiny <[email protected]> wrote:
>
> On Fri, Jun 05, 2020 at 05:11:35AM +0530, Vaibhav Jain wrote:
> > Introduce support for PAPR NVDIMM Specific Methods (PDSM) in papr_scm
> > module and add the command family NVDIMM_FAMILY_PAPR to the white list
> > of NVDIMM command sets. Also advertise support for ND_CMD_CALL for the
> > nvdimm command mask and implement necessary scaffolding in the module
> > to handle ND_CMD_CALL ioctl and PDSM requests that we receive.
> >
> > The layout of the PDSM request as we expect from libnvdimm/libndctl is
> > described in newly introduced uapi header 'papr_pdsm.h' which
> > defines a new 'struct nd_pdsm_cmd_pkg' header. This header is used
> > to communicate the PDSM request via member
> > 'nd_cmd_pkg.nd_command' and size of payload that need to be
> > sent/received for servicing the PDSM.
> >
> > A new function is_cmd_valid() is implemented that reads the args to
> > papr_scm_ndctl() and performs sanity tests on them. A new function
> > papr_scm_service_pdsm() is introduced and is called from
> > papr_scm_ndctl() in case of a PDSM request is received via ND_CMD_CALL
> > command from libnvdimm.
> >
> > Cc: "Aneesh Kumar K . V" <[email protected]>
> > Cc: Dan Williams <[email protected]>
> > Cc: Michael Ellerman <[email protected]>
> > Cc: Ira Weiny <[email protected]>
> > Signed-off-by: Vaibhav Jain <[email protected]>
> > ---
> > Changelog:
> >
> > v9..v10:
> > * Simplified 'struct nd_pdsm_cmd_pkg' by removing the
> > 'payload_version' field.
> > * Removed the corrosponding documentation on versioning and backward
> > compatibility from 'papr_pdsm.h'
> > * Reduced the size of reserved fields to 4-bytes making 'struct
> > nd_pdsm_cmd_pkg' 64 + 8 bytes long.
> > * Updated is_cmd_valid() to enforce validation checks on pdsm
> > commands. [ Dan Williams ]
> > * Added check for reserved fields being set to '0' in is_cmd_valid()
> > [ Ira ]
> > * Moved changes for checking cmd_rc == NULL and logging improvements
> > to a separate prelim patch [ Ira ].
> > * Moved pdsm package validation checks from papr_scm_service_pdsm()
> > to is_cmd_valid().
> > * Marked papr_scm_service_pdsm() return type as 'void' since errors
> > are reported in nd_pdsm_cmd_pkg.cmd_status field.
> >
> > Resend:
> > * Added ack from Aneesh.
> >
> > v8..v9:
> > * Reduced the usage of term SCM replacing it with appropriate
> > replacement [ Dan Williams, Aneesh ]
> > * Renamed 'papr_scm_pdsm.h' to 'papr_pdsm.h'
> > * s/PAPR_SCM_PDSM_*/PAPR_PDSM_*/g
> > * s/NVDIMM_FAMILY_PAPR_SCM/NVDIMM_FAMILY_PAPR/g
> > * Minor updates to 'papr_psdm.h' to replace usage of term 'SCM'.
> > * Minor update to patch description.
> >
> > v7..v8:
> > * Removed the 'payload_offset' field from 'struct
> > nd_pdsm_cmd_pkg'. Instead command payload is always assumed to start
> > at 'nd_pdsm_cmd_pkg.payload'. [ Aneesh ]
> > * To enable introducing new fields to 'struct nd_pdsm_cmd_pkg',
> > 'reserved' field of 10-bytes is introduced. [ Aneesh ]
> > * Fixed a typo in "Backward Compatibility" section of papr_scm_pdsm.h
> > [ Ira ]
> >
> > Resend:
> > * None
> >
> > v6..v7 :
> > * Removed the re-definitions of __packed macro from papr_scm_pdsm.h
> > [Mpe].
> > * Removed the usage of __KERNEL__ macros in papr_scm_pdsm.h [Mpe].
> > * Removed macros that were unused in papr_scm.c from papr_scm_pdsm.h
> > [Mpe].
> > * Made functions defined in papr_scm_pdsm.h as static inline. [Mpe]
> >
> > v5..v6 :
> > * Changed the usage of the term DSM to PDSM to distinguish it from the
> > ACPI term [ Dan Williams ]
> > * Renamed papr_scm_dsm.h to papr_scm_pdsm.h and updated various struct
> > to reflect the new terminology.
> > * Updated the patch description and title to reflect the new terminology.
> > * Squashed patch to introduce new command family in 'ndctl.h' with
> > this patch [ Dan Williams ]
> > * Updated the papr_scm_pdsm method starting index from 0x10000 to 0x0
> > [ Dan Williams ]
> > * Removed redundant license text from the papr_scm_psdm.h file.
> > [ Dan Williams ]
> > * s/envelop/envelope/ at various places [ Dan Williams ]
> > * Added '__packed' attribute to command package header to gaurd
> > against different compiler adding paddings between the fields.
> > [ Dan Williams]
> > * Converted various pr_debug to dev_debug [ Dan Williams ]
> >
> > v4..v5 :
> > * None
> >
> > v3..v4 :
> > * None
> >
> > v2..v3 :
> > * Updated the patch prefix to 'ndctl/uapi' [Aneesh]
> >
> > v1..v2 :
> > * None
> > ---
> > arch/powerpc/include/uapi/asm/papr_pdsm.h | 98 +++++++++++++++++++
> > arch/powerpc/platforms/pseries/papr_scm.c | 113 +++++++++++++++++++++-
> > include/uapi/linux/ndctl.h | 1 +
> > 3 files changed, 207 insertions(+), 5 deletions(-)
> > create mode 100644 arch/powerpc/include/uapi/asm/papr_pdsm.h
> >
> > diff --git a/arch/powerpc/include/uapi/asm/papr_pdsm.h b/arch/powerpc/include/uapi/asm/papr_pdsm.h
> > new file mode 100644
> > index 000000000000..8b1a4f8fa316
> > --- /dev/null
> > +++ b/arch/powerpc/include/uapi/asm/papr_pdsm.h
> > @@ -0,0 +1,98 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
> > +/*
> > + * PAPR nvDimm Specific Methods (PDSM) and structs for libndctl
> > + *
> > + * (C) Copyright IBM 2020
> > + *
> > + * Author: Vaibhav Jain <vaibhav at linux.ibm.com>
> > + */
> > +
> > +#ifndef _UAPI_ASM_POWERPC_PAPR_PDSM_H_
> > +#define _UAPI_ASM_POWERPC_PAPR_PDSM_H_
> > +
> > +#include <linux/types.h>
> > +
> > +/*
> > + * PDSM Envelope:
> > + *
> > + * The ioctl ND_CMD_CALL transfers data between user-space and kernel via
> > + * envelope which consists of a header and user-defined payload sections.
> > + * The header is described by 'struct nd_pdsm_cmd_pkg' which expects a
> > + * payload following it and accessible via 'nd_pdsm_cmd_pkg.payload' field.
> > + * There is reserved field that can used to introduce new fields to the
> > + * structure in future. It also tries to ensure that 'nd_pdsm_cmd_pkg.payload'
> > + * lies at a 8-byte boundary.
> > + *
> > + * +-------------+---------------------+---------------------------+
> > + * | 64-Bytes | 8-Bytes | Max 184-Bytes |
> > + * +-------------+---------------------+---------------------------+
> > + * | nd_pdsm_cmd_pkg | |
> > + * |-------------+ | |
> > + * | nd_cmd_pkg | | |
> > + * +-------------+---------------------+---------------------------+
> > + * | nd_family | | |
> > + * | nd_size_out | cmd_status | |
> > + * | nd_size_in | reserved | payload |
> > + * | nd_command | | |
> > + * | nd_fw_size | | |
> > + * +-------------+---------------------+---------------------------+
> > + *
> > + * PDSM Header:
> > + *
> > + * The header is defined as 'struct nd_pdsm_cmd_pkg' which embeds a
> > + * 'struct nd_cmd_pkg' instance. The PDSM command is assigned to member
> > + * 'nd_cmd_pkg.nd_command'. Apart from size information of the envelope which is
> > + * contained in 'struct nd_cmd_pkg', the header also has members following
> > + * members:
> > + *
> > + * 'cmd_status' : (Out) Errors if any encountered while servicing PDSM.
> > + * 'reserved' : Not used, reserved for future and should be set to 0.
> > + *
> > + * PDSM Payload:
> > + *
> > + * The layout of the PDSM Payload is defined by various structs shared between
> > + * papr_scm and libndctl so that contents of payload can be interpreted. During
> > + * servicing of a PDSM the papr_scm module will read input args from the payload
> > + * field by casting its contents to an appropriate struct pointer based on the
> > + * PDSM command. Similarly the output of servicing the PDSM 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).
> > + *
> > + */
> > +
> > +/* PDSM-header + payload expected with ND_CMD_CALL ioctl from libnvdimm */
> > +struct nd_pdsm_cmd_pkg {
> > + struct nd_cmd_pkg hdr; /* Package header containing sub-cmd */
> > + __s32 cmd_status; /* Out: Sub-cmd status returned back */
> > + __u16 reserved[2]; /* Ignored and to be used in future */
> > + __u8 payload[]; /* In/Out: Sub-cmd data buffer */
> > +} __packed;
> > +
> > +/*
> > + * Methods to be embedded in ND_CMD_CALL request. These are sent to the kernel
> > + * via 'nd_pdsm_cmd_pkg.hdr.nd_command' member of the ioctl struct
> > + */
> > +enum papr_pdsm {
> > + PAPR_PDSM_MIN = 0x0,
> > + PAPR_PDSM_MAX,
> > +};
> > +
> > +/* Convert a libnvdimm nd_cmd_pkg to pdsm specific pkg */
> > +static inline struct nd_pdsm_cmd_pkg *nd_to_pdsm_cmd_pkg(struct nd_cmd_pkg *cmd)
> > +{
> > + return (struct nd_pdsm_cmd_pkg *)cmd;
> > +}
> > +
> > +/* Return the payload pointer for a given pcmd */
> > +static inline void *pdsm_cmd_to_payload(struct nd_pdsm_cmd_pkg *pcmd)
> > +{
> > + if (pcmd->hdr.nd_size_in == 0 && pcmd->hdr.nd_size_out == 0)
> > + return NULL;
> > + else
> > + return (void *)(pcmd->payload);
> > +}
>
> I just realized these were in the uapi header. You don't want to do this as
> this code gets built into any user space program that uses it and there may be
> license issues if the user space code is not a compatible license.

Yes, include/uapi/linux/ndctl.h is specifically LGPL for this reason.

2020-06-06 11:25:44

by Vaibhav Jain

[permalink] [raw]
Subject: Re: [PATCH v10 4/6] powerpc/papr_scm: Improve error logging and handling papr_scm_ndctl()

Hi Ira and Dan,

Thanks for reviewing this patch. Have updated the patch based on your
feedback to upadate cmd_rc only when the nd_cmd was handled and return
'0' in that case.

Other errors in case the nd_cmd was unrecognized or invalid result in
error returned from this functions as you suggested.

~ Vaibhav

Dan Williams <[email protected]> writes:

> On Fri, Jun 5, 2020 at 10:13 AM Ira Weiny <[email protected]> wrote:
>>
>> On Fri, Jun 05, 2020 at 05:11:34AM +0530, Vaibhav Jain wrote:
>> > Since papr_scm_ndctl() can be called from outside papr_scm, its
>> > exposed to the possibility of receiving NULL as value of 'cmd_rc'
>> > argument. This patch updates papr_scm_ndctl() to protect against such
>> > possibility by assigning it pointer to a local variable in case cmd_rc
>> > == NULL.
>> >
>> > Finally the patch also updates the 'default' clause of the switch-case
>> > block removing a 'return' statement thereby ensuring that value of
>> > 'cmd_rc' is always logged when papr_scm_ndctl() returns.
>> >
>> > Cc: "Aneesh Kumar K . V" <[email protected]>
>> > Cc: Dan Williams <[email protected]>
>> > Cc: Michael Ellerman <[email protected]>
>> > Cc: Ira Weiny <[email protected]>
>> > Signed-off-by: Vaibhav Jain <[email protected]>
>> > ---
>> > Changelog:
>> >
>> > v9..v10
>> > * New patch in the series
>>
>> Thanks for making this a separate patch it is easier to see what is going on
>> here.
>>
>> > ---
>> > arch/powerpc/platforms/pseries/papr_scm.c | 10 ++++++++--
>> > 1 file changed, 8 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
>> > index 0c091622b15e..6512fe6a2874 100644
>> > --- a/arch/powerpc/platforms/pseries/papr_scm.c
>> > +++ b/arch/powerpc/platforms/pseries/papr_scm.c
>> > @@ -355,11 +355,16 @@ static int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc,
>> > {
>> > struct nd_cmd_get_config_size *get_size_hdr;
>> > struct papr_scm_priv *p;
>> > + 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)
>> > + cmd_rc = &rc;
>> > +
>>
>> This protects you from the NULL. However...
>>
>> > p = nvdimm_provider_data(nvdimm);
>> >
>> > switch (cmd) {
>> > @@ -381,12 +386,13 @@ static int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc,
>> > break;
>> >
>> > default:
>> > - return -EINVAL;
>> > + dev_dbg(&p->pdev->dev, "Unknown command = %d\n", cmd);
>> > + *cmd_rc = -EINVAL;
>>
>> ... I think you are conflating rc and cmd_rc...
>>
>> > }
>> >
>> > dev_dbg(&p->pdev->dev, "returned with cmd_rc = %d\n", *cmd_rc);
>> >
>> > - return 0;
>> > + return *cmd_rc;
>>
>> ... this changes the behavior of the current commands. Now if the underlying
>> papr_scm_meta_[get|set]() fails you return that failure as rc rather than 0.
>>
>> Is that ok?
>
> The expectation is that rc is "did the command get sent to the device,
> or did it fail for 'transport' reasons". The role of cmd_rc is to
> translate the specific status response of the command into a common
> error code. The expectations are:

>
> rc < 0: Error code, Linux terminated the ioctl before talking to hardware
>
> rc == 0: Linux successfully submitted the command to hardware, cmd_rc
> is valid for command specific response
>
> rc > 0: Linux successfully submitted the command, but detected that
> only a subset of the data was accepted for "write"-style commands, or
> that only subset of data was returned for "read"-style commands. I.e.
> short-write / short-read semantics. cmd_rc is valid in this case and
> its up to userspace to determine if a short transfer is an error or
> not.
>
>> Also 'logging cmd_rc' in the invalid cmd case does not seem quite right unless
>> you really want rc to be cmd_rc.
>>
>> The architecture is designed to separate errors which occur in the kernel vs
>> errors in the firmware/dimm. Are they always the same? The current code
>> differentiates them.
>
> Yeah, they're distinct, transport vs end-point / command-specific
> status returns.


--
Cheers
~ Vaibhav

2020-06-06 12:37:21

by Vaibhav Jain

[permalink] [raw]
Subject: Re: [PATCH v10 6/6] powerpc/papr_scm: Implement support for PAPR_PDSM_HEALTH

Ira Weiny <[email protected]> writes:

> On Fri, Jun 05, 2020 at 05:11:36AM +0530, Vaibhav Jain wrote:
>> This patch implements support for PDSM request 'PAPR_PDSM_HEALTH'
>> that returns a newly introduced 'struct nd_papr_pdsm_health' instance
>> containing dimm health information back to user space in response to
>> ND_CMD_CALL. This functionality is implemented in newly introduced
>> papr_pdsm_health() that queries the nvdimm health information and
>> then copies this information to the package payload whose layout is
>> defined by 'struct nd_papr_pdsm_health'.
>>
>> Cc: "Aneesh Kumar K . V" <[email protected]>
>> Cc: Dan Williams <[email protected]>
>> Cc: Michael Ellerman <[email protected]>
>> Cc: Ira Weiny <[email protected]>
>> Signed-off-by: Vaibhav Jain <[email protected]>
>> ---
>> Changelog:
>>
>> v9..v10:
>> * Removed code in papr_pdsm_health that performed validation on pdsm
>> payload version and corrosponding struct and defines used for
>> validation of payload version.
>> * Dropped usage of struct papr_pdsm_health in 'struct
>> papr_scm_priv'. Instead papr_psdm_health() now uses
>> 'papr_scm_priv.health_bitmap' to populate the pdsm payload.
>> * Above change also fixes the problem where this patch was removing
>> the code that was previously introduced in this patch-series.
>> [ Ira ]
>> * Introduced a new def ND_PDSM_ENVELOPE_HDR_SIZE that indicates the
>> space allocated to 'struct nd_pdsm_cmd_pkg' fields except 'struct
>> nd_cmd_pkg'. This def is useful in validating payload sizes.
>> * Reworked papr_pdsm_health() to enforce a specific payload size for
>> 'PAPR_PDSM_HEALTH' pdsm request.
>>
>> Resend:
>> * Added ack from Aneesh.
>>
>> v8..v9:
>> * s/PAPR_SCM_PDSM_HEALTH/PAPR_PDSM_HEALTH/g [ Dan , Aneesh ]
>> * s/PAPR_SCM_PSDM_DIMM_*/PAPR_PDSM_DIMM_*/g
>> * Renamed papr_scm_get_health() to papr_psdm_health()
>> * Updated patch description to replace papr-scm dimm with nvdimm.
>>
>> v7..v8:
>> * None
>>
>> Resend:
>> * None
>>
>> v6..v7:
>> * Updated flags_show() to use seq_buf_printf(). [Mpe]
>> * Updated papr_scm_get_health() to use newly introduced
>> __drc_pmem_query_health() bypassing the cache [Mpe].
>>
>> v5..v6:
>> * Added attribute '__packed' to 'struct nd_papr_pdsm_health_v1' to
>> gaurd against possibility of different compilers adding different
>> paddings to the struct [ Dan Williams ]
>>
>> * Updated 'struct nd_papr_pdsm_health_v1' to use __u8 instead of
>> 'bool' and also updated drc_pmem_query_health() to take this into
>> account. [ Dan Williams ]
>>
>> 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_pdsm.h | 33 +++++++++++
>> arch/powerpc/platforms/pseries/papr_scm.c | 70 +++++++++++++++++++++++
>> 2 files changed, 103 insertions(+)
>>
>> diff --git a/arch/powerpc/include/uapi/asm/papr_pdsm.h b/arch/powerpc/include/uapi/asm/papr_pdsm.h
>> index 8b1a4f8fa316..c4c990ede5d4 100644
>> --- a/arch/powerpc/include/uapi/asm/papr_pdsm.h
>> +++ b/arch/powerpc/include/uapi/asm/papr_pdsm.h
>> @@ -71,12 +71,17 @@ struct nd_pdsm_cmd_pkg {
>> __u8 payload[]; /* In/Out: Sub-cmd data buffer */
>> } __packed;
>>
>> +/* Calculate size used by the pdsm header fields minus 'struct nd_cmd_pkg' */
>> +#define ND_PDSM_ENVELOPE_HDR_SIZE \
>> + (sizeof(struct nd_pdsm_cmd_pkg) - sizeof(struct nd_cmd_pkg))
>> +
[ ]
>
> This is kind of a weird name for this.
>
> Isn't this just the ND PDSM header size? What is 'envelope' mean
> here?
Was referring to 'nd_cmd_pkg' envelop here. But yes removing 'ENVELOPE'
should make it more concise. Have already done this in v11 of this patch.

>
>> /*
>> * Methods to be embedded in ND_CMD_CALL request. These are sent to the kernel
>> * via 'nd_pdsm_cmd_pkg.hdr.nd_command' member of the ioctl struct
>> */
>> enum papr_pdsm {
>> PAPR_PDSM_MIN = 0x0,
>> + PAPR_PDSM_HEALTH,
>> PAPR_PDSM_MAX,
>> };
>>
>> @@ -95,4 +100,32 @@ static inline void *pdsm_cmd_to_payload(struct nd_pdsm_cmd_pkg *pcmd)
>> return (void *)(pcmd->payload);
>> }
>>
>> +/* Various nvdimm health indicators */
>> +#define PAPR_PDSM_DIMM_HEALTHY 0
>> +#define PAPR_PDSM_DIMM_UNHEALTHY 1
>> +#define PAPR_PDSM_DIMM_CRITICAL 2
>> +#define PAPR_PDSM_DIMM_FATAL 3
>> +
>> +/*
>> + * Struct exchanged between kernel & ndctl in for PAPR_PDSM_HEALTH
>> + * Various flags 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. One of PAPR_PDSM_DIMM_XXXX
>> + */
>> +struct nd_papr_pdsm_health {
>> + __u8 dimm_unarmed;
>> + __u8 dimm_bad_shutdown;
>> + __u8 dimm_bad_restore;
>> + __u8 dimm_scrubbed;
>> + __u8 dimm_locked;
>> + __u8 dimm_encrypted;
>> + __u16 dimm_health;
>> +} __packed;
>> +
>> #endif /* _UAPI_ASM_POWERPC_PAPR_PDSM_H_ */
>> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
>> index 05eb56ecab5e..984942be24c1 100644
>> --- a/arch/powerpc/platforms/pseries/papr_scm.c
>> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
>> @@ -421,6 +421,72 @@ 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_pdsm_health(struct papr_scm_priv *p,
>> + struct nd_pdsm_cmd_pkg *pkg)
>> +{
>> + int rc;
>> + struct nd_papr_pdsm_health health = { 0 };
>> + u16 copysize = sizeof(struct nd_papr_pdsm_health);
>> + u16 payload_size = pkg->hdr.nd_size_out - ND_PDSM_ENVELOPE_HDR_SIZE;
>> +
>> + /* Ensure correct payload size that can hold struct nd_papr_pdsm_health */
>> + if (payload_size != copysize) {
>> + dev_dbg(&p->pdev->dev,
>> + "Unexpected payload-size (%u). Expected (%u)",
>> + pkg->hdr.nd_size_out, copysize);
>> + rc = -ENOSPC;
>> + goto out;
>> + }
>> +
>> + /* Ensure dimm health mutex is taken preventing concurrent access */
>> + rc = mutex_lock_interruptible(&p->health_mutex);
>> + if (rc)
>> + goto out;
>> +
>> + /* Always fetch upto date dimm health data ignoring cached values */
>> + rc = __drc_pmem_query_health(p);
>> + if (rc) {
>> + mutex_unlock(&p->health_mutex);
>> + goto out;
>> + }
>> +
>> + /* update health struct with various flags derived from health bitmap */
>> + health = (struct nd_papr_pdsm_health) {
>> + .dimm_unarmed = p->health_bitmap & PAPR_PMEM_UNARMED_MASK,
>> + .dimm_bad_shutdown = p->health_bitmap & PAPR_PMEM_BAD_SHUTDOWN_MASK,
>> + .dimm_bad_restore = p->health_bitmap & PAPR_PMEM_BAD_RESTORE_MASK,
>> + .dimm_encrypted = p->health_bitmap & PAPR_PMEM_ENCRYPTED,
>> + .dimm_locked = p->health_bitmap & PAPR_PMEM_SCRUBBED_AND_LOCKED,
>> + .dimm_scrubbed = p->health_bitmap & PAPR_PMEM_SCRUBBED_AND_LOCKED,
>
> Are you sure these work? These are not assignments to a bool so I don't think
> gcc will do what you want here.
Yeah, somehow this slipped by and didnt show up in my tests. I checked
the assembly dump and seems GCC was silently skipping initializing these
fields without making any noise. Have rectified this in v11 by changing
these designated initilizers to

.dimm_locked = !!(p->health_bitmap & PAPR_PMEM_SCRUBBED_AND_LOCKED)

Thanks for catching this.

~ Vaibhav
>
> Ira
>
>> + .dimm_health = PAPR_PDSM_DIMM_HEALTHY,
>> + };
>> +
>> + /* Update field dimm_health based on health_bitmap flags */
>> + if (p->health_bitmap & PAPR_PMEM_HEALTH_FATAL)
>> + health.dimm_health = PAPR_PDSM_DIMM_FATAL;
>> + else if (p->health_bitmap & PAPR_PMEM_HEALTH_CRITICAL)
>> + health.dimm_health = PAPR_PDSM_DIMM_CRITICAL;
>> + else if (p->health_bitmap & PAPR_PMEM_HEALTH_UNHEALTHY)
>> + health.dimm_health = PAPR_PDSM_DIMM_UNHEALTHY;
>> +
>> + /* struct populated hence can release the mutex now */
>> + mutex_unlock(&p->health_mutex);
>> +
>> + dev_dbg(&p->pdev->dev, "Copying payload size=%u\n", copysize);
>> +
>> + /* Copy the health struct to the payload */
>> + memcpy(pdsm_cmd_to_payload(pkg), &health, copysize);
>> +
>> + /* Update fw size including size of struct nd_pdsm_cmd_pkg fields */
>> + pkg->hdr.nd_fw_size = copysize + ND_PDSM_ENVELOPE_HDR_SIZE;
>> +
>> +out:
>> + dev_dbg(&p->pdev->dev, "completion code = %d\n", rc);
>> +
>> + return rc;
>> +}
>> +
>> /*
>> * For a given pdsm request call an appropriate service function.
>> * Note: Use 'nd_pdsm_cmd_pkg.cmd_status to report psdm servicing errors. Hence
>> @@ -435,6 +501,10 @@ static void papr_scm_service_pdsm(struct papr_scm_priv *p,
>>
>> /* Call pdsm service function */
>> switch (pdsm) {
>> + case PAPR_PDSM_HEALTH:
>> + pkg->cmd_status = papr_pdsm_health(p, pkg);
>> + break;
>> +
>> default:
>> dev_dbg(&p->pdev->dev, "PDSM[0x%x]: Unsupported PDSM request\n",
>> pdsm);
>> --
>> 2.26.2
>>

--
Cheers
~ Vaibhav

2020-06-06 18:13:06

by Segher Boessenkool

[permalink] [raw]
Subject: Re: [PATCH v10 6/6] powerpc/papr_scm: Implement support for PAPR_PDSM_HEALTH

On Sat, Jun 06, 2020 at 06:04:11PM +0530, Vaibhav Jain wrote:
> >> + /* update health struct with various flags derived from health bitmap */
> >> + health = (struct nd_papr_pdsm_health) {
> >> + .dimm_unarmed = p->health_bitmap & PAPR_PMEM_UNARMED_MASK,
> >> + .dimm_bad_shutdown = p->health_bitmap & PAPR_PMEM_BAD_SHUTDOWN_MASK,
> >> + .dimm_bad_restore = p->health_bitmap & PAPR_PMEM_BAD_RESTORE_MASK,
> >> + .dimm_encrypted = p->health_bitmap & PAPR_PMEM_ENCRYPTED,
> >> + .dimm_locked = p->health_bitmap & PAPR_PMEM_SCRUBBED_AND_LOCKED,
> >> + .dimm_scrubbed = p->health_bitmap & PAPR_PMEM_SCRUBBED_AND_LOCKED,
> >
> > Are you sure these work? These are not assignments to a bool so I don't think
> > gcc will do what you want here.
> Yeah, somehow this slipped by and didnt show up in my tests. I checked
> the assembly dump and seems GCC was silently skipping initializing these
> fields without making any noise.

It's not "skipping" that, it initialises the field to 0, just like your
code said it should :-)

If you think GCC should warn for this, please open a PR? It is *normal*
for bit-fields to be truncated from what is assigned to it, but maybe we
could warn for it in the 1-bit case (we currently don't seem to, even
when the bit-field type is _Bool).

Thanks,


Segher