Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1678965 > unrolled thread

[PATCH v3 0/7] Enable DSM pass thru for root functions

Back to article view | Back to linux.kernel

Contents

  [PATCH v3 0/7] Enable DSM pass thru for root functions Jerry Hoemann <jerry.hoemann@hpe.com> - 2017-06-30 18:20 +0200
    [PATCH v4 5/6] libnvdimm: New ACPI 6.2 DSM functions Jerry Hoemann <jerry.hoemann@hpe.com> - 2017-06-30 18:20 +0200
    [PATCH v4 6/6] nfit allow override of root dsm mask Jerry Hoemann <jerry.hoemann@hpe.com> - 2017-06-30 18:20 +0200
      Re: [PATCH v4 6/6] nfit allow override of root dsm mask Dan Williams <dan.j.williams@intel.com> - 2017-07-01 06:00 +0200
    [PATCH v4 4/6] acpi, nfit: Show bus_dsm_mask in sysfs Jerry Hoemann <jerry.hoemann@hpe.com> - 2017-06-30 18:20 +0200
    [PATCH v4 3/6] libnvdimm, acpi, nfit: Add bus level dsm mask for pass thru. Jerry Hoemann <jerry.hoemann@hpe.com> - 2017-06-30 18:20 +0200
      Re: [PATCH v4 3/6] libnvdimm, acpi, nfit: Add bus level dsm mask for  pass thru. Dan Williams <dan.j.williams@intel.com> - 2017-07-01 06:00 +0200
        Re: [PATCH v4 3/6] libnvdimm, acpi, nfit: Add bus level dsm mask for  pass thru. Jerry Hoemann <jerry.hoemann@hpe.com> - 2017-07-01 22:00 +0200
          Re: [PATCH v4 3/6] libnvdimm, acpi, nfit: Add bus level dsm mask for  pass thru. Dan Williams <dan.j.williams@intel.com> - 2017-07-01 22:10 +0200
            Re: [PATCH v4 3/6] libnvdimm, acpi, nfit: Add bus level dsm mask for  pass thru. Dan Williams <dan.j.williams@intel.com> - 2017-07-01 22:20 +0200
              Re: [PATCH v4 3/6] libnvdimm, acpi, nfit: Add bus level dsm mask for  pass thru. Jerry Hoemann <jerry.hoemann@hpe.com> - 2017-07-01 22:40 +0200
                Re: [PATCH v4 3/6] libnvdimm, acpi, nfit: Add bus level dsm mask for  pass thru. Dan Williams <dan.j.williams@intel.com> - 2017-07-01 22:50 +0200

#1678965 — [PATCH v3 0/7] Enable DSM pass thru for root functions

The ACPI 6.2 spec added NVDIMM root DSM functions that managibility
and test software need to call.

This patch set enables the calling of root functions DSMs via the
pass thru mechanism.

Changes v4
----------
1. Minor refactoring of patch sequence.

Changes v3
----------
1. Fix checkpatch warnings

2. modify bus_dsm_mask_show to display name "dsm_mask" in sysfs.

3. modified submittal comments.

Changes v2
----------
1. Add bus_dsm_mask to filter root pass thru calls.

2. Add bus_dsm_mask_show to display bus_dsm_mask in sysfs

3. Extend override_dsm_mask to be used for bus_dms_mask also.




Details v1
----------

__nd_ioctl:
Check pass thru functions against nd_cmd_clear_to_send.

acpi_nfit_init_dsms:
Set additional bits in cmd_mask for new functions.

ndctl.h:
Define data structure for the new 6.2 functions.

