| Message ID | 20190323042028.4310-5-decui@microsoft.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 58fb5302b4c91ff3d00cc2de38bae34571802877 |
| Headers | show |
| Series | Add the support for Hyper-V virtual NVDIMM | expand |
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 *);
> From: Verma, Vishal L <vishal.l.verma@intel.com> > 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
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!
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
> From: Verma, Vishal L <vishal.l.verma@intel.com> > Sent: Wednesday, March 27, 2019 9:17 AM > To: Williams, Dan J <dan.j.williams@intel.com>; Dexuan Cui > <decui@microsoft.com>; Jiang, Dave <dave.jiang@intel.com>; > linux-nvdimm@lists.01.org; dexuan.cui@gmail.com; Michael Kelley > <mikelley@microsoft.com> > Cc: jthumshirn@suse.de; qi.fuli@fujitsu.com > Subject: Re: [ndctl PATCH v3 4/5] libndctl: Add a new dimm-op > cmd_is_supported() > > > On Tue, 2019-03-26 at 23:29 +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
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 *);
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(+)