From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: Ben Widawsky <ben.widawsky@intel.com>
Cc: <linux-cxl@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
<linux-pci@vger.kernel.org>,
"linux-acpi@vger.kernel.org, Ira Weiny" <ira.weiny@intel.com>,
Dan Williams <dan.j.williams@intel.com>,
"Vishal Verma" <vishal.l.verma@intel.com>,
"Kelley, Sean V" <sean.v.kelley@intel.com>,
Rafael Wysocki <rafael.j.wysocki@intel.com>,
"Bjorn Helgaas" <helgaas@kernel.org>,
Jon Masters <jcm@jonmasters.org>,
Chris Browy <cbrowy@avery-design.com>,
Randy Dunlap <rdunlap@infradead.org>,
"Christoph Hellwig" <hch@infradead.org>,
<daniel.lll@alibaba-inc.com>
Subject: Re: [RFC PATCH v3 10/16] cxl/mem: Add send command
Date: Thu, 14 Jan 2021 17:10:38 +0000 [thread overview]
Message-ID: <20210114171038.00003636@Huawei.com> (raw)
In-Reply-To: <20210111225121.820014-11-ben.widawsky@intel.com>
On Mon, 11 Jan 2021 14:51:14 -0800
Ben Widawsky <ben.widawsky@intel.com> wrote:
> The send command allows userspace to issue mailbox commands directly to
> the hardware. The driver will verify basic properties of the command and
> possible inspect the input (or output) payload to determine whether or
> not the command is allowed (or might taint the kernel).
>
> The list of allowed commands and their properties can be determined by
> using the QUERY IOCTL for CXL memory devices.
>
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> ---
> drivers/cxl/mem.c | 204 ++++++++++++++++++++++++++++++++++-
> include/uapi/linux/cxl_mem.h | 39 +++++++
> 2 files changed, 239 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index d4eb3f5b9469..f979788b4d9f 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -84,6 +84,13 @@ static DEFINE_IDR(cxl_mem_idr);
> /* protect cxl_mem_idr allocations */
> static DEFINE_MUTEX(cxl_memdev_lock);
>
> +#undef C
> +#define C(a, b) { b }
I'm not following why this is here?
> +static struct {
> + const char *name;
> +} command_names[] = { CMDS };
> +#undef C
> +
> #define CXL_CMD(_id, _flags, sin, sout, f) \
> [CXL_MEM_COMMAND_ID_##_id] = { \
> { \
...
> +
> +/**
> + * handle_mailbox_cmd_from_user() - Dispatch a mailbox command.
> + * @cxlmd: The CXL memory device to communicate with.
> + * @cmd: The validated command.
> + * @in_payload: Pointer to userspace's input payload.
> + * @out_payload: Pointer to userspace's output payload.
> + * @u: The command submitted by userspace. Has output fields.
> + *
> + * Return:
> + * * %0 - Mailbox transaction succeeded.
> + * * %-EFAULT - Something happened with copy_to/from_user.
> + * * %-EINTR - Mailbox acquisition interrupted.
> + * * %-E2BIG - Output payload would overrun buffer.
> + *
> + * Creates the appropriate mailbox command on behalf of a userspace request.
> + * Return value, size, and output payload are all copied out to @u. The
> + * parameters for the command must be validated before calling this function.
> + *
> + * A 0 return code indicates the command executed successfully, not that it was
> + * itself successful. IOW, the retval should always be checked if wanting to
cmd->retval perhaps to be more explicit?
> + * determine the actual result.
> + */
> +static int handle_mailbox_cmd_from_user(struct cxl_memdev *cxlmd,
> + const struct cxl_mem_command *cmd,
> + u64 in_payload,
> + u64 out_payload,
> + struct cxl_send_command __user *u)
> +{
> + struct mbox_cmd mbox_cmd = {
> + .opcode = cmd->opcode,
> + .size_in = cmd->info.size_in,
> + .payload = NULL, /* Copied by copy_to|from_user() */
> + };
> + int rc;
> +
> + if (cmd->info.size_in) {
> + /*
> + * Directly copy the userspace payload into the hardware. UAPI
> + * states that the buffer must already be little endian.
> + */
> + if (copy_from_user((__force void *)cxl_payload_regs(cxlmd->cxlm),
> + u64_to_user_ptr(in_payload),
> + cmd->info.size_in)) {
> + cxl_mem_mbox_put(cxlmd->cxlm);
mbox_get is after this point though it shouldn't be given we just
wrote into the mbox registers.
This seems unlikely to be a high performance path, so perhaps just
use a local buffer and let cxl_mem_mbox_send_cmd copy it into the registers.
> + return -EFAULT;
> + }
> + }
> +
> + rc = cxl_mem_mbox_get(cxlmd->cxlm, true);
> + if (rc)
> + return rc;
> +
> + dev_dbg(&cxlmd->dev,
> + "Submitting %s command for user\n"
> + "\topcode: %x\n"
> + "\tsize: %ub\n",
> + command_names[cmd->info.id].name, mbox_cmd.opcode,
> + cmd->info.size_in);
> +
> + rc = cxl_mem_mbox_send_cmd(cxlmd->cxlm, &mbox_cmd);
> + cxl_mem_mbox_put(cxlmd->cxlm);
> + if (rc)
> + return rc;
> +
> + if (mbox_cmd.size_out > cmd->info.size_out)
> + return -E2BIG;
> +
> + rc = put_user(mbox_cmd.return_code, &u->retval);
> + if (rc)
> + return rc;
> +
> + rc = put_user(mbox_cmd.size_out, &u->size_out);
> + if (rc)
> + return rc;
> +
> + if (mbox_cmd.size_out)
> + if (copy_to_user(u64_to_user_ptr(out_payload),
> + (__force void *)cxl_payload_regs(cxlmd->cxlm),
> + mbox_cmd.size_out))
> + return -EFAULT;
> +
> + return 0;
> +}
> +
...
>
> static long cxl_mem_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> @@ -479,6 +644,37 @@ static long cxl_mem_ioctl(struct file *file, unsigned int cmd, unsigned long arg
> if (j == n_commands)
> break;
> }
> +
> + return 0;
Ah. That should have been in the earlier patch. Explains why the code works :)
> + } else if (cmd == CXL_MEM_SEND_COMMAND) {
> + struct cxl_send_command send, __user *u = (void __user *)arg;
> + struct cxl_mem_command c;
> + int rc;
> +
> + dev_dbg(dev, "Send IOCTL\n");
> +
> + if (copy_from_user(&send, u, sizeof(send)))
> + return -EFAULT;
> +
> + rc = device_lock_interruptible(dev);
> + if (rc)
> + return rc;
> +
> + if (!get_live_device(dev)) {
> + device_unlock(dev);
> + return -ENXIO;
> + }
> +
> + rc = cxl_validate_cmd_from_user(cxlmd->cxlm, &send, &c);
> + if (!rc)
> + rc = handle_mailbox_cmd_from_user(cxlmd, &c,
> + send.in_payload,
> + send.out_payload, u);
> +
> + put_device(dev);
> + device_unlock(dev);
> +
> + return rc;
> }
>
> return -ENOTTY;
> @@ -837,7 +1033,7 @@ static int cxl_mem_identify(struct cxl_mem *cxlm)
> int rc;
>
> /* Retrieve initial device memory map */
> - rc = cxl_mem_mbox_get(cxlm);
> + rc = cxl_mem_mbox_get(cxlm, false);
> if (rc)
> return rc;
>
> diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h
> index 847f825bbe18..cb4e2bee5228 100644
> --- a/include/uapi/linux/cxl_mem.h
> +++ b/include/uapi/linux/cxl_mem.h
> @@ -26,6 +26,7 @@ extern "C" {
> */
>
> #define CXL_MEM_QUERY_COMMANDS _IOR(0xCE, 1, struct cxl_mem_query_commands)
> +#define CXL_MEM_SEND_COMMAND _IOWR(0xCE, 2, struct cxl_send_command)
>
> #undef CMDS
> #define CMDS \
> @@ -69,6 +70,7 @@ struct cxl_command_info {
> #define CXL_MEM_COMMAND_FLAG_NONE 0
> #define CXL_MEM_COMMAND_FLAG_KERNEL BIT(0)
> #define CXL_MEM_COMMAND_FLAG_MUTEX BIT(1)
> +#define CXL_MEM_COMMAND_FLAG_MASK GENMASK(31, 2)
Instinctively I'd expect FLAG_MASK to be GENMASK(1, 0)
and to be used as ~FLAG_MASK. As it's mask of flags, not
the mask to leave only valid flags.
>
> __s32 size_in;
> __s32 size_out;
> @@ -110,6 +112,43 @@ struct cxl_mem_query_commands {
> struct cxl_command_info __user commands[]; /* out: supported commands */
> };
>
> +/**
> + * struct cxl_send_command - Send a command to a memory device.
> + * @id: The command to send to the memory device. This must be one of the
> + * commands returned by the query command.
> + * @flags: Flags for the command (input).
> + * @rsvd: Must be zero.
> + * @retval: Return value from the memory device (output).
> + * @size_in: Size of the payload to provide to the device (input).
> + * @size_out: Size of the payload received from the device (input/output). This
> + * field is filled in by userspace to let the driver know how much
> + * space was allocated for output. It is populated by the driver to
> + * let userspace know how large the output payload actually was.
> + * @in_payload: Pointer to memory for payload input (little endian order).
> + * @out_payload: Pointer to memory for payload output (little endian order).
> + *
> + * Mechanism for userspace to send a command to the hardware for processing. The
> + * driver will do basic validation on the command sizes, but the payload input
> + * and output are not introspected. Userspace is required to allocate large
> + * enough buffers for max(size_in, size_out).
That sounds like both buffers must be the maximum between size_in and size_out.
Is intent that this is the maximum size_in for in_payload and max(size_out) for out_payload?
> + */
> +struct cxl_send_command {
> + __u32 id;
> + __u32 flags;
> + __u32 rsvd;
> + __u32 retval;
> +
> + struct {
> + __s32 size_in;
> + __u64 in_payload;
> + };
> +
> + struct {
> + __s32 size_out;
> + __u64 out_payload;
> + };
> +};
> +
> #if defined(__cplusplus)
> }
> #endif
next prev parent reply other threads:[~2021-01-14 17:12 UTC|newest]
Thread overview: 61+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-11 22:51 [RFC PATCH v3 00/16] CXL 2.0 Support Ben Widawsky
2021-01-11 22:51 ` [RFC PATCH v3 01/16] docs: cxl: Add basic documentation Ben Widawsky
2021-01-11 22:51 ` [RFC PATCH v3 02/16] cxl/acpi: Add an acpi_cxl module for the CXL interconnect Ben Widawsky
2021-01-12 7:08 ` Randy Dunlap
2021-01-12 18:43 ` Jonathan Cameron
2021-01-12 19:43 ` Dan Williams
2021-01-12 22:06 ` Jonathan Cameron
2021-01-13 17:55 ` Kaneda, Erik
2021-01-20 19:27 ` Dan Williams
2021-01-20 19:18 ` Verma, Vishal L
2021-01-13 12:40 ` Rafael J. Wysocki
2021-01-20 19:21 ` Verma, Vishal L
2021-01-11 22:51 ` [RFC PATCH v3 03/16] cxl/acpi: add OSC support Ben Widawsky
2021-01-12 15:09 ` Rafael J. Wysocki
2021-01-12 18:48 ` Jonathan Cameron
2021-01-11 22:51 ` [RFC PATCH v3 04/16] cxl/mem: Introduce a driver for CXL-2.0-Type-3 endpoints Ben Widawsky
2021-01-12 7:08 ` Randy Dunlap
2021-01-12 19:01 ` Jonathan Cameron
2021-01-12 20:06 ` Dan Williams
2021-01-11 22:51 ` [RFC PATCH v3 05/16] cxl/mem: Map memory device registers Ben Widawsky
2021-01-12 19:13 ` Jonathan Cameron
2021-01-12 19:21 ` Ben Widawsky
2021-01-12 20:40 ` Dan Williams
2021-01-11 22:51 ` [RFC PATCH v3 06/16] cxl/mem: Find device capabilities Ben Widawsky
2021-01-12 19:17 ` Jonathan Cameron
2021-01-12 19:22 ` Ben Widawsky
2021-01-11 22:51 ` [RFC PATCH v3 07/16] cxl/mem: Implement polled mode mailbox Ben Widawsky
2021-01-13 18:26 ` Jonathan Cameron
2021-01-14 17:40 ` Jonathan Cameron
2021-01-14 17:50 ` Ben Widawsky
2021-01-14 18:13 ` Jonathan Cameron
2021-01-11 22:51 ` [RFC PATCH v3 08/16] cxl/mem: Register CXL memX devices Ben Widawsky
2021-01-14 16:28 ` Jonathan Cameron
2021-01-11 22:51 ` [RFC PATCH v3 09/16] cxl/mem: Add basic IOCTL interface Ben Widawsky
2021-01-14 16:19 ` Jonathan Cameron
2021-01-11 22:51 ` [RFC PATCH v3 10/16] cxl/mem: Add send command Ben Widawsky
2021-01-14 17:10 ` Jonathan Cameron [this message]
2021-01-21 18:15 ` Ben Widawsky
2021-01-22 11:43 ` Jonathan Cameron
2021-01-22 17:08 ` Ben Widawsky
2021-01-11 22:51 ` [RFC PATCH v3 11/16] taint: add taint for direct hardware access Ben Widawsky
2021-01-11 22:51 ` [RFC PATCH v3 11/16] taint: add taint for unfettered " Ben Widawsky
2021-01-12 3:31 ` Ben Widawsky
2021-01-11 22:51 ` [RFC PATCH v3 12/16] cxl/mem: Add a "RAW" send command Ben Widawsky
2021-01-11 22:51 ` [RFC PATCH v3 13/16] cxl/mem: Create concept of enabled commands Ben Widawsky
2021-01-14 17:25 ` Jonathan Cameron
2021-01-21 18:40 ` Ben Widawsky
2021-01-22 11:28 ` Jonathan Cameron
2021-01-11 22:51 ` [RFC PATCH v3 14/16] cxl/mem: Use CEL for enabling commands Ben Widawsky
2021-01-14 18:02 ` Jonathan Cameron
2021-01-14 18:13 ` Ben Widawsky
2021-01-14 18:32 ` Jonathan Cameron
2021-01-14 19:04 ` Ben Widawsky
2021-01-14 19:24 ` Jonathan Cameron
2021-01-11 22:51 ` [RFC PATCH v3 15/16] cxl/mem: Add limited Get Log command (0401h) Ben Widawsky
2021-01-14 18:08 ` Jonathan Cameron
2021-01-23 0:14 ` Ben Widawsky
2021-01-11 22:51 ` [RFC PATCH v3 16/16] MAINTAINERS: Add maintainers of the CXL driver Ben Widawsky
2021-01-12 1:12 ` Joe Perches
[not found] ` <0f2a6d62-09d8-416f-e972-3e9869c3e1a6@alibaba-inc.com>
2021-01-12 15:17 ` [RFC PATCH v3 00/16] CXL 2.0 Support Ben Widawsky
2021-01-12 16:19 ` Bjorn Helgaas
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210114171038.00003636@Huawei.com \
--to=jonathan.cameron@huawei.com \
--cc=ben.widawsky@intel.com \
--cc=cbrowy@avery-design.com \
--cc=dan.j.williams@intel.com \
--cc=daniel.lll@alibaba-inc.com \
--cc=hch@infradead.org \
--cc=helgaas@kernel.org \
--cc=ira.weiny@intel.com \
--cc=jcm@jonmasters.org \
--cc=linux-cxl@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=rafael.j.wysocki@intel.com \
--cc=rdunlap@infradead.org \
--cc=sean.v.kelley@intel.com \
--cc=vishal.l.verma@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).