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?)
BTW, I found VDUSE_SET_API_VERSION could be set multiple times (even
after the device is create), do we need to fix this?
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
> >
>
