[PATCH v2 0/3] nvdimm: Add an IOCTL pass thru for DSM calls

[PATCH v2 0/3] nvdimm: Add an IOCTL pass thru for DSM calls

[Jerry Hoemann]

The NVDIMM code in the kernel supports an IOCTL interface to user
space based upon the Intel Example DSM:

	http://pmem.io/documents/NVDIMM_DSM_Interface_Example.pdf

This interface cannot be used by other NVDIMM DSMs that support
incompatible functions.

This patch set adds a generic "passthru" IOCTL interface which
is not tied to a particular DSM.

A new _IOC_NR ND_CMD_PASSTHRU == "100" is added for the pass thru call.

The new data structure nd_passthru_pkg serves as a wrapper for the passthru
calls.  This wrapper supplies the data that the kernel needs to
make the _DSM call.

Unlike the definitions of the _DSM functions themselves, the nd_passthru_pkg
provides the calling information (input/output sizes) in an uniform
manner making the kernel marshaling of the arguments straight
forward.

This shifts the marshaling burden from the kernel to the user
space application while still permitting the kernel to internally
call _DSM functions.

To make the resultant kernel code easier to understand the existing
functions acpi_nfit_ctl and __nd_ioctl were renamed to .*_intel to
denote calling mechanism as in 4.3 tailored to the Intel Example DSM.
New functions acpi_nfit_ctl_passthru and __nd_ioctl_passthru were
created to supply the pass thru interface.


These changes are based upon the 4.3 kernel.


Changes in version 2:
---------------------
1. Cleanup access mode check in nd_ioctl and nvdimm_ioctl.
2. Change name of ndn_pkg to nd_passthru_pkg
3. Adjust sizes in nd_passthru_pkg. DSM intergers are 64 bit.
4. No new ioctl type, instead tunnel into the existing number space.
5. Push down one function level where determine ioctl cmd type.
6. re-work diagnostic print/dump message in pass-thru functions.



Jerry Hoemann (3):
  nvdimm: Clean-up access mode check.
  nvdimm: Add wrapper for IOCTL pass thru
  nvdimm: Add IOCTL pass thru functions

 drivers/acpi/nfit.c        |  93 ++++++++++++++++++++++++++++++++++++++++-
 drivers/nvdimm/bus.c       | 101 +++++++++++++++++++++++++++++++++++++++++----
 include/uapi/linux/ndctl.h |  16 +++++++
 3 files changed, 202 insertions(+), 8 deletions(-)

-- 
1.7.11.3

[PATCH v2 1/3] nvdimm: Clean-up access mode check.

[Jerry Hoemann]

Change nd_ioctl and nvdimm_ioctl access mode check to use O_RDONLY.

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

diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index 7e2c43f..1c81203 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -602,14 +602,14 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 static long nd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	long id = (long) file->private_data;
-	int rc = -ENXIO, read_only;
+	int rc = -ENXIO, ro;
 	struct nvdimm_bus *nvdimm_bus;
 
-	read_only = (O_RDWR != (file->f_flags & O_ACCMODE));
+	ro = ((file->f_flags & O_ACCMODE) == O_RDONLY);
 	mutex_lock(&nvdimm_bus_list_mutex);
 	list_for_each_entry(nvdimm_bus, &nvdimm_bus_list, list) {
 		if (nvdimm_bus->id == id) {
-			rc = __nd_ioctl(nvdimm_bus, NULL, read_only, cmd, arg);
+			rc = __nd_ioctl(nvdimm_bus, NULL, ro, cmd, arg);
 			break;
 		}
 	}
@@ -633,10 +633,10 @@ static int match_dimm(struct device *dev, void *data)
 
 static long nvdimm_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
-	int rc = -ENXIO, read_only;
+	int rc = -ENXIO, ro;
 	struct nvdimm_bus *nvdimm_bus;
 
