On Thu, Oct 9, 2025 at 9:43 AM Jason Wang <[email protected]> wrote:
>
> On Tue, Sep 30, 2025 at 12:52 AM Eugenio Pérez <[email protected]> wrote:
> >
> > Add support for assigning Address Space Identifiers (ASIDs) to each VQ
> > group. This enables mapping each group into a distinct memory space.
> >
> > Now that the driver can change ASID in the middle of operation, the
> > domain that each vq address point is also protected by domain_lock.
> >
> > Signed-off-by: Eugenio Pérez <[email protected]>
> > ---
> > v6:
> > * Make vdpa_dev_add use gotos for error handling (MST).
> > * s/(dev->api_version < 1) ?/(dev->api_version < VDUSE_API_VERSION_1) ?/
> > (MST).
> > * Fix struct name not matching in the doc.
> >
> > v5:
> > * Properly return errno if copy_to_user returns >0 in VDUSE_IOTLB_GET_FD
> > ioctl (Jason).
> > * Properly set domain bounce size to divide equally between nas (Jason).
> > * Exclude "padding" member from the only >V1 members in
> > vduse_dev_request.
> >
> > v4:
> > * Divide each domain bounce size between the device bounce size (Jason).
> > * revert unneeded addr = NULL assignment (Jason)
> > * Change if (x && (y || z)) return to if (x) { if (y) return; if (z)
> > return; } (Jason)
> > * Change a bad multiline comment, using @ caracter instead of * (Jason).
> > * Consider config->nas == 0 as a fail (Jason).
> >
> > v3:
> > * Get the vduse domain through the vduse_as in the map functions
> > (Jason).
> > * Squash with the patch creating the vduse_as struct (Jason).
> > * Create VDUSE_DEV_MAX_AS instead of comparing agains a magic number
> > (Jason)
> >
> > v2:
> > * Convert the use of mutex to rwlock.
> >
> > RFC v3:
> > * Increase VDUSE_MAX_VQ_GROUPS to 0xffff (Jason). It was set to a lower
> > value to reduce memory consumption, but vqs are already limited to
> > that value and userspace VDUSE is able to allocate that many vqs.
> > * Remove TODO about merging VDUSE_IOTLB_GET_FD ioctl with
> > VDUSE_IOTLB_GET_INFO.
> > * Use of array_index_nospec in VDUSE device ioctls.
> > * Embed vduse_iotlb_entry into vduse_iotlb_entry_v2.
> > * Move the umem mutex to asid struct so there is no contention between
> > ASIDs.
> >
> > RFC v2:
> > * Make iotlb entry the last one of vduse_iotlb_entry_v2 so the first
> > part of the struct is the same.
> > ---
> > drivers/vdpa/vdpa_user/vduse_dev.c | 347 ++++++++++++++++++++---------
> > include/uapi/linux/vduse.h | 53 ++++-
> > 2 files changed, 290 insertions(+), 110 deletions(-)
> >
>
> [...]
>
> > @@ -2154,7 +2280,8 @@ static int vdpa_dev_add(struct vdpa_mgmt_dev *mdev,
> > const char *name,
> > const struct vdpa_dev_set_config *config)
> > {
> > struct vduse_dev *dev;
> > - int ret;
> > + size_t domain_bounce_size;
> > + int ret, i;
> >
> > mutex_lock(&vduse_lock);
> > dev = vduse_find_dev(name);
> > @@ -2168,29 +2295,35 @@ static int vdpa_dev_add(struct vdpa_mgmt_dev *mdev,
> > const char *name,
> > return ret;
> >
> > write_lock(&dev->domain_lock);
> > - if (!dev->domain)
> > - dev->domain = vduse_domain_create(VDUSE_IOVA_SIZE - 1,
> > - dev->bounce_size);
> > - write_unlock(&dev->domain_lock);
> > - if (!dev->domain) {
> > - ret = -ENOMEM;
> > - goto domain_err;
> > + ret = 0;
> > +
> > + domain_bounce_size = dev->bounce_size / dev->nas;
>
> Could dev->nas be zero here?
>
> It looks like we only have the check like this:
>
> dev->nas = (dev->api_version < VDUSE_API_VERSION_1) ? 1 : config->nas;
>
> When API version >= 1, userspace could fill zero here.
>
It's protected in the change in vduse_validate_config of this patch:
- if (api_version >= VDUSE_API_VERSION_1 &&
- (!config->ngroups || config->ngroups > VDUSE_DEV_MAX_GROUPS))
- return false;
+ if (api_version >= VDUSE_API_VERSION_1) {
+ if (!config->ngroups || config->ngroups > VDUSE_DEV_MAX_GROUPS)
+ return false;
+
+ if (!config->nas || config->nas > VDUSE_DEV_MAX_AS)
+ return false;
+ }
> > + for (i = 0; i < dev->nas; ++i) {
> > + dev->as[i].domain = vduse_domain_create(VDUSE_IOVA_SIZE - 1,
> > + domain_bounce_size);
> > + if (!dev->as[i].domain) {
> > + ret = -ENOMEM;
> > + goto err;
> > + }
> > }
> >
> > + write_unlock(&dev->domain_lock);
> > +
> > ret = _vdpa_register_device(&dev->vdev->vdpa, dev->vq_num);
> > - if (ret) {
> > - goto register_err;
> > - }
> > + if (ret)
> > + goto err;
>
> We don't hold domain_lock here but err tries to unlock it.
>
Ouch, good point! fixing it, thanks!
> >
> > return 0;
> >
> > -register_err:
> > - write_lock(&dev->domain_lock);
> > - vduse_domain_destroy(dev->domain);
> > - dev->domain = NULL;
> > +err:
> > + for (int j = 0; j < i; j++) {
> > + if (dev->as[j].domain) {
> > + vduse_domain_destroy(dev->as[j].domain);
> > + dev->as[j].domain = NULL;
> > + }
> > + }
> > write_unlock(&dev->domain_lock);
> >
>
> Thanks
>
