All of lore.kernel.org
 help / color / mirror / Atom feed
* [ndctl PATCH v2 4/4] ndctl, monitor: support NVDIMM_FAMILY_HYPERV
@ 2019-02-20  5:11 Dexuan Cui
  2019-02-21  8:40 ` qi.fuli
  2019-03-21  1:54 ` Verma, Vishal L
  0 siblings, 2 replies; 19+ messages in thread
From: Dexuan Cui @ 2019-02-20  5:11 UTC (permalink / raw)
  To: Dave Jiang, Vishal Verma, Dan Williams,
	linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw, Michael Kelley,
	qi.fuli-LMvhtfratI1BDgjK7y7TUQ, Johannes Thumshirn


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-01000000","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 <decui-0li6OtcxBFHby3iVrkZq2A@public.gmane.org>
---
 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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* RE: [ndctl PATCH v2 4/4] ndctl, monitor: support NVDIMM_FAMILY_HYPERV
  2019-02-20  5:11 [ndctl PATCH v2 4/4] ndctl, monitor: support NVDIMM_FAMILY_HYPERV Dexuan Cui
@ 2019-02-21  8:40 ` qi.fuli
       [not found]   ` <OSBPR01MB50009900A5FE2E8588AA6367F77E0-zhdMvvdcU0ZckHoPmfvix3colHNk5qUtvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
  2019-03-21  1:54 ` Verma, Vishal L
  1 sibling, 1 reply; 19+ messages in thread
From: qi.fuli @ 2019-02-21  8:40 UTC (permalink / raw)
  To: 'Dexuan Cui',
	Dave Jiang, Vishal Verma, Dan Williams, linux-nvdimm,
	Michael Kelley, Johannes Thumshirn



> -----Original Message-----
> From: Dexuan Cui [mailto:decui@microsoft.com]
> Sent: Wednesday, February 20, 2019 2:12 PM
> To: Dave Jiang <dave.jiang@intel.com>; Vishal Verma
> <vishal.l.verma@intel.com>; Dan Williams <dan.j.williams@intel.com>;
> linux-nvdimm@lists.01.org; Michael Kelley <mikelley@microsoft.com>; Qi,
> Fuli/斉 福利 <qi.fuli@fujitsu.com>; Johannes Thumshirn <jthumshirn@suse.de>
> Subject: [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-01000000","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 <decui@microsoft.com>
> ---
>  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;
> +

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

> +			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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [ndctl PATCH v2 4/4] ndctl, monitor: support NVDIMM_FAMILY_HYPERV
       [not found]   ` <OSBPR01MB50009900A5FE2E8588AA6367F77E0-zhdMvvdcU0ZckHoPmfvix3colHNk5qUtvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
@ 2019-02-21 18:30     ` Dexuan Cui
  2019-02-25  4:48       ` qi.fuli
  0 siblings, 1 reply; 19+ messages in thread
From: Dexuan Cui @ 2019-02-21 18:30 UTC (permalink / raw)
  To: qi.fuli-LMvhtfratI1BDgjK7y7TUQ, Dave Jiang, Vishal Verma,
	Dan Williams, linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw,
	Michael Kelley, Johannes Thumshirn

> From: qi.fuli-LMvhtfratI1BDgjK7y7TUQ@public.gmane.org <qi.fuli-LMvhtfratI1BDgjK7y7TUQ@public.gmane.org>
> 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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [ndctl PATCH v2 4/4] ndctl, monitor: support NVDIMM_FAMILY_HYPERV
  2019-02-21 18:30     ` Dexuan Cui
@ 2019-02-25  4:48       ` qi.fuli
       [not found]         ` <OSAPR01MB4993163079F35EEF06DFBB44F77A0-OxqQCv4d1nTzcNm8D4NKs3colHNk5qUtvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
  0 siblings, 1 reply; 19+ messages in thread
From: qi.fuli @ 2019-02-25  4:48 UTC (permalink / raw)
  To: 'Dexuan Cui',
	Dave Jiang, Vishal Verma, Dan Williams, linux-nvdimm,
	Michael Kelley, Johannes Thumshirn

> 
> 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.
Though I have a little concern about that there may be other events for Hyper-V Virtual NVDIMM can be monitored in future...

Thanks
 Qi
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [ndctl PATCH v2 4/4] ndctl, monitor: support NVDIMM_FAMILY_HYPERV
       [not found]         ` <OSAPR01MB4993163079F35EEF06DFBB44F77A0-OxqQCv4d1nTzcNm8D4NKs3colHNk5qUtvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