Jerry Hoemann (6):
  libnvdimm: passthru functions clear to send
  acpi, nfit: Enable DSM pass thru for root functions.
  libnvdimm, acpi, nfit: Add bus level dsm mask for pass thru.
  acpi, nfit: Show bus_dsm_mask in sysfs
  libnvdimm: New ACPI 6.2 DSM functions
  nfit allow override of root dsm mask

 drivers/acpi/nfit/core.c   | 23 +++++++++++++++++++++++
 drivers/nvdimm/bus.c       |  4 +++-
 include/linux/libnvdimm.h  |  1 +
 include/uapi/linux/ndctl.h | 41 ++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 67 insertions(+), 2 deletions(-)

-- 
1.8.5.6

[toc] | [next] | [standalone]

#1678967 — [PATCH v4 5/6] libnvdimm: New ACPI 6.2 DSM functions

ACPI 6.2 added new NVDIMM root DSM functions.  Define their
data structures.

Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
---
 include/uapi/linux/ndctl.h | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h
index e23c37f..e15768f 100644
--- a/include/uapi/linux/ndctl.h
+++ b/include/uapi/linux/ndctl.h
@@ -105,7 +105,8 @@ struct nd_cmd_ars_cap {
 	__u32 status;
 	__u32 max_ars_out;
 	__u32 clear_err_unit;
-	__u32 reserved;
+	__u16 flags;
+	__u16 reserved;
 } __packed;
 
 struct nd_cmd_ars_start {
@@ -144,6 +145,43 @@ struct nd_cmd_clear_error {
 	__u64 cleared;
 } __packed;
 
+struct nd_cmd_trans_spa {
+	__u64 spa;
+	__u32 status;
+	__u8  flags;
+	__u8  _reserved[3];
+	__u64 trans_length;
+	__u32 num_nvdimms;
+	struct nd_nvdimm_device {
+		__u32 nfit_device_handle;
+		__u32 _reserved;
+		__u64 dpa;
+	} __packed devices[0];
+
+} __packed;
+
+struct nd_cmd_ars_err_inj {
+	__u64 err_inj_spa_range_base;
+	__u64 err_inj_spa_range_length;
+	__u8  err_inj_options;
+	__u32 status;
+} __packed;
+
+struct nd_cmd_ars_err_inj_clr {
+	__u64 err_inj_clr_spa_range_base;
+	__u64 err_inj_clr_spa_range_length;
+	__u32 status;
+} __packed;
+
+struct nd_cmd_ars_err_inj_stat {
+	__u32 status;
+	__u32 inj_err_rec_count;
+	struct nd_error_stat_query_record {
+		__u64 err_inj_stat_spa_range_base;
+		__u64 err_inj_stat_spa_range_length;
+	} __packed record[0];
+} __packed;
+
 enum {
 	ND_CMD_IMPLEMENTED = 0,
 
-- 
1.8.5.6

[toc] | [prev] | [next] | [standalone]

#1678972 — [PATCH v4 6/6] nfit allow override of root dsm mask

Have module parameter override_dsm_mask override the dsm_mask for
root calls like it does for non-root dsm calls.

Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
---
 drivers/acpi/nfit/core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 4e24d69..87acaf2 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -1641,6 +1641,8 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
 	set_bit(ND_CMD_CALL, &nd_desc->cmd_mask);
 
 	dsm_mask = 0x3bf;
+	if (override_dsm_mask)
+		dsm_mask = override_dsm_mask;
 	for_each_set_bit(i, &dsm_mask, BITS_PER_LONG)
 		if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
 			set_bit(i, &nd_desc->bus_dsm_mask);
-- 
1.8.5.6

[toc] | [prev] | [next] | [standalone]

#1679261 — Re: [PATCH v4 6/6] nfit allow override of root dsm mask

On Fri, Jun 30, 2017 at 9:09 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> Have module parameter override_dsm_mask override the dsm_mask for
> root calls like it does for non-root dsm calls.
>
> Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> ---
>  drivers/acpi/nfit/core.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 4e24d69..87acaf2 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -1641,6 +1641,8 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
>         set_bit(ND_CMD_CALL, &nd_desc->cmd_mask);
>
>         dsm_mask = 0x3bf;
> +       if (override_dsm_mask)
> +               dsm_mask = override_dsm_mask;
>         for_each_set_bit(i, &dsm_mask, BITS_PER_LONG)
>                 if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
>                         set_bit(i, &nd_desc->bus_dsm_mask);

Another reason not to apply this is to take a look at what happened
with the introduction of _LSI, _LSR, and _LSW. We should not expect
that _DSM will keep being used for future NVDIMM root bus methods.

[toc] | [prev] | [next] | [standalone]

#1678975 — [PATCH v4 4/6] acpi, nfit: Show bus_dsm_mask in sysfs

Display bus_dsm_mask in sysfs as /sys/bus/nd/devices/ndbusX/nfit/dsm_mask.

Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
---
 drivers/acpi/nfit/core.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 5e4c137..4e24d69 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -929,6 +929,17 @@ static int nfit_mem_init(struct acpi_nfit_desc *acpi_desc)
 	return 0;
 }
 
+static ssize_t bus_dsm_mask_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct nvdimm_bus *nvdimm_bus = to_nvdimm_bus(dev);
+	struct nvdimm_bus_descriptor *nd_desc = to_nd_desc(nvdimm_bus);
+
+	return sprintf(buf, "%#lx\n", nd_desc->bus_dsm_mask);
+}
+static struct device_attribute dev_attr_bus_dsm_mask =
+		__ATTR(dsm_mask, 0444, bus_dsm_mask_show, NULL);
+
 static ssize_t revision_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
