Message-ID: <20210114171038.00003636@Huawei.com>
Date:   Thu, 14 Jan 2021 17:10:38 +0000
From:   Jonathan Cameron <Jonathan.Cameron@...wei.com>
To:     Ben Widawsky <ben.widawsky@...el.com>
CC:     <linux-cxl@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-pci@...r.kernel.org>,
        "linux-acpi@...r.kernel.org, Ira Weiny" <ira.weiny@...el.com>,
        Dan Williams <dan.j.williams@...el.com>,
        "Vishal Verma" <vishal.l.verma@...el.com>,
        "Kelley, Sean V" <sean.v.kelley@...el.com>,
        Rafael Wysocki <rafael.j.wysocki@...el.com>,
        "Bjorn Helgaas" <helgaas@...nel.org>,
        Jon Masters <jcm@...masters.org>,
        Chris Browy <cbrowy@...ry-design.com>,
        Randy Dunlap <rdunlap@...radead.org>,
        "Christoph Hellwig" <hch@...radead.org>,
        <daniel.lll@...baba-inc.com>
Subject: Re: [RFC PATCH v3 10/16] cxl/mem: Add send command

On Mon, 11 Jan 2021 14:51:14 -0800
Ben Widawsky <ben.widawsky@...el.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@...el.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

Powered by blists - more mailing lists