2019-01-23 20:52:24

by Dexuan Cui

[permalink] [raw]
Subject: [PATCH] nfit: add Hyper-V NVDIMM DSM command set to white list


Add the Hyper-V _DSM command set to the white list of NVDIMM command
sets.

This command set is documented at http://www.uefi.org/RFIC_LIST
(see the link to "Virtual NVDIMM 0x1901" on the page).

Signed-off-by: Dexuan Cui <[email protected]>
---

I'm going to change the user-space utility "ndctl" to support Hyper-V Virtual NVDIMM.
This kernel patch is required first.

drivers/acpi/nfit/core.c | 5 ++++-
drivers/acpi/nfit/nfit.h | 6 +++++-
include/uapi/linux/ndctl.h | 1 +
3 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 011d3db19c80..fb48cb17a519 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -1840,7 +1840,7 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
dev_set_drvdata(&adev_dimm->dev, nfit_mem);

/*
- * Until standardization materializes we need to consider 4
+ * Until standardization materializes we need to consider 5
* different command sets. Note, that checking for function0 (bit0)
* tells us if any commands are reachable through this GUID.
*/
@@ -1865,6 +1865,8 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
dsm_mask &= ~(1 << 8);
} else if (nfit_mem->family == NVDIMM_FAMILY_MSFT) {
dsm_mask = 0xffffffff;
+ } else if (nfit_mem->family == NVDIMM_FAMILY_HYPERV) {
+ dsm_mask = 0x1f;
} else {
dev_dbg(dev, "unknown dimm command family\n");
nfit_mem->family = -1;
@@ -3707,6 +3709,7 @@ static __init int nfit_init(void)
guid_parse(UUID_NFIT_DIMM_N_HPE1, &nfit_uuid[NFIT_DEV_DIMM_N_HPE1]);
guid_parse(UUID_NFIT_DIMM_N_HPE2, &nfit_uuid[NFIT_DEV_DIMM_N_HPE2]);
guid_parse(UUID_NFIT_DIMM_N_MSFT, &nfit_uuid[NFIT_DEV_DIMM_N_MSFT]);
+ guid_parse(UUID_NFIT_DIMM_N_HYPERV, &nfit_uuid[NFIT_DEV_DIMM_N_HYPERV]);

nfit_wq = create_singlethread_workqueue("nfit");
if (!nfit_wq)
diff --git a/drivers/acpi/nfit/nfit.h b/drivers/acpi/nfit/nfit.h
index 33691aecfcee..9194022f29f3 100644
--- a/drivers/acpi/nfit/nfit.h
+++ b/drivers/acpi/nfit/nfit.h
@@ -34,11 +34,14 @@
/* https://msdn.microsoft.com/library/windows/hardware/mt604741 */
#define UUID_NFIT_DIMM_N_MSFT "1ee68b36-d4bd-4a1a-9a16-4f8e53d46e05"

+/* http://www.uefi.org/RFIC_LIST */
+#define UUID_NFIT_DIMM_N_HYPERV "5746c5f2-a9a2-4264-ad0e-e4ddc9e09e80"
+
#define ACPI_NFIT_MEM_FAILED_MASK (ACPI_NFIT_MEM_SAVE_FAILED \
| ACPI_NFIT_MEM_RESTORE_FAILED | ACPI_NFIT_MEM_FLUSH_FAILED \
| ACPI_NFIT_MEM_NOT_ARMED | ACPI_NFIT_MEM_MAP_FAILED)

-#define NVDIMM_FAMILY_MAX NVDIMM_FAMILY_MSFT
+#define NVDIMM_FAMILY_MAX NVDIMM_FAMILY_HYPERV

