RE: [PATCH] uapi: Add the BSD-2-Clause license to ndctl.h
> From: D Scott Phillips > Sent: Friday, October 25, 2019 10:56 AM > To: Dan Williams ; David Howells > ; Dexuan Cui ; Jerry > Hoemann ; stuart hayes > ; Toshi Kani ; Vishal Verma > ; linux-nvdimm@lists.01.org > Cc: linux-ker...@vger.kernel.org > Subject: [PATCH] uapi: Add the BSD-2-Clause license to ndctl.h > > Allow ndctl.h to be licensed with BSD-2-Clause so that other > operating systems can provide the same user level interface. > --- > > I've been working on nvdimm support in FreeBSD and would like to > offer the same ndctl API there to ease porting of application > code. Here I'm proposing to add the BSD-2-Clause license to this > header file, so that it can later be copied into FreeBSD. > > I believe that all the authors of changes to this file (in the To: > list) would need to agree to this change before it could be > accepted, so any signed-off-by is intentionally ommited for now. > Thanks, > > Scott Hi Scott, I agree to make the change if Dan and Vishal also agree. :-) Thanks, -- Dexuan ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
RE: [ndctl PATCH v3 4/5] libndctl: Add a new dimm-op cmd_is_supported()
> From: Verma, Vishal L > Sent: Wednesday, March 27, 2019 9:17 AM > To: Williams, Dan J ; Dexuan Cui > ; Jiang, Dave ; > linux-nvdimm@lists.01.org; dexuan@gmail.com; Michael Kelley > > Cc: jthumsh...@suse.de; qi.f...@fujitsu.com > Subject: Re: [ndctl PATCH v3 4/5] libndctl: Add a new dimm-op > cmd_is_supported() > > > On Tue, 2019-03-26 at 23:29 +, Dexuan Cui wrote: > > > Here ndctl is checking if ND_CMD_SMART is supported in > > dimm->cmd_mask. For all the non-NVDIMM_FAMILY_INTEL families, > > the check is false, including the HPE1 and MSFT families. > > > > So IMO we need this new dimm-ops to make "ndctl monitor" work > > for Hyper-V. I think the other non-INTEL families can't work with > > "ndctl monitor" either, but I don't have a hardward to test. > > > Ah ok makes sense - I think with that I don't have any other comments - > I'll queue the patches for the pending branch. > > Thanks, > -Vishal Great! Thanks you! -- Dexuan ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
RE: [ndctl PATCH v3 4/5] libndctl: Add a new dimm-op cmd_is_supported()
> From: Verma, Vishal L > Sent: Tuesday, March 26, 2019 2:56 PM > > Hi Dexuan, > > Thanks for making the changes, this revision /mostly/ looks good to me > except - > > The kernel dsm mask just seems to be hard-coded in [1] Yes, the kernel basically hard-codes the dsm_mask to 0x1F for Hyper-V, but the kernel doesn't export the dsm_mask to the userspace; instead, the kernel exports the "nvdimm->cmd_mask" to the userspace: see drivers/nvdimm/dimm_devs.c: commands_show(). The cmd_mask is initialized in acpi_nfit_register_dimms(), and it's not 1:1 mapped to the dsm_mask for non-NVDIMM_FAMILY_INTEL families: see acpi_nfit_register_dimms(). > So is there any reason that that can't simply be allowed to advertise > "everything is supported", similar to what the MSFT family does, and > that should remove the need for playing games with dimm-ops (i.e. now > there is another layer that can affect command support detection). Here ndctl is checking if ND_CMD_SMART is supported in dimm->cmd_mask. For all the non-NVDIMM_FAMILY_INTEL families, the check is false, including the HPE1 and MSFT families. So IMO we need this new dimm-ops to make "ndctl monitor" work for Hyper-V. I think the other non-INTEL families can't work with "ndctl monitor" either, but I don't have a hardward to test. Thanks, -- Dexuan ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
[ndctl PATCH v3 5/5] libndctl: Implement the "cmd_is_supported" dimm-op for Hyper-V
Currently "ndctl monitor" fails for Hyper-V virtual NVDIMM due to
"no smart support".
According to http://www.uefi.org/RFIC_LIST ("Virtual NVDIMM 0x1901"),
Hyper-V doesn't really support ND_CMD_SMART, but it supports
"Get Health Information (Function Index 1)" and
"Get Unsafe Shutdown Count (Function Index 2)", which is basically
a "weak" variant of ND_CMD_SMART.
Implement the dimm-ops to allow "ndctl monitor" to work with Hyper-V
NVDIMM. Now when an error happens, "ndctl monitor" can show such a
line:
{"timestamp":"1550547497.431731497","pid":1571,"event":
{"dimm-health-state":true},"dimm":{"dev":"nmem1",
"id":"04d5-01-1701-0100","handle":1,"phys_id":0,
"health":{"health_state":"fatal","shutdown_count":8}}}
Here the meaningful info is:
"health":{"health_state":"fatal","shutdown_count":8}
Signed-off-by: Dexuan Cui
---
ndctl/lib/hyperv.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/ndctl/lib/hyperv.c b/ndctl/lib/hyperv.c
index efd8b03..9b4fe12 100644
--- a/ndctl/lib/hyperv.c
+++ b/ndctl/lib/hyperv.c
@@ -9,6 +9,19 @@
#include "private.h"
#include "hyperv.h"
+static bool hyperv_cmd_is_supported(struct ndctl_dimm *dimm, int cmd)
+{
+ /*
+* "ndctl monitor" requires ND_CMD_SMART, which is not really supported
+* by Hyper-V virtual NVDIMM. Nevertheless, ND_CMD_SMART can be emulated
+* by ND_HYPERV_CMD_GET_HEALTH_INFO and ND_HYPERV_CMD_GET_SHUTDOWN_INFO.
+*/
+ if (cmd == ND_CMD_SMART )
+ return true;
+
+ return !!(dimm->cmd_mask & (1ULL << cmd));
+}
+
static struct ndctl_cmd *alloc_hyperv_cmd(struct ndctl_dimm *dimm,
unsigned int command)
{
@@ -161,6 +174,7 @@ static int hyperv_cmd_xlat_firmware_status(struct ndctl_cmd
*cmd)
}
struct ndctl_dimm_ops * const hyperv_dimm_ops = &(struct ndctl_dimm_ops) {
+ .cmd_is_supported = hyperv_cmd_is_supported,
.new_smart = hyperv_dimm_cmd_new_smart,
.smart_get_flags = hyperv_cmd_get_flags,
.smart_get_health = hyperv_cmd_get_health,
--
2.19.1
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
RE: [ndctl PATCH v2 4/4] ndctl, monitor: support NVDIMM_FAMILY_HYPERV
> From: Verma, Vishal L > Sent: Friday, March 22, 2019 11:29 AM > > On Fri, 2019-03-22 at 17:55 +0000, Dexuan Cui wrote: > > > From: Dan Williams > > > Sent: Thursday, March 21, 2019 11:12 PM > > > To: Dexuan Cui > > > > > > On Thu, Mar 21, 2019 at 11:03 PM Dexuan Cui > wrote: > > > > > From: Dan Williams > > > > > Sent: Thursday, March 21, 2019 10:37 PM > > > > > > > > > > No, I think you misunderstand. Hyper-V implements "function-1", > > > > > "command-1" support can be emulated. The request is to translate the > > > > > Hyper-V function-1 payload into the command-1 payload format. > > > > > > > > Then, yes, I think so. The first 2 patches of this patchset do the > translation: > > > > > > > > [ndctl,v2,1/4] libndctl: add support for NVDIMM_FAMILY_HYPERV's > _DSM > > > Function 1 > > > > [ndctl,v2,2/4] libndctl: NVDIMM_FAMILY_HYPERV: > > > add .smart_get_shutdown_count (Function 2) > > > > The problem is how to skip the checkings in ndctl/monitor.c: > > > > filter_dimm() > on > > > Hyper-V. > > > > If we return early in filter_dimm(), mfa->num_dimm will be zero, then > > > monitor_event() > > > > can't be called, and we have no chance to monitor the events and do the > > > translation. > > > > > > I'd rather change ndctl_dimm_cmd_is_supported() to call back into a > > > dimm-op so that the Hyper-V implementation can say "yes, I support > > > (emulate) the necessary monitor commands". > > > > Hi Dan, > > Then we need to add a new dimm-op monitor_supported(), and change > > ndctl/lib/intel.c and ndctl/lib/hyper.c to implement the new dimm-op, and > we > > need to change the common code, i.e. ndctl_dimm_cmd_is_supported(), to > use > > this new dimm-op. > > > > If this sounds reasonable to you and Verma, I'll try to make a patchset for > > you > > to review. > > Yep this approach sounds good to me! Hi all, I sent out the v3 patchset from my gmail account just now. (There is still an SMTP server issue when I use git send-email with my corporate SMTP server...) Thanks, -- Dexuan ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
[ndctl PATCH v3 0/5] Add the support for Hyper-V virtual NVDIMM
Hi all, I polished the first 2 patches following Vishal and Dan's comments. And, I dropped this v2 patch: [ndctl,v2,3/4] ndctl, lib: implement ndctl_dimm_get_cmd_family() and re-implemented the support for "ndctl monitor" for Hyper-V by following Vishal/Dan's suggestions. Thank you! Please review the patchset. Dexuan Cui (5): libndctl: Implement the "smart_get_health" dimm-op for Hyper-V libndctl: Implement the smart_get_shutdown_count dimm-op for Hyper-V ndctl, monitor: Don't require the support of ND_CMD_SMART_THRESHOLD libndctl: Add a new dimm-op cmd_is_supported() libndctl: Implement the "cmd_is_supported" dimm-op for Hyper-V ndctl/lib/Makefile.am | 1 + ndctl/lib/hyperv.c| 183 ++ ndctl/lib/hyperv.h| 38 + ndctl/lib/libndctl.c | 7 ++ ndctl/lib/private.h | 4 + ndctl/monitor.c | 12 +-- ndctl/ndctl.h | 1 + 7 files changed, 238 insertions(+), 8 deletions(-) create mode 100644 ndctl/lib/hyperv.c create mode 100644 ndctl/lib/hyperv.h -- 2.19.1 ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
[ndctl PATCH v3 3/5] ndctl, monitor: Don't require the support of ND_CMD_SMART_THRESHOLD
When a NVDIMM doesn't support ND_CMD_SMART_THRESHOLD, it may support
ND_CMD_SMART or a variant of ND_CMD_SMART. Allow such a NVDIMM to work
with "ndctl monitor".
Signed-off-by: Dexuan Cui
---
ndctl/monitor.c | 12
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/ndctl/monitor.c b/ndctl/monitor.c
index 43b2abe..346a6df 100644
--- a/ndctl/monitor.c
+++ b/ndctl/monitor.c
@@ -275,17 +275,13 @@ static void filter_dimm(struct ndctl_dimm *dimm, struct
util_filter_ctx *fctx)
err(&monitor, "%s: no smart support\n", name);
return;
}
- if (!ndctl_dimm_is_cmd_supported(dimm, ND_CMD_SMART_THRESHOLD)) {
- err(&monitor, "%s: no smart threshold support\n", name);
- return;
- }
- if (!ndctl_dimm_is_flag_supported(dimm, ND_SMART_ALARM_VALID)) {
+ if (!ndctl_dimm_is_cmd_supported(dimm, ND_CMD_SMART_THRESHOLD)) {
+ dbg(&monitor, "%s: no smart threshold support\n", name);
+ } else if (!ndctl_dimm_is_flag_supported(dimm, ND_SMART_ALARM_VALID)) {
err(&monitor, "%s: smart alarm invalid\n", name);
return;
- }
-
- if (enable_dimm_supported_threshold_alarms(dimm)) {
+ } else if (enable_dimm_supported_threshold_alarms(dimm)) {
err(&monitor, "%s: enable supported threshold alarms failed\n",
name);
return;
}
--
2.19.1
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
[ndctl PATCH v3 4/5] libndctl: Add a new dimm-op cmd_is_supported()
A NVDIMM family may need to report that it supports a command, even if
the command is not set in dimm->cmd_mask, e.g. a non-NVDIMM_FAMILY_INTEL
famimy may support ND_CMD_SMART or some kind of variant of ND_CMD_SMART,
while the kernel only sets ND_CMD_SMART in the nvdimm->cmd_mask for
NVDIMM_FAMILY_INTEL.
Signed-off-by: Dexuan Cui
---
ndctl/lib/libndctl.c | 5 +
ndctl/lib/private.h | 1 +
2 files changed, 6 insertions(+)
diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
index 24b8ad3..4acfb03 100644
--- a/ndctl/lib/libndctl.c
+++ b/ndctl/lib/libndctl.c
@@ -1769,6 +1769,11 @@ NDCTL_EXPORT int ndctl_dimm_failed_map(struct ndctl_dimm
*dimm)
NDCTL_EXPORT int ndctl_dimm_is_cmd_supported(struct ndctl_dimm *dimm,
int cmd)
{
+ struct ndctl_dimm_ops *ops = dimm->ops;
+
+ if (ops && ops->cmd_is_supported)
+ return ops->cmd_is_supported(dimm, cmd);
+
return !!(dimm->cmd_mask & (1ULL << cmd));
}
diff --git a/ndctl/lib/private.h b/ndctl/lib/private.h
index a9d35c5..2ddc1d2 100644
--- a/ndctl/lib/private.h
+++ b/ndctl/lib/private.h
@@ -292,6 +292,7 @@ struct ndctl_bb {
struct ndctl_dimm_ops {
const char *(*cmd_desc)(int);
+ bool (*cmd_is_supported)(struct ndctl_dimm *, int);
struct ndctl_cmd *(*new_smart)(struct ndctl_dimm *);
unsigned int (*smart_get_flags)(struct ndctl_cmd *);
unsigned int (*smart_get_health)(struct ndctl_cmd *);
--
2.19.1
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
[ndctl PATCH v3 2/5] libndctl: Implement the smart_get_shutdown_count dimm-op for Hyper-V
The "smart_get_shutdown_count" dimm-op is expected to return a shutdown
count, but for the Hyper-V family this is only available from a separate
DSM. Perform a command submission in the "hyperv_cmd_get_shutdown_count"
dimm-op and the "hyperv_cmd_get_flags" dimm-op to retrieve this info, so
that a smart health listing can display this info.
Now "ndctl list --dimms --health" can show a line of "shutdown_count"
for Hyper-V NVDIMM, e.g.
{
"dev":"nmem0",
"id":"04d5-01-1701-",
"handle":0,
"phys_id":0,
"health":{
"health_state":"ok",
"shutdown_count":2
}
}
Signed-off-by: Dexuan Cui
---
ndctl/lib/hyperv.c | 43 +++
ndctl/lib/hyperv.h | 8
2 files changed, 51 insertions(+)
diff --git a/ndctl/lib/hyperv.c b/ndctl/lib/hyperv.c
index 6ed2125..efd8b03 100644
--- a/ndctl/lib/hyperv.c
+++ b/ndctl/lib/hyperv.c
@@ -72,9 +72,33 @@ static int hyperv_valid_health_info(struct ndctl_cmd *cmd)
return hyperv_cmd_valid(cmd, ND_HYPERV_CMD_GET_HEALTH_INFO);
}
+static int hyperv_get_shutdown_count(struct ndctl_cmd *cmd, unsigned int
*count)
+{
+ unsigned int command = ND_HYPERV_CMD_GET_SHUTDOWN_INFO;
+ struct ndctl_cmd *cmd_get_shutdown_info;
+ int rc;
+
+ cmd_get_shutdown_info = alloc_hyperv_cmd(cmd->dimm, command);
+ if (!cmd_get_shutdown_info)
+ return -EINVAL;
+
+ if (ndctl_cmd_submit_xlat(cmd_get_shutdown_info) < 0 ||
+ hyperv_cmd_valid(cmd_get_shutdown_info, command) < 0) {
+ rc = -EINVAL;
+ goto out;
+ }
+
+ *count = cmd_get_shutdown_info->hyperv->u.shutdown_info.count;
+ rc = 0;
+out:
+ ndctl_cmd_unref(cmd_get_shutdown_info);
+ return rc;
+}
+
static unsigned int hyperv_cmd_get_flags(struct ndctl_cmd *cmd)
{
unsigned int flags = 0;
+ unsigned int count;
int rc;
rc = hyperv_valid_health_info(cmd);
@@ -84,6 +108,9 @@ static unsigned int hyperv_cmd_get_flags(struct ndctl_cmd
*cmd)
}
flags |= ND_SMART_HEALTH_VALID;
+ if (hyperv_get_shutdown_count(cmd, &count) == 0)
+ flags |= ND_SMART_SHUTDOWN_COUNT_VALID;
+
return flags;
}
@@ -113,6 +140,21 @@ static unsigned int hyperv_cmd_get_health(struct ndctl_cmd
*cmd)
return health;
}
+static unsigned int hyperv_cmd_get_shutdown_count(struct ndctl_cmd *cmd)
+{
+ unsigned int count;
+ int rc;;
+
+ rc = hyperv_get_shutdown_count(cmd, &count);
+
+ if (rc < 0) {
+ errno = -rc;
+ return UINT_MAX;
+ }
+
+ return count;
+}
+
static int hyperv_cmd_xlat_firmware_status(struct ndctl_cmd *cmd)
{
return cmd->hyperv->u.status == 0 ? 0 : -EINVAL;
@@ -122,5 +164,6 @@ struct ndctl_dimm_ops * const hyperv_dimm_ops = &(struct
ndctl_dimm_ops) {
.new_smart = hyperv_dimm_cmd_new_smart,
.smart_get_flags = hyperv_cmd_get_flags,
.smart_get_health = hyperv_cmd_get_health,
+ .smart_get_shutdown_count = hyperv_cmd_get_shutdown_count,
.xlat_firmware_status = hyperv_cmd_xlat_firmware_status,
};
diff --git a/ndctl/lib/hyperv.h b/ndctl/lib/hyperv.h
index 45bbc12..0c1677f 100644
--- a/ndctl/lib/hyperv.h
+++ b/ndctl/lib/hyperv.h
@@ -9,6 +9,7 @@
enum {
ND_HYPERV_CMD_QUERY_SUPPORTED_FUNCTIONS = 0,
ND_HYPERV_CMD_GET_HEALTH_INFO = 1,
+ ND_HYPERV_CMD_GET_SHUTDOWN_INFO = 2,
};
/* Get Health Information (Function Index 1) */
@@ -17,9 +18,16 @@ struct nd_hyperv_health_info {
__u32 health;
} __attribute__((packed));
+/* Get Unsafe Shutdown Count (Function Index 2) */
+struct nd_hyperv_shutdown_info {
+__u32 status;
+__u32 count;
+} __attribute__((packed));
+
union nd_hyperv_cmd {
__u32 status;
struct nd_hyperv_health_infohealth_info;
+ struct nd_hyperv_shutdown_info shutdown_info;
} __attribute__((packed));
struct nd_pkg_hyperv {
--
2.19.1
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
[ndctl PATCH v3 1/5] libndctl: Implement the "smart_get_health" dimm-op for Hyper-V
Show the health info of the NVDIMM by
"Get Health Information (Function Index 1)"
See http://www.uefi.org/RFIC_LIST ("Virtual NVDIMM 0x1901").
Now "ndctl list --dimms --health" can show a line
"health_state":"ok"
for Hyper-V NVDIMM, e.g.
{
"dev":"nmem0",
"id":"04d5-01-1701-",
"handle":0,
"phys_id":0,
"health":{
"health_state":"ok"
}
}
If the NVDIMM has an error, the string "ok" will be replaced with
"fatal", "critical", or "non-critical".
If ndctl fails to retrieve the health info due to some reason, a string
of "unknown" will be shown.
Signed-off-by: Dexuan Cui
---
ndctl/lib/Makefile.am | 1 +
ndctl/lib/hyperv.c| 126 ++
ndctl/lib/hyperv.h| 30 ++
ndctl/lib/libndctl.c | 2 +
ndctl/lib/private.h | 3 +
ndctl/ndctl.h | 1 +
6 files changed, 163 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 000..6ed2125
--- /dev/null
+++ b/ndctl/lib/hyperv.c
@@ -0,0 +1,126 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright(c) 2019, Microsoft Corporation. All rights reserved. */
+
+#include
+#include
+#include
+#include
+#include
+#include "private.h"
+#include "hyperv.h"
+
+static struct ndctl_cmd *alloc_hyperv_cmd(struct ndctl_dimm *dimm,
+ unsigned int command)
+{
+ struct ndctl_bus *bus = ndctl_dimm_get_bus(dimm);
+ struct ndctl_ctx *ctx = ndctl_bus_get_ctx(bus);
+ struct nd_pkg_hyperv *hyperv;
+ struct ndctl_cmd *cmd;
+ size_t size;
+
+ if (!ndctl_dimm_is_cmd_supported(dimm, ND_CMD_CALL)) {
+ dbg(ctx, "unsupported cmd\n");
+ return NULL;
+ }
+
+ if (test_dimm_dsm(dimm, command) == 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;
+
+ ndctl_cmd_ref(cmd);
+
+ cmd->dimm = dimm;
+ cmd->type = ND_CMD_CALL;
+ cmd->size = size;
+ cmd->status = 1;
+
+ hyperv = cmd->hyperv;
+ hyperv->gen.nd_family = NVDIMM_FAMILY_HYPERV;
+ hyperv->gen.nd_command = command;
+ hyperv->gen.nd_size_out = sizeof(hyperv->u.health_info);
+
+ cmd->firmware_status = &hyperv->u.health_info.status;
+ return cmd;
+}
+
+static struct ndctl_cmd *hyperv_dimm_cmd_new_smart(struct ndctl_dimm *dimm)
+{
+ return alloc_hyperv_cmd(dimm, ND_HYPERV_CMD_GET_HEALTH_INFO);
+}
+
+static int hyperv_cmd_valid(struct ndctl_cmd *cmd, unsigned int command)
+{
+ if (cmd->type != ND_CMD_CALL ||
+ cmd->size != sizeof(*cmd) + sizeof(struct nd_pkg_hyperv) ||
+ cmd->hyperv->gen.nd_family != NVDIMM_FAMILY_HYPERV ||
+ cmd->hyperv->gen.nd_command != command ||
+ cmd->status != 0 ||
+ cmd->hyperv->u.status != 0)
+ return cmd->status < 0 ? cmd->status : -EINVAL;
+
+ return 0;
+}
+
+static int hyperv_valid_health_info(struct ndctl_cmd *cmd)
+{
+ return hyperv_cmd_valid(cmd, ND_HYPERV_CMD_GET_HEALTH_INFO);
+}
+
+static unsigned int hyperv_cmd_get_flags(struct ndctl_cmd *cmd)
+{
+ unsigned int flags = 0;
+ int rc;
+
+ rc = hyperv_valid_health_info(cmd);
+ if (rc < 0) {
+ errno = -rc;
+ return 0;
+ }
+ flags |= ND_SMART_HEALTH_VALID;
+
+ return flags;
+}
+
+static unsigned int hyperv_cmd_get_health(struct ndctl_cmd *cmd)
+{
+ unsigned int health = 0;
+ __u32 num;
+ int rc;
+
+ rc = hyperv_valid_health_info(cmd);
+ if (rc < 0) {
+ errno = -rc;
+ return UINT_MAX;
+ }
+
+ num = cmd->hyperv->u.health_info.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;
+}
+
+static int hyperv_cmd_xlat_firmware_status(struct ndctl_cmd *cmd)
+{
+ return cmd
RE: [ndctl PATCH v2 4/4] ndctl, monitor: support NVDIMM_FAMILY_HYPERV
> From: Dan Williams > Sent: Friday, March 22, 2019 11:04 AM > > On Fri, Mar 22, 2019 at 10:56 AM Dexuan Cui wrote: > > > > > I'd rather change ndctl_dimm_cmd_is_supported() to call back into a > > > dimm-op so that the Hyper-V implementation can say "yes, I support > > > (emulate) the necessary monitor commands". > > > > Hi Dan, > > Then we need to add a new dimm-op monitor_supported() > > Where would the monitor_supported() op be used? I would expect a > cmd_is_supported() op? Maybe cmd_is_supported() is better. I'll work on the details and keeo you updated. Thanks -- Dexuan ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
RE: [ndctl PATCH v2 4/4] ndctl, monitor: support NVDIMM_FAMILY_HYPERV
> From: Dan Williams > Sent: Thursday, March 21, 2019 11:12 PM > To: Dexuan Cui > > On Thu, Mar 21, 2019 at 11:03 PM Dexuan Cui wrote: > > > > > From: Dan Williams > > > Sent: Thursday, March 21, 2019 10:37 PM > > > > > > No, I think you misunderstand. Hyper-V implements "function-1", > > > "command-1" support can be emulated. The request is to translate the > > > Hyper-V function-1 payload into the command-1 payload format. > > > > Then, yes, I think so. The first 2 patches of this patchset do the > > translation: > > > > [ndctl,v2,1/4] libndctl: add support for NVDIMM_FAMILY_HYPERV's _DSM > Function 1 > > [ndctl,v2,2/4] libndctl: NVDIMM_FAMILY_HYPERV: > add .smart_get_shutdown_count (Function 2) > > > > The problem is how to skip the checkings in ndctl/monitor.c: filter_dimm() > > on > Hyper-V. > > If we return early in filter_dimm(), mfa->num_dimm will be zero, then > monitor_event() > > can't be called, and we have no chance to monitor the events and do the > translation. > > I'd rather change ndctl_dimm_cmd_is_supported() to call back into a > dimm-op so that the Hyper-V implementation can say "yes, I support > (emulate) the necessary monitor commands". Hi Dan, Then we need to add a new dimm-op monitor_supported(), and change ndctl/lib/intel.c and ndctl/lib/hyper.c to implement the new dimm-op, and we need to change the common code, i.e. ndctl_dimm_cmd_is_supported(), to use this new dimm-op. If this sounds reasonable to you and Verma, I'll try to make a patchset for you to review. Thanks, -- Dexuan ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
RE: [ndctl PATCH v2 4/4] ndctl, monitor: support NVDIMM_FAMILY_HYPERV
> From: Dan Williams > Sent: Thursday, March 21, 2019 10:37 PM > > No, I think you misunderstand. Hyper-V implements "function-1", > "command-1" support can be emulated. The request is to translate the > Hyper-V function-1 payload into the command-1 payload format. Then, yes, I think so. The first 2 patches of this patchset do the translation: [ndctl,v2,1/4] libndctl: add support for NVDIMM_FAMILY_HYPERV's _DSM Function 1 [ndctl,v2,2/4] libndctl: NVDIMM_FAMILY_HYPERV: add .smart_get_shutdown_count (Function 2) (https://patchwork.kernel.org/project/linux-nvdimm/list/?series=82629) The problem is how to skip the checkings in ndctl/monitor.c: filter_dimm() on Hyper-V. If we return early in filter_dimm(), mfa->num_dimm will be zero, then monitor_event() can't be called, and we have no chance to monitor the events and do the translation. Thanks, -- Dexuan ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
RE: [ndctl PATCH v2 4/4] ndctl, monitor: support NVDIMM_FAMILY_HYPERV
> From: Dan Williams
> Sent: Thursday, March 21, 2019 9:13 PM
> > ...
> > Actually, this _is_ an issue for NVDIMM_FAMILY_HYPERV (and the other
> > families except for NVDIMM_FAMILY_INTEL) : see the kernel function
> > acpi_nfit_register_dimms(), where ND_CMD_SMART is set in the
> > "cmd_mask" only for NVDIMM_FAMILY_INTEL.
> >
> > So, on Hyper-V, ndctl_dimm_is_cmd_supported(dimm, ND_CMD_SMART)
> > is always false, and "ndctl monitor" exits with "no smart support".
>
> Can the Hyper-V implementation emulate those commands? That's the
> expectation, i.e. that the implementation can return they required
> payloads, but it's fine if the payloads disclaim support for certain
> fields.
Hyper-V Virtual NVDIMM doesn't emulate ND_CMD_SMART(1).
The _DSM Function 1 on Hyper-V is "Get Health Information" [1], which is
incomptabile with NVDIMM_FAMILY_INTEL's ND_CMD_SMART(1).
Even if Hyper-V could emulate ND_CMD_SMART, the current "ndctl monitor"
code still wouldn't work for Hyper-V (and the other families): the essence of
the
issue is that:
1. the kernel only sets ND_CMD_SMART flag for NVDIMM_FAMILY_INTEL.
2. "ndctl monitor" assumes the ND_CMD_SMART should be set by checking
/sys/class/nd/ndctl0/device/nmem0/commands.
This means ndctl_dimm_is_cmd_supported(dimm, ND_CMD_SMART) can
only be true on NVDIMM_FAMILY_INTEL.
My patch skips the assumption of ndctl on Hyper-V, so "ndctl monitor"
can work on Hyper-V.
It looks we do need an ops->monitor_supported() in ndctl?
Thanks,
-- Dexuan
[1] See http://www.uefi.org/RFIC_LIST ("Virtual NVDIMM 0x1901").
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
RE: [ndctl PATCH v2 4/4] ndctl, monitor: support NVDIMM_FAMILY_HYPERV
> From: Dexuan Cui > Sent: Thursday, March 21, 2019 7:09 PM > IMO there are 2 issues in ndctl/monitor.c: filter_dimm(): > > 1. IMO the cmd numbers ND_CMD_SMART (1) and > ND_CMD_SMART_THRESHOLD(2) are not really device-neutral. They > work for ndctl/lib/intel.c and it looks they happen to work for > ndctl/lib/hpe1.c, as hpe "happens" to have: >NDN_HPE1_CMD_SMART = 1, >NDN_HPE1_CMD_SMART_THRESHOLD = 2, > > But for ndctl/lib/msft.c, 1 and 2 mean different things. See [1]. > So the current "ndctl monitor" can't support msft.c. > > For ndctl/lib/hyperv.c, 1 and 2 means: > ND_HYPERV_CMD_GET_HEALTH_INFO = 1, > ND_HYPERV_CMD_GET_SHUTDOWN_INFO = 2, > They're also incompatible with ND_CMD_SMART and > ND_CMD_SMART_THRESHOLD. > Of source, the current code can "work" since these ND_HYPERV_CMD* > numbers happen to be the same as ND_CMD_SMART and > ND_CMD_SMART_THRESHOLD. So this may not be an isue for hyperv.c. Actually, this _is_ an issue for NVDIMM_FAMILY_HYPERV (and the other families except for NVDIMM_FAMILY_INTEL) : see the kernel function acpi_nfit_register_dimms(), where ND_CMD_SMART is set in the "cmd_mask" only for NVDIMM_FAMILY_INTEL. So, on Hyper-V, ndctl_dimm_is_cmd_supported(dimm, ND_CMD_SMART) is always false, and "ndctl monitor" exits with "no smart support". > 2. The current code assumes ND_SMART_ALARM_VALID must be supported. > However, NVDIMM_FAMILY_HYPERV doesn't support it, so currently > ndctl/monitor.c: filter_dimm() reports an error and returns -- for Hyper-V, > we should not return... hence I made this patch. > > Of source, we can add a new op to dimm->ops, e.g. > ops->monitor_supported() and then change the common code accordingly, > but I can imagine that would require a lot more changes and I guess it may not > worth that effort? Please share your insights! Looking forward to your suggestion! Thanks, -- Dexuan ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
RE: [ndctl PATCH v2 4/4] ndctl, monitor: support NVDIMM_FAMILY_HYPERV
> From: Verma, Vishal L
> Sent: Wednesday, March 20, 2019 6:55 PM
> ...
> >
> > -static void filter_dimm(struct ndctl_dimm *dimm, struct util_filter_ctx
> > *fctx)
> > +static bool ndctl_dimm_test_and_enable_notification(struct ndctl_dimm
> *dimm)
> > {
> > - struct monitor_dimm *mdimm;
> > - struct monitor_filter_arg *mfa = fctx->monitor;
> > const char *name = ndctl_dimm_get_devname(dimm);
> >
> > + /*
> > +* Hyper-V Virtual NVDIMM doesn't use ND_CMD_SMART to get the
> health
> > +* info. Instead, it uses ND_CMD_CALL, so the checking here can't
> > +* apply, and it doesn't support threshold alarms -- actually it only
> > +* supports one event: ND_EVENT_HEALTH_STATE.
> > +*/
> > + if (ndctl_dimm_get_cmd_family(dimm) == NVDIMM_FAMILY_HYPERV) {
> > + if (monitor.event_flags != ND_EVENT_HEALTH_STATE) {
> > + monitor.event_flags = ND_EVENT_HEALTH_STATE;
> > +
> > + notice(&monitor,
> > + "%s: only dimm-health-state can be monitored\n",
> > + name);
> > + }
> > + return true;
> > + }
> > +
>
> I'm not sure about the family-specific special casing -- it leaks family
> specific details into the common implementation, something that's been
> avoidable thus far.
>
> Is there any reason why the ndctl_dimm_is_cmd_supported() can't be made
> to fail appropriately for anything that is not supported, by adding the
> necessary functions to dimm-ops?
IMO there are 2 issues in ndctl/monitor.c: filter_dimm():
1. IMO the cmd numbers ND_CMD_SMART (1) and
ND_CMD_SMART_THRESHOLD(2) are not really device-neutral. They
work for ndctl/lib/intel.c and it looks they happen to work for
ndctl/lib/hpe1.c, as hpe "happens" to have:
NDN_HPE1_CMD_SMART = 1,
NDN_HPE1_CMD_SMART_THRESHOLD = 2,
But for ndctl/lib/msft.c, 1 and 2 mean different things. See [1].
So the current "ndctl monitor" can't support msft.c.
For ndctl/lib/hyperv.c, 1 and 2 means:
ND_HYPERV_CMD_GET_HEALTH_INFO = 1,
ND_HYPERV_CMD_GET_SHUTDOWN_INFO = 2,
They're also incompatible with ND_CMD_SMART and
ND_CMD_SMART_THRESHOLD.
Of source, the current code can "work" since these ND_HYPERV_CMD*
numbers happen to be the same as ND_CMD_SMART and
ND_CMD_SMART_THRESHOLD. So this may not be an isue for hyperv.c.
2. The current code assumes ND_SMART_ALARM_VALID must be supported.
However, NVDIMM_FAMILY_HYPERV doesn't support it, so currently
ndctl/monitor.c: filter_dimm() reports an error and returns -- for Hyper-V,
we should not return... hence I made this patch.
Of source, we can add a new op to dimm->ops, e.g.
ops->monitor_supported() and then change the common code accordingly,
but I can imagine that would require a lot more changes and I guess it may not
worth that effort? Please share your insights!
> That way if the user enters any of the unsupported options, they will
> just fail normally, and the user will be expected to provide the right
> options for the environment they know they're running in.
When the user enters any of the unsupported options, they will only
get an "no dimms to monitor" error, which gives no hint why the
error happens, and confuses the user...
> Indeed, I think that patch 3 of this series should never be required :)
Please see the above.
Thanks,
-- Dexuan
[1]
https://docs.microsoft.com/en-us/windows-hardware/drivers/storage/-dsm-interface-for-byte-addressable-energy-backed-function-class--function-interface-1-
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
RE: [ndctl PATCH v2 3/4] ndctl, lib: implement ndctl_dimm_get_cmd_family()
> From: Verma, Vishal L
> Sent: Wednesday, March 20, 2019 6:42 PM
> On Wed, 2019-02-20 at 05:11 +0000, Dexuan Cui wrote:
> > Let's export the family info so we can do some family-specific
> > handling in ndctl/monitor.c for Hyper-V NVDIMM.
>
> s/Let's//
Will fix it.
> > ndctl/lib/libndctl.c | 5 +
> > ndctl/lib/libndctl.sym | 1 +
> > ndctl/libndctl.h | 1 +
> > 3 files changed, 7 insertions(+)
> >
> > diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
> > index 48bdb27..1186579 100644
> > --- a/ndctl/lib/libndctl.c
> > +++ b/ndctl/lib/libndctl.c
> > @@ -1550,6 +1550,11 @@ NDCTL_EXPORT struct ndctl_dimm
> *ndctl_dimm_get_next(struct ndctl_dimm *dimm)
> > return list_next(&bus->dimms, dimm, list);
> > }
> >
> > +NDCTL_EXPORT unsigned long ndctl_dimm_get_cmd_family(struct
> ndctl_dimm *dimm)
> > +{
> > + return dimm->cmd_family;
> > +}
> > +
> > NDCTL_EXPORT unsigned int ndctl_dimm_get_handle(struct ndctl_dimm
> *dimm)
> > {
> > return dimm->handle;
> > diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym
> > index cb9f769..470e895 100644
> > --- a/ndctl/lib/libndctl.sym
> > +++ b/ndctl/lib/libndctl.sym
> > @@ -38,6 +38,7 @@ global:
> > ndctl_bus_wait_probe;
> > ndctl_dimm_get_first;
> > ndctl_dimm_get_next;
> > + ndctl_dimm_get_cmd_family;
>
> Any new APIs need to go in a new LIBNDCTL_XX section (for this release
> that would be LIBNDCTL_20).
> If you rebase to the current 'pending' branch on github, the section has
> already been created at the bottom, and you can just add to that.
Will fix it
Thanks,
-- Dexuan
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
RE: [ndctl PATCH v2 2/4] libndctl: NVDIMM_FAMILY_HYPERV: add .smart_get_shutdown_count (Function 2)
> From: Verma, Vishal L
> Sent: Wednesday, March 20, 2019 6:34 PM
> > ...
> > My feeling is that it's not very good to directly call ndctl_cmd_submit(),
> > but by doing this we don't need to make any change to the common code,
> and
> > I'm unsure if it's good to change the common code just for Hyper-V.
>
> I thought about this, and this approach seems reasonable to me. I agree
> that there is not precedent for submitting a command from the various
> family libraries, but I think the way dimm-ops are structured, this
> seems warranted. The way I see it is a dimm op means "get X information
> for this family", and however it needs to be obtained is just a detail
> specific to that DSM family. For intel it happens to be in the smart
> information, but if it happens to be a separate DSM, that is also fine.
Thanks! I'm gald you think this is fine. :-)
> I do think this commentary in the changelog can be reworded. Consider
> changing the first-person narrative to a more 'informational' or
> 'imperative' one. For example: "The 'smart_get_shutdown_count' dimm-op
> is expected to return a shutdown count, but for the HYPERV family, this
> is only available from a separate DSM. Perform a command submission in
> the 'hyperv_cmd_smart_get_shutdown_count' dimm op to retrieve this, so
> that a smart health listing can display this information"
Will fix it.
> Apart from the commit message comments in the previous patch, also
> consider wrapping commit messages to a 72 column width [1].
Thanks for another good read! I'll fix it.
> > -static struct ndctl_cmd *hyperv_dimm_cmd_new_smart(struct ndctl_dimm
> *dimm)
> > +static struct ndctl_cmd *hyperv_dimm_cmd_new_cmd(struct ndctl_dimm
> *dimm,
> > +unsigned int command)
>
> The 'new' in the function name is a bit confusing, as 'new' functions
> are also used for cmd allocation. I'd suggest following the intel.c
> convention and calling it 'alloc_hyperv_cmd'.
Will fix it.
> Maybe consider calling it this right from the start in patch 1, and also
> having the wrapper for the new smart command in patch 1 - this way there
> is less unrelated thrash in this patch, and makes it easier to review.
Ok. Will do.
> > -static int hyperv_smart_valid(struct ndctl_cmd *cmd)
> > +static struct ndctl_cmd *hyperv_dimm_cmd_new_smart(struct ndctl_dimm
> *dimm)
> Similar comment as above. In general this sort of refactoring can be
> done in two ways:
>
>1. If you know the end result at the start, just create the generic
>helpers then, so future patches don't have the thrash.
>
>2. If the changes are in prior code, and if the refactoring is
>extensive, split it out into its own functionally equivalent patch,
>so that the meat of *this* change is not cluttered by unrelated
>refactoring.
Will fix it.
> > static int hyperv_cmd_xlat_firmware_status(struct ndctl_cmd *cmd)
> > {
> > return CMD_HYPERV_STATUS(cmd) == 0 ? 0 : -EINVAL;
> > }
> >
> > +static int hyperv_get_shutdown_count(struct ndctl_cmd *cmd,
> > +unsigned int *count)
>
> No need to split this line, it fits within 80 columns.
Ok. Will fix it.
> > +{
> > + unsigned int command = ND_HYPERV_CMD_GET_SHUTDOWN_INFO;
> > + struct ndctl_cmd *cmd_get_shutdown_info;
> > + int rc;
> > +
> > + cmd_get_shutdown_info = hyperv_dimm_cmd_new_cmd(cmd->dimm,
> command);
> > + if (!cmd_get_shutdown_info)
> > + return -EINVAL;
> > +
> > + if (ndctl_cmd_submit(cmd_get_shutdown_info) < 0 ||
>
> The return value from ndctl_cmd_submit() only indicates that the kernel
> was successfully able to send the DSM to the platform firmware. It does
> *not* capture any failures indicated by the platform in the 'status'
> field of the cmd struct. We should be ensuring that was also successful
> by calling the hyperv_cmd_xlat_firmware_status for the cmd.
> Alternatively, instead of the ndctl_cmd_submit() API, you could also use
> the newly added (in ndctl-64) ndctl_cmd_submit_xlat() API, which calls
> the dimm-op for 'xlat_firmware_status' if present to do a status
> translation, and present it in the return value.
Will fix it.
> > +static unsigned int hyperv_cmd_smart_get_shutdown_count(struct
> ndctl_cmd *cmd)
> > +{
> > + unsigned int count;
> > +
> > + return hyperv_get_shutdown_count(cmd, &count) == 0 ? count :
> UINT_MAX;
>
> I'd prefer the long form rc = func(); if (rc) .. here.
> Also, shouldn't we set errno appropriately in the UINT_MAX case?
Will fix it.
Thanks,
-- Dexuan
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
RE: [ndctl PATCH v2 1/4] libndctl: add support for NVDIMM_FAMILY_HYPERV's _DSM Function 1
> From: Verma, Vishal L
> Sent: Wednesday, March 20, 2019 5:23 PM
> ...
> 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.
OK, I'll use git-send-email. Previously due to some SMTP server issue I
couldn't use that, but now I can.
>
> On Wed, 2019-02-20 at 05:10 +, 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].
Thanks for sharing the good read! I'll update my changelog accordingly.
> > diff --git a/ndctl/lib/hyperv.c b/ndctl/lib/hyperv.c
> > new file mode 100644
> For new files, use the SPDX license identifiers, for an example, see:
Ok, will do.
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#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.
I agree. Will change it.
Previously I tried to follow the same style in
ndctl/lib/hpe1.c and ndctl/lib/msft.c.
> > + size = sizeof(*cmd) + sizeof(struct nd_pkg_hyperv);
> > + ...
> > + 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.
Will fix it.
> > + 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 :)
Will fix it.
> > --- /dev/null
> > +++ b/ndctl/lib/hyperv.h
> > @@ -0,0 +1,51 @@
> > +/*
> > + * Copyright (c) 2019, Microsoft Corporation.
> > ...
>
> Same comment about SPDX License format as above.
Will fix it.
> > +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?
Hyper-V guys told me that 0x1F is always supported, and we do
check if a cmd is supported or not in the kernel in
acpi_nfit_add_dimm() and acpi_nfit_ctl(). In case a cmd is not supported,
acpi_nfit_ctl() returns -ENOTTY. So IMO we don't bother to check it in ndctl.
> 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"
Will fix it.
> > +
> > + /* 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
> */
I copid the sentences from ndctl/lib/msft.h. :-)
Will fix it.
Thanks,
-- Dexuan
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
RE: [PATCH v4] nvdimm, btt: fix LBA masking during 'free list' population
> From: Verma, Vishal L
> Sent: Wednesday, February 27, 2019 10:13 AM
> ...
> > I suppose the new line should not be added, and the line
> > if (ent_e_flag(log_new.old_map)) {
> > should be changed to
> > if (ent_e_flag(log_new.old_map) && !
> ent_z_flag(log_new.old_map)) {
> > ?
> >
> Ah yes good catch, I broke my own rule, in that freelist[i].block should
> not have flag bits since it is used as is. The freelist has a separate
> has_err field to allow for error clearing, and we should be setting
> that.
>
> Does the following incremental patch fix it? Let me know and I can send
> a new version including it.
Yes, the patch works for me. Thank you!
But, should we really set the has_err field? Here the enry has both
the zere/error flags set. As I understand, this is not an error, because
the UEFI spec says "... both Error and Zero are set to indicate a map entry
in its normal, non-error state".
> diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c
> index 294c48e45e74..1e7c1a66cef8 100644
> --- a/drivers/nvdimm/btt.c
> +++ b/drivers/nvdimm/btt.c
> @@ -569,7 +569,7 @@ static int btt_freelist_init(struct arena_info
> *arena)
>* the BTT read-only
>*/
> if (ent_e_flag(log_new.old_map)) {
> - set_e_flag(arena->freelist[i].block);
> + arena->freelist[i].has_err = 1;
> ret = arena_clear_freelist_error(arena, i);
> if (ret)
> dev_err_ratelimited(to_dev(arena),
Thanks,
-- Dexuan
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
RE: [PATCH v4] nvdimm, btt: fix LBA masking during 'free list' population
> From: Vishal Verma
> Sent: Tuesday, February 26, 2019 3:14 PM
> ...
> diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c
> @@ -555,16 +555,21 @@ static int btt_freelist_init(struct arena_info *arena)
> if (new < 0)
> return new;
>
> + /* old and new map entries with any flags stripped out */
> + log_oldmap = ent_lba(le32_to_cpu(log_new.old_map));
> + log_newmap = ent_lba(le32_to_cpu(log_new.new_map));
> +
> /* sub points to the next one to be overwritten */
> arena->freelist[i].sub = 1 - new;
> arena->freelist[i].seq = nd_inc_seq(le32_to_cpu(log_new.seq));
> - arena->freelist[i].block = le32_to_cpu(log_new.old_map);
> + arena->freelist[i].block = log_oldmap;
>
> /*
>* FIXME: if error clearing fails during init, we want to make
>* the BTT read-only
>*/
> if (ent_e_flag(log_new.old_map)) {
> + set_e_flag(arena->freelist[i].block);
Hi Vishal,
The logic doesn't look quite right to me: as I understand, if both the zero
flag and
the error flag are set, it means a normal map entry rather than an error.
Windows can indeed set both flags:
[3.756239] freelist_init: i=0, log_old: lba=bcc01, old_map=c00bcc30,
new_map=c00bcc02, seq=2
[3.765583] freelist_init: i=0, log_new: lba=bcc00, old_map=c0001b7b,
new_map=c00bcc30, seq=3
(For the full log, see
https://github.com/dcui/linux/commit/b472968e01d72be3bd42e4568befc4602f9ac396 )
Due to this new line, the 'block' can exceed the normal internal_nlba, and
cause I/O failure:
[ 103.589766] btt_write_pg failed: lane=0x0, new_postmap=0x40001b7b,
internal_nlba=0x1ff8018
[ 103.602387] nd_pmem btt1.1: io error in WRITE sector 33056, len 4096,
[ 103.611662] Buffer I/O error on dev pmem1s2, logical block 36, lost async
page write
I suppose the new line should not be added, and the line
if (ent_e_flag(log_new.old_map)) {
should be changed to
if (ent_e_flag(log_new.old_map) && !
ent_z_flag(log_new.old_map)) {
?
> ret = arena_clear_freelist_error(arena, i);
> if (ret)
> dev_err_ratelimited(to_dev(arena),
Thanks,
-- Dexuan
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
RE: [ndctl PATCH v2 4/4] ndctl, monitor: support NVDIMM_FAMILY_HYPERV
> From: qi.f...@fujitsu.com > Sent: Sunday, February 24, 2019 8:48 PM > To: Dexuan Cui ; Dave Jiang ; > Vishal Verma ; Dan Williams > ; linux-nvdimm@lists.01.org; Michael Kelley > ; Johannes Thumshirn > Subject: RE: [ndctl PATCH v2 4/4] ndctl, monitor: support > NVDIMM_FAMILY_HYPERV > > > > > Hi Qi, > > Generally speaking, I agree with you, but here the fact is that we can only > monitor this > > one single event "dimm-health-state" for Hyper-V Virtual NVDIMM, and the > other > > events are meaningless for Hyper-V Virtual NVDIMM, as they're not > supported by > > Hyper-V Virtual NVDIMM. > > > > So, if the user specifies more events than just "dimm-health-state", or the > user doesn't > > specify "dimm-health-state", it's reasonable to force monitor.event_flags to > equal to > > ND_EVENT_HEALTH_STATE, and I explicitly print a warning. > > > > The line "monitor.event_flags = ND_EVENT_HEALTH_STATE" only runs in a > Linux VM > > running on Hyper-V. It can't adversely affect bare metal cases. In a Linux > > VM > on > > Hyper-V, all the NVDIMMs appearing in the VM are Hyper-V Virtual > NVDIMMs. > > > > A physical NVDIMM can't be directly passed through to a VM and the > hypervisors (e.g., > > Xen, KVM, Hyper-V, Qemu, etc.) have to virtualize it to make it available > > to a > VM. In a > > VM, the NVDIMM_FAMILY is NVDIMM_FAMILY_HYPERV only when the VM > runs on > > Hyper-V. > > > > So I think we should be good. :-) > > > > Thanks, > > -- Dexuan > > Hi Dexuan, > > Thanks for your explanation. > I think at least we should document it into man page, so that the users could > know that their options may be modified by monitor. Good suggestion! After the patch receives more comments and/or after it's accepted, I'll add a new patch for the man page: Documentation/ndctl/ndctl-monitor.txt. > Though I have a little concern about that there may be other events for > Hyper-V Virtual NVDIMM can be monitored in future... > Qi As far as I know, that's unlikely, at least in the foreseeable future. :-) The VM is supposed to see a virtual NVDIMM, for which the hypervisor only emulates the minimal necessary info, based on the physical NVDIMM's state. Thanks, --Dexuan ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
RE: [ndctl PATCH v2 4/4] ndctl, monitor: support NVDIMM_FAMILY_HYPERV
> From: qi.f...@fujitsu.com
> Sent: Thursday, February 21, 2019 12:40 AM
> > ...
> > + /*
> > +* Hyper-V Virtual NVDIMM doesn't use ND_CMD_SMART to get the
> > health info. Instead, it uses ND_CMD_CALL, so the checking here can't
> > +* apply, and it doesn't support threshold alarms -- actually it only
> > +* supports one event: ND_EVENT_HEALTH_STATE.
> > +*/
> > + if (ndctl_dimm_get_cmd_family(dimm) == NVDIMM_FAMILY_HYPERV) {
> > + if (monitor.event_flags != ND_EVENT_HEALTH_STATE) {
> > + monitor.event_flags = ND_EVENT_HEALTH_STATE;
> > +
> > + notice(&monitor, "%s: only dimm-health-state can be
> > monitored\n", name);
> Hi Dexuan,
> I think that "monitor" should be a "read-only" command and can't force the
> users to change their options.
> The monitor.event_flags will work for all NVDIMMs to be monitored.
> I don't know whether a physical NVDIMM could be mounted on the OS
> running inside of a virtual machine.
> If yes, this forced changing may cause an exception of monitoring smart
> threshold events on NVDIMM.
>
> Thanks,
> Qi
Hi Qi,
Generally speaking, I agree with you, but here the fact is that we can only
monitor this one single event "dimm-health-state" for Hyper-V Virtual NVDIMM,
and the other events are meaningless for Hyper-V Virtual NVDIMM, as they're
not supported by Hyper-V Virtual NVDIMM.
So, if the user specifies more events than just "dimm-health-state", or the user
doesn't specify "dimm-health-state", it's reasonable to force
monitor.event_flags
to equal to ND_EVENT_HEALTH_STATE, and I explicitly print a warning.
The line "monitor.event_flags = ND_EVENT_HEALTH_STATE" only runs in a Linux
VM running on Hyper-V. It can't adversely affect bare metal cases. In a Linux
VM on Hyper-V, all the NVDIMMs appearing in the VM are Hyper-V Virtual NVDIMMs.
A physical NVDIMM can't be directly passed through to a VM and the hypervisors
(e.g., Xen, KVM, Hyper-V, Qemu, etc.) have to virtualize it to make it
available to a
VM. In a VM, the NVDIMM_FAMILY is NVDIMM_FAMILY_HYPERV only when
the VM runs on Hyper-V.
So I think we should be good. :-)
Thanks,
-- Dexuan
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
[ndctl PATCH v2 4/4] ndctl, monitor: support NVDIMM_FAMILY_HYPERV
Currently "ndctl monitor" fails for NVDIMM_FAMILY_HYPERV due to
"no smart support".
NVDIMM_FAMILY_HYPERV doesn't use ND_CMD_SMART to get the health info.
Instead, it uses ND_CMD_CALL, so the checking here can't apply,and it
doesn't support threshold alarms -- actually it only supports one event:
ND_EVENT_HEALTH_STATE.
See http://www.uefi.org/RFIC_LIST ("Virtual NVDIMM 0x1901").
Let's skip the unnecessary checking for NVDIMM_FAMILY_HYPERV, and make
sure we only monitor the "dimm-health-state" event and ignore the others.
With the patch, when an error happens, we log it with such a message:
{"timestamp":"1550547497.431731497","pid":1571,"event":
{"dimm-health-state":true},"dimm":{"dev":"nmem1",
"id":"04d5-01-1701-0100","handle":1,"phys_id":0,
"health":{"health_state":"fatal","shutdown_count":8}}}
Here the meaningful info is:
"health":{"health_state":"fatal","shutdown_count":8}
Signed-off-by: Dexuan Cui
---
ndctl/monitor.c | 42 +++---
1 file changed, 35 insertions(+), 7 deletions(-)
diff --git a/ndctl/monitor.c b/ndctl/monitor.c
index 43b2abe..43beb06 100644
--- a/ndctl/monitor.c
+++ b/ndctl/monitor.c
@@ -265,31 +265,59 @@ static bool filter_region(struct ndctl_region *region,
return true;
}
-static void filter_dimm(struct ndctl_dimm *dimm, struct util_filter_ctx *fctx)
+static bool ndctl_dimm_test_and_enable_notification(struct ndctl_dimm *dimm)
{
- struct monitor_dimm *mdimm;
- struct monitor_filter_arg *mfa = fctx->monitor;
const char *name = ndctl_dimm_get_devname(dimm);
+ /*
+* Hyper-V Virtual NVDIMM doesn't use ND_CMD_SMART to get the health
+* info. Instead, it uses ND_CMD_CALL, so the checking here can't
+* apply, and it doesn't support threshold alarms -- actually it only
+* supports one event: ND_EVENT_HEALTH_STATE.
+*/
+ if (ndctl_dimm_get_cmd_family(dimm) == NVDIMM_FAMILY_HYPERV) {
+ if (monitor.event_flags != ND_EVENT_HEALTH_STATE) {
+ monitor.event_flags = ND_EVENT_HEALTH_STATE;
+
+ notice(&monitor,
+ "%s: only dimm-health-state can be monitored\n",
+ name);
+ }
+ return true;
+ }
+
if (!ndctl_dimm_is_cmd_supported(dimm, ND_CMD_SMART)) {
err(&monitor, "%s: no smart support\n", name);
- return;
+ return false;
}
if (!ndctl_dimm_is_cmd_supported(dimm, ND_CMD_SMART_THRESHOLD)) {
err(&monitor, "%s: no smart threshold support\n", name);
- return;
+ return false;
}
if (!ndctl_dimm_is_flag_supported(dimm, ND_SMART_ALARM_VALID)) {
err(&monitor, "%s: smart alarm invalid\n", name);
- return;
+ return false;
}
if (enable_dimm_supported_threshold_alarms(dimm)) {
err(&monitor, "%s: enable supported threshold alarms failed\n",
name);
- return;
+ return false;
}
+ return true;
+}
+
+static void filter_dimm(struct ndctl_dimm *dimm, struct util_filter_ctx *fctx)
+{
+ struct monitor_dimm *mdimm;
+ struct monitor_filter_arg *mfa = fctx->monitor;
+ const char *name = ndctl_dimm_get_devname(dimm);
+
+
+ if (!ndctl_dimm_test_and_enable_notification(dimm))
+ return;
+
mdimm = calloc(1, sizeof(struct monitor_dimm));
if (!mdimm) {
err(&monitor, "%s: calloc for monitor dimm failed\n", name);
--
2.19.1
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
[ndctl PATCH v2 1/4] libndctl: add support for NVDIMM_FAMILY_HYPERV's _DSM Function 1
This patch retrieves the health info by Hyper-V _DSM method Function 1:
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-",
"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
---
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 000..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.
+ */
+#include
+#include
+#include
+#include
+#include
+#include "private.h"
+#include "hyperv.h"
+
+#define CMD_HYPERV(_c) ((_c)->hyperv)
+#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;
+
+ 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 ||
+ 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;
+
[ndctl PATCH v2 3/4] ndctl, lib: implement ndctl_dimm_get_cmd_family()
Let's export the family info so we can do some family-specific
handling in ndctl/monitor.c for Hyper-V NVDIMM.
Signed-off-by: Dexuan Cui
---
ndctl/lib/libndctl.c | 5 +
ndctl/lib/libndctl.sym | 1 +
ndctl/libndctl.h | 1 +
3 files changed, 7 insertions(+)
diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
index 48bdb27..1186579 100644
--- a/ndctl/lib/libndctl.c
+++ b/ndctl/lib/libndctl.c
@@ -1550,6 +1550,11 @@ NDCTL_EXPORT struct ndctl_dimm
*ndctl_dimm_get_next(struct ndctl_dimm *dimm)
return list_next(&bus->dimms, dimm, list);
}
+NDCTL_EXPORT unsigned long ndctl_dimm_get_cmd_family(struct ndctl_dimm *dimm)
+{
+ return dimm->cmd_family;
+}
+
NDCTL_EXPORT unsigned int ndctl_dimm_get_handle(struct ndctl_dimm *dimm)
{
return dimm->handle;
diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym
index cb9f769..470e895 100644
--- a/ndctl/lib/libndctl.sym
+++ b/ndctl/lib/libndctl.sym
@@ -38,6 +38,7 @@ global:
ndctl_bus_wait_probe;
ndctl_dimm_get_first;
ndctl_dimm_get_next;
+ ndctl_dimm_get_cmd_family;
ndctl_dimm_get_handle;
ndctl_dimm_get_phys_id;
ndctl_dimm_get_vendor;
diff --git a/ndctl/libndctl.h b/ndctl/libndctl.h
index 0debdb6..cb5a8fc 100644
--- a/ndctl/libndctl.h
+++ b/ndctl/libndctl.h
@@ -145,6 +145,7 @@ struct ndctl_dimm *ndctl_dimm_get_next(struct ndctl_dimm
*dimm);
for (dimm = ndctl_dimm_get_first(bus); \
dimm != NULL; \
dimm = ndctl_dimm_get_next(dimm))
+unsigned long ndctl_dimm_get_cmd_family(struct ndctl_dimm *dimm);
unsigned int ndctl_dimm_get_handle(struct ndctl_dimm *dimm);
unsigned short ndctl_dimm_get_phys_id(struct ndctl_dimm *dimm);
unsigned short ndctl_dimm_get_vendor(struct ndctl_dimm *dimm);
--
2.19.1
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
[ndctl PATCH v2 2/4] libndctl: NVDIMM_FAMILY_HYPERV: add .smart_get_shutdown_count (Function 2)
With the patch, "ndctl list --dimms --health --idle" can show
"shutdown_count" now, e.g.
{
"dev":"nmem0",
"id":"04d5-01-1701-",
"handle":0,
"phys_id":0,
"health":{
"health_state":"ok",
"shutdown_count":2
}
}
The patch has to directly call ndctl_cmd_submit() in
hyperv_cmd_smart_get_flags() and hyperv_cmd_smart_get_shutdown_count() to
get the needed info, because util_dimm_health_to_json() only submits *one*
command, and unluckily for Hyper-V Virtual NVDIMM we need to call both
Function 1 and 2 to get the needed info.
My feeling is that it's not very good to directly call ndctl_cmd_submit(),
but by doing this we don't need to make any change to the common code, and
I'm unsure if it's good to change the common code just for Hyper-V.
Signed-off-by: Dexuan Cui
---
ndctl/lib/hyperv.c | 62 --
ndctl/lib/hyperv.h | 7 ++
2 files changed, 62 insertions(+), 7 deletions(-)
diff --git a/ndctl/lib/hyperv.c b/ndctl/lib/hyperv.c
index b303d50..e8ec142 100644
--- a/ndctl/lib/hyperv.c
+++ b/ndctl/lib/hyperv.c
@@ -22,7 +22,8 @@
#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)
+static struct ndctl_cmd *hyperv_dimm_cmd_new_cmd(struct ndctl_dimm *dimm,
+unsigned int command)
{
struct ndctl_bus *bus = ndctl_dimm_get_bus(dimm);
struct ndctl_ctx *ctx = ndctl_bus_get_ctx(bus);
@@ -35,8 +36,7 @@ static struct ndctl_cmd *hyperv_dimm_cmd_new_smart(struct
ndctl_dimm *dimm)
return NULL;
}
- if (test_dimm_dsm(dimm, ND_HYPERV_CMD_GET_HEALTH_INFO) ==
- DIMM_DSM_UNSUPPORTED) {
+ if (test_dimm_dsm(dimm, command) == DIMM_DSM_UNSUPPORTED) {
dbg(ctx, "unsupported function\n");
return NULL;
}
@@ -54,7 +54,7 @@ static struct ndctl_cmd *hyperv_dimm_cmd_new_smart(struct
ndctl_dimm *dimm)
hyperv = CMD_HYPERV(cmd);
hyperv->gen.nd_family = NVDIMM_FAMILY_HYPERV;
- hyperv->gen.nd_command = ND_HYPERV_CMD_GET_HEALTH_INFO;
+ hyperv->gen.nd_command = command;
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);
@@ -65,34 +65,74 @@ static struct ndctl_cmd *hyperv_dimm_cmd_new_smart(struct
ndctl_dimm *dimm)
return cmd;
}
-static int hyperv_smart_valid(struct ndctl_cmd *cmd)
+static struct ndctl_cmd *hyperv_dimm_cmd_new_smart(struct ndctl_dimm *dimm)
+{
+ return hyperv_dimm_cmd_new_cmd(dimm, ND_HYPERV_CMD_GET_HEALTH_INFO);
+}
+
+static int hyperv_cmd_valid(struct ndctl_cmd *cmd, unsigned int command)
{
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 ||
+ CMD_HYPERV(cmd)->gen.nd_command != command ||
cmd->status != 0 ||
CMD_HYPERV_STATUS(cmd) != 0)
return cmd->status < 0 ? cmd->status : -EINVAL;
return 0;
}
+static int hyperv_smart_valid(struct ndctl_cmd *cmd)
+{
+ return hyperv_cmd_valid(cmd, ND_HYPERV_CMD_GET_HEALTH_INFO);
+}
+
static int hyperv_cmd_xlat_firmware_status(struct ndctl_cmd *cmd)
{
return CMD_HYPERV_STATUS(cmd) == 0 ? 0 : -EINVAL;
}
+static int hyperv_get_shutdown_count(struct ndctl_cmd *cmd,
+unsigned int *count)
+{
+ unsigned int command = ND_HYPERV_CMD_GET_SHUTDOWN_INFO;
+ struct ndctl_cmd *cmd_get_shutdown_info;
+ int rc;
+
+ cmd_get_shutdown_info = hyperv_dimm_cmd_new_cmd(cmd->dimm, command);
+ if (!cmd_get_shutdown_info)
+ return -EINVAL;
+
+ if (ndctl_cmd_submit(cmd_get_shutdown_info) < 0 ||
+ hyperv_cmd_valid(cmd_get_shutdown_info, command) < 0) {
+ rc = -EINVAL;
+ goto out;
+ }
+
+ *count = CMD_HYPERV(cmd_get_shutdown_info)->u.shutdown_info.count;
+ rc = 0;
+out:
+ ndctl_cmd_unref(cmd_get_shutdown_info);
+ return rc;
+}
+
static unsigned int hyperv_cmd_smart_get_flags(struct ndctl_cmd *cmd)
{
int rc;
+ unsigned int count;
+ unsigned int flags = 0;
rc = hyperv_smart_valid(cmd);
if (rc < 0) {
errno = -rc;
return 0;
}
+ flags |= ND_SMART_HEALTH_VALID;
- return ND_SMART_HEALTH_VALID;
+ if (hyperv_get_shutdown
[ndctl PATCH v2 0/4] add the support for NVDIMM_FAMILY_HYPERV
NVDIMM_FAMILY_HYPERV has been supported on this branch of the kernel: https://git.kernel.org/pub/scm/linux/kernel/git/djbw/nvdimm.git/log/?h=libnvdimm-pending Now, let's add the ndctl part as well. Patch 0001 and 0002 have been posted on Feb 5, and this is just a resend. Patch 0003 and 0004 are a split version of the single patch I posted on Feb 14. In v2, I split the single patch into 2 separate patches for easy review, and I also added an explicit warning if the user specifies unsupported events for "ndctl monitor". Thanks Qi Fuli for the suggestion, and thanks Johannes Thumshirn for reviewing the patch! Please review the new patchset. Thanks! Dexuan Cui (4): libndctl: add support for NVDIMM_FAMILY_HYPERV's _DSM Function 1 libndctl: NVDIMM_FAMILY_HYPERV: add .smart_get_shutdown_count (Function 2) ndctl, lib: implement ndctl_dimm_get_cmd_family() ndctl, monitor: support NVDIMM_FAMILY_HYPERV ndctl/lib/Makefile.am | 1 + ndctl/lib/hyperv.c | 177 + ndctl/lib/hyperv.h | 58 ++ ndctl/lib/libndctl.c | 7 ++ ndctl/lib/libndctl.sym | 1 + ndctl/lib/private.h| 3 + ndctl/libndctl.h | 1 + ndctl/monitor.c| 42 -- ndctl/ndctl.h | 1 + 9 files changed, 284 insertions(+), 7 deletions(-) create mode 100644 ndctl/lib/hyperv.c create mode 100644 ndctl/lib/hyperv.h -- 2.19.1 ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
RE: [ndctl PATCH] ndctl, monitor: support NVDIMM_FAMILY_HYPERV
> From: qi.f...@fujitsu.com > Sent: Sunday, February 17, 2019 5:52 PM > ... > I am sorry I didn't explain it clearly enough. > I want to say that users may not know that NVDIMM_FAMILY_HYPERV doesn't > support monitoring smart threshold events. > If users setup monitoring smart threshold events on > NVDIMM_FAMILY_HYPERV by mistake, > it would be more friendly to send them a notification. > > Qi Got it. I'll add an explicit warning and don't monitor unsupported events. Thanks, -- Dexuan ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
RE: [ndctl PATCH] ndctl, monitor: support NVDIMM_FAMILY_HYPERV
> From: qi.f...@fujitsu.com
> Sent: Friday, February 15, 2019 1:26 AM
> > ...
> > --- a/ndctl/monitor.c
> > +++ b/ndctl/monitor.c
> > @@ -265,31 +265,50 @@ static bool filter_region(struct ndctl_region
> *region,
> > return true;
> > }
> >
> > -static void filter_dimm(struct ndctl_dimm *dimm, struct util_filter_ctx
> > *fctx)
> > +static bool ndctl_dimm_test_and_enable_notification(struct ndctl_dimm
> *dimm)
> > {
> > - struct monitor_dimm *mdimm;
> > - struct monitor_filter_arg *mfa = fctx->monitor;
> > const char *name = ndctl_dimm_get_devname(dimm);
> >
> > + /*
> > +* Hyper-V Virtual NVDIMM doesn't use ND_CMD_SMART to get the
> > health
> > +* info. Instead, it uses ND_CMD_CALL, so the checking here can't
> > +* apply, and it doesn't support threshold alarms.
> > +*/
> > + if (ndctl_dimm_get_cmd_family(dimm) == NVDIMM_FAMILY_HYPERV)
> > + return true;
>
> Hi,
>
> I think it would be better to add a checking monitor.event_flags step here.
> Users should be notified if they setup monitoring smart threshold events on
> NVDIMM_FAMILY_HYPERV.
>
> QI
Hi Qi,
Unluckily NVDIMM_FAMILY_HYPERV doesn't support monitoring smart
threshold events. Please see the _DSM Interface for Hyper-V Virtual NVDIMM
at https://uefi.org/RFIC_LIST (Virtual NVDIMM 0x1901).
So there is no ops->new_smart_threshold defined for
NVDIMM_FAMILY_HYPERV.
The patch only skips the checks for NVDIMM_FAMILY_HYPERV, and the behavior
for the others remains the same.
Thanks,
-- Dexuan
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
[ndctl PATCH] ndctl, monitor: support NVDIMM_FAMILY_HYPERV
Currently "ndctl monitor" fails for NVDIMM_FAMILY_HYPERV due to
"no smart support".
Actually NVDIMM_FAMILY_HYPERV doesn't use ND_CMD_SMART to get the health
info. Instead, it uses ND_CMD_CALL, so the checking here can't apply,
and NVDIMM_FAMILY_HYPERV doesn't support threshold alarms.
Let's skip the unnecessary checking for NVDIMM_FAMILY_HYPERV.
With the patch, when an error happens, we can log it with such a message:
{"timestamp":"1550209474.683237420","pid":3874,"event":
{"dimm-spares-remaining":false,"dimm-media-temperature":false,
"dimm-controller-temperature":false,"dimm-health-state":true,
"dimm-unclean-shutdown":false},"dimm":{"dev":"nmem1",
"id":"04d5-01-1701-0100","handle":1,"phys_id":0,"health":
{"health_state":"critical","shutdown_count":8}}}
Here the meaning info is:
"health": {"health_state":"critical","shutdown_count":8}
Signed-off-by: Dexuan Cui
---
ndctl/lib/libndctl.c | 5 +
ndctl/lib/libndctl.sym | 1 +
ndctl/libndctl.h | 1 +
ndctl/monitor.c| 33 ++---
4 files changed, 33 insertions(+), 7 deletions(-)
diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
index 48bdb27..1186579 100644
--- a/ndctl/lib/libndctl.c
+++ b/ndctl/lib/libndctl.c
@@ -1550,6 +1550,11 @@ NDCTL_EXPORT struct ndctl_dimm
*ndctl_dimm_get_next(struct ndctl_dimm *dimm)
return list_next(&bus->dimms, dimm, list);
}
+NDCTL_EXPORT unsigned long ndctl_dimm_get_cmd_family(struct ndctl_dimm *dimm)
+{
+ return dimm->cmd_family;
+}
+
NDCTL_EXPORT unsigned int ndctl_dimm_get_handle(struct ndctl_dimm *dimm)
{
return dimm->handle;
diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym
index cb9f769..470e895 100644
--- a/ndctl/lib/libndctl.sym
+++ b/ndctl/lib/libndctl.sym
@@ -38,6 +38,7 @@ global:
ndctl_bus_wait_probe;
ndctl_dimm_get_first;
ndctl_dimm_get_next;
+ ndctl_dimm_get_cmd_family;
ndctl_dimm_get_handle;
ndctl_dimm_get_phys_id;
ndctl_dimm_get_vendor;
diff --git a/ndctl/libndctl.h b/ndctl/libndctl.h
index 0debdb6..cb5a8fc 100644
--- a/ndctl/libndctl.h
+++ b/ndctl/libndctl.h
@@ -145,6 +145,7 @@ struct ndctl_dimm *ndctl_dimm_get_next(struct ndctl_dimm
*dimm);
for (dimm = ndctl_dimm_get_first(bus); \
dimm != NULL; \
dimm = ndctl_dimm_get_next(dimm))
+unsigned long ndctl_dimm_get_cmd_family(struct ndctl_dimm *dimm);
unsigned int ndctl_dimm_get_handle(struct ndctl_dimm *dimm);
unsigned short ndctl_dimm_get_phys_id(struct ndctl_dimm *dimm);
unsigned short ndctl_dimm_get_vendor(struct ndctl_dimm *dimm);
diff --git a/ndctl/monitor.c b/ndctl/monitor.c
index 43b2abe..6adc305 100644
--- a/ndctl/monitor.c
+++ b/ndctl/monitor.c
@@ -265,31 +265,50 @@ static bool filter_region(struct ndctl_region *region,
return true;
}
-static void filter_dimm(struct ndctl_dimm *dimm, struct util_filter_ctx *fctx)
+static bool ndctl_dimm_test_and_enable_notification(struct ndctl_dimm *dimm)
{
- struct monitor_dimm *mdimm;
- struct monitor_filter_arg *mfa = fctx->monitor;
const char *name = ndctl_dimm_get_devname(dimm);
+ /*
+* Hyper-V Virtual NVDIMM doesn't use ND_CMD_SMART to get the health
+* info. Instead, it uses ND_CMD_CALL, so the checking here can't
+* apply, and it doesn't support threshold alarms.
+*/
+ if (ndctl_dimm_get_cmd_family(dimm) == NVDIMM_FAMILY_HYPERV)
+ return true;
+
if (!ndctl_dimm_is_cmd_supported(dimm, ND_CMD_SMART)) {
err(&monitor, "%s: no smart support\n", name);
- return;
+ return false;
}
if (!ndctl_dimm_is_cmd_supported(dimm, ND_CMD_SMART_THRESHOLD)) {
err(&monitor, "%s: no smart threshold support\n", name);
- return;
+ return false;
}
if (!ndctl_dimm_is_flag_supported(dimm, ND_SMART_ALARM_VALID)) {
err(&monitor, "%s: smart alarm invalid\n", name);
- return;
+ return false;
}
if (enable_dimm_supported_threshold_alarms(dimm)) {
err(&monitor, "%s: enable supported threshold alarms failed\n",
name);
- return;
+ return false;
}
+ return true;
+}
+
+static void filter_dimm(struct ndctl_dimm *dimm, struct util_filter_ctx *fctx)
+{
+ struct monitor_dimm *mdimm;
+ struct monitor_filter_arg *mfa = fctx->monitor;
+ const char *name = ndctl_dimm_get_devname(dimm);
+
+
+ if (!ndctl_dimm_test_and_enable_notification(dimm))
+ return;
+
mdimm = calloc(1, sizeof(struct monitor_dimm));
if (!mdimm) {
err(&monitor, "%s: calloc for monitor dimm failed\n", name);
--
2.19.1
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
RE: [ndctl PATCH 2/2] libndctl: NVDIMM_FAMILY_HYPERV: add .smart_get_shutdown_count (Function 2)
> From: Linux-nvdimm On Behalf Of
> Dexuan Cui
> Sent: Tuesday, February 5, 2019 7:15 PM
> To: Dan Williams ; linux-nvdimm@lists.01.org
> Cc: Michael Kelley
> Subject: [ndctl PATCH 2/2] libndctl: NVDIMM_FAMILY_HYPERV:
> add .smart_get_shutdown_count (Function 2)
>
>
> With the patch, "ndctl list --dimms --health --idle" can show
> "shutdown_count" now, e.g.
>
> {
> "dev":"nmem0",
> "id":"04d5-01-1701-",
> "handle":0,
> "phys_id":0,
> "health":{
> "health_state":"ok",
> "shutdown_count":2
> }
> }
>
> The patch has to directly call ndctl_cmd_submit() in
> hyperv_cmd_smart_get_flags() and hyperv_cmd_smart_get_shutdown_count()
> to
> get the needed info, because util_dimm_health_to_json() only submits one
> command, and unluckily for Hyper-V Virtual NVDIMM we need to call both
> Function 1 and 2 to get the needed info.
>
> My feeling is that it's not good to directly call ndctl_cmd_submit(), but
> doing this requires no change to the common code, and I'm unsure if it's
> better to change the common code just for Hyper-V.
>
> Signed-off-by: Dexuan Cui
> ---
> ndctl/lib/hyperv.c | 62
> --
> ndctl/lib/hyperv.h | 7 ++
> 2 files changed, 62 insertions(+), 7 deletions(-)
>
> diff --git a/ndctl/lib/hyperv.c b/ndctl/lib/hyperv.c
> index b303d50..e8ec142 100644
> --- a/ndctl/lib/hyperv.c
> +++ b/ndctl/lib/hyperv.c
> @@ -22,7 +22,8 @@
> #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)
> +static struct ndctl_cmd *hyperv_dimm_cmd_new_cmd(struct ndctl_dimm
> *dimm,
> + unsigned int command)
> {
> struct ndctl_bus *bus = ndctl_dimm_get_bus(dimm);
> struct ndctl_ctx *ctx = ndctl_bus_get_ctx(bus);
> @@ -35,8 +36,7 @@ static struct ndctl_cmd
> *hyperv_dimm_cmd_new_smart(struct ndctl_dimm *dimm)
> return NULL;
> }
>
> - if (test_dimm_dsm(dimm, ND_HYPERV_CMD_GET_HEALTH_INFO) ==
> - DIMM_DSM_UNSUPPORTED) {
> + if (test_dimm_dsm(dimm, command) == DIMM_DSM_UNSUPPORTED) {
> dbg(ctx, "unsupported function\n");
> return NULL;
> }
> @@ -54,7 +54,7 @@ static struct ndctl_cmd
> *hyperv_dimm_cmd_new_smart(struct ndctl_dimm *dimm)
>
> hyperv = CMD_HYPERV(cmd);
> hyperv->gen.nd_family = NVDIMM_FAMILY_HYPERV;
> - hyperv->gen.nd_command = ND_HYPERV_CMD_GET_HEALTH_INFO;
> + hyperv->gen.nd_command = command;
> 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);
> @@ -65,34 +65,74 @@ static struct ndctl_cmd
> *hyperv_dimm_cmd_new_smart(struct ndctl_dimm *dimm)
> return cmd;
> }
>
> -static int hyperv_smart_valid(struct ndctl_cmd *cmd)
> +static struct ndctl_cmd *hyperv_dimm_cmd_new_smart(struct ndctl_dimm
> *dimm)
> +{
> + return hyperv_dimm_cmd_new_cmd(dimm,
> ND_HYPERV_CMD_GET_HEALTH_INFO);
> +}
> +
> +static int hyperv_cmd_valid(struct ndctl_cmd *cmd, unsigned int command)
> {
> 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 ||
> + CMD_HYPERV(cmd)->gen.nd_command != command ||
> cmd->status != 0 ||
> CMD_HYPERV_STATUS(cmd) != 0)
> return cmd->status < 0 ? cmd->status : -EINVAL;
> return 0;
> }
>
> +static int hyperv_smart_valid(struct ndctl_cmd *cmd)
> +{
> + return hyperv_cmd_valid(cmd, ND_HYPERV_CMD_GET_HEALTH_INFO);
> +}
> +
> static int hyperv_cmd_xlat_firmware_status(struct ndctl_cmd *cmd)
> {
> return CMD_HYPERV_STATUS(cmd) == 0 ? 0 : -EINVAL;
> }
>
> +static int hyperv_get_shutdown_count(struct ndctl_cmd *cmd,
> + unsigned int *count)
> +{
> + unsigned int command = ND_HYPERV_CMD_GET_SHUTDOWN_INFO;
> + struct ndctl_cmd *cmd_get_shutdown_info;
> + int rc;
> +
> + cmd_get_shutdown_info = hyperv_dimm_cmd_new_cmd(cmd->dimm,
> command);
> + if (!cmd_get_shutdown_info)
> + return
RE: [ndctl PATCH 1/2] libndctl: add support for NVDIMM_FAMILY_HYPERV's _DSM Function 1
> From: Linux-nvdimm On Behalf Of
> Dexuan Cui
> Sent: Tuesday, February 5, 2019 7:13 PM
> To: Dan Williams ; linux-nvdimm@lists.01.org
> Cc: Michael Kelley
> Subject: [ndctl PATCH 1/2] libndctl: add support for
> NVDIMM_FAMILY_HYPERV's _DSM Function 1
>
>
> This patch retrieves the health info by Hyper-V _DSM method Function 1:
>
> Get Health Information (Function Index 1)
> See https : // uefi.org/RFIC_LIST
>
> Now "ndctl list --dimms --health --idle" can show a line "health_state":"ok",
> e.g.
>
> {
> "dev":"nmem0",
> "id":"04d5-01-1701-",
> "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".
>
> Hyper-V also supports "Get Unsafe Shutdown Count (Function Index 2)", but
> unluckily util_dimm_health_to_json() only submits *one* command, so we
> don't
> have a chance to call Function 2 here, and hence we can't show a line
> "shutdown_count" in the output. If a user in a Linux virtual machine running
> on Hyper-V is interested in the Unsafe Shutdown Count, please directly run:
> "cat /sys/bus/nd/devices/nmem*/nfit/dirty_shutdown" (there is a pending
> patch
> submitted to the kernel for this sysfs node).
>
> Signed-off-by: Dexuan Cui
> ---
> 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 000..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.
> + */
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include "private.h"
> +#include "hyperv.h"
> +
> +#define CMD_HYPERV(_c) ((_c)->hyperv)
> +#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);
[ndctl PATCH 2/2] libndctl: NVDIMM_FAMILY_HYPERV: add .smart_get_shutdown_count (Function 2)
With the patch, "ndctl list --dimms --health --idle" can show
"shutdown_count" now, e.g.
{
"dev":"nmem0",
"id":"04d5-01-1701-",
"handle":0,
"phys_id":0,
"health":{
"health_state":"ok",
"shutdown_count":2
}
}
The patch has to directly call ndctl_cmd_submit() in
hyperv_cmd_smart_get_flags() and hyperv_cmd_smart_get_shutdown_count() to
get the needed info, because util_dimm_health_to_json() only submits one
command, and unluckily for Hyper-V Virtual NVDIMM we need to call both
Function 1 and 2 to get the needed info.
My feeling is that it's not good to directly call ndctl_cmd_submit(), but
doing this requires no change to the common code, and I'm unsure if it's
better to change the common code just for Hyper-V.
Signed-off-by: Dexuan Cui
---
ndctl/lib/hyperv.c | 62 --
ndctl/lib/hyperv.h | 7 ++
2 files changed, 62 insertions(+), 7 deletions(-)
diff --git a/ndctl/lib/hyperv.c b/ndctl/lib/hyperv.c
index b303d50..e8ec142 100644
--- a/ndctl/lib/hyperv.c
+++ b/ndctl/lib/hyperv.c
@@ -22,7 +22,8 @@
#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)
+static struct ndctl_cmd *hyperv_dimm_cmd_new_cmd(struct ndctl_dimm *dimm,
+unsigned int command)
{
struct ndctl_bus *bus = ndctl_dimm_get_bus(dimm);
struct ndctl_ctx *ctx = ndctl_bus_get_ctx(bus);
@@ -35,8 +36,7 @@ static struct ndctl_cmd *hyperv_dimm_cmd_new_smart(struct
ndctl_dimm *dimm)
return NULL;
}
- if (test_dimm_dsm(dimm, ND_HYPERV_CMD_GET_HEALTH_INFO) ==
- DIMM_DSM_UNSUPPORTED) {
+ if (test_dimm_dsm(dimm, command) == DIMM_DSM_UNSUPPORTED) {
dbg(ctx, "unsupported function\n");
return NULL;
}
@@ -54,7 +54,7 @@ static struct ndctl_cmd *hyperv_dimm_cmd_new_smart(struct
ndctl_dimm *dimm)
hyperv = CMD_HYPERV(cmd);
hyperv->gen.nd_family = NVDIMM_FAMILY_HYPERV;
- hyperv->gen.nd_command = ND_HYPERV_CMD_GET_HEALTH_INFO;
+ hyperv->gen.nd_command = command;
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);
@@ -65,34 +65,74 @@ static struct ndctl_cmd *hyperv_dimm_cmd_new_smart(struct
ndctl_dimm *dimm)
return cmd;
}
-static int hyperv_smart_valid(struct ndctl_cmd *cmd)
+static struct ndctl_cmd *hyperv_dimm_cmd_new_smart(struct ndctl_dimm *dimm)
+{
+ return hyperv_dimm_cmd_new_cmd(dimm, ND_HYPERV_CMD_GET_HEALTH_INFO);
+}
+
+static int hyperv_cmd_valid(struct ndctl_cmd *cmd, unsigned int command)
{
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 ||
+ CMD_HYPERV(cmd)->gen.nd_command != command ||
cmd->status != 0 ||
CMD_HYPERV_STATUS(cmd) != 0)
return cmd->status < 0 ? cmd->status : -EINVAL;
return 0;
}
+static int hyperv_smart_valid(struct ndctl_cmd *cmd)
+{
+ return hyperv_cmd_valid(cmd, ND_HYPERV_CMD_GET_HEALTH_INFO);
+}
+
static int hyperv_cmd_xlat_firmware_status(struct ndctl_cmd *cmd)
{
return CMD_HYPERV_STATUS(cmd) == 0 ? 0 : -EINVAL;
}
+static int hyperv_get_shutdown_count(struct ndctl_cmd *cmd,
+unsigned int *count)
+{
+ unsigned int command = ND_HYPERV_CMD_GET_SHUTDOWN_INFO;
+ struct ndctl_cmd *cmd_get_shutdown_info;
+ int rc;
+
+ cmd_get_shutdown_info = hyperv_dimm_cmd_new_cmd(cmd->dimm, command);
+ if (!cmd_get_shutdown_info)
+ return -EINVAL;
+
+ if (ndctl_cmd_submit(cmd_get_shutdown_info) < 0 ||
+ hyperv_cmd_valid(cmd_get_shutdown_info, command) < 0) {
+ rc = -EINVAL;
+ goto out;
+ }
+
+ *count = CMD_HYPERV(cmd_get_shutdown_info)->u.shutdown_info.count;
+ rc = 0;
+out:
+ ndctl_cmd_unref(cmd_get_shutdown_info);
+ return rc;
+}
+
static unsigned int hyperv_cmd_smart_get_flags(struct ndctl_cmd *cmd)
{
int rc;
+ unsigned int count;
+ unsigned int flags = 0;
rc = hyperv_smart_valid(cmd);
if (rc < 0) {
errno = -rc;
return 0;
}
+ flags |= ND_SMART_HEALTH_VALID;
- return ND_SMART_HEALTH_VALID;
+ if (hyperv_get_shutdown_count(cm
[ndctl PATCH 1/2] libndctl: add support for NVDIMM_FAMILY_HYPERV's _DSM Function 1
This patch retrieves the health info by Hyper-V _DSM method Function 1:
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-",
"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".
Hyper-V also supports "Get Unsafe Shutdown Count (Function Index 2)", but
unluckily util_dimm_health_to_json() only submits *one* command, so we don't
have a chance to call Function 2 here, and hence we can't show a line
"shutdown_count" in the output. If a user in a Linux virtual machine running
on Hyper-V is interested in the Unsafe Shutdown Count, please directly run:
"cat /sys/bus/nd/devices/nmem*/nfit/dirty_shutdown" (there is a pending patch
submitted to the kernel for this sysfs node).
Signed-off-by: Dexuan Cui
---
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 000..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.
+ */
+#include
+#include
+#include
+#include
+#include
+#include "private.h"
+#include "hyperv.h"
+
+#define CMD_HYPERV(_c) ((_c)->hyperv)
+#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;
+
+ 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 ||
+ 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;
+
RE: [PATCH] libnvdimm/dimm: Add a no-BLK quirk based on NVDIMM family
> From: Dan Williams > Sent: Tuesday, February 5, 2019 9:12 AM > On Tue, Feb 5, 2019 at 8:53 AM Dexuan Cui wrote: > > > > > From: Dan Williams > > > Sent: Sunday, February 3, 2019 11:14 AM > > > > ... > > > > As I understand, the essence of the issue is: Hyper-V emulates the > > > > label mechanism (i.e. it supports _LSI and LSR), but doesn't do it > > > > right (i.e. it doesn't support _LSW). > > > > > > > > To manage the namespaces, Linux can choose to use label or not. > > > > > > > > If _LSI/_LSR are supported, Linux assumes _LSW is supported as well > > > > and chooses to use label (i.e. the label mode), but since Hyper-V > > > > doesn't > > > > support _LSW, Linux fails to change the namespace configuration. > > > > > > No, that's not quite right. The reason Linux does not see the fsdax > > > mode configuration is due to the missing "address abstraction GUID" in > > > the label produced the default Hyper-V configuration. > > Hi Dan, > > Do you mean NVDIMM_DAX_GUID? > > Correct. FYI: in create_namespace_pmem(), label0->abstraction_guid is guid_null in the case of Hyper-V NVDIMM. It looks Hyper-V does't use the guid. > > > In label-less mode there is no "address abstraction GUID" to validate so > > > it falls back to just using the info-block directly. > > In the case of not using label storage area, as I understand the info-block > > resides in regular data storage area. Can you please tell me where exactly > > the info-block is and how its location is decided? > > And I suppose the info-block contains the NVDIMM_DAX_GUID? > > The info-block always lives in the data-storage area, even with > labels. It's placed at namespace base address + 4K. > > When labels are present the label gives the namespace a uuid and the > info-block "parent uuid" field must match that value. Without labels > the "parent uuid" field is unused / filled with zero's. Also with v1.2 > labels the address abstraction uuid must match the info-block type. Thanks for elaborating on this! It looks the label mechanism is for advanced usages, e.g., a PMEM namespace can consist of multiple NVDIMMs, or a namespace only uses part of the capacity of a NVDIMM, or a NVDIMM contains both PMEM and Block-Mode namespaces. For a simple usage, e.g. if a NVDIMM only contains one PMEM namespace which consumes all the storage space of the NVDIMM, it looks the namespace can be normally used with the help of the info-block, and we don't really need the label. IMO this is the case of Hyper-V, i.e. we don't use the label at all, since _LSW is not implemented. > > I'm asking because I found I lose ~500MBytes of the 32GBytes Hyper-V > > NVDIMM device, when the namespace is in fsdax mode. When it's in > > raw mode, I'm able to use all of the 32GB space. > > Yes. A portion of the capacity is reserved for page structures. Got it. It looks the size of the info-block (and the related padding?) is 2MB, and "the overhead is 64-bytes per 4K (16GB per 1TB) on x86", so the total overhead in my 32GB case is: 2MB + 32GB/(4096/64) = 514MB. Thanks for sharing the link! Now I realized we can use the -M parameter to not store the page metadata in the NVDIMM: -M; --map= A pmem namespace in “fsdax” or “devdax” mode requires allocation of per-page metadata. The allocation can be drawn from either: “mem”: typical system memory “dev”: persistent memory reserved from the namespace Thanks, --Dexuan ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
RE: [PATCH] libnvdimm/dimm: Add a no-BLK quirk based on NVDIMM family
> From: Dan Williams > Sent: Sunday, February 3, 2019 11:14 AM > > ... > > As I understand, the essence of the issue is: Hyper-V emulates the > > label mechanism (i.e. it supports _LSI and LSR), but doesn't do it > > right (i.e. it doesn't support _LSW). > > > > To manage the namespaces, Linux can choose to use label or not. > > > > If _LSI/_LSR are supported, Linux assumes _LSW is supported as well > > and chooses to use label (i.e. the label mode), but since Hyper-V doesn't > > support _LSW, Linux fails to change the namespace configuration. > > No, that's not quite right. The reason Linux does not see the fsdax > mode configuration is due to the missing "address abstraction GUID" in > the label produced the default Hyper-V configuration. Hi Dan, Do you mean NVDIMM_DAX_GUID? > In label-less mode there is no "address abstraction GUID" to validate so > it falls back to just using the info-block directly. In the case of not using label storage area, as I understand the info-block resides in regular data storage area. Can you please tell me where exactly the info-block is and how its location is decided? And I suppose the info-block contains the NVDIMM_DAX_GUID? I'm asking because I found I lose ~500MBytes of the 32GBytes Hyper-V NVDIMM device, when the namespace is in fsdax mode. When it's in raw mode, I'm able to use all of the 32GB space. > The _LSW issue is if you wanted to reconfigure a raw namespace to > fsdax mode. The current flow tries to delete the label, but that's > only for reconfiguration, not the initial boot-up case that is > currently failing. The deletion would fail on finding no label-write > capability, but to be clear the boot-up case does not perform any > writes. Thanks for the explanation! > > In Ubuntu 19.04 (4.18), the kernel is not aware of Hyper-V _LSI/_LSR, so > > the created namespace is in "label-less" mode, and hence can't be used > > with the libnvdimm-pending branch + this patch, unless we add a quirk > > in Linux to explicitly not use the label. > > > > I agree ideally Hyper-V should hide the label capability, and I'll request > > Hyper-V team to do that. I hope Hyper-V guys are still able to do that > > in time so we won't need a quirk in Linux kernel. > > After some more thought, the "no regressions" rule means that Linux > should ship a quirk for this by default. I think a good heuristic is > to disable label support by default if no _LSW method is detected. An > opt-in can be specified to accept "read-only" configurations, but > that's an exceptional case. I'll send a patch for this. > > > BTW, I suppose Windows VM should be in "label-less" mode. > > I expect Windows mandates labeled operation. This label-less concept > was something Linux shipped in advance of a specification being > ratified and to support early NVDIMMs that don't advertise label > space. Since Hyper-V NVDIMM doesn't support _LSW, IMO Windows VM can't update the LSI area either, so I guess Windows VM can't use label either, just like Linux VM? I guess Windows VM also stores an "info block" when the namespace is in dax mode, just like Linux VM? BTW, it looks the dax mode configuration in Linux VM is incompatible with that in Windows VM(?) When I create a DAX-enabled NTFS partition in Windows VM in the same Hyper-V NVDIMM, Windows VM is able to use all of the 32GB space (As I mentioned above, in Linux I lose ~500MB). And the "dax" mode namespace created in Windows VM is detected as "raw", and vice verse (I think). I'm trying to understand why I lose ~500MB in Linux dax mode. Your insights are appreciated! Thanks, --Dexuan ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
RE: [PATCH] acpi/nfit: Require opt-in for read-only label configurations
> From: Dan Williams
> Sent: Sunday, February 3, 2019 11:29 AM
> ...
> Recent fixes to command handling enabled Linux to read label
> configurations that it could not before. Unfortunately that means that
> configurations that were operating in label-less mode will be broken as
> the kernel ignores the existing namespace configuration and tries to
> honor the new found labels.
>
> Fortunately this seems limited to a case where Linux can quirk the
> behavior and maintain the existing label-less semantics by default.
> When the platform does not emit an _LSW method, disable all label access
> methods. Provide a 'force_labels' module parameter to allow read-only
> label operation.
>
> Fixes: 11189c1089da ("acpi/nfit: Fix command-supported detection")
> Reported-by: Dexuan Cui
> Signed-off-by: Dan Williams
Hi Dan,
Thanks for the patch and all the detailed explanation! With this patch,
the "fsdax" namespace can be properly detected now!
Thanks,
-- Dexuan
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
RE: [PATCH] libnvdimm/dimm: Add a no-BLK quirk based on NVDIMM family
> From: Dan Williams > Sent: Sunday, February 3, 2019 9:49 AM > > ... > > It looks the namespace created by Ubuntu 19.04 (4.18) is incompatible with > > the libnvdimm-pending branch + this patch. > > This is correct, the configuration switched from label-less by default > to labeled. Thanks for the confirmation! > > It looks we need to completely disable the label usage for > NVDIMM_FAMILY_HYPERV > > due to compability? > > It's either going to be a quirk in Linux or a quirk in the Hyper-V > configuration. I think it's more manageable for Hyper-V to ship a > "label disable" switch than to try to ship a workaround in Linux. The > "noblk" quirk in Linux works around the LOCAL bit in the labels. > However, the namespace mode regression can only be resolved by hiding > the label capability until the administrator manually switches from > label-less to labeled operation. As I understand, the essence of the issue is: Hyper-V emulates the label mechanism (i.e. it supports _LSI and LSR), but doesn't do it right (i.e. it doesn't support _LSW). To manage the namespaces, Linux can choose to use label or not. If _LSI/_LSR are supported, Linux assumes _LSW is supported as well and chooses to use label (i.e. the label mode), but since Hyper-V doesn't support _LSW, Linux fails to change the namespace configuration. In Ubuntu 19.04 (4.18), the kernel is not aware of Hyper-V _LSI/_LSR, so the created namespace is in "label-less" mode, and hence can't be used with the libnvdimm-pending branch + this patch, unless we add a quirk in Linux to explicitly not use the label. I agree ideally Hyper-V should hide the label capability, and I'll request Hyper-V team to do that. I hope Hyper-V guys are still able to do that in time so we won't need a quirk in Linux kernel. BTW, I suppose Windows VM should be in "label-less" mode. Thanks for the help, Dan! Thanks, -- Dexuan ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
RE: [PATCH] libnvdimm/dimm: Add a no-BLK quirk based on NVDIMM family
> From: kbuild test robot
> Sent: Sunday, February 3, 2019 6:38 AM
> ...
> Hi Dan,
>
> I love your patch! Yet something to improve:
>
> [auto build test ERROR on linux-nvdimm/libnvdimm-for-next]
> [also build test ERROR on v5.0-rc4 next-20190201]
> [if your patch is applied to the wrong git tree, please drop us a note to help
> improve the system]
> ...
> config: x86_64-allmodconfig (attached as .config)
> compiler: gcc-8 (Debian 8.2.0-14) 8.2.0
> reproduce:
> # save the attached .config to linux build tree
> make ARCH=x86_64
>
> All errors (new ones prefixed by >>):
>
>drivers/acpi/nfit/core.c: In function 'acpi_nfit_register_dimms':
> >> drivers/acpi/nfit/core.c:2003:27: error: 'NVDIMM_FAMILY_HYPERV'
> undeclared (first use in this function); did you mean 'NVDIMM_FAMILY_HPE1'?
> if (nfit_mem->family == NVDIMM_FAMILY_HYPERV)
> ^~~~
> NVDIMM_FAMILY_HPE1
>drivers/acpi/nfit/core.c:2003:27: note: each undeclared identifier is
> reported only once for each function it appears in
This is a false alarm, too. The patch is only for
https://git.kernel.org/pub/scm/linux/kernel/git/djbw/nvdimm.git/log/?h=libnvdimm-pending
where we have the dependant commit:
1194c4133195 ("nfit: Add Hyper-V NVDIMM DSM command set to white list")
Thanks,
-- Dexuan
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
RE: [PATCH] libnvdimm/dimm: Add a no-BLK quirk based on NVDIMM family
> From: kbuild test robot
> Sent: Sunday, February 3, 2019 6:32 AM
> To: Dan Williams
> Cc: kbuild-...@01.org; linux-nvdimm@lists.01.org; Dexuan Cui
> ; linux-ker...@vger.kernel.org
> Subject: Re: [PATCH] libnvdimm/dimm: Add a no-BLK quirk based on NVDIMM
> family
>
> Hi Dan,
>
> I love your patch! Yet something to improve:
>
> [auto build test ERROR on linux-nvdimm/libnvdimm-for-next]
> [also build test ERROR on v5.0-rc4 next-20190201]
> [if your patch is applied to the wrong git tree, please drop us a note to help
> improve the system]
> ...
> All errors (new ones prefixed by >>):
>
>drivers/acpi//nfit/core.c: In function 'acpi_nfit_register_dimms':
> >> drivers/acpi//nfit/core.c:2003:27: error: 'NVDIMM_FAMILY_HYPERV'
> undeclared (first use in this function); did you mean 'NVDIMM_FAMILY_HPE2'?
> if (nfit_mem->family == NVDIMM_FAMILY_HYPERV)
> ^~~~
> NVDIMM_FAMILY_HPE2
>drivers/acpi//nfit/core.c:2003:27: note: each undeclared identifier is
> reported only once for each function it appears in
This is a false alarm. The patch is only for
https://git.kernel.org/pub/scm/linux/kernel/git/djbw/nvdimm.git/log/?h=libnvdimm-pending
where we have the dependant commid
1194c4133195 ("nfit: Add Hyper-V NVDIMM DSM command set to white list")
Thanks,
-- Dexuan
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
RE: [PATCH] libnvdimm/dimm: Add a no-BLK quirk based on NVDIMM family
> From: Dan Williams
> Sent: Saturday, February 2, 2019 5:13 PM
> ...
> As Dexuan reports the NVDIMM_FAMILY_HYPERV platform is incompatible with
> the existing Linux namespace implementation because it uses
> NSLABEL_FLAG_LOCAL for x1-width PMEM interleave sets. Quirk it as an
> platform / DIMM that does not provide BLK-aperture access. Allow the
> libnvdimm core to assume no potential for aliasing. In case other
> implementations make the same mistake, provide a "noblk" module
> parameter to force-enable the quirk.
Hi Dan,
Thanks very much for the patch! With it, "ndctl list" can show the below:
root@decui-gen2-u1904:~/ndctl# ndctl list
[
{
"dev":"namespace1.0",
"mode":"raw",
"size":137438953472,
"uuid":"c258aaab-f72b-e546-bfa5-be5e07761dbc",
"blockdev":"pmem1",
"name":"Microsoft Hyper-V NVDIMM 1 Label"
},
{
"dev":"namespace0.0",
"mode":"raw",
"size":34359738368,
"uuid":"9f0497a7-4453-7c40-ad35-21a791e00345",
"blockdev":"pmem0",
"name":"Microsoft Hyper-V NVDIMM 0 Label"
}
]
And /dev/pmem0 can appear, but /dev/pmem0p1 can not appear, and the "mode" of
"namespace0.0" is not correct. With the Ubuntu 19.04 4.18 kenel, I get the
below:
root@decui-gen2-u1904:~/ndctl# ndctl list
[
{
"dev":"namespace1.0",
"mode":"raw",
"size":137438953472,
"blockdev":"pmem1"
},
{
"dev":"namespace0.0",
"mode":"fsdax",
"map":"dev",
"size":33820770304,
"uuid":"ef028c4e-2b1f-4bf8-b92a-1109d7a1c914",
"blockdev":"pmem0"
}
]
and /dev/pmem0p1 can appear.
It looks the namespace created by Ubuntu 19.04 (4.18) is incompatible with
the libnvdimm-pending branch + this patch.
It looks we need to completely disable the label usage for NVDIMM_FAMILY_HYPERV
due to compability?
Thanks,
-- Dexuan
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
RE: [PATCH v2] nfit: add Hyper-V NVDIMM DSM command set to white list
> From: Dan Williams > Sent: Friday, February 1, 2019 5:29 PM > ... > Honestly, the quickest path to something functional for Linux is to > simply delete the _LSR support and use raw mode defined namespaces. > Why have labels if they are read-only and the region is sufficient for > defining boundaries? Hyper-V Virtual NVDIMM can already work with Ubuntu 19.04 virtual machine running on Hyper-V, i.e. I can create a raw or fsdax namesapce, and create a xfs or ext4 file sysetm in /dev/pmem0p1, and mount the file system with and without "-o dax". The basic functionality is good. My recent work is mainly for ndctl support, i.e. get the health info by ndctl, and use ndctl to monitor the error events (if applicable). Thanks, -- Dexuan ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
RE: [PATCH v2] nfit: add Hyper-V NVDIMM DSM command set to white list
> From: Dan Williams
> Sent: Friday, February 1, 2019 5:29 PM
> > ...
> > The "size" and "mode" still don't look right, but the improvement is that
> > now I can see a good descriptive "name", which I suppose is retrieved
> > from Hyper-V.
>
> Mode is right, there is no way for Hyper-V to create Linux fsdax mode
> namespaces it requires some setup using variables only Linux knows.
> Can you send the output of:
>
> ndctl read-labels -j all
The output is from a kernel built with the libnvdimm-pending branch plus
the one-line patch (label->flags &= ~NSLABEL_FLAG_LOCAL) in
init_active_labels():
root@decui-gen2-u1904:~# ndctl read-labels -j all
[
{
"dev":"nmem1",
"index":[
{
"signature":"NAMESPACE_INDEX",
"major":1,
"minor":2,
"labelsize":256,
"seq":1,
"nslot":2
},
{
"signature":"NAMESPACE_INDEX",
"major":1,
"minor":2,
"labelsize":256,
"seq":2,
"nslot":2
}
],
"label":[
{
"uuid":"c258aaab-f72b-e546-bfa5-be5e07761dbc",
"name":"Microsoft Hyper-V NVDIMM 1 Label",
"slot":0,
"position":0,
"nlabel":1,
"isetcookie":708891662257476870,
"lbasize":0,
"dpa":0,
"rawsize":137438953472,
"type_guid":"79d3f066-f3b4-7440-ac43-0d3318b78cdb",
"abstraction_guid":"----"
}
]
},
{
"dev":"nmem0",
"index":[
{
"signature":"NAMESPACE_INDEX",
"major":1,
"minor":2,
"labelsize":256,
"seq":1,
"nslot":2
},
{
"signature":"NAMESPACE_INDEX",
"major":1,
"minor":2,
"labelsize":256,
"seq":2,
"nslot":2
}
],
"label":[
{
"uuid":"9f0497a7-4453-7c40-ad35-21a791e00345",
"name":"Microsoft Hyper-V NVDIMM 0 Label",
"slot":0,
"position":0,
"nlabel":1,
"isetcookie":708891619307803909,
"lbasize":0,
"dpa":0,
"rawsize":34359738368,
"type_guid":"79d3f066-f3b4-7440-ac43-0d3318b78cdb",
"abstraction_guid":"----"
}
]
}
]
read 2 nmems
> > With Ubuntu 19.04 (4.18.0-11-generic), I get this:
> > (Note: the "mode" and "size" are correct. The "uuid" is different from
> > the above "9f0497a7-4453-7c40-ad35-21a791e00345" -- this is weird.)
> >
> > root@decui-gen2-u1904:~# ndctl list
> > [
> > {
> > "dev":"namespace1.0",
> > "mode":"raw",
> > "size":137438953472,
> > "blockdev":"pmem1"
> > },
> > {
> > "dev":"namespace0.0",
> > "mode":"fsdax",
> > "map":"dev",
> > "size":33820770304,
> > "uuid":"35018886-397e-4fe7-a348-0a4d16eec44d",
> > "blockdev":"pmem0"
> > }
> > ]
>
> This is because the Ubuntu kernel has the bug that causes _LSR to fail
> so Linux falls back to a namespace defined by the region boundary. On
> that namespace there is an "fsdax" info block located at the region
> base +4K. That info block is tagged with the uuid of
> "35018886-397e-4fe7-a348-0a4d16eec44d".
Thanks for the explanation!
> > I'm trying to find out the correct solution. I apprecite your insights!
>
> It's a mess. First we need to figure out whether the label is actually
> specifying a size of zero, or there is some other bug.
I agree.
> However, the next problem is going to be adding "fsdax" mode support
> on top of the read-only defined namespaces. The ndctl reconfiguration
> flow:
>
>ndctl create-namespace -e namespace0.0 -m fsdax -f
>
> ...will likely fail because deleting the previous namespace in the
> labels is part of that flow. It's always that labels are writable.
>
> Honestly, the quickest path to something functional for Linux is to
> simply delete the _LSR support and use raw mode defined namespaces.
> Why have labels if they are read-only and the region is sufficient for
> defining boundaries?
Just now Hyper-V team confirmed _LSW is not supported.
But with Ubuntu 19.04 kernel (4.18.0-11-generic), I'm able to run the commands
without any issue:
root@decui-gen2-u1904:~# ndctl list
[
{
"dev":"namespace1.0",
"mode":"raw",
"size":137438953472,
"blockdev":"pmem1"
},
{
"dev":"namespace0.0",
"mode":"fsdax",
"map":"dev",
"size":33820770304,
"uuid":"35018886-397e-4fe7-a348-0a4d16eec44d",
"blockdev":"pmem0"
}
]
root@decui-gen2-u1904:~# ndctl destroy-namespace "namespace0.0"
Error: namespace0.0 is active, specify --force for re-configuration
error destroying namespaces: Device or resource busy
destroyed 0 namespaces
root@decui-gen2-u1904:~# ndctl destroy-namespace "namespace0.0" --force
destroyed 1 namespace
root@decui-gen2-u1904:~# ndctl list
[
{
"dev":"namespace1.0",
"mode":"raw",
"size":137438953472,
"blockdev":"pmem1"
}
]
root@decui-gen2-u1904:~# ndctl create-namespace -e namespace0.0 -m fsdax -f
{
"dev
RE: [PATCH v2] nfit: add Hyper-V NVDIMM DSM command set to white list
> From: Linux-nvdimm On Behalf Of
> Dexuan Cui
> Sent: Friday, February 1, 2019 4:34 PM
> > > > ...
> > > > Those reads find a namespace index block
> > > > and a label. Unfortunately the label has the LOCAL flag set and Linux
> > > > explicitly ignores pmem namespace labels with that bit set. The reason
> > > > for that is due to the fact that the original definition of the LOCAL
> > > > bit from v1.1 of the namespace label implementation [1] explicitly
> > > > limited the LOCAL flag to "block aperture" regions. If you clear that
> > > > LOCAL flag I expect it will work. To my knowledge Windows pretends
> > > > that the v1.1 definition never existed.
On the libnvdimm-pending branch, I get this:
root@decui-gen2-u1904:~/nvdimm# ndctl list
root@decui-gen2-u1904:~/nvdimm# ndctl list --idle
[
{
"dev":"namespace1.0",
"mode":"raw",
"size":0,
"uuid":"----",
"state":"disabled"
},
{
"dev":"namespace0.0",
"mode":"raw",
"size":0,
"uuid":"----",
"state":"disabled"
}
]
With the patch that clears the LOCAL label (BTW, the initial value of flags is
0x3,
meaning a read-only local label) :
@@ -2496,6 +2500,7 @@ static int init_active_labels(struct nd_region *nd_region)
if (!label_ent)
break;
label = nd_label_active(ndd, j);
+ label->flags &= ~NSLABEL_FLAG_LOCAL;
label_ent->label = label;
I get this:
root@decui-gen2-u1904:~/nvdimm# ndctl list
root@decui-gen2-u1904:~/nvdimm# ndctl list --idle
[
{
"dev":"namespace1.0",
"mode":"raw",
"size":0,
"uuid":"c258aaab-f72b-e546-bfa5-be5e07761dbc",
"state":"disabled",
"name":"Microsoft Hyper-V NVDIMM 1 Label"
},
{
"dev":"namespace0.0",
"mode":"raw",
"size":0,
"uuid":"9f0497a7-4453-7c40-ad35-21a791e00345",
"state":"disabled",
"name":"Microsoft Hyper-V NVDIMM 0 Label"
}
]
The "size" and "mode" still don't look right, but the improvement is that
now I can see a good descriptive "name", which I suppose is retrieved
from Hyper-V.
With Ubuntu 19.04 (4.18.0-11-generic), I get this:
(Note: the "mode" and "size" are correct. The "uuid" is different from
the above "9f0497a7-4453-7c40-ad35-21a791e00345" -- this is weird.)
root@decui-gen2-u1904:~# ndctl list
[
{
"dev":"namespace1.0",
"mode":"raw",
"size":137438953472,
"blockdev":"pmem1"
},
{
"dev":"namespace0.0",
"mode":"fsdax",
"map":"dev",
"size":33820770304,
"uuid":"35018886-397e-4fe7-a348-0a4d16eec44d",
"blockdev":"pmem0"
}
]
I'm trying to find out the correct solution. I apprecite your insights!
Thanks,
-- Dexuan
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
RE: [PATCH v2] nfit: add Hyper-V NVDIMM DSM command set to white list
> From: Dan Williams > Sent: Friday, February 1, 2019 3:47 PM > To: Dexuan Cui > > I believe it's the same reason. Without 11189c1089da the _LSR method > will fail, and otherwise it works and finds the label that it doesn't > like. Exactly. > I'm not seeing "invalid" data in your failure log. Could you double > check that it's just not the success of _LSR that causes the issue? acpi_label_read() never fails for me. By "invalid", I only mean the messages in the dmesg.bad.txt I previously attached (I'm just reading the specs to learn the details about NVDIMM namespace's labels, so my description might be inaccurate) : [4.832367] nvdimm nmem1: nsindex0 labelsize 1 invalid [4.832369] nvdimm nmem1: nsindex1 labelsize 1 invalid ... [5.259017] nd_pmem namespace0.0: 0x, too small must be at least 0x1000 > > > The regression you are seeing is the fact that the patch enables the > > > kernel > to > > > enable nvdimm-namespace-label reads. > > Yes. > > > > > Those reads find a namespace index block > > > and a label. Unfortunately the label has the LOCAL flag set and Linux > > > explicitly ignores pmem namespace labels with that bit set. The reason > > Can you please point out the function that ignores the flag? > > > > I checked where NSLABEL_FLAG_LOCAL is used, but it looks I can't find a > > related function. > > scan_labels() is where the namespace label is validated relative to > the region type: > > if (is_nd_blk(&nd_region->dev) > == !!(flags & NSLABEL_FLAG_LOCAL)) > /* pass, region matches label type */; > else > continue; > > It also has meaning for the namespace capacity allocation > implementation that needed that flag to distinguish aliased capacity > between Block Aperture Mode and PMEM Mode access. Thanks for the pointer! I'm looking at this function. > > > for that is due to the fact that the original definition of the LOCAL > > > bit from v1.1 of the namespace label implementation [1] explicitly > > > limited the LOCAL flag to "block aperture" regions. If you clear that > > > LOCAL flag I expect it will work. To my knowledge Windows pretends > > > that the v1.1 definition never existed. > > I'm trying to find out where the flag is used and how to clear it. > > Assuming Hyper-V implements _LSW, you can recreate / reinitialize the > label area: I think Hyper-V only implements _LSR: [4.720623] nfit ACPI0012:00: device:00: has _LSR [4.723683] nfit ACPI0012:00: device:01: has _LSR > > > The UEFI 2.7 specification for v1.2 labels states that setting the > > > LOCAL flag is optional when "nlabel", number of labels in the set, is > > > 1. Linux makes that mandatory as LOCAL is redundant when nlabel is 1. > > > > > > That said, the Robustness Principle makes a case that Linux should > > > tolerate the bit being set. However, it's just a non-trivial amount of > > > work to unwind the ingrained block-aperture assumptions of that bit. > > Can you please explain this a bit more? Sorry, I'm new to this area... > > The short story is that Linux enforces that LOCAL == Block Mode > Namespaces. See section 2.2 Namespace Label Layout in the original > spec [1]. The EFI 2.7 definition tried to allow for LOCAL to be set > when an interleave-set was comprised of a single NVDIMM, but then also > states its optional when Nlabel is 1. It has zero functional use for > interleave-set based namespaces even when the interleave-set-width is > 1. So Linux takes the option to never set it, and goes further to > reject it if it's set and the region-type does not match, because that > follows the v1.1 meaning of the flag. > > [1]: Thanks for the link! I'll read it. BTW, it looks Hyper-V only supports PMEM namespace, at least so far. Thanks, -- Dexuan ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
RE: [PATCH v2] nfit: add Hyper-V NVDIMM DSM command set to white list
> From: Dan Williams
> Sent: Friday, February 1, 2019 9:29 AM
> > Hi Dan,
> > Unluckily it looks this commit causes a regression ...
> > With the patch, "ndctl list" shows nothing, and /dev/pmem0 can't appear.
> > If I revert the patch, it will be back to normal.
> >
> > I attached the config/logs. In the bad case, "dmesg" shows a line
> > [5.259017] nd_pmem namespace0.0: 0x, too small
> must be at least 0x1000
> > Any idea why this happens? I'm digging into the details and I appreciate
> > your
> insights.
>
> Looks like it is working as expected.
I was working on linux-next tree's next-20190107 and this patch did "work fine"
there. The "regression" happens on djbw/nvdimm.git tree's libnvdimm-pending
branch because we have this recent commit (Jan 19):
11189c1089da ("acpi/nfit: Fix command-supported detection"), which makes such
a change in acpi_nfit_ctl():
- if (!test_bit(cmd, &cmd_mask) || !test_bit(func, &dsm_mask))
+ if (cmd == ND_CMD_CALL && !test_bit(func, &dsm_mask))
+ return -ENOTTY;
+ else if (!test_bit(cmd, &cmd_mask))
return -ENOTTY;
So previously ND_CMD_GET_CONFIG_DATA fails with -ENOTTY and we're good.
Now the command succeeds, but it looks the returned data is inavlid, and I see
the "regression".
> The regression you are seeing is the fact that the patch enables the kernel to
> enable nvdimm-namespace-label reads.
Yes.
> Those reads find a namespace index block
> and a label. Unfortunately the label has the LOCAL flag set and Linux
> explicitly ignores pmem namespace labels with that bit set. The reason
Can you please point out the function that ignores the flag?
I checked where NSLABEL_FLAG_LOCAL is used, but it looks I can't find a
related function.
> for that is due to the fact that the original definition of the LOCAL
> bit from v1.1 of the namespace label implementation [1] explicitly
> limited the LOCAL flag to "block aperture" regions. If you clear that
> LOCAL flag I expect it will work. To my knowledge Windows pretends
> that the v1.1 definition never existed.
I'm trying to find out where the flag is used and how to clear it.
> The UEFI 2.7 specification for v1.2 labels states that setting the
> LOCAL flag is optional when "nlabel", number of labels in the set, is
> 1. Linux makes that mandatory as LOCAL is redundant when nlabel is 1.
>
> That said, the Robustness Principle makes a case that Linux should
> tolerate the bit being set. However, it's just a non-trivial amount of
> work to unwind the ingrained block-aperture assumptions of that bit.
Can you please explain this a bit more? Sorry, I'm new to this area...
Thanks,
-- Dexuan
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
RE: [PATCH] libndctl, dimm: Don't require an xlat function
> From: Linux-nvdimm On Behalf Of Dan
> Williams
> Sent: Wednesday, January 30, 2019 11:08 AM
> To: Verma, Vishal L
> Cc: linux-nvdimm@lists.01.org
> Subject: Re: [PATCH] libndctl, dimm: Don't require an xlat function
>
> On Wed, Jan 30, 2019 at 11:05 AM Verma, Vishal L
> wrote:
> >
> >
> > On Wed, 2019-01-30 at 08:38 -0800, Dan Williams wrote:
> > > On Wed, Jan 30, 2019 at 2:27 AM Oliver O'Halloran
> wrote:
> > > > commit 62bbfce3cb62 ("libndctl, intel: Add infrastructure for
> > > > firmware_status translation") has the unfortunate side effect of making
> > > > all NDCTL commands fail with -ENOMSG unless an xlat_firmware_status
> > > > function is defined for the DIMM family. This means that none of the
> > > > DIMM label manipulation commands work anymore, unless you happen
> to be
> > > > using an Intel DIMM.
> > > >
> > > > Cc: Vishal Verma
> > > > Fixes: 62bbfce3cb62 ("libndctl, intel: Add infrastructure for
> firmware_status translation")
> > > > Signed-off-by: Oliver O'Halloran
> > > > ---
> > > > ndctl/lib/libndctl.c | 3 +++
> > > > 1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
> > > > index 06f835d76117..80d107394a74 100644
> > > > --- a/ndctl/lib/libndctl.c
> > > > +++ b/ndctl/lib/libndctl.c
> > > > @@ -2846,6 +2846,9 @@ NDCTL_EXPORT int
> ndctl_cmd_submit_xlat(struct ndctl_cmd *cmd)
> > > > * useful), then the xlat function is available separately as
> > > > well.
> > > > */
> > > > xlat_rc = ndctl_cmd_xlat_firmware_status(cmd);
> > > > + if (xlat_rc == -ENOMSG)
> > > > + return rc;
> > > > +
> > >
> > > Ah, good point, however I think we should do this instead:
> > >
> > > diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
> > > index 830b791339d2..2fa89162e05e 100644
> > > --- a/ndctl/lib/libndctl.c
> > > +++ b/ndctl/lib/libndctl.c
> > > @@ -2710,7 +2710,7 @@ NDCTL_EXPORT int
> > > ndctl_cmd_xlat_firmware_status(struct ndctl_cmd *cmd)
> > > struct ndctl_dimm_ops *ops = dimm ? dimm->ops : NULL;
> > >
> > > if (!dimm || !ops || !ops->xlat_firmware_status)
> > > - return -ENOMSG;
> > > + return 0;
> > > return ops->xlat_firmware_status(cmd);
> > > }
> > >
> > >
> > > A failure to translate just means proceed with the information that we
> > > *do* have, not a failure.
> >
> > Ah good catch and I agree. Dan, will you send a proper patch for this
> > or shall I?
>
> Go for it :)
Glad to know this will be fixed soon.
Thanks,
-- Dexuan
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
[PATCH] nfit: Document sysfs interface dirty_shutdown
The new sysfs node was added in Sep 2018 in:
commit 0ead11181fe0 ("acpi, nfit: Collect shutdown status")
Now let's document it.
Signed-off-by: Dexuan Cui
---
Documentation/ABI/testing/sysfs-bus-nfit | 8
1 file changed, 8 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-bus-nfit
b/Documentation/ABI/testing/sysfs-bus-nfit
index a1cb44dcb908..bd6cde8c0ea9 100644
--- a/Documentation/ABI/testing/sysfs-bus-nfit
+++ b/Documentation/ABI/testing/sysfs-bus-nfit
@@ -153,6 +153,14 @@ Description:
subsystem controller vendor.
+What: /sys/bus/nd/devices/nmemX/nfit/dirty_shutdown
+Date: Sep, 2018
+KernelVersion: v4.20
+Contact: linux-nvdimm@lists.01.org
+Description:
+ (RO) Unsafe Shutdown Count for the NVDIMM device.
+
+
What: /sys/bus/nd/devices/ndbusX/nfit/revision
Date: Jun, 2015
KernelVersion: v4.2
--
2.19.1
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
RE: [PATCH] nfit: Collect shutdown status for NVDIMM_FAMILY_HYPERV
> From: Linux-nvdimm On Behalf Of > Dexuan Cui > Sent: Wednesday, January 30, 2019 12:03 PM > To: Greg KH > Cc: Josh Poulson ; linux-nvdimm@lists.01.org; > Haiyang Zhang ; > driverdev-de...@linuxdriverproject.org; Rafael J. Wysocki > ; linux-ker...@vger.kernel.org; Michael Kelley > ; Sasha Levin ; > linux-a...@vger.kernel.org; Ross Zwisler ; Stephen > Hemminger ; Len Brown > Subject: RE: [PATCH] nfit: Collect shutdown status for > NVDIMM_FAMILY_HYPERV > > > From: Greg KH > > Sent: Wednesday, January 30, 2019 11:38 AM > > > > On Wed, Jan 30, 2019 at 06:48:40PM +, Dexuan Cui wrote: > > > > > > Let's expose the info to the userspace (e.g. ntctl) via sysfs. > > > > If you add a new sysfs file, you need to add a new Documentation/ABI/ > > update as well :( > > It's an existing sysfs node, which was originally added by Dan in Sep 2018: > /sys/bus/nd/devices/nmem0/nfit/dirty_shutdown. > > Before this patch, the node doesn't appear when Linux runs on Hyper-V, > and with this patch, the node can appear now. > > However, indeed, the node and the other related nodes have not been > documented in Documentation/ABI/testing/sysfs-bus-nfit yet. > I suppose Dan would add that? Actually the other nodes have been documented, and only the "dirty_shutdown" is missed. It should be straightfoward. Let me make a patch for this. Thanks for the reminder, Greg! Thanks, -- Dexuan ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
RE: [PATCH] nfit: Collect shutdown status for NVDIMM_FAMILY_HYPERV
> From: Greg KH > Sent: Wednesday, January 30, 2019 11:38 AM > > On Wed, Jan 30, 2019 at 06:48:40PM +0000, Dexuan Cui wrote: > > > > Let's expose the info to the userspace (e.g. ntctl) via sysfs. > > If you add a new sysfs file, you need to add a new Documentation/ABI/ > update as well :( It's an existing sysfs node, which was originally added by Dan in Sep 2018: /sys/bus/nd/devices/nmem0/nfit/dirty_shutdown. Before this patch, the node doesn't appear when Linux runs on Hyper-V, and with this patch, the node can appear now. However, indeed, the node and the other related nodes have not been documented in Documentation/ABI/testing/sysfs-bus-nfit yet. I suppose Dan would add that? Thanks, -- Dexuan ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
[PATCH] nfit: Collect shutdown status for NVDIMM_FAMILY_HYPERV
See http://www.uefi.org/RFIC_LIST ("Virtual NVDIMM 0x1901"):
"Get Unsafe Shutdown Count (Function Index 2)".
Let's expose the info to the userspace (e.g. ntctl) via sysfs.
Signed-off-by: Dexuan Cui
---
drivers/acpi/nfit/core.c | 51 ++
drivers/acpi/nfit/hyperv.h | 26 +++
2 files changed, 77 insertions(+)
create mode 100644 drivers/acpi/nfit/hyperv.h
diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index d5a164b6833a..d504da34ce34 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -25,6 +25,7 @@
#include
#include
#include "intel.h"
+#include "hyperv.h"
#include "nfit.h"
/*
@@ -1802,6 +1803,54 @@ __weak void nfit_intel_shutdown_status(struct nfit_mem
*nfit_mem)
}
}
+__weak void nfit_hyperv_shutdown_status(struct nfit_mem *nfit_mem)
+{
+ struct device *dev = &nfit_mem->adev->dev;
+ struct nd_hyperv_shutdown_status status;
+ union acpi_object in_buf = {
+ .buffer.type = ACPI_TYPE_BUFFER,
+ .buffer.length = 0,
+ };
+ union acpi_object in_obj = {
+ .package.type = ACPI_TYPE_PACKAGE,
+ .package.count = 1,
+ .package.elements = &in_buf,
+ };
+ const u8 func = ND_HYPERV_GET_UNSAFE_SHUTDOWN_COUNT;
+ const guid_t *guid = to_nfit_uuid(nfit_mem->family);
+ u8 revid = nfit_dsm_revid(nfit_mem->family, func);
+ struct acpi_device *adev = nfit_mem->adev;
+ acpi_handle handle = adev->handle;
+ union acpi_object *out_obj;
+
+ if ((nfit_mem->dsm_mask & BIT(func)) == 0)
+ return;
+
+ out_obj = acpi_evaluate_dsm(handle, guid, revid, func, &in_obj);
+ if (!out_obj || out_obj->type != ACPI_TYPE_BUFFER
+|| out_obj->buffer.length < sizeof(status)) {
+ dev_dbg(dev->parent,
+ "%s: failed to get Unsafe Shutdown Count\n",
+ dev_name(dev));
+ ACPI_FREE(out_obj);
+ return;
+ }
+
+ memcpy(&status, out_obj->buffer.pointer, sizeof(status));
+ ACPI_FREE(out_obj);
+
+ if (status.general_err != 0) {
+ dev_dbg(dev->parent,
+ "%s: failed to get Unsafe Shutdown Count: err=0x%x\n",
+ dev_name(dev), status.status);
+ return;
+ }
+
+ set_bit(NFIT_MEM_DIRTY_COUNT, &nfit_mem->flags);
+ nfit_mem->dirty_shutdown = status.shutdown_count;
+}
+
+
static void populate_shutdown_status(struct nfit_mem *nfit_mem)
{
/*
@@ -1811,6 +1860,8 @@ static void populate_shutdown_status(struct nfit_mem
*nfit_mem)
*/
if (nfit_mem->family == NVDIMM_FAMILY_INTEL)
nfit_intel_shutdown_status(nfit_mem);
+ else if (nfit_mem->family == NVDIMM_FAMILY_HYPERV)
+ nfit_hyperv_shutdown_status(nfit_mem);
}
static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
diff --git a/drivers/acpi/nfit/hyperv.h b/drivers/acpi/nfit/hyperv.h
new file mode 100644
index ..c4a25b8624cc
--- /dev/null
+++ b/drivers/acpi/nfit/hyperv.h
@@ -0,0 +1,26 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright(c) 2019 Microsoft Corporation. All rights reserved.
+ * Hyper-V specific definitions for _DSM of Hyper-V Virtual NVDIMM
+ *
+ * See http://www.uefi.org/RFIC_LIST (Virtual NVDIMM 0x1901)
+ */
+#ifndef _NFIT_HYPERV_H_
+#define _NFIT_HYPERV_H_
+
+#define ND_HYPERV_GET_UNSAFE_SHUTDOWN_COUNT 2
+
+struct nd_hyperv_shutdown_status {
+ union {
+ u32 status;
+ struct {
+ u16 general_err;
+ u8 func_err;
+ u8 vendor_err;
+ };
+ };
+ u32 shutdown_count;
+} __packed;
+
+
+#endif
--
2.19.1
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
[PATCH] nfit: acpi_nfit_ctl(): check out_obj->type in the right place
In the case of ND_CMD_CALL, we should also check out_obj->type.
The patch uses out_obj->type, which is a short alias to
out_obj->package.type.
Fixes: 31eca76ba2fc ("nfit, libnvdimm: limited/whitelisted dimm command
marshaling mechanism")
Cc:
Signed-off-by: Dexuan Cui
---
drivers/acpi/nfit/core.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 0a49c57334cc..1fa378435dd2 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -554,6 +554,13 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,
struct nvdimm *nvdimm,
return -EINVAL;
}
+ if (out_obj->type != ACPI_TYPE_BUFFER) {
+ dev_dbg(dev, "%s unexpected output object type cmd: %s type:
%d\n",
+ dimm_name, cmd_name, out_obj->type);
+ rc = -EINVAL;
+ goto out;
+ }
+
if (call_pkg) {
call_pkg->nd_fw_size = out_obj->buffer.length;
memcpy(call_pkg->nd_payload + call_pkg->nd_size_in,
@@ -572,13 +579,6 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,
struct nvdimm *nvdimm,
return 0;
}
- if (out_obj->package.type != ACPI_TYPE_BUFFER) {
- dev_dbg(dev, "%s unexpected output object type cmd: %s type:
%d\n",
- dimm_name, cmd_name, out_obj->type);
- rc = -EINVAL;
- goto out;
- }
-
dev_dbg(dev, "%s cmd: %s output length: %d\n", dimm_name,
cmd_name, out_obj->buffer.length);
print_hex_dump_debug(cmd_name, DUMP_PREFIX_OFFSET, 4, 4,
--
2.19.1
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
RE: [PATCH] nfit: Fix nfit_intel_shutdown_status() command submission
> From: Dan Williams
> Sent: Monday, January 28, 2019 9:22 PM
> To: linux-nvdimm@lists.01.org
> Cc: sta...@vger.kernel.org; Dexuan Cui ;
> linux-ker...@vger.kernel.org
> Subject: [PATCH] nfit: Fix nfit_intel_shutdown_status() command submission
>
> The implementation is broken in all the ways the unit test did not touch:
>
> 1/ The local definition of in_buf and in_obj violated C99 initializer
>expectations for zeroing. By only initializing 2 out of the three
>struct members the compiler was free to zero-initialize the remaining
>entry even though the aliased location in the union was initialized.
>
> 2/ The implementation made assumptions about the state of the 'smart'
>payload after command execution that are satisfied by
>acpi_nfit_ctl(), but not acpi_evaluate_dsm().
>
> 3/ populate_shutdown_status() is skipped on Intel NVDIMMs due to the early
>return for skipping the common _LS{I,R,W} enabling.
>
> 4/ The input length should be zero.
>
> This breakage was missed due to the unit test implementation only
> testing the case where nfit_intel_shutdown_status() returns a valid
> payload.
>
> Much of this complexity would be saved if acpi_nfit_ctl() could be used, but
> that currently requires a 'struct nvdimm *' argument and one is not created
> until later in the init process. The health result is needed before the device
> is created because the payload gates whether the nmemX/nfit/dirty_shutdown
> property is visible in sysfs.
>
> Cc:
> Fixes: 0ead11181fe0 ("acpi, nfit: Collect shutdown status")
> Reported-by: Dexuan Cui
> Signed-off-by: Dan Williams
> ---
> drivers/acpi/nfit/core.c | 41 -
> 1 file changed, 24 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index e18ade5d74e9..0a49c57334cc 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -1759,14 +1759,14 @@ static bool acpi_nvdimm_has_method(struct
> acpi_device *adev, char *method)
>
> __weak void nfit_intel_shutdown_status(struct nfit_mem *nfit_mem)
> {
> + struct device *dev = &nfit_mem->adev->dev;
> struct nd_intel_smart smart = { 0 };
> union acpi_object in_buf = {
> - .type = ACPI_TYPE_BUFFER,
> - .buffer.pointer = (char *) &smart,
> - .buffer.length = sizeof(smart),
> + .buffer.type = ACPI_TYPE_BUFFER,
> + .buffer.length = 0,
> };
> union acpi_object in_obj = {
> - .type = ACPI_TYPE_PACKAGE,
> + .package.type = ACPI_TYPE_PACKAGE,
> .package.count = 1,
> .package.elements = &in_buf,
> };
> @@ -1781,8 +1781,15 @@ __weak void nfit_intel_shutdown_status(struct
> nfit_mem *nfit_mem)
> return;
>
> out_obj = acpi_evaluate_dsm(handle, guid, revid, func, &in_obj);
> - if (!out_obj)
> + if (!out_obj || out_obj->type != ACPI_TYPE_BUFFER
> + || out_obj->buffer.length < sizeof(smart)) {
> + dev_dbg(dev->parent, "%s: failed to retrieve initial health\n",
> + dev_name(dev));
> + ACPI_FREE(out_obj);
> return;
> + }
> + memcpy(&smart, out_obj->buffer.pointer, sizeof(smart));
> + ACPI_FREE(out_obj);
>
> if (smart.flags & ND_INTEL_SMART_SHUTDOWN_VALID) {
> if (smart.shutdown_state)
> @@ -1793,7 +1800,6 @@ __weak void nfit_intel_shutdown_status(struct
> nfit_mem *nfit_mem)
> set_bit(NFIT_MEM_DIRTY_COUNT, &nfit_mem->flags);
> nfit_mem->dirty_shutdown = smart.shutdown_count;
> }
> - ACPI_FREE(out_obj);
> }
>
> static void populate_shutdown_status(struct nfit_mem *nfit_mem)
> @@ -1915,18 +1921,19 @@ static int acpi_nfit_add_dimm(struct
> acpi_nfit_desc *acpi_desc,
> | 1 << ND_CMD_SET_CONFIG_DATA;
> if (family == NVDIMM_FAMILY_INTEL
> && (dsm_mask & label_mask) == label_mask)
> - return 0;
> -
> - if (acpi_nvdimm_has_method(adev_dimm, "_LSI")
> - && acpi_nvdimm_has_method(adev_dimm, "_LSR")) {
> - dev_dbg(dev, "%s: has _LSR\n", dev_name(&adev_dimm->dev));
> - set_bit(NFIT_MEM_LSR, &nfit_mem->flags);
> - }
> + /* skip _LS{I,R,W} enabling */;
> + else {
> + if (acpi_nvdimm_has_method(adev_dimm, "_LSI")
> + && acpi_nvd
[PATCH v2] nfit: add Hyper-V NVDIMM DSM command set to white list
Add the Hyper-V _DSM command set to the white list of NVDIMM command
sets.
This command set is documented at http://www.uefi.org/RFIC_LIST
(see "Virtual NVDIMM 0x1901").
Thanks Dan Williams for writing the
comment change.
Signed-off-by: Dexuan Cui
Reviewed-by: Michael Kelley
---
Changes in v2:
Updated the comment and changelog (Thanks, Dan!)
Rebased to the tag libnvdimm-fixes-5.0-rc4 of the nvdimm tree.
drivers/acpi/nfit/core.c | 17 ++---
drivers/acpi/nfit/nfit.h | 6 +-
include/uapi/linux/ndctl.h | 1 +
3 files changed, 20 insertions(+), 4 deletions(-)
diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index e18ade5d74e9..a9270c99be72 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -1861,9 +1861,17 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc
*acpi_desc,
dev_set_drvdata(&adev_dimm->dev, nfit_mem);
/*
-* Until standardization materializes we need to consider 4
-* different command sets. Note, that checking for function0 (bit0)
-* tells us if any commands are reachable through this GUID.
+* There are 4 "legacy" NVDIMM command sets
+* (NVDIMM_FAMILY_{INTEL,MSFT,HPE1,HPE2}) that were created before
+* an EFI working group was established to constrain this
+* proliferation. The nfit driver probes for the supported command
+* set by GUID. Note, if you're a platform developer looking to add
+* a new command set to this probe, consider using an existing set,
+* or otherwise seek approval to publish the command set at
+* http://www.uefi.org/RFIC_LIST.
+*
+* Note, that checking for function0 (bit0) tells us if any commands
+* are reachable through this GUID.
*/
for (i = 0; i <= NVDIMM_FAMILY_MAX; i++)
if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1))
@@ -1886,6 +1894,8 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc
*acpi_desc,
dsm_mask &= ~(1 << 8);
} else if (nfit_mem->family == NVDIMM_FAMILY_MSFT) {
dsm_mask = 0x;
+ } else if (nfit_mem->family == NVDIMM_FAMILY_HYPERV) {
+ dsm_mask = 0x1f;
} else {
dev_dbg(dev, "unknown dimm command family\n");
nfit_mem->family = -1;
@@ -3729,6 +3739,7 @@ static __init int nfit_init(void)
guid_parse(UUID_NFIT_DIMM_N_HPE1, &nfit_uuid[NFIT_DEV_DIMM_N_HPE1]);
guid_parse(UUID_NFIT_DIMM_N_HPE2, &nfit_uuid[NFIT_DEV_DIMM_N_HPE2]);
guid_parse(UUID_NFIT_DIMM_N_MSFT, &nfit_uuid[NFIT_DEV_DIMM_N_MSFT]);
+ guid_parse(UUID_NFIT_DIMM_N_HYPERV, &nfit_uuid[NFIT_DEV_DIMM_N_HYPERV]);
nfit_wq = create_singlethread_workqueue("nfit");
if (!nfit_wq)
diff --git a/drivers/acpi/nfit/nfit.h b/drivers/acpi/nfit/nfit.h
index 33691aecfcee..4de167b4f76f 100644
--- a/drivers/acpi/nfit/nfit.h
+++ b/drivers/acpi/nfit/nfit.h
@@ -34,11 +34,14 @@
/* https://msdn.microsoft.com/library/windows/hardware/mt604741 */
#define UUID_NFIT_DIMM_N_MSFT "1ee68b36-d4bd-4a1a-9a16-4f8e53d46e05"
+/* http://www.uefi.org/RFIC_LIST (see "Virtual NVDIMM 0x1901") */
+#define UUID_NFIT_DIMM_N_HYPERV "5746c5f2-a9a2-4264-ad0e-e4ddc9e09e80"
+
#define ACPI_NFIT_MEM_FAILED_MASK (ACPI_NFIT_MEM_SAVE_FAILED \
| ACPI_NFIT_MEM_RESTORE_FAILED | ACPI_NFIT_MEM_FLUSH_FAILED \
| ACPI_NFIT_MEM_NOT_ARMED | ACPI_NFIT_MEM_MAP_FAILED)
-#define NVDIMM_FAMILY_MAX NVDIMM_FAMILY_MSFT
+#define NVDIMM_FAMILY_MAX NVDIMM_FAMILY_HYPERV
#define NVDIMM_STANDARD_CMDMASK \
(1 << ND_CMD_SMART | 1 << ND_CMD_SMART_THRESHOLD | 1 << ND_CMD_DIMM_FLAGS \
@@ -94,6 +97,7 @@ enum nfit_uuids {
NFIT_DEV_DIMM_N_HPE1 = NVDIMM_FAMILY_HPE1,
NFIT_DEV_DIMM_N_HPE2 = NVDIMM_FAMILY_HPE2,
NFIT_DEV_DIMM_N_MSFT = NVDIMM_FAMILY_MSFT,
+ NFIT_DEV_DIMM_N_HYPERV = NVDIMM_FAMILY_HYPERV,
NFIT_SPA_VOLATILE,
NFIT_SPA_PM,
NFIT_SPA_DCR,
diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h
index f57c9e434d2d..de5d90212409 100644
--- a/include/uapi/linux/ndctl.h
+++ b/include/uapi/linux/ndctl.h
@@ -243,6 +243,7 @@ struct nd_cmd_pkg {
#define NVDIMM_FAMILY_HPE1 1
#define NVDIMM_FAMILY_HPE2 2
#define NVDIMM_FAMILY_MSFT 3
+#define NVDIMM_FAMILY_HYPERV 4
#define ND_IOCTL_CALL _IOWR(ND_IOCTL, ND_CMD_CALL,\
struct nd_cmd_pkg)
--
2.19.1
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
RE: [PATCH] nfit: add Hyper-V NVDIMM DSM command set to white list
> From: Dan Williams
> Sent: Monday, January 28, 2019 1:55 PM
>
> On Mon, Jan 28, 2019 at 1:40 PM Dexuan Cui wrote:
>
> > I made the below simple change. Is this enough? Please suggest the proper
> > wording/sentence, as I'm relatively new to NVDIMM, and I don't really know
> the history of the standardization process.
>
> How about something a bit more relevant for the code in question:
>
> There are 4 "legacy" NVDIMM command sets
> (NVDIMM_FAMILY_{INTEL,MSFT,HPE1,HPE2}) that were created before an EFI
> working group was established to constrain this proliferation. The
> nfit driver probes for the supported command set by GUID. Note, If
> you're a platform developer looking to add a new command set to this
> probe consider using an existing set, or otherwise seek approval to
> publish the command set at
> http://www.uefi.org/RFIC_LIST
Looks perfect! Let me use this, rebase the patch to
https://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm.git/tag/?h=libnvdimm-fixes-5.0-rc4
and post a v2 later today.
Thanks,
-- Dexuan
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
RE: [PATCH] nfit: add Hyper-V NVDIMM DSM command set to white list
> From: Dan Williams
> Sent: Monday, January 28, 2019 1:01 PM
>
> Hi Dexuan,
> Looks good. Just one update request and a note below...
>
> On Wed, Jan 23, 2019 at 12:51 PM Dexuan Cui wrote:
> > ...
> > --- a/drivers/acpi/nfit/core.c
> > +++ b/drivers/acpi/nfit/core.c
> > @@ -1840,7 +1840,7 @@ static int acpi_nfit_add_dimm(struct
> acpi_nfit_desc *acpi_desc,
> > dev_set_drvdata(&adev_dimm->dev, nfit_mem);
> >
> > /*
> > -* Until standardization materializes we need to consider 4
> > +* Until standardization materializes we need to consider 5
> > * different command sets. Note, that checking for function0
> (bit0)
> > * tells us if any commands are reachable through this GUID.
> > */
>
> This comment is stale. This "HYPERV" family is the first example of
> the "right" way to define a new NVDIMM command set. Lets update it to
> mention the process and considerations for submitting new command sets
> to UEFI (http://www.uefi.org/RFIC_LIST). The fact that there's now a
> defined process is a step forward from when this comment was initially
> written. Also, the fact that the process encourages "adopt" vs
> "supersede" addresses the main concern about vendor-specific
> command-set proliferation.
I made the below simple change. Is this enough? Please suggest the proper
wording/sentence, as I'm relatively new to NVDIMM, and I don't really know the
history of the standardization process.
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -1732,8 +1732,10 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc
*acpi_desc,
dev_set_drvdata(&adev_dimm->dev, nfit_mem);
/*
-* Until standardization materializes we need to consider 4
-* different command sets. Note, that checking for function0 (bit0)
+* New command sets should be submitted to UEFI
+* http://www.uefi.org/RFIC_LIST.
+*
+* Note, that checking for function0 (bit0)
* tells us if any commands are reachable through this GUID.
*/
for (i = 0; i <= NVDIMM_FAMILY_MAX; i++)
> > @@ -1865,6 +1865,8 @@ static int acpi_nfit_add_dimm(struct
> acpi_nfit_desc *acpi_desc,
> > dsm_mask &= ~(1 << 8);
> > } else if (nfit_mem->family == NVDIMM_FAMILY_MSFT) {
> > dsm_mask = 0x;
> > + } else if (nfit_mem->family == NVDIMM_FAMILY_HYPERV) {
> > + dsm_mask = 0x1f;
>
> Just a note that starting with commit 5e9e38d0db1d "acpi/nfit: Block
> function zero DSMs", bit0 in this mask will be cleared to ensure that
> only the kernel has the ability to probe for supported DSM functions.
Thanks for the note!
Thanks,
-- Dexuan
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
How can we better support vender specific _DSM commands in ndctl?
Hi all,
Hyper-V implements virtualized NVDIMM for guests running on it [1]. The
command set is documented on the RFIC_LIST page on uefi.org [2].
After Linux guest boots up, I can see these dev files:
/dev/ndctl0 /dev/nmem0 /dev/pmem0
and I can create an xfs file system on /dev/pmem0, mount it and use it.
Everything works just fine except for one thing: I can't call the Hyper-V
specific _DSM methods. So I wrote a patch [3] to allow the methods to
be called (I Cc'd you when posting the patch. Please review it.).
And then I found it looks ndctl doesn't allow me to easily call *all* the
_DSM methods, and I can't easily use ndctl to interpret the output in a
way I prefer.
My understanding is that: it looks when the kernel nvdimm driver and ndctl
were originally implemented, the code was mainly for Intel NVDIMM. Later
people introduced the support for NVDIMM_FAMILY_{HPE1, HPE2, MSFT},
and introduced the kernel ND_CMD_CALL ioctl to allow ndctl to call
vender-specific methods, but up to now, it looks the majority of the cmds
(if not all of the cmds) shown by "ndctl --list-cmds" are not very useful to
call vender specific methods, especially for NVDIMM_FAMILY_MSFT:
e.g.: for NVDIMM_FAMILY_MSFT, only Function 11 is used, and the
only useful info from "ndctl list --dimms --health" is:
"health":{
"health_state":"ok",
"temperature_celsius":27.00,
"life_used_percentage":3
}
And, in msft_cmd_smart_get_health(), we only count the number of error bits,
and we drop the detailed info about the errors, and we lose some info
from the struct ndn_msft_smart_data, e.g. err_thresh_stat, warn_thresh_stat,
count_dram_uncorr_err and count_dram_corr_err, which IMO could be useful.
(See "libndctl: add support for the MSFT family of DSM functions" [4])
And, NVDIMM_FAMILY_MSFT has 32 methods [5], but except Function 11, it
looks the other methods can not be mapped to the existing ndctl commands.
Now, the new NVDIMM type NVDIMM_FAMILY_HYPERV has 4 Hyper-V specific
_DSM methods [2]:
Get Health Information (Function Index 1)
Get Unsafe Shutdown Count (Function Index 2)
Inject Error (Function Index 3)
Query Injected Errors (Function Index 4)
For Function 1, I made a patch
"libndctl: add support for the HYPER-V family of DSM functions" (please see [6],
especially hyperv_cmd_smart_get_health() in the patch) to use it, but the
existing ndctl interface, i.e. util_dimm_health_to_json(), only allows me to
return to the user a string of "fatal", "critical", "non-critical" or "ok",
while
I would like to return more details about the errors to the user.
Function 3 can not be mapped to ndctl/inject-smart.c: smart_inject(), because
it's incompatible with Intel NVDIMM.
Function 2 and 4 can't be mapped to the existing ndctl commands either.
I'm wondering how I should use the 4 Hyper-V specific methods in ndctl.
I appreciate your insights. Thank you!
--Dexuan
[1]
https://docs.microsoft.com/en-us/windows-server/virtualization/hyper-v/manage/persistent-memory-cmdlets
[2] http://www.uefi.org/RFIC_LIST, see the link to "Virtual NVDIMM 0x1901" on
the page.
[3] https://www.lkml.org/lkml/2019/1/23/761
[4]
https://github.com/pmem/ndctl/commit/2cf2acca0288389a39ac823a42a5e048575d52db
[5]
https://docs.microsoft.com/en-us/windows-hardware/drivers/storage/-dsm-interface-for-byte-addressable-energy-backed-function-class--function-interface-1-
[6]
https://github.com/dcui/ndctl/commit/12feb081c881b24fee9f6cf088037c2e30248252
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
[PATCH] nfit: add Hyper-V NVDIMM DSM command set to white list
Add the Hyper-V _DSM command set to the white list of NVDIMM command
sets.
This command set is documented at http://www.uefi.org/RFIC_LIST
(see the link to "Virtual NVDIMM 0x1901" on the page).
Signed-off-by: Dexuan Cui
---
I'm going to change the user-space utility "ndctl" to support Hyper-V Virtual
NVDIMM.
This kernel patch is required first.
drivers/acpi/nfit/core.c | 5 -
drivers/acpi/nfit/nfit.h | 6 +-
include/uapi/linux/ndctl.h | 1 +
3 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 011d3db19c80..fb48cb17a519 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -1840,7 +1840,7 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc
*acpi_desc,
dev_set_drvdata(&adev_dimm->dev, nfit_mem);
/*
-* Until standardization materializes we need to consider 4
+* Until standardization materializes we need to consider 5
* different command sets. Note, that checking for function0 (bit0)
* tells us if any commands are reachable through this GUID.
*/
@@ -1865,6 +1865,8 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc
*acpi_desc,
dsm_mask &= ~(1 << 8);
} else if (nfit_mem->family == NVDIMM_FAMILY_MSFT) {
dsm_mask = 0x;
+ } else if (nfit_mem->family == NVDIMM_FAMILY_HYPERV) {
+ dsm_mask = 0x1f;
} else {
dev_dbg(dev, "unknown dimm command family\n");
nfit_mem->family = -1;
@@ -3707,6 +3709,7 @@ static __init int nfit_init(void)
guid_parse(UUID_NFIT_DIMM_N_HPE1, &nfit_uuid[NFIT_DEV_DIMM_N_HPE1]);
guid_parse(UUID_NFIT_DIMM_N_HPE2, &nfit_uuid[NFIT_DEV_DIMM_N_HPE2]);
guid_parse(UUID_NFIT_DIMM_N_MSFT, &nfit_uuid[NFIT_DEV_DIMM_N_MSFT]);
+ guid_parse(UUID_NFIT_DIMM_N_HYPERV, &nfit_uuid[NFIT_DEV_DIMM_N_HYPERV]);
nfit_wq = create_singlethread_workqueue("nfit");
if (!nfit_wq)
diff --git a/drivers/acpi/nfit/nfit.h b/drivers/acpi/nfit/nfit.h
index 33691aecfcee..9194022f29f3 100644
--- a/drivers/acpi/nfit/nfit.h
+++ b/drivers/acpi/nfit/nfit.h
@@ -34,11 +34,14 @@
/* https://msdn.microsoft.com/library/windows/hardware/mt604741 */
#define UUID_NFIT_DIMM_N_MSFT "1ee68b36-d4bd-4a1a-9a16-4f8e53d46e05"
+/* http://www.uefi.org/RFIC_LIST */
+#define UUID_NFIT_DIMM_N_HYPERV "5746c5f2-a9a2-4264-ad0e-e4ddc9e09e80"
+
#define ACPI_NFIT_MEM_FAILED_MASK (ACPI_NFIT_MEM_SAVE_FAILED \
| ACPI_NFIT_MEM_RESTORE_FAILED | ACPI_NFIT_MEM_FLUSH_FAILED \
| ACPI_NFIT_MEM_NOT_ARMED | ACPI_NFIT_MEM_MAP_FAILED)
-#define NVDIMM_FAMILY_MAX NVDIMM_FAMILY_MSFT
+#define NVDIMM_FAMILY_MAX NVDIMM_FAMILY_HYPERV
#define NVDIMM_STANDARD_CMDMASK \
(1 << ND_CMD_SMART | 1 << ND_CMD_SMART_THRESHOLD | 1 << ND_CMD_DIMM_FLAGS \
@@ -94,6 +97,7 @@ enum nfit_uuids {
NFIT_DEV_DIMM_N_HPE1 = NVDIMM_FAMILY_HPE1,
NFIT_DEV_DIMM_N_HPE2 = NVDIMM_FAMILY_HPE2,
NFIT_DEV_DIMM_N_MSFT = NVDIMM_FAMILY_MSFT,
+ NFIT_DEV_DIMM_N_HYPERV = NVDIMM_FAMILY_HYPERV,
NFIT_SPA_VOLATILE,
NFIT_SPA_PM,
NFIT_SPA_DCR,
diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h
index f57c9e434d2d..de5d90212409 100644
--- a/include/uapi/linux/ndctl.h
+++ b/include/uapi/linux/ndctl.h
@@ -243,6 +243,7 @@ struct nd_cmd_pkg {
#define NVDIMM_FAMILY_HPE1 1
#define NVDIMM_FAMILY_HPE2 2
#define NVDIMM_FAMILY_MSFT 3
+#define NVDIMM_FAMILY_HYPERV 4
#define ND_IOCTL_CALL _IOWR(ND_IOCTL, ND_CMD_CALL,\
struct nd_cmd_pkg)
--
2.19.1
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