@ 2019-02-25  5:16           ` Dexuan Cui
  0 siblings, 0 replies; 19+ messages in thread
From: Dexuan Cui @ 2019-02-25  5:16 UTC (permalink / raw)
  To: qi.fuli-LMvhtfratI1BDgjK7y7TUQ, Dave Jiang, Vishal Verma,
	Dan Williams, linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw,
	Michael Kelley, Johannes Thumshirn

> From: qi.fuli-LMvhtfratI1BDgjK7y7TUQ@public.gmane.org <qi.fuli-LMvhtfratI1BDgjK7y7TUQ@public.gmane.org>
> Sent: Sunday, February 24, 2019 8:48 PM
> To: Dexuan Cui <decui-0li6OtcxBFHby3iVrkZq2A@public.gmane.org>; Dave Jiang <dave.jiang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>;
> Vishal Verma <vishal.l.verma-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>; Dan Williams
> <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>; linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw@public.gmane.org; Michael Kelley
> <mikelley-0li6OtcxBFHby3iVrkZq2A@public.gmane.org>; Johannes Thumshirn <jthumshirn-l3A5Bk7waGM@public.gmane.org>
> 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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [ndctl PATCH v2 4/4] ndctl, monitor: support NVDIMM_FAMILY_HYPERV
  2019-02-20  5:11 [ndctl PATCH v2 4/4] ndctl, monitor: support NVDIMM_FAMILY_HYPERV Dexuan Cui
  2019-02-21  8:40 ` qi.fuli
@ 2019-03-21  1:54 ` Verma, Vishal L
       [not found]   ` <9876fd8a491e339f2f41a47e2195d354bf0d5fb2.camel-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
  1 sibling, 1 reply; 19+ messages in thread
From: Verma, Vishal L @ 2019-03-21  1:54 UTC (permalink / raw)
  To: Williams, Dan J, decui, Jiang, Dave, linux-nvdimm, jthumshirn,
	mikelley, qi.fuli


On Wed, 2019-02-20 at 05:11 +0000, Dexuan Cui wrote:
> 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-01000000","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 <decui@microsoft.com>
> ---
>  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;
> +	}
> +

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?

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.

Indeed, I think that patch 3 of this series should never be required :)

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

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [ndctl PATCH v2 4/4] ndctl, monitor: support NVDIMM_FAMILY_HYPERV
       [not found]   ` <9876fd8a491e339f2f41a47e2195d354bf0d5fb2.camel-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
@ 2019-03-22  2:08     ` Dexuan Cui
  2019-03-22  3:31       ` Dan Williams
       [not found]       ` <PU1P153MB0169CA436F52268DD2BEF745BF430-7hHSCQUTt08p9lVGRpUb+2HfQuWdHs3hiGd9ebBGJoev3QGu/rdwKA@public.gmane.org>
  0 siblings, 2 replies; 19+ messages in thread
From: Dexuan Cui @ 2019-03-22  2:08 UTC (permalink / raw)
  To: Verma, Vishal L, Williams, Dan J, Jiang, Dave,
	linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw, jthumshirn-l3A5Bk7waGM,
	Michael Kelley, qi.fuli-LMvhtfratI1BDgjK7y7TUQ

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [ndctl PATCH v2 4/4] ndctl, monitor: support NVDIMM_FAMILY_HYPERV
  2019-03-22  2:08     ` Dexuan Cui
@ 2019-03-22  3:31       ` Dan Williams
       [not found]       ` <PU1P153MB0169CA436F52268DD2BEF745BF430-7hHSCQUTt08p9lVGRpUb+2HfQuWdHs3hiGd9ebBGJoev3QGu/rdwKA@public.gmane.org>
  1 sibling, 0 replies; 19+ messages in thread
From: Dan Williams @ 2019-03-22  3:31 UTC (permalink / raw)
  To: Dexuan Cui; +Cc: qi.fuli, linux-nvdimm, Michael Kelley

On Thu, Mar 21, 2019 at 7:09 PM Dexuan Cui <decui@microsoft.com> wrote:
[..]
> > 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...

For the monitor it should be ok to specify options and thresholds that
the DIMM does not support. The events simply won't fire. Now if the
user cares to know which events are supported / live we can add "list"
 enumeration interface for that, but I otherwise think monitor should
just silently handle unsupported monitor options.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [ndctl PATCH v2 4/4] ndctl, monitor: support NVDIMM_FAMILY_HYPERV
       [not found]       ` <PU1P153MB0169CA436F52268DD2BEF745BF430-7hHSCQUTt08p9lVGRpUb+2HfQuWdHs3hiGd9ebBGJoev3QGu/rdwKA@public.gmane.org>