#define NVDIMM_STANDARD_CMDMASK \
(1 << ND_CMD_SMART | 1 << ND_CMD_SMART_THRESHOLD | 1 << ND_CMD_DIMM_FLAGS \
@@ -94,6 +97,7 @@ enum nfit_uuids {
NFIT_DEV_DIMM_N_HPE1 = NVDIMM_FAMILY_HPE1,
NFIT_DEV_DIMM_N_HPE2 = NVDIMM_FAMILY_HPE2,
NFIT_DEV_DIMM_N_MSFT = NVDIMM_FAMILY_MSFT,
+ NFIT_DEV_DIMM_N_HYPERV = NVDIMM_FAMILY_HYPERV,
NFIT_SPA_VOLATILE,
NFIT_SPA_PM,
NFIT_SPA_DCR,
diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h
index f57c9e434d2d..de5d90212409 100644
--- a/include/uapi/linux/ndctl.h
+++ b/include/uapi/linux/ndctl.h
@@ -243,6 +243,7 @@ struct nd_cmd_pkg {
#define NVDIMM_FAMILY_HPE1 1
#define NVDIMM_FAMILY_HPE2 2
#define NVDIMM_FAMILY_MSFT 3
+#define NVDIMM_FAMILY_HYPERV 4

#define ND_IOCTL_CALL _IOWR(ND_IOCTL, ND_CMD_CALL,\
struct nd_cmd_pkg)
--
2.19.1



2019-01-27 05:44:56

by Michael Kelley (LINUX)

[permalink] [raw]
Subject: RE: [PATCH] nfit: add Hyper-V NVDIMM DSM command set to white list

From: Dexuan Cui <[email protected]> Sent: Wednesday, January 23, 2019 12:51 PM
>
> Add the Hyper-V _DSM command set to the white list of NVDIMM command
> sets.
>
> This command set is documented at
> http://www.uefi.org/RFIC_LIST (see the link to "Virtual NVDIMM 0x1901" on the page).
>
> Signed-off-by: Dexuan Cui <[email protected]>
> ---
>
> I'm going to change the user-space utility "ndctl" to support Hyper-V Virtual NVDIMM.
> This kernel patch is required first.
>
> drivers/acpi/nfit/core.c | 5 ++++-
> drivers/acpi/nfit/nfit.h | 6 +++++-
> include/uapi/linux/ndctl.h | 1 +
> 3 files changed, 10 insertions(+), 2 deletions(-)
>

Reviewed-by: Michael Kelley <[email protected]>

2019-01-28 21:01:55

by Dan Williams

[permalink] [raw]
Subject: Re: [PATCH] nfit: add Hyper-V NVDIMM DSM command set to white list

Hi Dexuan,

Looks good. Just one update request and a note below...

On Wed, Jan 23, 2019 at 12:51 PM Dexuan Cui <[email protected]> wrote:
>
>
> Add the Hyper-V _DSM command set to the white list of NVDIMM command
> sets.
>
> This command set is documented at http://www.uefi.org/RFIC_LIST
> (see the link to "Virtual NVDIMM 0x1901" on the page).
>
> Signed-off-by: Dexuan Cui <[email protected]>
> ---
>
> I'm going to change the user-space utility "ndctl" to support Hyper-V Virtual NVDIMM.
> This kernel patch is required first.
>
> drivers/acpi/nfit/core.c | 5 ++++-
> drivers/acpi/nfit/nfit.h | 6 +++++-
> include/uapi/linux/ndctl.h | 1 +
> 3 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 011d3db19c80..fb48cb17a519 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -1840,7 +1840,7 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
> dev_set_drvdata(&adev_dimm->dev, nfit_mem);
>
> /*
> - * Until standardization materializes we need to consider 4
> + * Until standardization materializes we need to consider 5
> * different command sets. Note, that checking for function0 (bit0)
> * tells us if any commands are reachable through this GUID.
> */

This comment is stale. This "HYPERV" family is the first example of
the "right" way to define a new NVDIMM command set. Lets update it to
mention the process and considerations for submitting new command sets
to UEFI (http://www.uefi.org/RFIC_LIST). The fact that there's now a
defined process is a step forward from when this comment was initially
written. Also, the fact that the process encourages "adopt" vs
"supersede" addresses the main concern about vendor-specific
command-set proliferation.

> @@ -1865,6 +1865,8 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
> dsm_mask &= ~(1 << 8);
> } else if (nfit_mem->family == NVDIMM_FAMILY_MSFT) {
> dsm_mask = 0xffffffff;
> + } else if (nfit_mem->family == NVDIMM_FAMILY_HYPERV) {
> + dsm_mask = 0x1f;

Just a note that starting with commit 5e9e38d0db1d "acpi/nfit: Block
function zero DSMs", bit0 in this mask will be cleared to ensure that
only the kernel has the ability to probe for supported DSM functions.

2019-01-28 21:40:23

by Dexuan Cui

[permalink] [raw]
Subject: RE: [PATCH] nfit: add Hyper-V NVDIMM DSM command set to white list

> From: Dan Williams <[email protected]>
> Sent: Monday, January 28, 2019 1:01 PM
>
> Hi Dexuan,
> Looks good. Just one update request and a note below...
>
> On Wed, Jan 23, 2019 at 12:51 PM Dexuan Cui <[email protected]> wrote:
> > ...
> > --- a/drivers/acpi/nfit/core.c
> > +++ b/drivers/acpi/nfit/core.c
> > @@ -1840,7 +1840,7 @@ static int acpi_nfit_add_dimm(struct
> acpi_nfit_desc *acpi_desc,
> > dev_set_drvdata(&adev_dimm->dev, nfit_mem);
> >
> > /*
> > - * Until standardization materializes we need to consider 4
> > + * Until standardization materializes we need to consider 5
> > * different command sets. Note, that checking for function0
> (bit0)
> > * tells us if any commands are reachable through this GUID.
> > */
>
> This comment is stale. This "HYPERV" family is the first example of
> the "right" way to define a new NVDIMM command set. Lets update it to
> mention the process and considerations for submitting new command sets
> to UEFI (http://www.uefi.org/RFIC_LIST). The fact that there's now a
> defined process is a step forward from when this comment was initially
> written. Also, the fact that the process encourages "adopt" vs
> "supersede" addresses the main concern about vendor-specific
> command-set proliferation.

I made the below simple change. Is this enough? Please suggest the proper
wording/sentence, as I'm relatively new to NVDIMM, and I don't really know the
history of the standardization process.

--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -1732,8 +1732,10 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
dev_set_drvdata(&adev_dimm->dev, nfit_mem);

/*
- * Until standardization materializes we need to consider 4
- * different command sets. Note, that checking for function0 (bit0)
+ * New command sets should be submitted to UEFI
+ * http://www.uefi.org/RFIC_LIST.
+ *
+ * Note, that checking for function0 (bit0)
* tells us if any commands are reachable through this GUID.
*/
for (i = 0; i <= NVDIMM_FAMILY_MAX; i++)


> > @@ -1865,6 +1865,8 @@ static int acpi_nfit_add_dimm(struct
> acpi_nfit_desc *acpi_desc,
> > dsm_mask &= ~(1 << 8);
> > } else if (nfit_mem->family == NVDIMM_FAMILY_MSFT) {
> > dsm_mask = 0xffffffff;
> > + } else if (nfit_mem->family == NVDIMM_FAMILY_HYPERV) {
> > + dsm_mask = 0x1f;
>
> Just a note that starting with commit 5e9e38d0db1d "acpi/nfit: Block
> function zero DSMs", bit0 in this mask will be cleared to ensure that
> only the kernel has the ability to probe for supported DSM functions.

Thanks for the note!

Thanks,
-- Dexuan

2019-01-28 21:56:15

by Dan Williams

[permalink] [raw]
Subject: Re: [PATCH] nfit: add Hyper-V NVDIMM DSM command set to white list

On Mon, Jan 28, 2019 at 1:40 PM Dexuan Cui <[email protected]> wrote:
>
> > From: Dan Williams <[email protected]>
> > Sent: Monday, January 28, 2019 1:01 PM
> >
> > Hi Dexuan,
> > Looks good. Just one update request and a note below...
> >
> > On Wed, Jan 23, 2019 at 12:51 PM Dexuan Cui <[email protected]> wrote:
> > > ...
> > > --- a/drivers/acpi/nfit/core.c
> > > +++ b/drivers/acpi/nfit/core.c
> > > @@ -1840,7 +1840,7 @@ static int acpi_nfit_add_dimm(struct
> > acpi_nfit_desc *acpi_desc,
> > > dev_set_drvdata(&adev_dimm->dev, nfit_mem);
> > >
> > > /*
> > > - * Until standardization materializes we need to consider 4
> > > + * Until standardization materializes we need to consider 5
> > > * different command sets. Note, that checking for function0
> > (bit0)
> > > * tells us if any commands are reachable through this GUID.
> > > */
> >
> > This comment is stale. This "HYPERV" family is the first example of
> > the "right" way to define a new NVDIMM command set. Lets update it to
> > mention the process and considerations for submitting new command sets
> > to UEFI (http://www.uefi.org/RFIC_LIST). The fact that there's now a
> > defined process is a step forward from when this comment was initially
> > written. Also, the fact that the process encourages "adopt" vs
> > "supersede" addresses the main concern about vendor-specific
> > command-set proliferation.
>
> I made the below simple change. Is this enough? Please suggest the proper
> wording/sentence, as I'm relatively new to NVDIMM, and I don't really know the
> history of the standardization process.
>
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -1732,8 +1732,10 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
> dev_set_drvdata(&adev_dimm->dev, nfit_mem);
>
> /*
> - * Until standardization materializes we need to consider 4
> - * different command sets. Note, that checking for function0 (bit0)
> + * New command sets should be submitted to UEFI
> + * http://www.uefi.org/RFIC_LIST.
> + *

How about something a bit more relevant for the code in question:

---

There are 4 "legacy" NVDIMM command sets
(NVDIMM_FAMILY_{INTEL,MSFT,HPE1,HPE2}) that were created before an EFI
working group was established to constrain this proliferation. The
nfit driver probes for the supported command set by GUID. Note, If
you're a platform developer looking to add a new command set to this
probe consider using an existing set, or otherwise seek approval to
publish the command set at http://www.uefi.org/RFIC_LIST.

2019-01-28 23:00:22

by Dexuan Cui

[permalink] [raw]
Subject: RE: [PATCH] nfit: add Hyper-V NVDIMM DSM command set to white list

> From: Dan Williams <[email protected]>
> Sent: Monday, January 28, 2019 1:55 PM
>
> On Mon, Jan 28, 2019 at 1:40 PM Dexuan Cui <[email protected]> wrote:
>
> > I made the below simple change. Is this enough? Please suggest the proper
> > wording/sentence, as I'm relatively new to NVDIMM, and I don't really know
> the history of the standardization process.
>
> How about something a bit more relevant for the code in question:
>

> There are 4 "legacy" NVDIMM command sets
> (NVDIMM_FAMILY_{INTEL,MSFT,HPE1,HPE2}) that were created before an EFI
> working group was established to constrain this proliferation. The
> nfit driver probes for the supported command set by GUID. Note, If
> you're a platform developer looking to add a new command set to this
> probe consider using an existing set, or otherwise seek approval to
> publish the command set at
> http://www.uefi.org/RFIC_LIST

Looks perfect! Let me use this, rebase the patch to
https://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm.git/tag/?h=libnvdimm-fixes-5.0-rc4
and post a v2 later today.

Thanks,
-- Dexuan