| Message ID | 3-v1-9912f1a11620+2a-fwctl_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduce fwctl subystem | expand |
On 6/3/24 8:53 AM, Jason Gunthorpe wrote: > Userspace will need to know some details about the fwctl interface being > used to locate the correct userspace code to communicate with the > kernel. Provide a simple device_type enum indicating what the kernel > driver is. > > Allow the device to provide a device specific info struct that contains > any additional information that the driver may need to provide to > userspace. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/fwctl/main.c | 54 ++++++++++++++++++++++++++++++++++++++ > include/linux/fwctl.h | 8 ++++++ > include/uapi/fwctl/fwctl.h | 29 ++++++++++++++++++++ > 3 files changed, 91 insertions(+) > > diff --git a/drivers/fwctl/main.c b/drivers/fwctl/main.c > index 7ecdabdd9dcb1e..10e3f504893892 100644 > --- a/drivers/fwctl/main.c > +++ b/drivers/fwctl/main.c > @@ -17,6 +17,8 @@ enum { > static dev_t fwctl_dev; > static DEFINE_IDA(fwctl_ida); > > +DEFINE_FREE(kfree_errptr, void *, if (!IS_ERR_OR_NULL(_T)) kfree(_T)); > + > struct fwctl_ucmd { > struct fwctl_uctx *uctx; > void __user *ubuffer; > @@ -24,8 +26,59 @@ struct fwctl_ucmd { > u32 user_size; > }; > > +static int ucmd_respond(struct fwctl_ucmd *ucmd, size_t cmd_len) > +{ > + if (copy_to_user(ucmd->ubuffer, ucmd->cmd, > + min_t(size_t, ucmd->user_size, cmd_len))) > + return -EFAULT; > + return 0; > +} > + > +static int copy_to_user_zero_pad(void __user *to, const void *from, > + size_t from_len, size_t user_len) > +{ > + size_t copy_len; > + > + copy_len = min(from_len, user_len); > + if (copy_to_user(to, from, copy_len)) > + return -EFAULT; > + if (copy_len < user_len) { > + if (clear_user(to + copy_len, user_len - copy_len)) > + return -EFAULT; > + } > + return 0; > +} > + > +static int fwctl_cmd_info(struct fwctl_ucmd *ucmd) > +{ > + struct fwctl_device *fwctl = ucmd->uctx->fwctl; > + struct fwctl_info *cmd = ucmd->cmd; > + size_t driver_info_len = 0; > + > + if (cmd->flags) > + return -EOPNOTSUPP; > + > + if (cmd->device_data_len) { > + void *driver_info __free(kfree_errptr) = NULL; > + > + driver_info = fwctl->ops->info(ucmd->uctx, &driver_info_len); Hi Jason, Are you open to pass in potential user input for the info query? I'm working on plumbing fwctl for CXL. The current CXL query command [1] takes a number of commands as input for its ioctl. For fwctl_cmd_info(), the current implementation is when ->info() is called no information about the user buffer length or an input buffer is provided. To make things work I can just return everything each ioctl call and user can sort it out by calling the ioctl twice and provide a u32 size buffer first to figure out the total number of commands and then provide a larger buffer for all the command info. Just trying to see if you are open to something a bit more cleaner than depending on a side effect of the ioctl to retrieve all the information. [1] https://elixir.bootlin.com/linux/v6.10-rc3/source/drivers/cxl/core/mbox.c#L526 DJ > + if (IS_ERR(driver_info)) > + return PTR_ERR(driver_info); > + > + if (copy_to_user_zero_pad(u64_to_user_ptr(cmd->out_device_data), > + driver_info, driver_info_len, > + cmd->device_data_len)) > + return -EFAULT; > + } > + > + cmd->out_device_type = fwctl->ops->device_type; > + cmd->device_data_len = driver_info_len; > + return ucmd_respond(ucmd, sizeof(*cmd)); > +} > + > /* On stack memory for the ioctl structs */ > union ucmd_buffer { > + struct fwctl_info info; > }; > > struct fwctl_ioctl_op { > @@ -45,6 +98,7 @@ struct fwctl_ioctl_op { > .execute = _fn, \ > } > static const struct fwctl_ioctl_op fwctl_ioctl_ops[] = { > + IOCTL_OP(FWCTL_INFO, fwctl_cmd_info, struct fwctl_info, out_device_data), > }; > > static long fwctl_fops_ioctl(struct file *filp, unsigned int cmd, > diff --git a/include/linux/fwctl.h b/include/linux/fwctl.h > index 1d9651de92fc19..9a906b861acf3a 100644 > --- a/include/linux/fwctl.h > +++ b/include/linux/fwctl.h > @@ -7,12 +7,14 @@ > #include <linux/device.h> > #include <linux/cdev.h> > #include <linux/cleanup.h> > +#include <uapi/fwctl/fwctl.h> > > struct fwctl_device; > struct fwctl_uctx; > > /** > * struct fwctl_ops - Driver provided operations > + * @device_type: The drivers assigned device_type number. This is uABI > * @uctx_size: The size of the fwctl_uctx struct to allocate. The first > * bytes of this memory will be a fwctl_uctx. The driver can use the > * remaining bytes as its private memory. > @@ -20,11 +22,17 @@ struct fwctl_uctx; > * used. > * @close_uctx: Called when the uctx is destroyed, usually when the FD is > * closed. > + * @info: Implement FWCTL_INFO. Return a kmalloc() memory that is copied to > + * out_device_data. On input length indicates the size of the user buffer > + * on output it indicates the size of the memory. The driver can ignore > + * length on input, the core code will handle everything. > */ > struct fwctl_ops { > + enum fwctl_device_type device_type; > size_t uctx_size; > int (*open_uctx)(struct fwctl_uctx *uctx); > void (*close_uctx)(struct fwctl_uctx *uctx); > + void *(*info)(struct fwctl_uctx *uctx, size_t *length); > }; > > /** > diff --git a/include/uapi/fwctl/fwctl.h b/include/uapi/fwctl/fwctl.h > index 0bdce95b6d69d9..39db9f09f8068e 100644 > --- a/include/uapi/fwctl/fwctl.h > +++ b/include/uapi/fwctl/fwctl.h > @@ -36,6 +36,35 @@ > */ > enum { > FWCTL_CMD_BASE = 0, > + FWCTL_CMD_INFO = 0, > + FWCTL_CMD_RPC = 1, > }; > > +enum fwctl_device_type { > + FWCTL_DEVICE_TYPE_ERROR = 0, > +}; > + > +/** > + * struct fwctl_info - ioctl(FWCTL_INFO) > + * @size: sizeof(struct fwctl_info) > + * @flags: Must be 0 > + * @out_device_type: Returns the type of the device from enum fwctl_device_type > + * @device_data_len: On input the length of the out_device_data memory. On > + * output the size of the kernel's device_data which may be larger or > + * smaller than the input. Maybe 0 on input. > + * @out_device_data: Pointer to a memory of device_data_len bytes. Kernel will > + * fill the entire memory, zeroing as required. > + * > + * Returns basic information about this fwctl instance, particularly what driver > + * is being used to define the device_data format. > + */ > +struct fwctl_info { > + __u32 size; > + __u32 flags; > + __u32 out_device_type; > + __u32 device_data_len; > + __aligned_u64 out_device_data; > +}; > +#define FWCTL_INFO _IO(FWCTL_TYPE, FWCTL_CMD_INFO) > + > #endif
On Thu, Jun 13, 2024 at 04:32:44PM -0700, Dave Jiang wrote: > Are you open to pass in potential user input for the info query? I'm > working on plumbing fwctl for CXL. Neat! > The current CXL query command [1] takes a number of commands as > input for its ioctl. For fwctl_cmd_info(), the current > implementation is when ->info() is called no information about the > user buffer length or an input buffer is provided. Right, the purpose of info is to report information about the fwctl driver. It is to allow the userspace to connect to the correct userspace driver. It shouldn't be doing much with the device. If you want to execute a info command *to the fw* then I'd expect you'd execute the command through the normal RPC channel? Does something prevent this? This is how the mlx5 driver is working where there are many info (called CAP) commands that return data, and they all run over the rpc channel. Thanks, Jason
On 6/13/24 4:40 PM, Jason Gunthorpe wrote: > On Thu, Jun 13, 2024 at 04:32:44PM -0700, Dave Jiang wrote: > >> Are you open to pass in potential user input for the info query? I'm >> working on plumbing fwctl for CXL. > > Neat! > >> The current CXL query command [1] takes a number of commands as >> input for its ioctl. For fwctl_cmd_info(), the current >> implementation is when ->info() is called no information about the >> user buffer length or an input buffer is provided. > > Right, the purpose of info is to report information about the fwctl > driver. It is to allow the userspace to connect to the correct > userspace driver. It shouldn't be doing much with the device. > > If you want to execute a info command *to the fw* then I'd expect > you'd execute the command through the normal RPC channel? Does > something prevent this? Ok that makes sense. I should be able to do it through RPC with some tweaks. > > This is how the mlx5 driver is working where there are many info > (called CAP) commands that return data, and they all run over the rpc > channel. > > Thanks, > Jason
diff --git a/drivers/fwctl/main.c b/drivers/fwctl/main.c index 7ecdabdd9dcb1e..10e3f504893892 100644 --- a/drivers/fwctl/main.c +++ b/drivers/fwctl/main.c @@ -17,6 +17,8 @@ enum { static dev_t fwctl_dev; static DEFINE_IDA(fwctl_ida); +DEFINE_FREE(kfree_errptr, void *, if (!IS_ERR_OR_NULL(_T)) kfree(_T)); + struct fwctl_ucmd { struct fwctl_uctx *uctx; void __user *ubuffer; @@ -24,8 +26,59 @@ struct fwctl_ucmd { u32 user_size; }; +static int ucmd_respond(struct fwctl_ucmd *ucmd, size_t cmd_len) +{ + if (copy_to_user(ucmd->ubuffer, ucmd->cmd, + min_t(size_t, ucmd->user_size, cmd_len))) + return -EFAULT; + return 0; +} + +static int copy_to_user_zero_pad(void __user *to, const void *from, + size_t from_len, size_t user_len) +{ + size_t copy_len; + + copy_len = min(from_len, user_len); + if (copy_to_user(to, from, copy_len)) + return -EFAULT; + if (copy_len < user_len) { + if (clear_user(to + copy_len, user_len - copy_len)) + return -EFAULT; + } + return 0; +} + +static int fwctl_cmd_info(struct fwctl_ucmd *ucmd) +{ + struct fwctl_device *fwctl = ucmd->uctx->fwctl; + struct fwctl_info *cmd = ucmd->cmd; + size_t driver_info_len = 0; + + if (cmd->flags) + return -EOPNOTSUPP; + + if (cmd->device_data_len) { + void *driver_info __free(kfree_errptr) = NULL; + + driver_info = fwctl->ops->info(ucmd->uctx, &driver_info_len); + if (IS_ERR(driver_info)) + return PTR_ERR(driver_info); + + if (copy_to_user_zero_pad(u64_to_user_ptr(cmd->out_device_data), + driver_info, driver_info_len, + cmd->device_data_len)) + return -EFAULT; + } + + cmd->out_device_type = fwctl->ops->device_type; + cmd->device_data_len = driver_info_len; + return ucmd_respond(ucmd, sizeof(*cmd)); +} + /* On stack memory for the ioctl structs */ union ucmd_buffer { + struct fwctl_info info; }; struct fwctl_ioctl_op { @@ -45,6 +98,7 @@ struct fwctl_ioctl_op { .execute = _fn, \ } static const struct fwctl_ioctl_op fwctl_ioctl_ops[] = { + IOCTL_OP(FWCTL_INFO, fwctl_cmd_info, struct fwctl_info, out_device_data), }; static long fwctl_fops_ioctl(struct file *filp, unsigned int cmd, diff --git a/include/linux/fwctl.h b/include/linux/fwctl.h index 1d9651de92fc19..9a906b861acf3a 100644 --- a/include/linux/fwctl.h +++ b/include/linux/fwctl.h @@ -7,12 +7,14 @@ #include <linux/device.h> #include <linux/cdev.h> #include <linux/cleanup.h> +#include <uapi/fwctl/fwctl.h> struct fwctl_device; struct fwctl_uctx; /** * struct fwctl_ops - Driver provided operations + * @device_type: The drivers assigned device_type number. This is uABI * @uctx_size: The size of the fwctl_uctx struct to allocate. The first * bytes of this memory will be a fwctl_uctx. The driver can use the * remaining bytes as its private memory. @@ -20,11 +22,17 @@ struct fwctl_uctx; * used. * @close_uctx: Called when the uctx is destroyed, usually when the FD is * closed. + * @info: Implement FWCTL_INFO. Return a kmalloc() memory that is copied to + * out_device_data. On input length indicates the size of the user buffer + * on output it indicates the size of the memory. The driver can ignore + * length on input, the core code will handle everything. */ struct fwctl_ops { + enum fwctl_device_type device_type; size_t uctx_size; int (*open_uctx)(struct fwctl_uctx *uctx); void (*close_uctx)(struct fwctl_uctx *uctx); + void *(*info)(struct fwctl_uctx *uctx, size_t *length); }; /** diff --git a/include/uapi/fwctl/fwctl.h b/include/uapi/fwctl/fwctl.h index 0bdce95b6d69d9..39db9f09f8068e 100644 --- a/include/uapi/fwctl/fwctl.h +++ b/include/uapi/fwctl/fwctl.h @@ -36,6 +36,35 @@ */ enum { FWCTL_CMD_BASE = 0, + FWCTL_CMD_INFO = 0, + FWCTL_CMD_RPC = 1, }; +enum fwctl_device_type { + FWCTL_DEVICE_TYPE_ERROR = 0, +}; + +/** + * struct fwctl_info - ioctl(FWCTL_INFO) + * @size: sizeof(struct fwctl_info) + * @flags: Must be 0 + * @out_device_type: Returns the type of the device from enum fwctl_device_type + * @device_data_len: On input the length of the out_device_data memory. On + * output the size of the kernel's device_data which may be larger or + * smaller than the input. Maybe 0 on input. + * @out_device_data: Pointer to a memory of device_data_len bytes. Kernel will + * fill the entire memory, zeroing as required. + * + * Returns basic information about this fwctl instance, particularly what driver + * is being used to define the device_data format. + */ +struct fwctl_info { + __u32 size; + __u32 flags; + __u32 out_device_type; + __u32 device_data_len; + __aligned_u64 out_device_data; +}; +#define FWCTL_INFO _IO(FWCTL_TYPE, FWCTL_CMD_INFO) + #endif
Userspace will need to know some details about the fwctl interface being used to locate the correct userspace code to communicate with the kernel. Provide a simple device_type enum indicating what the kernel driver is. Allow the device to provide a device specific info struct that contains any additional information that the driver may need to provide to userspace. Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> --- drivers/fwctl/main.c | 54 ++++++++++++++++++++++++++++++++++++++ include/linux/fwctl.h | 8 ++++++ include/uapi/fwctl/fwctl.h | 29 ++++++++++++++++++++ 3 files changed, 91 insertions(+)