Re: [PATCH v3 2/3] vduse: add VDUSE_GET_FEATURES ioctl

[Eugenio Perez Martin <eperezma@redhat.com>]

On Thu, Mar 12, 2026 at 4:56 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Wed, Mar 11, 2026 at 3:08 AM Eugenio Pérez <eperezma@redhat.com> 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@redhat.com>
> > ---
> > 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.

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.

> But we can hear from others.
>
> >         __u32 config_size;
> >         __u8 config[];
> >  };
> > @@ -63,6 +65,9 @@ struct vduse_dev_config {
> >   */
> >  #define VDUSE_DESTROY_DEV      _IOW(VDUSE_BASE, 0x03, char[VDUSE_NAME_MAX])
> >
> > +/* Get the VDUSE supported features */
> > +#define VDUSE_GET_FEATURES     _IOR(VDUSE_BASE, 0x04, __u64)
> > +
> >  /* The ioctls for VDUSE device (/dev/vduse/$NAME) */
> >
> >  /**
> > --
> > 2.53.0
> >
>
> Thanks
>