Re: [RFC PATCH v3 09/16] cxl/mem: Add basic IOCTL interface

From: Jonathan Cameron
Date: Thu Jan 14 2021 - 11:21:23 EST

Next message: kernel test robot: "Re: [PATCH 2/2] phy: stm32: register usbphyc as clock provider of ck_usbo_48m clock"
Previous message: Matthew Wilcox: "Re: Redesigning and modernising fscache"
In reply to: Ben Widawsky: "[RFC PATCH v3 09/16] cxl/mem: Add basic IOCTL interface"
Next in thread: Ben Widawsky: "[RFC PATCH v3 08/16] cxl/mem: Register CXL memX devices"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Mon, 11 Jan 2021 14:51:13 -0800
Ben Widawsky <ben.widawsky@xxxxxxxxx> wrote:

> Add a straightforward IOCTL that provides a mechanism for userspace to
> query the supported memory device commands. CXL commands as they appear
> to userspace are described as part of the UAPI kerneldoc. The command
> list returned via this IOCTL will contain the full set of commands that
> the driver supports, however, some of those commands may not be
> available for use by userspace.
>
> Memory device commands are specified in 8.2.9 of the CXL 2.0
> specification. They are submitted through a mailbox mechanism specified
> in 8.2.8.4.
>
> Signed-off-by: Ben Widawsky <ben.widawsky@xxxxxxxxx>
Hi Ben,

Maybe today I'll get a clear block to finish reading through these!

Anyhow, some comments inline.

Jonathan

> ---
> .clang-format | 1 +
> Documentation/cxl/memory-devices.rst | 9 +
> .../userspace-api/ioctl/ioctl-number.rst | 1 +
> drivers/cxl/mem.c | 156 ++++++++++++++++++
> include/uapi/linux/cxl_mem.h | 117 +++++++++++++
> 5 files changed, 284 insertions(+)
> create mode 100644 include/uapi/linux/cxl_mem.h
>
> diff --git a/.clang-format b/.clang-format
> index 10dc5a9a61b3..3f11c8901b43 100644
> --- a/.clang-format
> +++ b/.clang-format
> @@ -109,6 +109,7 @@ ForEachMacros:
> - 'css_for_each_child'
> - 'css_for_each_descendant_post'
> - 'css_for_each_descendant_pre'
> + - 'cxl_for_each_cmd'
> - 'device_for_each_child_node'
> - 'dma_fence_chain_for_each'
> - 'do_for_each_ftrace_op'
> diff --git a/Documentation/cxl/memory-devices.rst b/Documentation/cxl/memory-devices.rst
> index 5f723c25382b..ec54674b3822 100644
> --- a/Documentation/cxl/memory-devices.rst
> +++ b/Documentation/cxl/memory-devices.rst
> @@ -32,6 +32,15 @@ CXL Memory Device
> .. kernel-doc:: drivers/cxl/mem.c
> :internal:
>
> +CXL IOCTL Interface
> +-------------------
> +
> +.. kernel-doc:: include/uapi/linux/cxl_mem.h
> + :doc: UAPI
> +
> +.. kernel-doc:: include/uapi/linux/cxl_mem.h
> + :internal:
> +
> External Interfaces
> ===================
>
> diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
> index a4c75a28c839..6eb8e634664d 100644
> --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> @@ -352,6 +352,7 @@ Code Seq# Include File Comments
> <mailto:michael.klein@xxxxxxxxxxxxxxxxxxxx>
> 0xCC 00-0F drivers/misc/ibmvmc.h pseries VMC driver
> 0xCD 01 linux/reiserfs_fs.h
> +0xCE 01-02 uapi/linux/cxl_mem.h Compute Express Link Memory Devices
> 0xCF 02 fs/cifs/ioctl.c
> 0xDB 00-0F drivers/char/mwave/mwavepub.h
> 0xDD 00-3F ZFCP device driver see drivers/s390/scsi/
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index da2bf941fe92..d4eb3f5b9469 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -7,6 +7,7 @@
> #include <linux/idr.h>
> #include <linux/pci.h>
> #include <linux/io.h>
> +#include <uapi/linux/cxl_mem.h>
> #include "acpi.h"
> #include "pci.h"
> #include "cxl.h"
> @@ -41,6 +42,7 @@
> #define CXL_MAILBOX_TIMEOUT_US 2000
>
> enum opcode {
> + CXL_MBOX_OP_INVALID = 0x0000,
> CXL_MBOX_OP_IDENTIFY = 0x4000,
> CXL_MBOX_OP_MAX = 0x10000
> };
> @@ -82,6 +84,90 @@ static DEFINE_IDR(cxl_mem_idr);
> /* protect cxl_mem_idr allocations */
> static DEFINE_MUTEX(cxl_memdev_lock);
>
> +#define CXL_CMD(_id, _flags, sin, sout, f) \