-	read_only = (O_RDWR != (file->f_flags & O_ACCMODE));
+	ro = ((file->f_flags & O_ACCMODE) == O_RDONLY);
 	mutex_lock(&nvdimm_bus_list_mutex);
 	list_for_each_entry(nvdimm_bus, &nvdimm_bus_list, list) {
 		struct device *dev = device_find_child(&nvdimm_bus->dev,
@@ -647,7 +647,7 @@ static long nvdimm_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 			continue;
 
 		nvdimm = to_nvdimm(dev);
-		rc = __nd_ioctl(nvdimm_bus, nvdimm, read_only, cmd, arg);
+		rc = __nd_ioctl(nvdimm_bus, nvdimm, ro, cmd, arg);
 		put_device(dev);
 		break;
 	}
-- 
1.7.11.3

[PATCH v2 2/3] nvdimm: Add wrapper for IOCTL pass thru

[Jerry Hoemann]

Add struct nd_passthru_pkg which serves as a warapper for
the data being passed via a pass thru to a NVDIMM DSM.
This wrapper specifies the extra information in a uniform
manner allowing the kenrel to call a DSM without knowing
specifics of the DSM.

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

diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h
index 5b4a4be..934a49f 100644
--- a/include/uapi/linux/ndctl.h
+++ b/include/uapi/linux/ndctl.h
@@ -109,6 +109,7 @@ enum {
 	ND_CMD_VENDOR_EFFECT_LOG_SIZE = 7,
 	ND_CMD_VENDOR_EFFECT_LOG = 8,
 	ND_CMD_VENDOR = 9,
+	ND_CMD_PASSTHRU = 100,
 };
 
 enum {
@@ -204,4 +205,19 @@ enum ars_masks {
 	ARS_STATUS_MASK = 0x0000FFFF,
 	ARS_EXT_STATUS_SHIFT = 16,
 };
+
+
+struct nd_passthru_pkg {
+	struct {
+		__u8	dsm_uuid[16];
+		__u64	dsm_rev;		/* revision of dsm call  */
+		__u64	dsm_fun_idx;		/* DSM function id       */
+		__u32	dsm_in;			/* size of _DSM input    */
+		__u32	dsm_out;		/* size of user buffer   */
+		__u64	reserved[12];		/* reserved must be zero */
+		__u32	dsm_size;		/* size _DSM would write */
+	} h;
+	unsigned char buf[];
+};
+
 #endif /* __NDCTL_H__ */
-- 
1.7.11.3

[PATCH v2 3/3] nvdimm: Add IOCTL pass thru functions

[Jerry Hoemann]

Add functions acpi_nfit_ctl_passthru and __nd_ioctl_passthru which allow
kernel to call a nvdimm's _DSM as a passthru without the marshaling code
of the nd_cmd_desc.

Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
---
 drivers/acpi/nfit.c  | 93 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 drivers/nvdimm/bus.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 180 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
index c1b8d03..88844af 100644
--- a/drivers/acpi/nfit.c
+++ b/drivers/acpi/nfit.c
@@ -62,7 +62,7 @@ static struct acpi_device *to_acpi_dev(struct acpi_nfit_desc *acpi_desc)
 	return to_acpi_device(acpi_desc->dev);
 }
 
-static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,
+static int acpi_nfit_ctl_intel(struct nvdimm_bus_descriptor *nd_desc,
 		struct nvdimm *nvdimm, unsigned int cmd, void *buf,
 		unsigned int buf_len)
 {
@@ -190,6 +190,97 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,
 	return rc;
 }
 
+
+static int acpi_nfit_ctl_passthru(struct nvdimm_bus_descriptor *nd_desc,
+		struct nvdimm *nvdimm, unsigned int cmd, void *buf,
+		unsigned int buf_len)
+{
+	struct acpi_nfit_desc *acpi_desc = to_acpi_nfit_desc(nd_desc);
+	union acpi_object in_obj, in_buf, *out_obj;
+	struct device *dev = acpi_desc->dev;
+	const char *dimm_name;
+	acpi_handle handle;
+	const u8 *uuid;
+	int rc = 0;
+	__u64 rev = 0, func = 0;
+
+	struct nd_passthru_pkg *pkg = buf;
+
+	if (nvdimm) {
+		struct nfit_mem *nfit_mem = nvdimm_provider_data(nvdimm);
+		struct acpi_device *adev = nfit_mem->adev;
+
+		if (!adev)
+			return -ENOTTY;
+		dimm_name = nvdimm_name(nvdimm);
+		handle = adev->handle;
+	} else {
+		struct acpi_device *adev = to_acpi_dev(acpi_desc);
+
+		handle = adev->handle;
+		dimm_name = "bus";
+	}
+	uuid = pkg->h.dsm_uuid;
+	rev  = pkg->h.dsm_rev;
+	func = pkg->h.dsm_fun_idx;
+
+	in_obj.type = ACPI_TYPE_PACKAGE;
+	in_obj.package.count = 1;
+	in_obj.package.elements = &in_buf;
+	in_buf.type = ACPI_TYPE_BUFFER;
+	in_buf.buffer.pointer = (void *) &pkg->buf;
+
+	in_buf.buffer.length = pkg->h.dsm_in;
+
+	if (IS_ENABLED(CONFIG_ACPI_NFIT_DEBUG))
+		print_hex_dump_debug("nvdimm in  ", DUMP_PREFIX_OFFSET, 4, 4,
+			in_buf.buffer.pointer,
+			min_t(u32, 256, in_buf.buffer.length), true);
+
+	out_obj = acpi_evaluate_dsm(handle, uuid, rev, func, &in_obj);
+	if (!out_obj) {
+		dev_dbg(dev, "%s:%s _DSM failed idx: %llu\n", __func__,
+				dimm_name, func);
+		return -EINVAL;
+	}
+
+	if (out_obj->package.type != ACPI_TYPE_BUFFER) {
+		dev_dbg(dev, "%s:%s unexpected object type: %d type: %d\n",
+				__func__, dimm_name, cmd, out_obj->type);
+		rc = -EINVAL;
+		goto out;
+	}
+
+	if (IS_ENABLED(CONFIG_ACPI_NFIT_DEBUG))
+		print_hex_dump_debug("nvdimm out ", DUMP_PREFIX_OFFSET, 4, 4,
+			out_obj->buffer.pointer,
+			min_t(u32, 256, out_obj->buffer.length), true);
+
+	pkg->h.dsm_size = out_obj->buffer.length;
+	memcpy(pkg->buf + pkg->h.dsm_in,
+			out_obj->buffer.pointer,
+			min(pkg->h.dsm_size, pkg->h.dsm_out));
+
+
+ out:
+	ACPI_FREE(out_obj);
+
+	return rc;
+}
+
+static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,
+		struct nvdimm *nvdimm, unsigned int cmd, void *buf,
+		unsigned int len)
+{
+	switch (cmd) {
+	case ND_CMD_PASSTHRU:
+		return acpi_nfit_ctl_passthru(nd_desc, nvdimm, cmd, buf, len);
+	default:
+		return acpi_nfit_ctl_intel(nd_desc, nvdimm, cmd, buf, len);
+	}
+}
+
+
 static const char *spa_type_name(u16 type)
 {
 	static const char *to_name[] = {
diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index 1c81203..ca7f89c 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -479,7 +479,7 @@ static int nd_cmd_clear_to_send(struct nvdimm *nvdimm, unsigned int cmd)
 	return 0;
 }
 
-static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
+static int __nd_ioctl_intel(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 		int read_only, unsigned int ioctl_cmd, unsigned long arg)
 {
 	struct nvdimm_bus_descriptor *nd_desc = nvdimm_bus->nd_desc;
@@ -599,6 +599,93 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 	return rc;
 }
 
+
+static int __nd_ioctl_passthru(struct nvdimm_bus *nvdimm_bus,
+		struct nvdimm *nvdimm, int read_only, unsigned
+		int ioctl_cmd, unsigned long arg)
+{
+	struct nvdimm_bus_descriptor *nd_desc = nvdimm_bus->nd_desc;
+	size_t buf_len = 0, in_len = 0, out_len = 0;
+	unsigned int cmd = _IOC_NR(ioctl_cmd);
+	void __user *p = (void __user *) arg;
+	struct device *dev = &nvdimm_bus->dev;
+	const char *dimm_name = "";
+	void *buf = NULL;
+	int i, rc;
+	struct nd_passthru_pkg pkg;
+
+	if (nvdimm)
+		dimm_name = dev_name(&nvdimm->dev);
+	else
+		dimm_name = "bus";
+
+	if (copy_from_user(&pkg, p, sizeof(pkg))) {
+		rc = -EFAULT;
+		goto out;
+	}
+
+	/* Caller must tell us size of input to _DSM. */
+	/* This may be bigger that the fixed portion of the pakcage */
+	in_len  = pkg.h.dsm_in;
+	out_len = pkg.h.dsm_out;
+	buf_len = sizeof(pkg.h) + in_len + out_len;
+
+	dev_dbg(dev, "%s:%s rev: %llu, idx: %llu, in: %zu, out: %zu, len %zu\n",
+		__func__, dimm_name,
+		pkg.h.dsm_rev, pkg.h.dsm_fun_idx,
+		in_len, out_len, buf_len);
+
+	for (i = 0; i < ARRAY_SIZE(pkg.h.reserved); i++)
+		if (pkg.h.reserved[i])
+			return -EINVAL;
+
+	if (buf_len > ND_IOCTL_MAX_BUFLEN) {
+		dev_dbg(dev, "%s:%s cmd: %d, idx: %llu buf_len: %zu > %d\n",
+			__func__, dimm_name, cmd, pkg.h.dsm_fun_idx,
+			buf_len, ND_IOCTL_MAX_BUFLEN);
+		return -EINVAL;
+	}
+
+	buf = vmalloc(buf_len);
+	if (!buf)
+		return -ENOMEM;
+
+	if (copy_from_user(buf, p, buf_len)) {
+		rc = -EFAULT;
+		goto out;
+	}
+
+	nvdimm_bus_lock(&nvdimm_bus->dev);
+	rc = nd_cmd_clear_to_send(nvdimm, cmd);
+	if (rc)
+		goto out_unlock;
+
+	rc = nd_desc->ndctl(nd_desc, nvdimm, cmd, buf, buf_len);
+	if (rc < 0)
+		goto out_unlock;
+	if (copy_to_user(p, buf, buf_len))
+		rc = -EFAULT;
+ out_unlock:
+	nvdimm_bus_unlock(&nvdimm_bus->dev);
+ out:
+	vfree(buf);
+	return rc;
+}
+
+static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
+		int ro, unsigned int ioctl_cmd, unsigned long arg)
+
+{
+	unsigned int cmd = _IOC_NR(ioctl_cmd);
+
+	switch (cmd) {
+	case ND_CMD_PASSTHRU:
+		return __nd_ioctl_passthru(nvdimm_bus, nvdimm, ro, ioctl_cmd, arg);
+	default:
+		return __nd_ioctl_intel(nvdimm_bus, nvdimm, ro, ioctl_cmd, arg);
+	}
+}
+
 static long nd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	long id = (long) file->private_data;