@ 2019-03-22  4:05         ` Dexuan Cui
  2019-03-22  4:13           ` Dan Williams
  0 siblings, 1 reply; 19+ messages in thread
From: Dexuan Cui @ 2019-03-22  4:05 UTC (permalink / raw)
  To: Verma, Vishal L, Williams, Dan J, Jiang, Dave,
	linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw, jthumshirn-l3A5Bk7waGM,
	Michael Kelley, qi.fuli-LMvhtfratI1BDgjK7y7TUQ

> 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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [ndctl PATCH v2 4/4] ndctl, monitor: support NVDIMM_FAMILY_HYPERV
  2019-03-22  4:05         ` Dexuan Cui
@ 2019-03-22  4:13           ` Dan Williams
       [not found]             ` <CAPcyv4hVZLW2OSMZ9NqqsQWMg7vxyS9J1ekDZW8G44cmD9_YCw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 1 reply; 19+ messages in thread
From: Dan Williams @ 2019-03-22  4:13 UTC (permalink / raw)
  To: Dexuan Cui; +Cc: qi.fuli, linux-nvdimm, Michael Kelley

On Thu, Mar 21, 2019 at 9:06 PM Dexuan Cui <decui@microsoft.com> wrote:
>
> > 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".

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.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [ndctl PATCH v2 4/4] ndctl, monitor: support NVDIMM_FAMILY_HYPERV
       [not found]             ` <CAPcyv4hVZLW2OSMZ9NqqsQWMg7vxyS9J1ekDZW8G44cmD9_YCw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2019-03-22  5:09               ` Dexuan Cui
  2019-03-22  5:36                 ` Dan Williams
  0 siblings, 1 reply; 19+ messages in thread
From: Dexuan Cui @ 2019-03-22  5:09 UTC (permalink / raw)
  To: Dan Williams, Verma, Vishal L
  Cc: Michael Kelley, qi.fuli-LMvhtfratI1BDgjK7y7TUQ,
	linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw

> From: Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 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").

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [ndctl PATCH v2 4/4] ndctl, monitor: support NVDIMM_FAMILY_HYPERV
  2019-03-22  5:09               ` Dexuan Cui
@ 2019-03-22  5:36                 ` Dan Williams
       [not found]                   ` <CAPcyv4hM_DTcMJdWhWbtPFfVH_V-kYxXSSCg=MQZLoxVka=Mng-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 1 reply; 19+ messages in thread
From: Dan Williams @ 2019-03-22  5:36 UTC (permalink / raw)
  To: Dexuan Cui; +Cc: qi.fuli, linux-nvdimm, Michael Kelley

On Thu, Mar 21, 2019 at 10:09 PM Dexuan Cui <decui@microsoft.com> wrote:
>
> > From: Dan Williams <dan.j.williams@intel.com>
> > 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?

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.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [ndctl PATCH v2 4/4] ndctl, monitor: support NVDIMM_FAMILY_HYPERV
       [not found]                   ` <CAPcyv4hM_DTcMJdWhWbtPFfVH_V-kYxXSSCg=MQZLoxVka=Mng-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2019-03-22  6:03                     ` Dexuan Cui
  2019-03-22  6:11                       ` Dan Williams
  0 siblings, 1 reply; 19+ messages in thread
From: Dexuan Cui @ 2019-03-22  6:03 UTC (permalink / raw)
  To: Dan Williams
  Cc: qi.fuli-LMvhtfratI1BDgjK7y7TUQ,
	linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw, Michael Kelley

