Message-ID: <358d2e11-59e2-46eb-a7f4-3c69e6befe02@intel.com>
Date: Thu, 13 Jun 2024 16:32:44 -0700
From: Dave Jiang <dave.jiang@...el.com>
To: Jason Gunthorpe <jgg@...dia.com>, Jonathan Corbet <corbet@....net>,
 Itay Avraham <itayavr@...dia.com>, Jakub Kicinski <kuba@...nel.org>,
 Leon Romanovsky <leon@...nel.org>, linux-doc@...r.kernel.org,
 linux-rdma@...r.kernel.org, netdev@...r.kernel.org,
 Paolo Abeni <pabeni@...hat.com>, Saeed Mahameed <saeedm@...dia.com>,
 Tariq Toukan <tariqt@...dia.com>
Cc: Andy Gospodarek <andrew.gospodarek@...adcom.com>,
 Aron Silverton <aron.silverton@...cle.com>,
 Dan Williams <dan.j.williams@...el.com>, David Ahern <dsahern@...nel.org>,
 Christoph Hellwig <hch@...radead.org>, Jiri Pirko <jiri@...dia.com>,
 Leonid Bloch <lbloch@...dia.com>, Leon Romanovsky <leonro@...dia.com>,
 linux-cxl@...r.kernel.org, patches@...ts.linux.dev
Subject: Re: [PATCH 3/8] fwctl: FWCTL_INFO to return basic information about
 the device



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@...dia.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

Powered by blists - more mailing lists