-- 
1.7.11.3

Re: [PATCH v2 0/3] nvdimm: Add an IOCTL pass thru for DSM calls

[Dan Williams]

On Mon, Nov 16, 2015 at 10:38 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
>
> The NVDIMM code in the kernel supports an IOCTL interface to user
> space based upon the Intel Example DSM:
>
>         http://pmem.io/documents/NVDIMM_DSM_Interface_Example.pdf
>
> This interface cannot be used by other NVDIMM DSMs that support
> incompatible functions.
>
> This patch set adds a generic "passthru" IOCTL interface which
> is not tied to a particular DSM.
>
> A new _IOC_NR ND_CMD_PASSTHRU == "100" is added for the pass thru call.
>
> The new data structure nd_passthru_pkg serves as a wrapper for the passthru
> calls.  This wrapper supplies the data that the kernel needs to
> make the _DSM call.
>
> Unlike the definitions of the _DSM functions themselves, the nd_passthru_pkg
> provides the calling information (input/output sizes) in an uniform
> manner making the kernel marshaling of the arguments straight
> forward.
>
> This shifts the marshaling burden from the kernel to the user
> space application while still permitting the kernel to internally
> call _DSM functions.
>
> To make the resultant kernel code easier to understand the existing
> functions acpi_nfit_ctl and __nd_ioctl were renamed to .*_intel to
> denote calling mechanism as in 4.3 tailored to the Intel Example DSM.
> New functions acpi_nfit_ctl_passthru and __nd_ioctl_passthru were
> created to supply the pass thru interface.

