[ndctl PATCH v3 0/5] Add the support for Hyper-V virtual NVDIMM

[ndctl PATCH v3 0/5] Add the support for Hyper-V virtual NVDIMM

[Dexuan Cui]

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 1/5] libndctl: Implement the "smart_get_health" dimm-op for Hyper-V

[Dexuan Cui]

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-00000000",
    "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 <decui@microsoft.com>
---
 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 0000000..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 <stdlib.h>
+#include <limits.h>
+#include <util/bitmap.h>
+#include <util/log.h>
+#include <ndctl/libndctl.h>
+#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->hyperv->u.status == 0 ? 0 : -EINVAL;
+}
+
+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,
+	.xlat_firmware_status = hyperv_cmd_xlat_firmware_status,
+};
diff --git a/ndctl/lib/hyperv.h b/ndctl/lib/hyperv.h
new file mode 100644
index 0000000..45bbc12
--- /dev/null
+++ b/ndctl/lib/hyperv.h
@@ -0,0 +1,30 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright(c) 2019, Microsoft Corporation. All rights reserved. */
+
+#ifndef __NDCTL_HYPERV_H__
+#define __NDCTL_HYPERV_H__
+
+/* See http://www.uefi.org/RFIC_LIST ("Virtual NVDIMM 0x1901") */
+
+enum {
+	ND_HYPERV_CMD_QUERY_SUPPORTED_FUNCTIONS	= 0,
+	ND_HYPERV_CMD_GET_HEALTH_INFO		= 1,
+};
+
+/* Get Health Information (Function Index 1) */
+struct nd_hyperv_health_info {
+	__u32	status;
+	__u32	health;
+} __attribute__((packed));
+
+union nd_hyperv_cmd {
+	__u32				status;
+	struct nd_hyperv_health_info	health_info;
+} __attribute__((packed));
+
+struct nd_pkg_hyperv {
+	struct nd_cmd_pkg	gen;
+	union  nd_hyperv_cmd	u;
+} __attribute__((packed));
+
+#endif /* __NDCTL_HYPERV_H__ */
diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
index 98893bc..24b8ad3 100644
--- a/ndctl/lib/libndctl.c
+++ b/ndctl/lib/libndctl.c
@@ -1543,6 +1543,8 @@ static void *add_dimm(void *parent, int id, const char *dimm_base)
 		dimm->ops = hpe1_dimm_ops;
 	if (dimm->cmd_family == NVDIMM_FAMILY_MSFT)
 		dimm->ops = msft_dimm_ops;
+	if (dimm->cmd_family == NVDIMM_FAMILY_HYPERV)
+		dimm->ops = hyperv_dimm_ops;
 
 	sprintf(path, "%s/nfit/dsm_mask", dimm_base);
 	if (sysfs_read_attr(ctx, path, buf) == 0)
diff --git a/ndctl/lib/private.h b/ndctl/lib/private.h
index a387b0b..a9d35c5 100644
--- a/ndctl/lib/private.h
+++ b/ndctl/lib/private.h
@@ -31,6 +31,7 @@
 #include "intel.h"
 #include "hpe1.h"
 #include "msft.h"
+#include "hyperv.h"
 
 struct nvdimm_data {
 	struct ndctl_cmd *cmd_read;
@@ -270,6 +271,7 @@ struct ndctl_cmd {
 		struct nd_cmd_pkg pkg[0];
 		struct ndn_pkg_hpe1 hpe1[0];
 		struct ndn_pkg_msft msft[0];
+		struct nd_pkg_hyperv hyperv[0];
 		struct nd_pkg_intel intel[0];
 		struct nd_cmd_get_config_size get_size[0];
 		struct nd_cmd_get_config_data_hdr get_data[0];
@@ -344,6 +346,7 @@ struct ndctl_dimm_ops {
 struct ndctl_dimm_ops * const intel_dimm_ops;
 struct ndctl_dimm_ops * const hpe1_dimm_ops;
 struct ndctl_dimm_ops * const msft_dimm_ops;
+struct ndctl_dimm_ops * const hyperv_dimm_ops;
 
 static inline struct ndctl_bus *cmd_to_bus(struct ndctl_cmd *cmd)
 {
diff --git a/ndctl/ndctl.h b/ndctl/ndctl.h
index c6aaa4c..008f81c 100644
--- a/ndctl/ndctl.h
+++ b/ndctl/ndctl.h
@@ -262,6 +262,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

[ndctl PATCH v3 2/5] libndctl: Implement the smart_get_shutdown_count dimm-op for Hyper-V

[Dexuan Cui]

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-00000000",
    "handle":0,
    "phys_id":0,
    "health":{
      "health_state":"ok",
      "shutdown_count":2
    }
}

Signed-off-by: Dexuan Cui <decui@microsoft.com>
---
 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_info	health_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 3/5] ndctl, monitor: Don't require the support of ND_CMD_SMART_THRESHOLD

[Dexuan Cui]

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 <decui@microsoft.com>
---
 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()

[Dexuan Cui]

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 <decui@microsoft.com>
---
 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

Re: [ndctl PATCH v3 4/5] libndctl: Add a new dimm-op cmd_is_supported()

[Verma, Vishal L]

On Sat, 2019-03-23 at 04:20 +0000, Dexuan Cui wrote:
> 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.

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]

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

[1]: https://patchwork.kernel.org/patch/10785277/

> 
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
>  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 *);

_______________________________________________
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()

[Dexuan Cui]

> From: Verma, Vishal L <vishal.l.verma-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 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

Re: [ndctl PATCH v3 4/5] libndctl: Add a new dimm-op cmd_is_supported()

[Dan Williams]

On Tue, Mar 26, 2019 at 4:29 PM Dexuan Cui <decui@microsoft.com> wrote:
[..]
> 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.

I'm certain you're right. Thanks for blazing the trail!
_______________________________________________
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()

[Verma, Vishal L]

On Tue, 2019-03-26 at 23:29 +0000, 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
_______________________________________________
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()

[Dexuan Cui]

> From: Verma, Vishal L <vishal.l.verma-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Sent: Wednesday, March 27, 2019 9:17 AM
> To: Williams, Dan J <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>; Dexuan Cui
> <decui-0li6OtcxBFHby3iVrkZq2A@public.gmane.org>; Jiang, Dave <dave.jiang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>;
> linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw@public.gmane.org; dexuan.cui-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org; Michael Kelley
> <mikelley-0li6OtcxBFHby3iVrkZq2A@public.gmane.org>
> Cc: jthumshirn-l3A5Bk7waGM@public.gmane.org; qi.fuli-LMvhtfratI1BDgjK7y7TUQ@public.gmane.org
> Subject: Re: [ndctl PATCH v3 4/5] libndctl: Add a new dimm-op
> cmd_is_supported()
> 
> 
> On Tue, 2019-03-26 at 23:29 +0000, 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