| Message ID | PU1P153MB0169DF967C5BAAC8334FCC42BF990@PU1P153MB0169.APCP153.PROD.OUTLOOK.COM (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | nfit: add Hyper-V NVDIMM DSM command set to white list | expand |
From: Dexuan Cui <decui@microsoft.com> 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 <decui@microsoft.com> > --- > > 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 <mikelley@microsoft.com>
Hi Dexuan, Looks good. Just one update request and a note below... On Wed, Jan 23, 2019 at 12:51 PM Dexuan Cui <decui@microsoft.com> 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 <decui@microsoft.com> > --- > > 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.
> From: Dan Williams <dan.j.williams@intel.com> > 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 <decui@microsoft.com> 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
On Mon, Jan 28, 2019 at 1:40 PM Dexuan Cui <decui@microsoft.com> wrote: > > > From: Dan Williams <dan.j.williams@intel.com> > > 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 <decui@microsoft.com> 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.
> From: Dan Williams <dan.j.williams@intel.com> > Sent: Monday, January 28, 2019 1:55 PM > > On Mon, Jan 28, 2019 at 1:40 PM Dexuan Cui <decui@microsoft.com> 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
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)
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 <decui@microsoft.com> --- 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(-)