Let's not do the _intel vs _passthru split.  I want to convert the
existing commands over to this new interface and deprecate the old
ioctl-command formats.  I.e. it isn't the case that this will be a
always be a blind "passthru" mechanism, the kernel will need to crack
open this payload in some circumstances.

Re: [PATCH v2 0/3] nvdimm: Add an IOCTL pass thru for DSM calls

[Jerry Hoemann]

On Mon, Nov 16, 2015 at 11:00:20AM -0800, Dan Williams wrote:
> On Mon, Nov 16, 2015 at 10:38 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> >
> > The NVDIMM code in the kernel supports an IOCTL interface to user
> > space based upon the Intel Example DSM:
> >
> >         http://pmem.io/documents/NVDIMM_DSM_Interface_Example.pdf
> >
> > This interface cannot be used by other NVDIMM DSMs that support
> > incompatible functions.
> >
> > This patch set adds a generic "passthru" IOCTL interface which
> > is not tied to a particular DSM.
> >
> > A new _IOC_NR ND_CMD_PASSTHRU == "100" is added for the pass thru call.
> >
> > The new data structure nd_passthru_pkg serves as a wrapper for the passthru
> > calls.  This wrapper supplies the data that the kernel needs to
> > make the _DSM call.
> >
> > Unlike the definitions of the _DSM functions themselves, the nd_passthru_pkg
> > provides the calling information (input/output sizes) in an uniform
> > manner making the kernel marshaling of the arguments straight
> > forward.
> >
> > This shifts the marshaling burden from the kernel to the user
> > space application while still permitting the kernel to internally
> > call _DSM functions.
> >
> > To make the resultant kernel code easier to understand the existing
> > functions acpi_nfit_ctl and __nd_ioctl were renamed to .*_intel to
> > denote calling mechanism as in 4.3 tailored to the Intel Example DSM.
> > New functions acpi_nfit_ctl_passthru and __nd_ioctl_passthru were
> > created to supply the pass thru interface.
> 
> Let's not do the _intel vs _passthru split.  I want to convert the
> existing commands over to this new interface and deprecate the old
> ioctl-command formats.  I.e. it isn't the case that this will be a
> always be a blind "passthru" mechanism, the kernel will need to crack
> open this payload in some circumstances.


