[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CACGkMEuRMXrt3zyQtrW8QvHJSVPqh5XGXtxBtir3UxGzjSmXOQ@mail.gmail.com>
Date: Thu, 9 Oct 2025 15:42:59 +0800
From: Jason Wang <jasowang@...hat.com>
To: Eugenio Pérez <eperezma@...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 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.
> + 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.
>
> 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