[ndctl PATCH v2 1/4] libndctl: add support for NVDIMM_FAMILY_HYPERV's _DSM Function 1

[ndctl PATCH v2 1/4] libndctl: add support for NVDIMM_FAMILY_HYPERV's _DSM Function 1

[Dexuan Cui]

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

If there is an error with the NVDIMM, the "ok" will be replaced with "unknown",
"fatal", "critical", or "non-critical".

Signed-off-by: Dexuan Cui <decui-0li6OtcxBFHby3iVrkZq2A@public.gmane.org>
---
 ndctl/lib/Makefile.am |   1 +
 ndctl/lib/hyperv.c    | 129 ++++++++++++++++++++++++++++++++++++++++++
 ndctl/lib/hyperv.h    |  51 +++++++++++++++++
 ndctl/lib/libndctl.c  |   2 +
 ndctl/lib/private.h   |   3 +
 ndctl/ndctl.h         |   1 +
 6 files changed, 187 insertions(+)
 create mode 100644 ndctl/lib/hyperv.c
 create mode 100644 ndctl/lib/hyperv.h

diff --git a/ndctl/lib/Makefile.am b/ndctl/lib/Makefile.am
index 7797039..fb75fda 100644
--- a/ndctl/lib/Makefile.am
+++ b/ndctl/lib/Makefile.am
@@ -20,6 +20,7 @@ libndctl_la_SOURCES =\
 	intel.c \
 	hpe1.c \
 	msft.c \
+	hyperv.c \
 	ars.c \
 	firmware.c \
 	libndctl.c