I'm confused.

In this version there is only 1 ioctl 'N'.  The pass thru is using
number 100.  This is what I thought you wanted from prior comments.

The split are for internal functions that deal specifically w/
the argument marshaling code and copy-in/copy-out.  These mechanisms
are different.

I understand that you want to switch over, but don't you (at least for
the time being) need to keep the old marshaling code for the current
use case?   I was assuming a sequence like:
	1. The pass thru code gets submitted.
	2. The current tools are converted over to using the pass thru,
	3. The marshaling code using nd_cmd_in_size etc., would then
		be removed.

Are you wanting to make one big change and not in separate steps?

Now I don't currently have provision for "snooping" out specific functions
in the pass thru flow. The 4.3 code doesn't do that either per se.  It
will do the calls asked of it from the user.

Snooping is a feature that could be added, but since I'm not sure your
plans here, the best I can do at the moment is to put in a hook to
a function that is a  no-op.  The meat of this function to be added
when plans become more firm.  Do you have more specific info here?



-- 

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

Re: [PATCH v2 0/3] nvdimm: Add an IOCTL pass thru for DSM calls

[Dan Williams]

On Mon, Nov 16, 2015 at 1:10 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> On Mon, Nov 16, 2015 at 11:00:20AM -0800, Dan Williams wrote:
>> On Mon, Nov 16, 2015 at 10:38 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
>> >
>> > The NVDIMM code in the kernel supports an IOCTL interface to user
>> > space based upon the Intel Example DSM:
>> >
>> >         http://pmem.io/documents/NVDIMM_DSM_Interface_Example.pdf
>> >
>> > This interface cannot be used by other NVDIMM DSMs that support
>> > incompatible functions.
>> >
>> > This patch set adds a generic "passthru" IOCTL interface which
>> > is not tied to a particular DSM.
>> >
>> > A new _IOC_NR ND_CMD_PASSTHRU == "100" is added for the pass thru call.
>> >
>> > The new data structure nd_passthru_pkg serves as a wrapper for the passthru
>> > calls.  This wrapper supplies the data that the kernel needs to
>> > make the _DSM call.
>> >
>> > Unlike the definitions of the _DSM functions themselves, the nd_passthru_pkg
>> > provides the calling information (input/output sizes) in an uniform
>> > manner making the kernel marshaling of the arguments straight
>> > forward.
>> >
>> > This shifts the marshaling burden from the kernel to the user
>> > space application while still permitting the kernel to internally
>> > call _DSM functions.
>> >
>> > To make the resultant kernel code easier to understand the existing
>> > functions acpi_nfit_ctl and __nd_ioctl were renamed to .*_intel to
>> > denote calling mechanism as in 4.3 tailored to the Intel Example DSM.
>> > New functions acpi_nfit_ctl_passthru and __nd_ioctl_passthru were
>> > created to supply the pass thru interface.
>>
>> Let's not do the _intel vs _passthru split.  I want to convert the
>> existing commands over to this new interface and deprecate the old
>> ioctl-command formats.  I.e. it isn't the case that this will be a
>> always be a blind "passthru" mechanism, the kernel will need to crack
>> open this payload in some circumstances.
>
>
> I'm confused.
>
> In this version there is only 1 ioctl 'N'.  The pass thru is using
> number 100.  This is what I thought you wanted from prior comments.

