Re: [PATCH v3 2/3] vduse: add VDUSE_GET_FEATURES ioctl
From: Eugenio Perez Martin
Date: Fri May 29 2026 - 06:37:08 EST
On Fri, Mar 13, 2026 at 7:46 AM Eugenio Perez Martin
<eperezma@xxxxxxxxxx> wrote:
>
> On Fri, Mar 13, 2026 at 6:56 AM Jason Wang <jasowang@xxxxxxxxxx> wrote:
> >
> > On Thu, Mar 12, 2026 at 3:12 PM Eugenio Perez Martin
> > <eperezma@xxxxxxxxxx> wrote:
> > >
> > > On Thu, Mar 12, 2026 at 4:56 AM Jason Wang <jasowang@xxxxxxxxxx> wrote:
> > > >
> > > > On Wed, Mar 11, 2026 at 3:08 AM Eugenio Pérez <eperezma@xxxxxxxxxx> wrote:
> > > > >
> > > > > Add an ioctl to allow VDUSE instances to query the available features
> > > > > supported by the kernel module.
> > > > >
> > > > > Signed-off-by: Eugenio Pérez <eperezma@xxxxxxxxxx>
> > > > > ---
> > > > > A simple u64 bitmap is used for feature flags. While a flexible array
> > > > > could support indefinite expansion, 64 bits is sufficient for the
> > > > > foreseeable future and simplifies the implementation.
> > > > >
> > > > > Also, dev_dbg is used for logging rather than dev_err as these can be
> > > > > triggered from userspace.
> > > > > ---
> > > > > v3:
> > > > > * Remove check for API_VERSION < 2
> > > > > * Add comment about struct vduse_dev_config:vduse_features is only valid
> > > > > if VDUSE_GET_FEATURES success.
> > > > >
> > > > > v2:
> > > > > * return -EINVAL if ioctl called with version < 2, so userland visible
> > > > > reply is kept (Jason).
> > > > > ---
> > > > > drivers/vdpa/vdpa_user/vduse_dev.c | 7 +++++++
> > > > > include/uapi/linux/vduse.h | 7 ++++++-
> > > > > 2 files changed, 13 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > index d1da7c15d98b..17e0358d3a68 100644
> > > > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > @@ -52,6 +52,9 @@
> > > > >
> > > > > #define IRQ_UNBOUND -1
> > > > >
> > > > > +/* Supported VDUSE features */
> > > > > +static const uint64_t vduse_features;
> > > > > +
> > > > > /*
> > > > > * VDUSE instance have not asked the vduse API version, so assume 0.
> > > > > *
> > > > > @@ -2207,6 +2210,10 @@ static long vduse_ioctl(struct file *file, unsigned int cmd,
> > > > > ret = vduse_destroy_dev(name);
> > > > > break;
> > > > > }
> > > > > + case VDUSE_GET_FEATURES:
> > > > > + ret = put_user(vduse_features, (u64 __user *)argp);
> > > > > + break;
> > > > > +
> > > > > default:
> > > > > ret = -EINVAL;
> > > > > break;
> > > > > diff --git a/include/uapi/linux/vduse.h b/include/uapi/linux/vduse.h
> > > > > index 361eea511c21..e9b5f32a452d 100644
> > > > > --- a/include/uapi/linux/vduse.h
> > > > > +++ b/include/uapi/linux/vduse.h
> > > > > @@ -33,6 +33,7 @@
> > > > > * @vq_align: the allocation alignment of virtqueue's metadata
> > > > > * @ngroups: number of vq groups that VDUSE device declares
> > > > > * @nas: number of address spaces that VDUSE device declares
> > > > > + * @vduse_features: VDUSE features
> > > > > * @reserved: for future use, needs to be initialized to zero
> > > > > * @config_size: the size of the configuration space
> > > > > * @config: the buffer of the configuration space
> > > > > @@ -49,7 +50,8 @@ struct vduse_dev_config {
> > > > > __u32 vq_align;
> > > > > __u32 ngroups; /* if VDUSE_API_VERSION >= 1 */
> > > > > __u32 nas; /* if VDUSE_API_VERSION >= 1 */
> > > > > - __u32 reserved[11];
> > > > > + __u64 vduse_features; /* if VDUSE_GET_FEATURES is not EINVAL */
> > > > > + __u32 reserved[9];
> > > >
> > > > If we go with VDUSE_GET_FEATURES, I'd perfer to go with
> > > > VDUSE_SET_FEATURES intead of this.
> > > >
> > >
> > > I didn't realize the lack of symmetry, but that approach grows and
> > > complicates the possible states of struct vduse_control and the
> > > vduse_ioctl code for little benefit in my opinion. Making it atomic in
> > > VDUSE_CREATE_DEV ioctl helps centralize all the potential errors.
> > > Also, from previous experience with vhost_vdpa, the ioctls slow down
> > > the device creation, which could affect things like VDUSE adoption for
> > > containers etc.
> >
> > Maybe, but if we really care about the time spent on device creation,
> > uAPI needs redesigning. Or at least more feature in the future
> > wouldn't slow down further.
> >
> > >
> > > On the other hand, it would make it easier to tell if the device
> > > couldn't be created because of invalid features set. This is highly
> > > unlikely, as the VDUSE device should retrieve the features by calling
> > > VDUSE_GET_FEATURES earlier.
> > >
> > > I don't think it is worth it, but if you think we should go that way
> > > I'm ok with the change.
> >
> > It depends on the intialziation flow. After getting features, I'd
> > expect it's something like this
> >
> > A:
> >
> > 1) GET_API_VERSION
> > 2) SET_API_VERSION
> > 3) GET_FEATURES
> > 4) SET_FEATURES
> > 5) CREATE_DEV
> >
> > Or B in this patch:
> >
> > 1) GET_API_VERSION
> > 2) SET_API_VERSION
> > 3) GET_FEATURES
> > 4) CREATE_DEV
> >
> > I think it's better to stick the symmetry as API_VERSION (technically,
> > API could be set via config as well?)
> >
>
> Yes, I'd have chosen to set the API_VERSION via config too.
>
> > BTW, I found VDUSE_SET_API_VERSION could be set multiple times (even
> > after the device is create), do we need to fix this?
> >
>
> It shouldn't be a problem, each device has its own copy of
> api_version. It seems clear to me that the API setting only affects
> devices created after the change, but I can add a guard after the
> first device is created.
>
I keep thinking that we are increasing the complexity for little
benefit TBH. This is the diff on top of the current series. MST, do
you prefer a new VDUSE_SET_FEATURES ioctl to follow the api version
API and keep the symmetry with GET_VDUSE_FEATURES or use the config
struct like the group and asid configuration?
---
drivers/vdpa/vdpa_user/vduse_dev.c | 27 +++++++++++++++++++++++----
include/uapi/linux/vduse.h | 7 ++++---
2 files changed, 27 insertions(+), 7 deletions(-)
diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c
b/drivers/vdpa/vdpa_user/vduse_dev.c
index 4f642b95a7cb..a5427dcebf29 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -162,6 +162,7 @@ struct vduse_dev_msg {
struct vduse_control {
u64 api_version;
+ u64 vduse_features;
};
static DEFINE_MUTEX(vduse_lock);
@@ -2079,7 +2080,8 @@ static struct attribute *vduse_dev_attrs[] = {
ATTRIBUTE_GROUPS(vduse_dev);
static int vduse_create_dev(struct vduse_dev_config *config,
- void *config_buf, u64 api_version)
+ void *config_buf, u64 api_version,
+ u64 vduse_features)
{
int ret;
struct vduse_dev *dev;
@@ -2101,9 +2103,9 @@ static int vduse_create_dev(struct
vduse_dev_config *config,
dev->device_features = config->features;
dev->device_id = config->device_id;
dev->vendor_id = config->vendor_id;
- dev->vduse_features = config->vduse_features;
+ dev->vduse_features = vduse_features;
dev_dbg(vduse_ctrl_dev, "Creating device %s with features 0x%llx",
- config->name, config->vduse_features);
+ config->name, vduse_features);
dev->nas = (dev->api_version < VDUSE_API_VERSION_1) ? 1 : config->nas;
dev->as = kcalloc(dev->nas, sizeof(dev->as[0]), GFP_KERNEL);
@@ -2199,6 +2201,21 @@ static long vduse_ioctl(struct file *file,
unsigned int cmd,
control->api_version = api_version;
break;
}
+ case VDUSE_SET_FEATURES: {
+ u64 features;
+
+ ret = -EFAULT;
+ if (get_user(features, (u64 __user *)argp))
+ break;
+
+ ret = -EINVAL;
+ if (features & ~vduse_features)
+ break;
+
+ ret = 0;
+ control->vduse_features = features;
+ break;
+ }
case VDUSE_CREATE_DEV: {
struct vduse_dev_config config;
unsigned long size = offsetof(struct vduse_dev_config, config);
@@ -2220,7 +2237,8 @@ static long vduse_ioctl(struct file *file,
unsigned int cmd,
break;
}
config.name[VDUSE_NAME_MAX - 1] = '\0';
- ret = vduse_create_dev(&config, buf, control->api_version);
+ ret = vduse_create_dev(&config, buf, control->api_version,
+ control->vduse_features);
if (ret)
kvfree(buf);
break;
@@ -2266,6 +2284,7 @@ static int vduse_open(struct inode *inode,
struct file *file)
return -ENOMEM;
control->api_version = VDUSE_API_VERSION_NOT_ASKED;
+ control->vduse_features = 0;
file->private_data = control;
return 0;
diff --git a/include/uapi/linux/vduse.h b/include/uapi/linux/vduse.h
index 7324faea5df4..6e45f60aa015 100644
--- a/include/uapi/linux/vduse.h
+++ b/include/uapi/linux/vduse.h
@@ -26,6 +26,9 @@
/* Set the version of VDUSE API that userspace supported. */
#define VDUSE_SET_API_VERSION _IOW(VDUSE_BASE, 0x01, __u64)
+/* Set the VDUSE features that userspace supported. */
+#define VDUSE_SET_FEATURES _IOW(VDUSE_BASE, 0x05, __u64)
+
/**
* struct vduse_dev_config - basic configuration of a VDUSE device
* @name: VDUSE device name, needs to be NUL terminated
@@ -36,7 +39,6 @@
* @vq_align: the allocation alignment of virtqueue's metadata
* @ngroups: number of vq groups that VDUSE device declares
* @nas: number of address spaces that VDUSE device declares
- * @vduse_features: VDUSE features
* @reserved: for future use, needs to be initialized to zero
* @config_size: the size of the configuration space
* @config: the buffer of the configuration space
@@ -53,8 +55,7 @@ struct vduse_dev_config {
__u32 vq_align;
__u32 ngroups; /* if VDUSE_API_VERSION >= 1 */
__u32 nas; /* if VDUSE_API_VERSION >= 1 */
- __u64 vduse_features; /* if VDUSE_GET_FEATURES is not EINVAL */
- __u32 reserved[9];
+ __u32 reserved[11];
__u32 config_size;
__u8 config[];
};