Re: [RFC v9 4/5] nvdimm: Add concept of cmd mask

From: Dan Williams <dan.j.williams@intel.com>
To: Jerry Hoemann <Jerry.Hoemann@hpe.com>
Cc: "linux-nvdimm@lists.01.org" <linux-nvdimm@lists.01.org>
Subject: Re: [RFC v9 4/5] nvdimm: Add concept of cmd mask
Date: Thu, 21 Apr 2016 11:25:37 -0700	[thread overview]
Message-ID: <CAPcyv4gt9_KbhYJb0RQ0_Pd8wx98HH6u0_Z+zy25f=wj5F8FHA@mail.gmail.com> (raw)
In-Reply-To: <20160421172825.GB115325@tevye.fc.hp.com>

On Thu, Apr 21, 2016 at 10:28 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> On Mon, Apr 18, 2016 at 08:03:14PM -0700, Dan Williams wrote:
>> On Sun, Apr 17, 2016 at 4:38 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
>
> ...
>
>
>> >  struct cmd_family_tbl nfit_cmd_family_tbl[] = {
>> > -       { NFIT_DEV_DIMM,        ND_TYPE_DIMM_INTEL1,    1},
>> > -       { NFIT_DEV_DIMM_N_HPE1, ND_TYPE_DIMM_N_HPE1,    1},
>> > -       { NFIT_DEV_DIMM_N_HPE2, ND_TYPE_DIMM_N_HPE2,    1},
>> > +       { NFIT_DEV_DIMM,        ND_TYPE_DIMM_INTEL1,    1, 1},
>> > +       { NFIT_DEV_DIMM_N_HPE1, ND_TYPE_DIMM_N_HPE1,    1, 0},
>> > +       { NFIT_DEV_DIMM_N_HPE2, ND_TYPE_DIMM_N_HPE2,    1, 0},
>>
>> Lets replace does_ioctl with a mask of the valid function numbers see below...
>>
>
> Is this new mask to be used in the construction of cmd_mask or in a bit
> and with dsm_mask?
>

The latter, "and with dsm_mask".

>  ....
>
>> > +static int determine_cmd_mask(struct cmd_family_tbl *tbl, u8 const *cmd_uuid)
>> > +{
>> > +       int i;
>> > +
>> > +       for (i = 0;  tbl[i].key_uuid >= 0 ; i++) {
>> > +               const u8 *uuid = to_nfit_uuid(tbl[i].key_uuid);
>> > +
>> > +               if (memcmp(uuid, cmd_uuid, 16) == 0)
>> > +                       return tbl[i].does_ioctls;
>> > +       }
>> > +       return 0;
>> > +}
>> > +
>> >
>> >  static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
>> >                 struct nfit_mem *nfit_mem, u32 device_handle)
>> > @@ -1027,6 +1041,10 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
>> >                 if (acpi_check_dsm(adev_dimm->handle, uuid, 1, 1ULL << i))
>> >                         set_bit(i, &nfit_mem->dsm_mask);
>>
>> Now that there is no longer a 1:1 relationship with the commands the
>> kernel knows and the DSM functions reported via acpi_check_dsm(), I
>> want the kernel to mask off unknown function numbers in dsm_mask.  At
>
> The dsm_mask is generated by what firmware reports it supports.
>
> Are you concerned with FW returning back bogus data?

I'm primarily concerned with vendors developing undocumented commands
without updating the kernel, and probing for 64 commands per-dimm when
we know only 9 commands are valid, for example.

>> a minimum we should be doing input validation on the publicly
>> documented commands.
>
> cmd_msk is used by __nd_ioctl to verify calls function numbers
> are are valid (but not the arguments to the function.)
>
>>
>> > +       nfit_mem->cmd_mask = (1 << ND_CMD_CALL);
>> > +       if (determine_cmd_mask(nfit_cmd_family_tbl, uuid))
>> > +               nfit_mem->cmd_mask |= nfit_mem->dsm_mask;
>> > +
>>
>> For now just have an open coded if ND_TYPE_DIMM_INTEL1 cmd_mask ==
>> dsm_mask.  Later I do want to have a translation routine to convert
>> something like ND_TYPE_DIMM_N_HPE1 Function 2 (Get SMART) output into
>> the ND_CMD_SMART format.  Its ridiculous that we have two commands
>> that do slightly different things.
>
> I am not familiar with the phrase "open coded."  Googling variations of
> the phrase suggests in-lining, that doesn't help me.
>
> For ND_TYPE_DIMM_INTEL1, cmd_mask should never equal dsm_mask as
> cmd_mask should include (1 << ND_CMD_CALL) where dsm_mask shouldn't. Or,
> are you planning to add functions to the Intel DSM that would collide
> with the value of (1 << ND_CMD_CALL)?  Do we need to change value
> of ND_CMD_CALL to avoid a collision?
>
> Now if what you're wanting is determine_cmd_mask returning back
> a statically defined cmd mask such that we can do:
>
>         nfit_mem->cmd_mask = determine_cmd_mask( ... );
>
> And the values of these per UUID statically defined mask is what I am dynamically
> determining in the current version of my patch (e.g. a 7(?) line change)
> that is doable.

We still need the dynamic step to check which of the possible commands
in the 'family' are implemented by the given dimm.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm