On Tue, Mar 03, 2026 at 03:20:22PM +0800, Jason Wang wrote: > On Fri, Feb 13, 2026 at 3:39 PM Eugenio Perez Martin > <[email protected]> wrote: > > > > On Fri, Feb 13, 2026 at 2:25 AM Jason Wang <[email protected]> wrote: > > > > > > On Thu, Feb 12, 2026 at 4:23 PM Eugenio Perez Martin > > > <[email protected]> wrote: > > > > > > > > On Thu, Feb 12, 2026 at 9:07 AM Jason Wang <[email protected]> wrote: > > > > > > > > > > On Tue, Feb 10, 2026 at 4:26 PM 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. > > > > > > --- > > > > > > v2: > > > > > > * return -EINVAL if ioctl called with version < 2, so userland > > > > > > visible > > > > > > reply is kept (Jason). > > > > > > --- > > > > > > drivers/vdpa/vdpa_user/vduse_dev.c | 28 > > > > > > ++++++++++++++++++++++++++++ > > > > > > include/uapi/linux/vduse.h | 7 ++++++- > > > > > > 2 files changed, 34 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c > > > > > > b/drivers/vdpa/vdpa_user/vduse_dev.c > > > > > > index d1da7c15d98b..7458cbaed4c7 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. > > > > > > * > > > > > > @@ -1947,6 +1950,19 @@ static bool vduse_validate_config(struct > > > > > > vduse_dev_config *config, > > > > > > sizeof(config->reserved))) > > > > > > return false; > > > > > > > > > > > > + if (api_version < VDUSE_API_VERSION_2) { > > > > > > + if (config->vduse_features) { > > > > > > + dev_dbg(vduse_ctrl_dev, > > > > > > + "config->vduse_features with > > > > > > version %llu", > > > > > > + api_version); > > > > > > + return false; > > > > > > + } > > > > > > + } else { > > > > > > + if (config->vduse_features & ~vduse_features) > > > > > > + return false; > > > > > > + } > > > > > > + > > > > > > + > > > > > > if (api_version < VDUSE_API_VERSION_1 && > > > > > > (config->ngroups || config->nas)) > > > > > > return false; > > > > > > @@ -2207,6 +2223,18 @@ static long vduse_ioctl(struct file *file, > > > > > > unsigned int cmd, > > > > > > ret = vduse_destroy_dev(name); > > > > > > break; > > > > > > } > > > > > > + case VDUSE_GET_FEATURES: > > > > > > + if (control->api_version < VDUSE_API_VERSION_2) { > > > > > > + dev_dbg(vduse_ctrl_dev, > > > > > > + "VDUSE_GET_FEATURES ioctl with > > > > > > version %llu", > > > > > > + control->api_version); > > > > > > + ret = -EINVAL; > > > > > > + break; > > > > > > + } > > > > > > + > > > > > > + 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 27832d46084c..8898d9daa777 100644 > > > > > > --- a/include/uapi/linux/vduse.h > > > > > > +++ b/include/uapi/linux/vduse.h > > > > > > @@ -37,6 +37,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 > > > > > > @@ -53,7 +54,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; > > > > > > > > > > Nit: Should we document " /* if VDUSE_API_VERSION >= 2 */ > > > > > > > > > > But I have an open question, is this better to just introduce a > > > > > VDUSE_SET_FEATURES then we can do negotiation to avoid bumping API > > > > > versions? > > > > > > > > > > > > > How do you know if you can call VDUSE_GET_FEATURES and > > > > VDUSE_SET_FEATURES then? vduse_ioctl does not even return -ENOIOCMD > > > > but -EINVAL if you call the equivalent of VDUSE_GET_FEATURES on > > > > current Linux master. Or should the VDUSE instance assume that if the > > > > ioctl returns -EINVAL the kernel does not support them? > > > > > > If we go that way, yes. > > > > > > > I'm ok with doing the change. Kind of, I still think that increasing > > the API version is way cleaner, so no ioctl returns any error in > > regular initialization. But python's style "ask forgiveness not > > permission" is also ok for me. > > > > But I'm really missing the motivation of it. What's the problem with > > the increase of the version that you are trying to solve here? > > For example, if a feature is simply added via a new ioctl. I think we > don't need to bump the version. An example is the umem reg/dereg. > VDUSE starts from this choice so I think we should stick to that. > > I think a better time to bump the version are > > 1) an ioctl is expected to behave differently > 2) new ioctls are mutually exclusive with one(s) of the existing ioctls > 3) new initialization protocol > > etc. > > > > > Can we make a rule to know when to increase the version, and when it > > is not needed and it is ok for ioctls to just return -EINVAL or > > -ENOIOCTLCMD, so future series can hit the right behavior from earlier > > versions? > > See above. > > > > > From your comment I extract that we don't need to increase the API > > version number if we just add ioctls commands to the device, like > > probing if an ioctl exists. Also, we don't need to increase the > > version if we can tell if a member in vduse_dev_config is known to > > exist because a previous ioctl (or feature flag) succeeded. > > > > Am I missing something? > > I might be wrong, just some thoughts. Please correct me if I was wrong. > > Thanks > > >
Not bumping version is nice as people can cherry pick features. OTOH if people are cherry picking features then userspace has to support all kind of combinations of features. So I think there can not be a simple rule, it boils down to this: if userspace can handle all failures easily - just add an ioctl. If userspace can not and would merely assert on success - update the version. -- MST