Perhaps this would be better after the structure definition?

> + [CXL_MEM_COMMAND_ID_##_id] = { \
> + { \
I would suggest naming the sub structure in this initializer.
.info = { ...
> + .id = CXL_MEM_COMMAND_ID_##_id, \
> + .flags = CXL_MEM_COMMAND_FLAG_##_flags, \
> + .size_in = sin, \
> + .size_out = sout, \
> + }, \
> + .flags = CXL_CMD_INTERNAL_FLAG_##f, \

These look to be indented too far.

> + .opcode = CXL_MBOX_OP_##_id, \
> + }
> +
> +/**
> + * struct cxl_mem_command - Driver representation of a memory device command
> + * @info: Command information as it exists for the UAPI
> + * @opcode: The actual bits used for the mailbox protocol
> + * @flags: Set of flags reflecting the state of the command.
> + *
> + * * %CXL_CMD_INTERNAL_FLAG_HIDDEN: Command is hidden from userspace. This
> + * would typically be used for deprecated commands.
> + * * %CXL_CMD_FLAG_MANDATORY: Hardware must support this command. This flag is
> + * only used internally by the driver for sanity checking.
> + *
> + * The cxl_mem_command is the driver's internal representation of commands that
> + * are supported by the driver. Some of these commands may not be supported by
> + * the hardware. The driver will use @info to validate the fields passed in by
> + * the user then submit the @opcode to the hardware.
> + *
> + * See struct cxl_command_info.
> + */
> +struct cxl_mem_command {
> + const struct cxl_command_info info;
> + enum opcode opcode;
> + u32 flags;
> +#define CXL_CMD_INTERNAL_FLAG_NONE 0
> +#define CXL_CMD_INTERNAL_FLAG_HIDDEN BIT(0)
> +#define CXL_CMD_INTERNAL_FLAG_MANDATORY BIT(1)
> +};
> +
> +/*
> + * This table defines the supported mailbox commands for the driver. This table
> + * is made up of a UAPI structure. Non-negative values as parameters in the
> + * table will be validated against the user's input. For example, if size_in is
> + * 0, and the user passed in 1, it is an error.
> + */
> +static struct cxl_mem_command mem_commands[] = {
> + CXL_CMD(INVALID, KERNEL, 0, 0, HIDDEN),
> + CXL_CMD(IDENTIFY, NONE, 0, 0x43, MANDATORY),
> +};
> +
> +#define cxl_for_each_cmd(cmd) \
> + for ((cmd) = &mem_commands[0]; \
> + ((cmd) - mem_commands) < ARRAY_SIZE(mem_commands); (cmd)++)
> +
> +static inline int cxl_cmd_index(const struct cxl_mem_command *c)
> +{
> + if (unlikely(c >= mem_commands &&
> + c < (mem_commands + sizeof(mem_commands)))) {

This path could do with an explanatory comment.

> + struct cxl_mem_command *p;
> +
> + cxl_for_each_cmd(p)
> + if (p->opcode == c->opcode &&
> + p->flags == c->flags)
> + return p - mem_commands;
> + } else {
> + return c - mem_commands;
> + }
> +
> + WARN_ON_ONCE(1);
> + return 0;
> +}
> +
> +static inline struct cxl_mem_command *cxl_mem_find_command(u16 opcode)
> +{
> + struct cxl_mem_command *c;
> +
> + cxl_for_each_cmd(c) {
> + if (c->opcode == opcode)
> + return c;
> + }
> +
> + return NULL;
> +}
> +
> static int cxl_mem_wait_for_doorbell(struct cxl_mem *cxlm)
> {
> const int timeout = msecs_to_jiffies(CXL_MAILBOX_TIMEOUT_US);
> @@ -140,6 +226,7 @@ static void cxl_mem_mbox_timeout(struct cxl_mem *cxlm,
> static int cxl_mem_mbox_send_cmd(struct cxl_mem *cxlm,
> struct mbox_cmd *mbox_cmd)
> {
> + const struct cxl_mem_command *cmd;
> u64 cmd_reg, status_reg;
> size_t out_len;
> int rc;
> @@ -158,6 +245,13 @@ static int cxl_mem_mbox_send_cmd(struct cxl_mem *cxlm,
> * 8. If output payload is non-empty, host reads Command Payload Registers
> */
>
> + cmd = cxl_mem_find_command(mbox_cmd->opcode);
> + if (!cmd) {
> + dev_info(&cxlm->pdev->dev,
> + "Unknown opcode 0x%04x being sent to hardware\n",
> + mbox_cmd->opcode);
> + }
> +
> /* #1 */
> WARN_ON(cxl_doorbell_busy(cxlm));
>
> @@ -200,6 +294,20 @@ static int cxl_mem_mbox_send_cmd(struct cxl_mem *cxlm,
> out_len = CXL_GET_FIELD(cmd_reg, CXLDEV_MB_CMD_PAYLOAD_LENGTH);
> mbox_cmd->size_out = out_len;
>
> + /*
> + * If the command had a fixed size output, but the hardware did
> + * something unexpected, just print an error and move on. It would be
> + * worth sending a bug report.
> + */
> + if (cmd && cmd->info.size_out >= 0 &&
> + mbox_cmd->size_out != cmd->info.size_out) {
> + bool too_big = mbox_cmd->size_out > cmd->info.size_out;
> +
> + dev_err(&cxlm->pdev->dev,
> + "payload was %s than driver expectations\n",
> + too_big ? "larger" : "smaller");
> + }
> +
> /* #8 */
> if (out_len && mbox_cmd->payload)
> memcpy_fromio(mbox_cmd->payload, cxl_payload_regs(cxlm),
> @@ -323,8 +431,56 @@ static int cxl_mem_release(struct inode *inode, struct file *file)
> return 0;
> }
>
> +static int cxl_mem_count_commands(void)
> +{
> + struct cxl_mem_command *c;
> + int n = 0;
> +
> + cxl_for_each_cmd(c) {
> + if (c->flags & CXL_CMD_INTERNAL_FLAG_HIDDEN)
> + continue;
> + n++;
> + }
> +
> + return n;
> +}
> +
> static long cxl_mem_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> {
> + struct cxl_memdev *cxlmd = file->private_data;
> + struct device *dev = &cxlmd->dev;
> +
> + if (cmd == CXL_MEM_QUERY_COMMANDS) {
> + struct cxl_mem_query_commands __user *q = (void __user *)arg;
> + struct cxl_mem_command *cmd;
> + u32 n_commands;
> + int j = 0;
> +
> + dev_dbg(dev, "Query IOCTL\n");
> +
> + if (get_user(n_commands, (u32 __user *)arg))
> + return -EFAULT;
> +
> + /* returns the total number if 0 elements are requested. */
> + if (n_commands == 0)
> + return put_user(cxl_mem_count_commands(),
> + (u32 __user *)arg);

Perhaps clearer to fill in the n_commands field of q?

> +
> + /* otherwise, return max(n_commands, total commands) */
Doesn't seem to be doing this...

> + cxl_for_each_cmd(cmd) {
> + const struct cxl_command_info *info = &cmd->info;
> +
> + if (cmd->flags & CXL_CMD_INTERNAL_FLAG_HIDDEN)
> + continue;
> +
> + if (copy_to_user(&q->commands[j++], info, sizeof(*info)))
> + return -EFAULT;
> +
> + if (j == n_commands)
> + break;

That doesn't look right. Shouldn't be returning -ENOTTY in either of the success
paths.
> + }
> + }
> +
> return -ENOTTY;
> }
>
> diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h
> new file mode 100644
> index 000000000000..847f825bbe18
> --- /dev/null
> +++ b/include/uapi/linux/cxl_mem.h
> @@ -0,0 +1,117 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * CXL IOCTLs for Memory Devices
> + */
> +
> +#ifndef _UAPI_CXL_MEM_H_
> +#define _UAPI_CXL_MEM_H_
> +
> +#if defined(__cplusplus)
> +extern "C" {
> +#endif
> +
> +/**
> + * DOC: UAPI
> + *
> + * CXL memory devices expose UAPI to have a standard user interface.
> + * Userspace can refer to these structure definitions and UAPI formats
> + * to communicate to driver. The commands themselves are somewhat obfuscated
> + * with macro magic. They have the form CXL_MEM_COMMAND_ID_<name>.
> + *
> + * For example "CXL_MEM_COMMAND_ID_INVALID"
> + *
> + * Not all of all commands that the driver supports are always available for use
> + * by userspace. Userspace must check the results from the QUERY command in
> + * order to determine the live set of commands.
> + */
> +
> +#define CXL_MEM_QUERY_COMMANDS _IOR(0xCE, 1, struct cxl_mem_query_commands)
> +
> +#undef CMDS
> +#define CMDS \
> + C(INVALID, "Invalid Command"), \
> + C(IDENTIFY, "Identify Command"), \
> + C(MAX, "Last command")
> +#undef C
> +#define C(a, b) CXL_MEM_COMMAND_ID_##a
> +
> +enum { CMDS };
> +
> +/**
> + * struct cxl_command_info - Command information returned from a query.
> + * @id: ID number for the command.
> + * @flags: Flags that specify command behavior.
> + *
> + * * %CXL_MEM_COMMAND_FLAG_KERNEL: This command is reserved for exclusive
> + * kernel use.
> + * * %CXL_MEM_COMMAND_FLAG_MUTEX: This command may require coordination with
> + * the kernel in order to complete successfully.
> + *
> + * @size_in: Expected input size, or -1 if variable length.
> + * @size_out: Expected output size, or -1 if variable length.
> + *
> + * Represents a single command that is supported by both the driver and the
> + * hardware. The is returned as part of an array from the query ioctl. The

This is?

> + * following would be a command named "foobar" that takes a variable length
> + * input and returns 0 bytes of output.
> + *
> + * - @id = 10
> + * - @flags = CXL_MEM_COMMAND_FLAG_MUTEX
> + * - @size_in = -1
> + * - @size_out = 0
> + *
> + * See struct cxl_mem_query_commands.
> + */
> +struct cxl_command_info {
> + __u32 id;
> +
> + __u32 flags;
> +#define CXL_MEM_COMMAND_FLAG_NONE 0
> +#define CXL_MEM_COMMAND_FLAG_KERNEL BIT(0)
> +#define CXL_MEM_COMMAND_FLAG_MUTEX BIT(1)
> +
> + __s32 size_in;
> + __s32 size_out;
> +};
> +
> +/**
> + * struct cxl_mem_query_commands - Query supported commands.
> + * @n_commands: In/out parameter. When @n_commands is > 0, the driver will
> + * return min(num_support_commands, n_commands). When @n_commands
> + * is 0, driver will return the number of total supported commands.
> + * @rsvd: Reserved for future use.
> + * @commands: Output array of supported commands. This array must be allocated
> + * by userspace to be at least min(num_support_commands, @n_commands)
> + *
> + * Allow userspace to query the available commands supported by both the driver,
> + * and the hardware. Commands that aren't supported by either the driver, or the
> + * hardware are not returned in the query.
> + *
> + * Examples:
> + *
> + * - { .n_commands = 0 } // Get number of supported commands
> + * - { .n_commands = 15, .commands = buf } // Return first 15 (or less)
> + * supported commands
> + *
> + * See struct cxl_command_info.
> + */
> +struct cxl_mem_query_commands {
> + /*
> + * Input: Number of commands to return (space allocated by user)
> + * Output: Number of commands supported by the driver/hardware
> + *
> + * If n_commands is 0, kernel will only return number of commands and
> + * not try to populate commands[], thus allowing userspace to know how
> + * much space to allocate
> + */
> + __u32 n_commands;
> + __u32 rsvd;
> +
> + struct cxl_command_info __user commands[]; /* out: supported commands */
> +};
> +
> +#if defined(__cplusplus)
> +}
> +#endif
> +
> +#endif

Next message: kernel test robot: "Re: [PATCH 2/2] phy: stm32: register usbphyc as clock provider of ck_usbo_48m clock"
Previous message: Matthew Wilcox: "Re: Redesigning and modernising fscache"
In reply to: Ben Widawsky: "[RFC PATCH v3 09/16] cxl/mem: Add basic IOCTL interface"
Next in thread: Ben Widawsky: "[RFC PATCH v3 08/16] cxl/mem: Register CXL memX devices"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]