It is indeed, I like that change.

> The split are for internal functions that deal specifically w/
> the argument marshaling code and copy-in/copy-out.  These mechanisms
> are different.
>
> I understand that you want to switch over, but don't you (at least for
> the time being) need to keep the old marshaling code for the current
> use case?   I was assuming a sequence like:
>         1. The pass thru code gets submitted.
>         2. The current tools are converted over to using the pass thru,
>         3. The marshaling code using nd_cmd_in_size etc., would then
>                 be removed.
>
> Are you wanting to make one big change and not in separate steps?

I want to do it in separate steps, I'd just like to see cmd number 100
added to the existing __nd_ioctl and acpi_nfit_ctl routines.  That
plus quibbling about the name "ND_CMD_PASSTHRU".  Given the plans to
eventually replace the existing commands we can call it something like
'ND_DSM_GENERIC'.

> Now I don't currently have provision for "snooping" out specific functions
> in the pass thru flow. The 4.3 code doesn't do that either per se.  It
> will do the calls asked of it from the user.
>
> Snooping is a feature that could be added, but since I'm not sure your
> plans here, the best I can do at the moment is to put in a hook to
> a function that is a  no-op.  The meat of this function to be added
> when plans become more firm.  Do you have more specific info here?

No need to add the snooping support now, I only made the comment in
reference to the "passthru" name.  We can add snooping support later
after we finalize this initial marshaling code.