> From: Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [ndctl PATCH v2 4/4] ndctl, monitor: support NVDIMM_FAMILY_HYPERV
  2019-03-22  6:03                     ` Dexuan Cui
@ 2019-03-22  6:11                       ` Dan Williams
       [not found]                         ` <CAPcyv4h4tAydacTbmZRKy7hmTqWTM22r6hD+Ucm_f3H2tQUm3g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 1 reply; 19+ messages in thread
From: Dan Williams @ 2019-03-22  6:11 UTC (permalink / raw)
  To: Dexuan Cui; +Cc: qi.fuli, linux-nvdimm, Michael Kelley

On Thu, Mar 21, 2019 at 11:03 PM Dexuan Cui <decui@microsoft.com> wrote:
>
> > From: Dan Williams <dan.j.williams@intel.com>
> > 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.

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".
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [ndctl PATCH v2 4/4] ndctl, monitor: support NVDIMM_FAMILY_HYPERV
       [not found]                         ` <CAPcyv4h4tAydacTbmZRKy7hmTqWTM22r6hD+Ucm_f3H2tQUm3g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2019-03-22 17:55                           ` Dexuan Cui
  2019-03-22 18:03                             ` Dan Williams
  2019-03-22 18:28                             ` Verma, Vishal L
  0 siblings, 2 replies; 19+ messages in thread
From: Dexuan Cui @ 2019-03-22 17:55 UTC (permalink / raw)
  To: Dan Williams, Verma, Vishal L
  Cc: Michael Kelley, qi.fuli-LMvhtfratI1BDgjK7y7TUQ,
	linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw

> From: Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Sent: Thursday, March 21, 2019 11:12 PM
> To: Dexuan Cui <decui-0li6OtcxBFHby3iVrkZq2A@public.gmane.org>
> 
> On Thu, Mar 21, 2019 at 11:03 PM Dexuan Cui <decui-0li6OtcxBFHby3iVrkZq2A@public.gmane.org> wrote:
> >
> > > From: Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > > 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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [ndctl PATCH v2 4/4] ndctl, monitor: support NVDIMM_FAMILY_HYPERV
  2019-03-22 17:55                           ` Dexuan Cui
@ 2019-03-22 18:03                             ` Dan Williams
       [not found]                               ` <CAPcyv4gMexBmWduqR8G=1H3gKkxbqfBADOLG595nVRipAxVukA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  2019-03-22 18:28                             ` Verma, Vishal L
  1 sibling, 1 reply; 19+ messages in thread
From: Dan Williams @ 2019-03-22 18:03 UTC (permalink / raw)
  To: Dexuan Cui; +Cc: qi.fuli, linux-nvdimm, Michael Kelley

On Fri, Mar 22, 2019 at 10:56 AM Dexuan Cui <decui@microsoft.com> wrote:
>
> > From: Dan Williams <dan.j.williams@intel.com>
> > Sent: Thursday, March 21, 2019 11:12 PM
> > To: Dexuan Cui <decui@microsoft.com>
> >
> > On Thu, Mar 21, 2019 at 11:03 PM Dexuan Cui <decui@microsoft.com> wrote:
> > >
> > > > From: Dan Williams <dan.j.williams@intel.com>
> > > > 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()

Where would the monitor_supported() op be used? I would expect a
cmd_is_supported() op?

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

This sounds good.

> If this sounds reasonable to you and Verma, I'll try to make a patchset for you
> to review.

Yes, just the question of what you meant the monitor_supported() op to perform.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [ndctl PATCH v2 4/4] ndctl, monitor: support NVDIMM_FAMILY_HYPERV
       [not found]                               ` <CAPcyv4gMexBmWduqR8G=1H3gKkxbqfBADOLG595nVRipAxVukA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2019-03-22 18:09                                 ` Dexuan Cui
  0 siblings, 0 replies; 19+ messages in thread
From: Dexuan Cui @ 2019-03-22 18:09 UTC (permalink / raw)
  To: Dan Williams
  Cc: qi.fuli-LMvhtfratI1BDgjK7y7TUQ,
	linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw, Michael Kelley

> From: Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Sent: Friday, March 22, 2019 11:04 AM
> 
> On Fri, Mar 22, 2019 at 10:56 AM Dexuan Cui <decui-0li6OtcxBFHby3iVrkZq2A@public.gmane.org> 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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [ndctl PATCH v2 4/4] ndctl, monitor: support NVDIMM_FAMILY_HYPERV
  2019-03-22 17:55                           ` Dexuan Cui
  2019-03-22 18:03                             ` Dan Williams
@ 2019-03-22 18:28                             ` Verma, Vishal L
       [not found]                               ` <837eed30b0da59f91e5114aad97e919068a077e8.camel-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
  1 sibling, 1 reply; 19+ messages in thread
From: Verma, Vishal L @ 2019-03-22 18:28 UTC (permalink / raw)
  To: Williams, Dan J, decui; +Cc: mikelley, qi.fuli, linux-nvdimm


On Fri, 2019-03-22 at 17:55 +0000, Dexuan Cui wrote:
> > From: Dan Williams <dan.j.williams@intel.com>
> > Sent: Thursday, March 21, 2019 11:12 PM
> > To: Dexuan Cui <decui@microsoft.com>
> > 
> > On Thu, Mar 21, 2019 at 11:03 PM Dexuan Cui <decui@microsoft.com> wrote:
> > > > From: Dan Williams <dan.j.williams@intel.com>
> > > > 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!

> 
> Thanks,
> -- Dexuan

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [ndctl PATCH v2 4/4] ndctl, monitor: support NVDIMM_FAMILY_HYPERV
       [not found]                               ` <837eed30b0da59f91e5114aad97e919068a077e8.camel-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