@@ -1065,6 +1076,7 @@ static umode_t nfit_visible(struct kobject *kobj, struct attribute *a, int n)
 	&dev_attr_revision.attr,
 	&dev_attr_scrub.attr,
 	&dev_attr_hw_error_scrub.attr,
+	&dev_attr_bus_dsm_mask.attr,
 	NULL,
 };
 
-- 
1.8.5.6

[toc] | [prev] | [next] | [standalone]

#1678976 — [PATCH v4 3/6] libnvdimm, acpi, nfit: Add bus level dsm mask for pass thru.

Add a bus level dsm_mask to nvdimm_bus_descriptor to allow the passthru
calling mechanism to specify a different mask from the cmd_mask.

Populate bus_dsm_mask and use it to filter dsm calls that user can
make through the pass thru interface.

Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
---
 drivers/acpi/nfit/core.c  | 8 ++++++++
 include/linux/libnvdimm.h | 1 +
 2 files changed, 9 insertions(+)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index b46fca2..5e4c137 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -253,6 +253,8 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
 		cmd_name = nvdimm_bus_cmd_name(cmd);
 		cmd_mask = nd_desc->cmd_mask;
 		dsm_mask = cmd_mask;
+		if (cmd == ND_CMD_CALL)
+			dsm_mask = nd_desc->bus_dsm_mask;
 		desc = nd_cmd_bus_desc(cmd);
 		uuid = to_nfit_uuid(NFIT_DEV_BUS);
 		handle = adev->handle;
@@ -1613,6 +1615,7 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
 	struct nvdimm_bus_descriptor *nd_desc = &acpi_desc->nd_desc;
 	const u8 *uuid = to_nfit_uuid(NFIT_DEV_BUS);
 	struct acpi_device *adev;
+	unsigned long dsm_mask;
 	int i;
 
 	nd_desc->cmd_mask = acpi_desc->bus_cmd_force_en;
@@ -1624,6 +1627,11 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
 		if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
 			set_bit(i, &nd_desc->cmd_mask);
 	set_bit(ND_CMD_CALL, &nd_desc->cmd_mask);