Re: [PATCH v2 0/3] nvdimm: Add an IOCTL pass thru for DSM calls

[Jerry Hoemann]

On Mon, Nov 16, 2015 at 05:29:41PM -0800, Dan Williams wrote:
> On Mon, Nov 16, 2015 at 1:10 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> > On Mon, Nov 16, 2015 at 11:00:20AM -0800, Dan Williams wrote:
> >> On Mon, Nov 16, 2015 at 10:38 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> >> >
> >>

...

> >> Let's not do the _intel vs _passthru split.  I want to convert the
> >> existing commands over to this new interface and deprecate the old
> >> ioctl-command formats.  I.e. it isn't the case that this will be a
> >> always be a blind "passthru" mechanism, the kernel will need to crack
> >> open this payload in some circumstances.
> >
> >
> > I'm confused.
> >
> > In this version there is only 1 ioctl 'N'.  The pass thru is using
> > number 100.  This is what I thought you wanted from prior comments.
> 
> It is indeed, I like that change.
> 
> > The split are for internal functions that deal specifically w/
> > the argument marshaling code and copy-in/copy-out.  These mechanisms
> > are different.
> >
> > I understand that you want to switch over, but don't you (at least for
> > the time being) need to keep the old marshaling code for the current
> > use case?   I was assuming a sequence like:
> >         1. The pass thru code gets submitted.
> >         2. The current tools are converted over to using the pass thru,
> >         3. The marshaling code using nd_cmd_in_size etc., would then
> >                 be removed.
> >
> > Are you wanting to make one big change and not in separate steps?
> 
> I want to do it in separate steps, I'd just like to see cmd number 100
> added to the existing __nd_ioctl and acpi_nfit_ctl routines.  That

   Why?

> plus quibbling about the name "ND_CMD_PASSTHRU".  Given the plans to
> eventually replace the existing commands we can call it something like
> 'ND_DSM_GENERIC'.


  No problem.  I'll change the name for ndn_passthru_pkg in a similar fashion.


  Question:	Are you planning to add other CMDs to the IOCTL in the future?
		(eg. ones not directly related to calling _dsm?)

	 	Or, is the ultimate goal to have an IOCTL that supports
		only the generic DSM call?

-- 

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

Re: [PATCH v2 0/3] nvdimm: Add an IOCTL pass thru for DSM calls

[Dan Williams]

On Tue, Nov 17, 2015 at 8:56 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> On Mon, Nov 16, 2015 at 05:29:41PM -0800, Dan Williams wrote:
>> On Mon, Nov 16, 2015 at 1:10 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
>> > On Mon, Nov 16, 2015 at 11:00:20AM -0800, Dan Williams wrote:
>> >> On Mon, Nov 16, 2015 at 10:38 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
>> >> >
>> >>
>
> ...
>
>> >> Let's not do the _intel vs _passthru split.  I want to convert the
>> >> existing commands over to this new interface and deprecate the old
>> >> ioctl-command formats.  I.e. it isn't the case that this will be a
>> >> always be a blind "passthru" mechanism, the kernel will need to crack
>> >> open this payload in some circumstances.
>> >
>> >
>> > I'm confused.
>> >
>> > In this version there is only 1 ioctl 'N'.  The pass thru is using
>> > number 100.  This is what I thought you wanted from prior comments.
>>
>> It is indeed, I like that change.
>>
>> > The split are for internal functions that deal specifically w/
>> > the argument marshaling code and copy-in/copy-out.  These mechanisms
>> > are different.
>> >
>> > I understand that you want to switch over, but don't you (at least for
>> > the time being) need to keep the old marshaling code for the current
>> > use case?   I was assuming a sequence like:
>> >         1. The pass thru code gets submitted.
>> >         2. The current tools are converted over to using the pass thru,
>> >         3. The marshaling code using nd_cmd_in_size etc., would then
>> >                 be removed.
>> >
>> > Are you wanting to make one big change and not in separate steps?
>>
>> I want to do it in separate steps, I'd just like to see cmd number 100
>> added to the existing __nd_ioctl and acpi_nfit_ctl routines.  That
>
>    Why?

