lkml.org 
[lkml]   [2021]   [Jan]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC PATCH v3 10/16] cxl/mem: Add send command
    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

    \
     
     \ /
      Last update: 2021-01-14 18:14    [W:4.617 / U:0.328 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site