+
+	dsm_mask = 0x3bf;
+	for_each_set_bit(i, &dsm_mask, BITS_PER_LONG)
+		if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
+			set_bit(i, &nd_desc->bus_dsm_mask);
 }
 
 static ssize_t range_index_show(struct device *dev,
diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
index 6c80701..f8b8f43 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -54,6 +54,7 @@ typedef int (*ndctl_fn)(struct nvdimm_bus_descriptor *nd_desc,
 
 struct nvdimm_bus_descriptor {
 	const struct attribute_group **attr_groups;
+	unsigned long bus_dsm_mask;
 	unsigned long cmd_mask;
 	struct module *module;
 	char *provider_name;
-- 
1.8.5.6

[toc] | [prev] | [next] | [standalone]

#1679262 — Re: [PATCH v4 3/6] libnvdimm, acpi, nfit: Add bus level dsm mask for pass thru.

On Fri, Jun 30, 2017 at 9:09 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> Add a bus level dsm_mask to nvdimm_bus_descriptor to allow the passthru
> calling mechanism to specify a different mask from the cmd_mask.
>
> Populate bus_dsm_mask and use it to filter dsm calls that user can
> make through the pass thru interface.
>
> Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> ---
>  drivers/acpi/nfit/core.c  | 8 ++++++++
>  include/linux/libnvdimm.h | 1 +
>  2 files changed, 9 insertions(+)
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index b46fca2..5e4c137 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -253,6 +253,8 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
>                 cmd_name = nvdimm_bus_cmd_name(cmd);
>                 cmd_mask = nd_desc->cmd_mask;
>                 dsm_mask = cmd_mask;
> +               if (cmd == ND_CMD_CALL)
> +                       dsm_mask = nd_desc->bus_dsm_mask;
>                 desc = nd_cmd_bus_desc(cmd);
>                 uuid = to_nfit_uuid(NFIT_DEV_BUS);
>                 handle = adev->handle;
> @@ -1613,6 +1615,7 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
>         struct nvdimm_bus_descriptor *nd_desc = &acpi_desc->nd_desc;
>         const u8 *uuid = to_nfit_uuid(NFIT_DEV_BUS);
>         struct acpi_device *adev;
> +       unsigned long dsm_mask;
>         int i;
>
>         nd_desc->cmd_mask = acpi_desc->bus_cmd_force_en;
> @@ -1624,6 +1627,11 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
>                 if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
>                         set_bit(i, &nd_desc->cmd_mask);
>         set_bit(ND_CMD_CALL, &nd_desc->cmd_mask);
> +
> +       dsm_mask = 0x3bf;

I went ahead and fixed this up to use dsm_mask defined like this:

+       dsm_mask =
+               (1 << ND_CMD_ARS_CAP) |
+               (1 << ND_CMD_ARS_START) |
+               (1 << ND_CMD_ARS_STATUS) |
+               (1 << ND_CMD_CLEAR_ERROR) |
+               (1 << NFIT_CMD_TRANSLATE_SPA) |
+               (1 << NFIT_CMD_ARS_INJECT_SET) |
+               (1 << NFIT_CMD_ARS_INJECT_CLEAR) |
+               (1 << NFIT_CMD_ARS_INJECT_GET);

This drops function number 0 which userspace has no need to call.

[toc] | [prev] | [next] | [standalone]

#1679393 — Re: [PATCH v4 3/6] libnvdimm, acpi, nfit: Add bus level dsm mask for pass thru.

On Fri, Jun 30, 2017 at 08:55:22PM -0700, Dan Williams wrote:

...

> On Fri, Jun 30, 2017 at 9:09 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> > +               if (cmd == ND_CMD_CALL)
> > +                       dsm_mask = nd_desc->bus_dsm_mask;
> >                 desc = nd_cmd_bus_desc(cmd);
> >                 uuid = to_nfit_uuid(NFIT_DEV_BUS);
> >                 handle = adev->handle;
> > @@ -1613,6 +1615,7 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
> >         struct nvdimm_bus_descriptor *nd_desc = &acpi_desc->nd_desc;
> >         const u8 *uuid = to_nfit_uuid(NFIT_DEV_BUS);
> >         struct acpi_device *adev;
> > +       unsigned long dsm_mask;
> >         int i;
> >
> >         nd_desc->cmd_mask = acpi_desc->bus_cmd_force_en;
> > @@ -1624,6 +1627,11 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
> >                 if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
> >                         set_bit(i, &nd_desc->cmd_mask);
> >         set_bit(ND_CMD_CALL, &nd_desc->cmd_mask);
> > +
> > +       dsm_mask = 0x3bf;
> 
> I went ahead and fixed this up to use dsm_mask defined like this:
> 
> +       dsm_mask =
> +               (1 << ND_CMD_ARS_CAP) |
> +               (1 << ND_CMD_ARS_START) |
> +               (1 << ND_CMD_ARS_STATUS) |
> +               (1 << ND_CMD_CLEAR_ERROR) |
> +               (1 << NFIT_CMD_TRANSLATE_SPA) |
> +               (1 << NFIT_CMD_ARS_INJECT_SET) |
> +               (1 << NFIT_CMD_ARS_INJECT_CLEAR) |
> +               (1 << NFIT_CMD_ARS_INJECT_GET);
> 
> This drops function number 0 which userspace has no need to call.

Actually I like to call function 0.  Its an excellent test when
modifying the code path as its a no side effects function whose output
is known in advance and instantly recognizable.  I also use it when
testing new firmware.  

What is the downside to allowing it?  What bad things happen?

Also, I do have to ask why you allow function zero for NVDIMM_FAMILY_MSFT?
-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------

[toc] | [prev] | [next] | [standalone]

#1679395 — Re: [PATCH v4 3/6] libnvdimm, acpi, nfit: Add bus level dsm mask for pass thru.

On Sat, Jul 1, 2017 at 12:58 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> On Fri, Jun 30, 2017 at 08:55:22PM -0700, Dan Williams wrote:
>
> ...
>
>> On Fri, Jun 30, 2017 at 9:09 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
>> > +               if (cmd == ND_CMD_CALL)
>> > +                       dsm_mask = nd_desc->bus_dsm_mask;
>> >                 desc = nd_cmd_bus_desc(cmd);
>> >                 uuid = to_nfit_uuid(NFIT_DEV_BUS);
>> >                 handle = adev->handle;
>> > @@ -1613,6 +1615,7 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
>> >         struct nvdimm_bus_descriptor *nd_desc = &acpi_desc->nd_desc;
>> >         const u8 *uuid = to_nfit_uuid(NFIT_DEV_BUS);
>> >         struct acpi_device *adev;
>> > +       unsigned long dsm_mask;
>> >         int i;
>> >
>> >         nd_desc->cmd_mask = acpi_desc->bus_cmd_force_en;
>> > @@ -1624,6 +1627,11 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
>> >                 if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
>> >                         set_bit(i, &nd_desc->cmd_mask);
>> >         set_bit(ND_CMD_CALL, &nd_desc->cmd_mask);
>> > +
>> > +       dsm_mask = 0x3bf;
>>
>> I went ahead and fixed this up to use dsm_mask defined like this:
>>
>> +       dsm_mask =
>> +               (1 << ND_CMD_ARS_CAP) |
>> +               (1 << ND_CMD_ARS_START) |
>> +               (1 << ND_CMD_ARS_STATUS) |
>> +               (1 << ND_CMD_CLEAR_ERROR) |
>> +               (1 << NFIT_CMD_TRANSLATE_SPA) |
>> +               (1 << NFIT_CMD_ARS_INJECT_SET) |
>> +               (1 << NFIT_CMD_ARS_INJECT_CLEAR) |
>> +               (1 << NFIT_CMD_ARS_INJECT_GET);
>>
>> This drops function number 0 which userspace has no need to call.
>
> Actually I like to call function 0.  Its an excellent test when
> modifying the code path as its a no side effects function whose output
> is known in advance and instantly recognizable.  I also use it when
> testing new firmware.
>
> What is the downside to allowing it?  What bad things happen?

It allows implementations to bypass the standardization process and
ship new root DSMs. It's always possible to patch the kernel locally
for development, so I see no reason to ship this capability globally.

> Also, I do have to ask why you allow function zero for NVDIMM_FAMILY_MSFT?

Yeah, that's an oversight / mistake, but it's also benign since it
can't be used to add support for new function numbers to the family
since all 32 numbers are already taken. We also allow override for
leaf devices since there's quite a bit more per vendor differentiation
that might take a while to standardize.

[toc] | [prev] | [next] | [standalone]

#1679396 — Re: [PATCH v4 3/6] libnvdimm, acpi, nfit: Add bus level dsm mask for pass thru.

On Sat, Jul 1, 2017 at 1:08 PM, Dan Williams <dan.j.williams@intel.com> wrote:
> On Sat, Jul 1, 2017 at 12:58 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
>> On Fri, Jun 30, 2017 at 08:55:22PM -0700, Dan Williams wrote:
>>
>> ...
>>
>>> On Fri, Jun 30, 2017 at 9:09 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
>>> > +               if (cmd == ND_CMD_CALL)
>>> > +                       dsm_mask = nd_desc->bus_dsm_mask;
>>> >                 desc = nd_cmd_bus_desc(cmd);
>>> >                 uuid = to_nfit_uuid(NFIT_DEV_BUS);
>>> >                 handle = adev->handle;
>>> > @@ -1613,6 +1615,7 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
>>> >         struct nvdimm_bus_descriptor *nd_desc = &acpi_desc->nd_desc;
>>> >         const u8 *uuid = to_nfit_uuid(NFIT_DEV_BUS);
>>> >         struct acpi_device *adev;
>>> > +       unsigned long dsm_mask;
>>> >         int i;
>>> >
>>> >         nd_desc->cmd_mask = acpi_desc->bus_cmd_force_en;
>>> > @@ -1624,6 +1627,11 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
>>> >                 if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
>>> >                         set_bit(i, &nd_desc->cmd_mask);
>>> >         set_bit(ND_CMD_CALL, &nd_desc->cmd_mask);
>>> > +
>>> > +       dsm_mask = 0x3bf;
>>>
>>> I went ahead and fixed this up to use dsm_mask defined like this:
>>>
>>> +       dsm_mask =
>>> +               (1 << ND_CMD_ARS_CAP) |
>>> +               (1 << ND_CMD_ARS_START) |
>>> +               (1 << ND_CMD_ARS_STATUS) |
>>> +               (1 << ND_CMD_CLEAR_ERROR) |
>>> +               (1 << NFIT_CMD_TRANSLATE_SPA) |
>>> +               (1 << NFIT_CMD_ARS_INJECT_SET) |
>>> +               (1 << NFIT_CMD_ARS_INJECT_CLEAR) |
>>> +               (1 << NFIT_CMD_ARS_INJECT_GET);
>>>
>>> This drops function number 0 which userspace has no need to call.
>>
>> Actually I like to call function 0.  Its an excellent test when
>> modifying the code path as its a no side effects function whose output
>> is known in advance and instantly recognizable.  I also use it when
>> testing new firmware.
>>
>> What is the downside to allowing it?  What bad things happen?
>
> It allows implementations to bypass the standardization process and
> ship new root DSMs. It's always possible to patch the kernel locally
> for development, so I see no reason to ship this capability globally.

Actually, just the discovery portion does not lead to this leak, but
it's redundant when we have the 'dsm_mask' sysfs attribute.

[toc] | [prev] | [next] | [standalone]

#1679397 — Re: [PATCH v4 3/6] libnvdimm, acpi, nfit: Add bus level dsm mask for pass thru.

On Sat, Jul 01, 2017 at 01:10:31PM -0700, Dan Williams wrote:
> On Sat, Jul 1, 2017 at 1:08 PM, Dan Williams <dan.j.williams@intel.com> wrote:
> > On Sat, Jul 1, 2017 at 12:58 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> >> On Fri, Jun 30, 2017 at 08:55:22PM -0700, Dan Williams wrote:
> >>
> >> ...
> >>
> >>> On Fri, Jun 30, 2017 at 9:09 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> >>> > +               if (cmd == ND_CMD_CALL)
> >>> > +                       dsm_mask = nd_desc->bus_dsm_mask;
> >>> >                 desc = nd_cmd_bus_desc(cmd);
> >>> >                 uuid = to_nfit_uuid(NFIT_DEV_BUS);
> >>> >                 handle = adev->handle;
> >>> > @@ -1613,6 +1615,7 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
> >>> >         struct nvdimm_bus_descriptor *nd_desc = &acpi_desc->nd_desc;
> >>> >         const u8 *uuid = to_nfit_uuid(NFIT_DEV_BUS);
> >>> >         struct acpi_device *adev;
> >>> > +       unsigned long dsm_mask;
> >>> >         int i;
> >>> >
> >>> >         nd_desc->cmd_mask = acpi_desc->bus_cmd_force_en;
> >>> > @@ -1624,6 +1627,11 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
> >>> >                 if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
> >>> >                         set_bit(i, &nd_desc->cmd_mask);
> >>> >         set_bit(ND_CMD_CALL, &nd_desc->cmd_mask);
> >>> > +
> >>> > +       dsm_mask = 0x3bf;
> >>>
> >>> I went ahead and fixed this up to use dsm_mask defined like this:
> >>>
> >>> +       dsm_mask =
> >>> +               (1 << ND_CMD_ARS_CAP) |
> >>> +               (1 << ND_CMD_ARS_START) |
> >>> +               (1 << ND_CMD_ARS_STATUS) |
> >>> +               (1 << ND_CMD_CLEAR_ERROR) |
> >>> +               (1 << NFIT_CMD_TRANSLATE_SPA) |
> >>> +               (1 << NFIT_CMD_ARS_INJECT_SET) |
> >>> +               (1 << NFIT_CMD_ARS_INJECT_CLEAR) |
> >>> +               (1 << NFIT_CMD_ARS_INJECT_GET);
> >>>
> >>> This drops function number 0 which userspace has no need to call.
> >>
> >> Actually I like to call function 0.  Its an excellent test when
> >> modifying the code path as its a no side effects function whose output
> >> is known in advance and instantly recognizable.  I also use it when
> >> testing new firmware.
> >>
> >> What is the downside to allowing it?  What bad things happen?
> >
> > It allows implementations to bypass the standardization process and
> > ship new root DSMs. It's always possible to patch the kernel locally
> > for development, so I see no reason to ship this capability globally.

I don't understand this comment, but I think your next comment 
essentially says to disregard this comment?

> 
> Actually, just the discovery portion does not lead to this leak, but
> it's redundant when we have the 'dsm_mask' sysfs attribute.

No.  The generation of the mask in sysfs is not done by
executing the code in acpi_nfit_ctl.  One of the reasons I call
function 0 to test changes I am making to the ioctl path itself.
The sysfs has nothing to do with that path and cannot be used
to serve this purpose.

And since the content of sysfs has been edited it also can not be
used as a basic test of firmware.

What is the downside to allowing the calling of function 0?



-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------

[toc] | [prev] | [next] | [standalone]

#1679398 — Re: [PATCH v4 3/6] libnvdimm, acpi, nfit: Add bus level dsm mask for pass thru.

On Sat, Jul 1, 2017 at 1:38 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> On Sat, Jul 01, 2017 at 01:10:31PM -0700, Dan Williams wrote:
>> On Sat, Jul 1, 2017 at 1:08 PM, Dan Williams <dan.j.williams@intel.com> wrote:
>> > On Sat, Jul 1, 2017 at 12:58 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
>> >> On Fri, Jun 30, 2017 at 08:55:22PM -0700, Dan Williams wrote:
>> >>
>> >> ...
>> >>
>> >>> On Fri, Jun 30, 2017 at 9:09 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
>> >>> > +               if (cmd == ND_CMD_CALL)
>> >>> > +                       dsm_mask = nd_desc->bus_dsm_mask;
>> >>> >                 desc = nd_cmd_bus_desc(cmd);
>> >>> >                 uuid = to_nfit_uuid(NFIT_DEV_BUS);
>> >>> >                 handle = adev->handle;
>> >>> > @@ -1613,6 +1615,7 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
>> >>> >         struct nvdimm_bus_descriptor *nd_desc = &acpi_desc->nd_desc;
>> >>> >         const u8 *uuid = to_nfit_uuid(NFIT_DEV_BUS);
>> >>> >         struct acpi_device *adev;
>> >>> > +       unsigned long dsm_mask;
>> >>> >         int i;
>> >>> >
>> >>> >         nd_desc->cmd_mask = acpi_desc->bus_cmd_force_en;
>> >>> > @@ -1624,6 +1627,11 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
>> >>> >                 if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
>> >>> >                         set_bit(i, &nd_desc->cmd_mask);
>> >>> >         set_bit(ND_CMD_CALL, &nd_desc->cmd_mask);
>> >>> > +
>> >>> > +       dsm_mask = 0x3bf;
>> >>>
>> >>> I went ahead and fixed this up to use dsm_mask defined like this:
>> >>>
>> >>> +       dsm_mask =
>> >>> +               (1 << ND_CMD_ARS_CAP) |
>> >>> +               (1 << ND_CMD_ARS_START) |
>> >>> +               (1 << ND_CMD_ARS_STATUS) |
>> >>> +               (1 << ND_CMD_CLEAR_ERROR) |
>> >>> +               (1 << NFIT_CMD_TRANSLATE_SPA) |
>> >>> +               (1 << NFIT_CMD_ARS_INJECT_SET) |
>> >>> +               (1 << NFIT_CMD_ARS_INJECT_CLEAR) |
>> >>> +               (1 << NFIT_CMD_ARS_INJECT_GET);
>> >>>
>> >>> This drops function number 0 which userspace has no need to call.
>> >>
>> >> Actually I like to call function 0.  Its an excellent test when
>> >> modifying the code path as its a no side effects function whose output
>> >> is known in advance and instantly recognizable.  I also use it when
>> >> testing new firmware.
>> >>
>> >> What is the downside to allowing it?  What bad things happen?
>> >
>> > It allows implementations to bypass the standardization process and
>> > ship new root DSMs. It's always possible to patch the kernel locally
>> > for development, so I see no reason to ship this capability globally.
>
> I don't understand this comment, but I think your next comment
> essentially says to disregard this comment?

Yes, sorry.

>> Actually, just the discovery portion does not lead to this leak, but
>> it's redundant when we have the 'dsm_mask' sysfs attribute.
>
> No.  The generation of the mask in sysfs is not done by
> executing the code in acpi_nfit_ctl.  One of the reasons I call
> function 0 to test changes I am making to the ioctl path itself.
> The sysfs has nothing to do with that path and cannot be used
> to serve this purpose.
>
> And since the content of sysfs has been edited it also can not be
> used as a basic test of firmware.
>
> What is the downside to allowing the calling of function 0?

It needlessly expands the kernel ABI. I would suggest, if you want to
test acpi_nfit_ctl() path changes, expand the existing test
infrastructure we have in nfit_ctl_test(). If you want to test
firmware you don't need the upstream kernel to carry firmware debug
enabling in the production path, but I would support expanding
tools/testing/nvdimm/ to make it easier to test firmware.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web