Because there's no need for the intel vs passthru distinction, it's
just yet another command.

>> plus quibbling about the name "ND_CMD_PASSTHRU".  Given the plans to
>> eventually replace the existing commands we can call it something like
>> 'ND_DSM_GENERIC'.
>
>
>   No problem.  I'll change the name for ndn_passthru_pkg in a similar fashion.
>
>
>   Question:     Are you planning to add other CMDs to the IOCTL in the future?
>                 (eg. ones not directly related to calling _dsm?)
>
>                 Or, is the ultimate goal to have an IOCTL that supports
>                 only the generic DSM call?

I'm not ruling out the possibility that there may be a non-DSM command
in the future, but I don't see any need for that on the horizon.  Why
would it matter?

Re: [PATCH v2 0/3] nvdimm: Add an IOCTL pass thru for DSM calls

[Jerry Hoemann]

On Tue, Nov 17, 2015 at 09:05:28AM -0800, Dan Williams wrote:
> On Tue, Nov 17, 2015 at 8:56 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> > On Mon, Nov 16, 2015 at 05:29:41PM -0800, Dan Williams wrote:
> >> On Mon, Nov 16, 2015 at 1:10 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> >> > On Mon, Nov 16, 2015 at 11:00:20AM -0800, Dan Williams wrote:
> >> >> On Mon, Nov 16, 2015 at 10:38 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> >> >> >
> >> >>
> >
> > ...
> >
> >> I want to do it in separate steps, I'd just like to see cmd number 100
> >> added to the existing __nd_ioctl and acpi_nfit_ctl routines.  That
> >
> >    Why?
> 
> Because there's no need for the intel vs passthru distinction, it's
> just yet another command.

  Yes and no.  The way the two marshal their arguments in prep for
  the copy in/copy out is different and are not compatible.

  Also, the existing upstream acpi_nfit_ctl does multiple things that
  we don't want done in the "semi" passthru case.

  To accomodate these differences, I implemented in separate functions.
  I can merge the functions together, it will not be clean.

  This approach also creates testing issues I didn't have previously.  I
  was confident w/ code inspection that I wasn't breaking the existing
  usage case.  I will need your help in testing on hardware that I don't
  have access to.

  You expressed a desire to depricate the existing ioctl commands
  and transition to the semi passthru structure.

  What do you anticipate that code looking like?

> 
> >> plus quibbling about the name "ND_CMD_PASSTHRU".  Given the plans to
> >> eventually replace the existing commands we can call it something like
> >> 'ND_DSM_GENERIC'.
> >
> >
> >   No problem.  I'll change the name for ndn_passthru_pkg in a similar fashion.
> >
> >
> >   Question:     Are you planning to add other CMDs to the IOCTL in the future?
> >                 (eg. ones not directly related to calling _dsm?)
> >
> >                 Or, is the ultimate goal to have an IOCTL that supports
> >                 only the generic DSM call?
> 
> I'm not ruling out the possibility that there may be a non-DSM command
> in the future, but I don't see any need for that on the horizon.  Why
> would it matter?

  Neither the existing upstream apci_nfit_ctl nor the semi pass thru
  marshal arguments in a traditional straight forward manner.  So likely
  the marshaling code for any new commands would be different.

  Also, since it doesn't call DSM it wouldn't be doing the evaluate dsm.



-- 

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