Message-ID: <CAJaqyWcSeNo-182zAQdjUxs+3LhBH+ype2NYNcFjiBEEjBeVXw@mail.gmail.com>
Date: Fri, 10 Oct 2025 09:06:03 +0200
From: Eugenio Perez Martin <eperezma@...hat.com>
To: Jason Wang <jasowang@...hat.com>
Cc: "Michael S . Tsirkin" <mst@...hat.com>, linux-kernel@...r.kernel.org, 
	Xuan Zhuo <xuanzhuo@...ux.alibaba.com>, Laurent Vivier <lvivier@...hat.com>, 
	Yongji Xie <xieyongji@...edance.com>, Stefano Garzarella <sgarzare@...hat.com>, 
	Cindy Lu <lulu@...hat.com>, Maxime Coquelin <mcoqueli@...hat.com>, virtualization@...ts.linux.dev
Subject: Re: [PATCH v6 6/7] vduse: add vq group asid support

On Thu, Oct 9, 2025 at 9:43 AM Jason Wang <jasowang@...hat.com> wrote:
>
> On Tue, Sep 30, 2025 at 12:52 AM Eugenio Pérez <eperezma@...hat.com> 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 <eperezma@...hat.com>
> > ---
> > 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
>

Powered by blists - more mailing lists