diff --git a/ndctl/lib/hyperv.c b/ndctl/lib/hyperv.c
new file mode 100644
index 0000000..b303d50
--- /dev/null
+++ b/ndctl/lib/hyperv.c
@@ -0,0 +1,129 @@
+/*
+ * Copyright (c) 2019, Microsoft Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU Lesser General Public License,
+ * version 2.1, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT ANY
+ * WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
+ * FOR A PARTICULAR PURPOSE.  See the GNU Lesser General Public License for
+ * more details.
+ */
+#include <stdlib.h>
+#include <limits.h>
+#include <util/bitmap.h>
+#include <util/log.h>
+#include <ndctl/libndctl.h>
+#include "private.h"
+#include "hyperv.h"
+
+#define CMD_HYPERV(_c) ((_c)->hyperv)
+#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;
+	}
+
+	num = CMD_HYPERV_SMART_DATA(cmd)->health & 0x3F;
+
+	if (num & (BIT(0) | BIT(1)))
+		health |= ND_SMART_CRITICAL_HEALTH;
+
+	if (num & BIT(2))
+		health |= ND_SMART_FATAL_HEALTH;
+
+	if (num & (BIT(3) | BIT(4) | BIT(5)))
+		health |= ND_SMART_NON_CRITICAL_HEALTH;
+
+	return health;
+}
+
+struct ndctl_dimm_ops * const hyperv_dimm_ops = &(struct ndctl_dimm_ops) {
+	.new_smart = hyperv_dimm_cmd_new_smart,
+	.smart_get_flags = hyperv_cmd_smart_get_flags,
+	.smart_get_health = hyperv_cmd_smart_get_health,
+	.xlat_firmware_status = hyperv_cmd_xlat_firmware_status,
+};
diff --git a/ndctl/lib/hyperv.h b/ndctl/lib/hyperv.h
new file mode 100644
index 0000000..8e55a97
--- /dev/null
+++ b/ndctl/lib/hyperv.h
@@ -0,0 +1,51 @@
+/*
+ * Copyright (c) 2019, Microsoft Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU Lesser General Public License,
+ * version 2.1, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT ANY
+ * WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
+ * FOR A PARTICULAR PURPOSE.  See the GNU Lesser General Public License for
+ * more details.
+ */
+#ifndef __NDCTL_HYPERV_H__
+#define __NDCTL_HYPERV_H__
+
+/* See http://www.uefi.org/RFIC_LIST ("Virtual NVDIMM 0x1901") */
+enum {
+	ND_HYPERV_CMD_QUERY = 0,
+
+	/* 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));
+
+struct nd_hyperv_smart {
+	__u32	status;
+	union {
+		__u8 buf[4];
+		struct nd_hyperv_smart_data data[0];
+	};
+} __attribute__((packed));
+
+union nd_hyperv_cmd {
+	__u32			status;
+	struct nd_hyperv_smart	smart;
+} __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 c9e2875..48bdb27 100644
--- a/ndctl/lib/libndctl.c
+++ b/ndctl/lib/libndctl.c
@@ -1492,6 +1492,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

Re: [ndctl PATCH v2 1/4] libndctl: add support for NVDIMM_FAMILY_HYPERV's _DSM Function 1

[Verma, Vishal L]

Hi Dexuan,

Thanks for these patches - I had a few comments below.

Also on a more general note, the patches in this series don't appear to
be correctly threaded. Normally, patch emails in a series are replies to
the first patch (either 1/N or the cover letter), and this allows for
easier review as all related emails can be found in a single top-level
thread. It would be nice if you can fix this for the future - git send-
email should do this correctly automatically, if you use it for sending
the patches.


On Wed, 2019-02-20 at 05:10 +0000, Dexuan Cui wrote:
> This patch retrieves the health info by Hyper-V _DSM method Function 1:
> 

We should never use "This patch.." in a commit message. See 4.c in [1].

[1]: https://www.ozlabs.org/~akpm/stuff/tpp.txt


> Get Health Information (Function Index 1)
> See http://www.uefi.org/RFIC_LIST ("Virtual NVDIMM 0x1901").
> 
> Now "ndctl list --dimms --health --idle" can show a line "health_state":"ok",
> e.g.
> 
>   {
>     "dev":"nmem0",
>     "id":"04d5-01-1701-00000000",
>     "handle":0,
>     "phys_id":0,
>     "health":{
>       "health_state":"ok"
>     }
>   }
> 
> If there is an error with the NVDIMM, the "ok" will be replaced with "unknown",
> "fatal", "critical", or "non-critical".
> 
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
>  ndctl/lib/Makefile.am |   1 +
>  ndctl/lib/hyperv.c    | 129 ++++++++++++++++++++++++++++++++++++++++++
>  ndctl/lib/hyperv.h    |  51 +++++++++++++++++
>  ndctl/lib/libndctl.c  |   2 +
>  ndctl/lib/private.h   |   3 +
>  ndctl/ndctl.h         |   1 +
>  6 files changed, 187 insertions(+)
>  create mode 100644 ndctl/lib/hyperv.c
>  create mode 100644 ndctl/lib/hyperv.h
> 
> diff --git a/ndctl/lib/Makefile.am b/ndctl/lib/Makefile.am
> index 7797039..fb75fda 100644
> --- a/ndctl/lib/Makefile.am
> +++ b/ndctl/lib/Makefile.am
> @@ -20,6 +20,7 @@ libndctl_la_SOURCES =\
>  	intel.c \
>  	hpe1.c \
>  	msft.c \
> +	hyperv.c \
>  	ars.c \
>  	firmware.c \
>  	libndctl.c
> diff --git a/ndctl/lib/hyperv.c b/ndctl/lib/hyperv.c
> new file mode 100644
> index 0000000..b303d50
> --- /dev/null
> +++ b/ndctl/lib/hyperv.c
> @@ -0,0 +1,129 @@
> +/*
> + * Copyright (c) 2019, Microsoft Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU Lesser General Public License,
> + * version 2.1, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT ANY
> + * WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
> + * FOR A PARTICULAR PURPOSE.  See the GNU Lesser General Public License for
> + * more details.
> + */

For new files, use the SPDX license identifiers, for an example, see:
https://github.com/pmem/ndctl/blob/master/ndctl/load-keys.c#L1

> +#include <stdlib.h>
> +#include <limits.h>
> +#include <util/bitmap.h>
> +#include <util/log.h>
> +#include <ndctl/libndctl.h>
> +#include "private.h"
> +#include "hyperv.h"
> +
> +#define CMD_HYPERV(_c) ((_c)->hyperv)

I'm not sure this macro improves readability, in fact I think it rather
detracts from it in many cases - see further below.
Additionally, no need for the preceeding underscore in the macro
arguments - the rest of the code base doesn't do this, and I'm not sure
what value it provides.

> +#define CMD_HYPERV_STATUS(_c) (CMD_HYPERV(_c)->u.status)
> +#define CMD_HYPERV_SMART_DATA(_c) (CMD_HYPERV(_c)->u.smart.data)

> +
> +static struct ndctl_cmd *hyperv_dimm_cmd_new_smart(struct ndctl_dimm *dimm)
> +{
> +	struct ndctl_bus *bus = ndctl_dimm_get_bus(dimm);
> +	struct ndctl_ctx *ctx = ndctl_bus_get_ctx(bus);
> +	struct ndctl_cmd *cmd;
> +	size_t size;
> +	struct nd_pkg_hyperv *hyperv;
> +
> +	if (!ndctl_dimm_is_cmd_supported(dimm, ND_CMD_CALL)) {
> +		dbg(ctx, "unsupported cmd\n");
> +		return NULL;
> +	}
> +
> +	if (test_dimm_dsm(dimm, ND_HYPERV_CMD_GET_HEALTH_INFO) ==
> +			  DIMM_DSM_UNSUPPORTED) {
> +		dbg(ctx, "unsupported function\n");
> +		return NULL;
> +	}
> +
> +	size = sizeof(*cmd) + sizeof(struct nd_pkg_hyperv);
> +	cmd = calloc(1, size);
> +	if (!cmd)
> +		return NULL;
> +
> +	cmd->dimm = dimm;
> +	ndctl_cmd_ref(cmd);
> +	cmd->type = ND_CMD_CALL;
> +	cmd->size = size;
> +	cmd->status = 1;
> +
> +	hyperv = CMD_HYPERV(cmd);
> +	hyperv->gen.nd_family = NVDIMM_FAMILY_HYPERV;
> +	hyperv->gen.nd_command = ND_HYPERV_CMD_GET_HEALTH_INFO;
> +	hyperv->gen.nd_fw_size = 0;
> +	hyperv->gen.nd_size_in = offsetof(struct nd_hyperv_smart, status);
> +	hyperv->gen.nd_size_out = sizeof(hyperv->u.smart);
> +	hyperv->u.smart.status = 0;

calloc() zeroes the newly allocated memory - no need to set any of the
fields in the struct to '0' manually.

> +
> +	cmd->firmware_status = &hyperv->u.smart.status;
> +
> +	return cmd;
> +}
> +
> +static int hyperv_smart_valid(struct ndctl_cmd *cmd)
> +{
> +	if (cmd->type != ND_CMD_CALL ||
> +	    cmd->size != sizeof(*cmd) + sizeof(struct nd_pkg_hyperv) ||
> +	    CMD_HYPERV(cmd)->gen.nd_family != NVDIMM_FAMILY_HYPERV ||
> +	    CMD_HYPERV(cmd)->gen.nd_command != ND_HYPERV_CMD_GET_HEALTH_INFO ||

I feel in these cases, cmd->hyperv->stuff is /much/ more readable than
CMD_HYPERV(cmd)->stuff - and shorter as well as easier to type :)


> +	    cmd->status != 0 ||
> +	    CMD_HYPERV_STATUS(cmd) != 0)
> +		return cmd->status < 0 ? cmd->status : -EINVAL;
> +	return 0;
> +}
> +
> +static int hyperv_cmd_xlat_firmware_status(struct ndctl_cmd *cmd)
> +{
> +	return CMD_HYPERV_STATUS(cmd) == 0 ? 0 : -EINVAL;
> +}
> +
> +static unsigned int hyperv_cmd_smart_get_flags(struct ndctl_cmd *cmd)
> +{
> +	int rc;
> +
> +	rc = hyperv_smart_valid(cmd);
> +	if (rc < 0) {
> +		errno = -rc;
> +		return 0;
> +	}
> +
> +	return ND_SMART_HEALTH_VALID;
> +}
> +
> +static unsigned int hyperv_cmd_smart_get_health(struct ndctl_cmd *cmd)
> +{
> +	unsigned int health = 0;
> +	__u32 num;
> +	int rc;
> +
> +	rc = hyperv_smart_valid(cmd);
> +	if (rc < 0) {
> +		errno = -rc;
> +		return UINT_MAX;
> +	}
> +
> +	num = CMD_HYPERV_SMART_DATA(cmd)->health & 0x3F;
> +
> +	if (num & (BIT(0) | BIT(1)))
> +		health |= ND_SMART_CRITICAL_HEALTH;
> +
> +	if (num & BIT(2))
> +		health |= ND_SMART_FATAL_HEALTH;
> +
> +	if (num & (BIT(3) | BIT(4) | BIT(5)))
> +		health |= ND_SMART_NON_CRITICAL_HEALTH;
> +
> +	return health;
> +}
> +
> +struct ndctl_dimm_ops * const hyperv_dimm_ops = &(struct ndctl_dimm_ops) {
> +	.new_smart = hyperv_dimm_cmd_new_smart,
> +	.smart_get_flags = hyperv_cmd_smart_get_flags,
> +	.smart_get_health = hyperv_cmd_smart_get_health,
> +	.xlat_firmware_status = hyperv_cmd_xlat_firmware_status,
> +};
> diff --git a/ndctl/lib/hyperv.h b/ndctl/lib/hyperv.h
> new file mode 100644
> index 0000000..8e55a97
> --- /dev/null
> +++ b/ndctl/lib/hyperv.h
> @@ -0,0 +1,51 @@
> +/*
> + * Copyright (c) 2019, Microsoft Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU Lesser General Public License,
> + * version 2.1, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT ANY
> + * WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
> + * FOR A PARTICULAR PURPOSE.  See the GNU Lesser General Public License for
> + * more details.
> + */

