On 21-01-14 17:10:38, Jonathan Cameron wrote:
> On Mon, 11 Jan 2021 14:51:14 -0800
> Ben Widawsky <ben.widawsky@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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?
>
It's used for a debug message in handle_mailbox_cmd_from_user(). This is all the
macro magic stolen from ftrace. Or, did I miss the question?
> > +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.
>
You're correct about the get() needing to be first. I will fix it. As for
performance path - so while this does potentially help with performance, it
actually ends up being I think a little cleaner to not have to deal with a local
buffer.
How strongly do you feel about it? I'd say if you don't care so much, let's keep
it as is and find a reason to undo later.
> > + 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;
> > +}
> > +
>
> ...
Yeah...
>
> >
> > 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.
>
Fine with me.
> >
> > __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?
This comment reflects the way the interface was in v2. It needs fixing.
>
> > + */
> > +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
>
