Patchwork nfit: add Hyper-V NVDIMM DSM command set to white list

login
register
mail settings
Submitter Dexuan Cui
Date Jan. 28, 2019, 9:39 p.m.
Message ID <PU1P153MB0169CE6B02AE7D67E25DDD89BF960@PU1P153MB0169.APCP153.PROD.OUTLOOK.COM>
Download mbox | patch
Permalink /patch/711641/
State New
Headers show

Comments

Dexuan Cui - Jan. 28, 2019, 9:39 p.m.
> 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.


 
> > @@ -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
Dan Williams - Jan. 28, 2019, 9:54 p.m.
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.
Dexuan Cui - Jan. 28, 2019, 10:58 p.m.
> 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

Patch

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