Same comment about SPDX License format as above.

> +#ifndef __NDCTL_HYPERV_H__
> +#define __NDCTL_HYPERV_H__
> +
> +/* See http://www.uefi.org/RFIC_LIST ("Virtual NVDIMM 0x1901") */
> +enum {
> +	ND_HYPERV_CMD_QUERY = 0,

It sounds like the intention for this function index 0 was to function
as a supported DSM mask, but the spec says it just returns a static
value. Nonetheless, should we not include some "_cmd_is_supported"
helpers, and test them before submitting the smart command in this patch
for example?

Also the name of this enum field can be a bit ambiguous - query /what/?
(In other DSM families, there are functions to query ARS status,
firmware update status, etc.). It might be better to name it something
like "ND_HYPERV_CMD_QUERY_SUPPORTED_FUNCTIONS"

> +
> +	/* non-root commands */
> +	ND_HYPERV_CMD_GET_HEALTH_INFO = 1,
> +};
> +
> +/*
> + * This is actually Function 1's data,
> + * This is the closest I can find to match the "smart".
> + * Hyper-V _DSM methods don't have a smart function.
> + */
> +struct nd_hyperv_smart_data {
> +	__u32	health;
> +} __attribute__((packed));

I'm not sure I fully understand the comment above. Generally speaking,
we should avoid comments in the first person - i.e. instead of "This is
the closest thing I found..", it should simply be "X is the closest
thing to Y".

But I think you were trying to say:

/*
 * This corresponds to 'function 1' (Get Health Information) in the
 * HYPERV DSM spec referenced above
 */


_______________________________________________
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

[Dexuan Cui]

> From: Verma, Vishal L <vishal.l.verma-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 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 +0000, Dexuan Cui wrote:
> > This patch retrieves the health info by Hyper-V _DSM method Function 1:
> We should never use "This patch.." in a commit message. See 4.c in [1].

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 <stdlib.h>
> > +#include <limits.h>
> > +#include <util/bitmap.h>
> > +#include <util/log.h>
> > +#include <ndctl/libndctl.h>
> > +#include "private.h"
> > +#include "hyperv.h"
> > +
> > +#define CMD_HYPERV(_c) ((_c)->hyperv)
> 
> I'm not sure this macro improves readability, in fact I think it rather
> detracts from it in many cases - see further below.
> Additionally, no need for the preceeding underscore in the macro
> arguments - the rest of the code base doesn't do this, and I'm not sure
> what value it provides.

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