Hi Dexuan,
Thanks for these patches - I had a few comments below.
Also on a more general note, the patches in this series don't appear to
be correctly threaded. Normally, patch emails in a series are replies to
the first patch (either 1/N or the cover letter), and this allows for
easier review as all related emails can be found in a single top-level
thread. It would be nice if you can fix this for the future - git send-
email should do this correctly automatically, if you use it for sending
the patches.
On Wed, 2019-02-20 at 05:10 +0000, Dexuan Cui wrote:
This patch retrieves the health info by Hyper-V _DSM method Function
1:
We should never use "This patch.." in a commit message. See 4.c in [1].
[1]:
https://www.ozlabs.org/~akpm/stuff/tpp.txt
Get Health Information (Function Index 1)
See
http://www.uefi.org/RFIC_LIST ("Virtual NVDIMM 0x1901").
Now "ndctl list --dimms --health --idle" can show a line
"health_state":"ok",
e.g.
{
"dev":"nmem0",
"id":"04d5-01-1701-00000000",
"handle":0,
"phys_id":0,
"health":{
"health_state":"ok"
}
}
If there is an error with the NVDIMM, the "ok" will be replaced with
"unknown",
"fatal", "critical", or "non-critical".
Signed-off-by: Dexuan Cui <decui(a)microsoft.com>
---
ndctl/lib/Makefile.am | 1 +
ndctl/lib/hyperv.c | 129 ++++++++++++++++++++++++++++++++++++++++++
ndctl/lib/hyperv.h | 51 +++++++++++++++++
ndctl/lib/libndctl.c | 2 +
ndctl/lib/private.h | 3 +
ndctl/ndctl.h | 1 +
6 files changed, 187 insertions(+)
create mode 100644 ndctl/lib/hyperv.c
create mode 100644 ndctl/lib/hyperv.h
diff --git a/ndctl/lib/Makefile.am b/ndctl/lib/Makefile.am
index 7797039..fb75fda 100644
--- a/ndctl/lib/Makefile.am
+++ b/ndctl/lib/Makefile.am
@@ -20,6 +20,7 @@ libndctl_la_SOURCES =\
intel.c \
hpe1.c \
msft.c \
+ hyperv.c \
ars.c \
firmware.c \
libndctl.c
diff --git a/ndctl/lib/hyperv.c b/ndctl/lib/hyperv.c
new file mode 100644
index 0000000..b303d50
--- /dev/null
+++ b/ndctl/lib/hyperv.c
@@ -0,0 +1,129 @@
+/*
+ * Copyright (c) 2019, Microsoft Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU Lesser General Public License,
+ * version 2.1, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT ANY
+ * WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
+ * FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License for
+ * more details.
+ */
For new files, use the SPDX license identifiers, for an example, see:
https://github.com/pmem/ndctl/blob/master/ndctl/load-keys.c#L1
+#include <stdlib.h>
+#include <limits.h>
+#include <util/bitmap.h>
+#include <util/log.h>
+#include <ndctl/libndctl.h>
+#include "private.h"
+#include "hyperv.h"
+
+#define CMD_HYPERV(_c) ((_c)->hyperv)
I'm not sure this macro improves readability, in fact I think it rather
detracts from it in many cases - see further below.
Additionally, no need for the preceeding underscore in the macro
arguments - the rest of the code base doesn't do this, and I'm not sure
what value it provides.
+#define CMD_HYPERV_STATUS(_c) (CMD_HYPERV(_c)->u.status)
+#define CMD_HYPERV_SMART_DATA(_c) (CMD_HYPERV(_c)->u.smart.data)
+
+static struct ndctl_cmd *hyperv_dimm_cmd_new_smart(struct ndctl_dimm *dimm)
+{
+ struct ndctl_bus *bus = ndctl_dimm_get_bus(dimm);
+ struct ndctl_ctx *ctx = ndctl_bus_get_ctx(bus);
+ struct ndctl_cmd *cmd;
+ size_t size;
+ struct nd_pkg_hyperv *hyperv;
+
+ if (!ndctl_dimm_is_cmd_supported(dimm, ND_CMD_CALL)) {
+ dbg(ctx, "unsupported cmd\n");
+ return NULL;
+ }
+
+ if (test_dimm_dsm(dimm, ND_HYPERV_CMD_GET_HEALTH_INFO) ==
+ DIMM_DSM_UNSUPPORTED) {
+ dbg(ctx, "unsupported function\n");
+ return NULL;
+ }
+
+ size = sizeof(*cmd) + sizeof(struct nd_pkg_hyperv);
+ cmd = calloc(1, size);
+ if (!cmd)
+ return NULL;
+
+ cmd->dimm = dimm;
+ ndctl_cmd_ref(cmd);
+ cmd->type = ND_CMD_CALL;
+ cmd->size = size;
+ cmd->status = 1;
+
+ hyperv = CMD_HYPERV(cmd);
+ hyperv->gen.nd_family = NVDIMM_FAMILY_HYPERV;
+ hyperv->gen.nd_command = ND_HYPERV_CMD_GET_HEALTH_INFO;
+ hyperv->gen.nd_fw_size = 0;
+ hyperv->gen.nd_size_in = offsetof(struct nd_hyperv_smart, status);
+ hyperv->gen.nd_size_out = sizeof(hyperv->u.smart);
+ hyperv->u.smart.status = 0;
calloc() zeroes the newly allocated memory - no need to set any of the
fields in the struct to '0' manually.
+
+ cmd->firmware_status = &hyperv->u.smart.status;
+
+ return cmd;
+}
+
+static int hyperv_smart_valid(struct ndctl_cmd *cmd)
+{
+ if (cmd->type != ND_CMD_CALL ||
+ cmd->size != sizeof(*cmd) + sizeof(struct nd_pkg_hyperv) ||
+ CMD_HYPERV(cmd)->gen.nd_family != NVDIMM_FAMILY_HYPERV ||
+ CMD_HYPERV(cmd)->gen.nd_command != ND_HYPERV_CMD_GET_HEALTH_INFO ||
I feel in these cases, cmd->hyperv->stuff is /much/ more readable than
CMD_HYPERV(cmd)->stuff - and shorter as well as easier to type :)
+ cmd->status != 0 ||
+ CMD_HYPERV_STATUS(cmd) != 0)
+ return cmd->status < 0 ? cmd->status : -EINVAL;
+ return 0;
+}
+
+static int hyperv_cmd_xlat_firmware_status(struct ndctl_cmd *cmd)
+{
+ return CMD_HYPERV_STATUS(cmd) == 0 ? 0 : -EINVAL;
+}
+
+static unsigned int hyperv_cmd_smart_get_flags(struct ndctl_cmd *cmd)
+{
+ int rc;
+
+ rc = hyperv_smart_valid(cmd);
+ if (rc < 0) {
+ errno = -rc;
+ return 0;
+ }
+
+ return ND_SMART_HEALTH_VALID;
+}
+
+static unsigned int hyperv_cmd_smart_get_health(struct ndctl_cmd *cmd)
+{
+ unsigned int health = 0;
+ __u32 num;
+ int rc;
+
+ rc = hyperv_smart_valid(cmd);
+ if (rc < 0) {
+ errno = -rc;
+ return UINT_MAX;
+ }
+
+ num = CMD_HYPERV_SMART_DATA(cmd)->health & 0x3F;
+
+ if (num & (BIT(0) | BIT(1)))
+ health |= ND_SMART_CRITICAL_HEALTH;
+
+ if (num & BIT(2))
+ health |= ND_SMART_FATAL_HEALTH;
+
+ if (num & (BIT(3) | BIT(4) | BIT(5)))
+ health |= ND_SMART_NON_CRITICAL_HEALTH;
+
+ return health;
+}
+
+struct ndctl_dimm_ops * const hyperv_dimm_ops = &(struct ndctl_dimm_ops) {
+ .new_smart = hyperv_dimm_cmd_new_smart,
+ .smart_get_flags = hyperv_cmd_smart_get_flags,
+ .smart_get_health = hyperv_cmd_smart_get_health,
+ .xlat_firmware_status = hyperv_cmd_xlat_firmware_status,
+};
diff --git a/ndctl/lib/hyperv.h b/ndctl/lib/hyperv.h
new file mode 100644
index 0000000..8e55a97
--- /dev/null
+++ b/ndctl/lib/hyperv.h
@@ -0,0 +1,51 @@
+/*
+ * Copyright (c) 2019, Microsoft Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU Lesser General Public License,
+ * version 2.1, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT ANY
+ * WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
+ * FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License for
+ * more details.
+ */
Same comment about SPDX License format as above.
+#ifndef __NDCTL_HYPERV_H__
+#define __NDCTL_HYPERV_H__
+
+/* See
http://www.uefi.org/RFIC_LIST ("Virtual NVDIMM 0x1901") */
+enum {
+ ND_HYPERV_CMD_QUERY = 0,
It sounds like the intention for this function index 0 was to function
as a supported DSM mask, but the spec says it just returns a static
value. Nonetheless, should we not include some "_cmd_is_supported"
helpers, and test them before submitting the smart command in this patch
for example?
Also the name of this enum field can be a bit ambiguous - query /what/?
(In other DSM families, there are functions to query ARS status,
firmware update status, etc.). It might be better to name it something
like "ND_HYPERV_CMD_QUERY_SUPPORTED_FUNCTIONS"
+
+ /* non-root commands */
+ ND_HYPERV_CMD_GET_HEALTH_INFO = 1,
+};
+
+/*
+ * This is actually Function 1's data,
+ * This is the closest I can find to match the "smart".
+ * Hyper-V _DSM methods don't have a smart function.
+ */
+struct nd_hyperv_smart_data {
+ __u32 health;
+} __attribute__((packed));
I'm not sure I fully understand the comment above. Generally speaking,
we should avoid comments in the first person - i.e. instead of "This is
the closest thing I found..", it should simply be "X is the closest
thing to Y".
But I think you were trying to say:
/*
* This corresponds to 'function 1' (Get Health Information) in the
* HYPERV DSM spec referenced above
*/