On Fri, Mar 13, 2026 at 6:56 AM Jason Wang <[email protected]> wrote:
>
> On Thu, Mar 12, 2026 at 3:12 PM Eugenio Perez Martin
> <[email protected]> wrote:
> >
> > On Thu, Mar 12, 2026 at 4:56 AM Jason Wang <[email protected]> wrote:
> > >
> > > On Wed, Mar 11, 2026 at 3:08 AM Eugenio Pérez <[email protected]> wrote:
> > > >
> > > > Add an ioctl to allow VDUSE instances to query the available features
> > > > supported by the kernel module.
> > > >
> > > > Signed-off-by: Eugenio Pérez <[email protected]>
> > > > ---
> > > > 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.
> Thanks
>
> >
> > > 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
> > >
> >
>