@ 2019-03-23  4:24                                 ` Dexuan Cui
  0 siblings, 0 replies; 19+ messages in thread
From: Dexuan Cui @ 2019-03-23  4:24 UTC (permalink / raw)
  To: Verma, Vishal L, Williams, Dan J
  Cc: Michael Kelley, qi.fuli-LMvhtfratI1BDgjK7y7TUQ,
	linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw

> From: Verma, Vishal L <vishal.l.verma-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Sent: Friday, March 22, 2019 11:29 AM
> 
> On Fri, 2019-03-22 at 17:55 +0000, Dexuan Cui wrote:
> > > From: Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > > Sent: Thursday, March 21, 2019 11:12 PM
> > > To: Dexuan Cui <decui-0li6OtcxBFHby3iVrkZq2A@public.gmane.org>
> > >
> > > On Thu, Mar 21, 2019 at 11:03 PM Dexuan Cui <decui-0li6OtcxBFHby3iVrkZq2A@public.gmane.org>
> wrote:
> > > > > From: Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > > > > 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

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2019-03-23  4:24 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-20  5:11 [ndctl PATCH v2 4/4] ndctl, monitor: support NVDIMM_FAMILY_HYPERV Dexuan Cui
2019-02-21  8:40 ` qi.fuli
     [not found]   ` <OSBPR01MB50009900A5FE2E8588AA6367F77E0-zhdMvvdcU0ZckHoPmfvix3colHNk5qUtvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
2019-02-21 18:30     ` Dexuan Cui
2019-02-25  4:48       ` qi.fuli
     [not found]         ` <OSAPR01MB4993163079F35EEF06DFBB44F77A0-OxqQCv4d1nTzcNm8D4NKs3colHNk5qUtvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
2019-02-25  5:16           ` Dexuan Cui
2019-03-21  1:54 ` Verma, Vishal L
     [not found]   ` <9876fd8a491e339f2f41a47e2195d354bf0d5fb2.camel-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
2019-03-22  2:08     ` Dexuan Cui
2019-03-22  3:31       ` Dan Williams
     [not found]       ` <PU1P153MB0169CA436F52268DD2BEF745BF430-7hHSCQUTt08p9lVGRpUb+2HfQuWdHs3hiGd9ebBGJoev3QGu/rdwKA@public.gmane.org>
2019-03-22  4:05         ` Dexuan Cui
2019-03-22  4:13           ` Dan Williams
     [not found]             ` <CAPcyv4hVZLW2OSMZ9NqqsQWMg7vxyS9J1ekDZW8G44cmD9_YCw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2019-03-22  5:09               ` Dexuan Cui
2019-03-22  5:36                 ` Dan Williams
     [not found]                   ` <CAPcyv4hM_DTcMJdWhWbtPFfVH_V-kYxXSSCg=MQZLoxVka=Mng-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2019-03-22  6:03                     ` Dexuan Cui
2019-03-22  6:11                       ` Dan Williams
     [not found]                         ` <CAPcyv4h4tAydacTbmZRKy7hmTqWTM22r6hD+Ucm_f3H2tQUm3g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2019-03-22 17:55                           ` Dexuan Cui
2019-03-22 18:03                             ` Dan Williams
     [not found]                               ` <CAPcyv4gMexBmWduqR8G=1H3gKkxbqfBADOLG595nVRipAxVukA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2019-03-22 18:09                                 ` Dexuan Cui
2019-03-22 18:28                             ` Verma, Vishal L
     [not found]                               ` <837eed30b0da59f91e5114aad97e919068a077e8.camel-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
2019-03-23  4:24                                 ` Dexuan Cui

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.