| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAJaqyWdZv30TMedrdb-iF6qwwis7d_GJpVyk2dCur2J27+ywog@mail.gmail.com>
Date: Wed, 13 Aug 2025 11:02:40 +0200
From: Eugenio Perez Martin <eperezma@...hat.com>
To: Jason Wang <jasowang@...hat.com>
Cc: mst@...hat.com, xuanzhuo@...ux.alibaba.com, virtualization@...ts.linux.dev,
linux-kernel@...r.kernel.org, hch@...radead.org
Subject: Re: [PATCH V5 9/9] vduse: switch to use virtio map API instead of DMA API
On Wed, Aug 13, 2025 at 7:49 AM Jason Wang <jasowang@...hat.com> wrote:
>
> Lacking the support of device specific mapping supported in virtio,
> VDUSE must trick the DMA API in order to make virtio-vdpa transport
> work. This is done by advertising vDPA device as dma device with a
> VDUSE specific dma_ops even if it doesn't do DMA at all.
>
> This will be fixed by this patch. Thanks to the new mapping operations
> support by virtio and vDPA. VDUSE can simply switch to advertise its
> specific mappings operations to virtio via virtio-vdpa then DMA API is
> not needed for VDUSE any more and iova domain could be used as the
> mapping token instead.
>
> Signed-off-by: Jason Wang <jasowang@...hat.com>
> ---
> drivers/vdpa/Kconfig | 8 +--
> drivers/vdpa/vdpa_user/iova_domain.c | 2 +-
> drivers/vdpa/vdpa_user/iova_domain.h | 2 +-
> drivers/vdpa/vdpa_user/vduse_dev.c | 75 ++++++++++++++--------------
> 4 files changed, 41 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/vdpa/Kconfig b/drivers/vdpa/Kconfig
> index 559fb9d3271f..857cf288c876 100644
> --- a/drivers/vdpa/Kconfig
> +++ b/drivers/vdpa/Kconfig
> @@ -34,13 +34,7 @@ config VDPA_SIM_BLOCK
>
> config VDPA_USER
> tristate "VDUSE (vDPA Device in Userspace) support"
> - depends on EVENTFD && MMU && HAS_DMA
> - #
> - # This driver incorrectly tries to override the dma_ops. It should
> - # never have done that, but for now keep it working on architectures
> - # that use dma ops
> - #
> - depends on ARCH_HAS_DMA_OPS
> + depends on EVENTFD && MMU
> select VHOST_IOTLB
> select IOMMU_IOVA
> help
> diff --git a/drivers/vdpa/vdpa_user/iova_domain.c b/drivers/vdpa/vdpa_user/iova_domain.c
> index 58116f89d8da..ccaed24b7ef8 100644
> --- a/drivers/vdpa/vdpa_user/iova_domain.c
> +++ b/drivers/vdpa/vdpa_user/iova_domain.c
> @@ -447,7 +447,7 @@ void vduse_domain_unmap_page(struct vduse_iova_domain *domain,
>
> void *vduse_domain_alloc_coherent(struct vduse_iova_domain *domain,
> size_t size, dma_addr_t *dma_addr,
> - gfp_t flag, unsigned long attrs)
> + gfp_t flag)
> {
> struct iova_domain *iovad = &domain->consistent_iovad;
> unsigned long limit = domain->iova_limit;
> diff --git a/drivers/vdpa/vdpa_user/iova_domain.h b/drivers/vdpa/vdpa_user/iova_domain.h
> index 7f3f0928ec78..1f3c30be272a 100644
> --- a/drivers/vdpa/vdpa_user/iova_domain.h
> +++ b/drivers/vdpa/vdpa_user/iova_domain.h
> @@ -64,7 +64,7 @@ void vduse_domain_unmap_page(struct vduse_iova_domain *domain,
>
> void *vduse_domain_alloc_coherent(struct vduse_iova_domain *domain,
> size_t size, dma_addr_t *dma_addr,
> - gfp_t flag, unsigned long attrs);
> + gfp_t flag);
>
> void vduse_domain_free_coherent(struct vduse_iova_domain *domain, size_t size,
> void *vaddr, dma_addr_t dma_addr,
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> index f161059d543e..3260edefdf0d 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -88,6 +88,7 @@ struct vduse_dev {
> struct device *dev;
> struct vduse_virtqueue **vqs;
> struct vduse_iova_domain *domain;
> + struct vduse_iova_domain *dom;
*dom is not used
> char *name;
> struct mutex lock;
> spinlock_t msg_lock;
> @@ -814,59 +815,53 @@ static const struct vdpa_config_ops vduse_vdpa_config_ops = {
> .free = vduse_vdpa_free,
> };
>
> -static void vduse_dev_sync_single_for_device(struct device *dev,
> +static void vduse_dev_sync_single_for_device(void *token,
> dma_addr_t dma_addr, size_t size,
> enum dma_data_direction dir)
> {
> - struct vduse_dev *vdev = dev_to_vduse(dev);
> - struct vduse_iova_domain *domain = vdev->domain;
> + struct vduse_iova_domain *domain = token;
If I add ASID support to VDUSE I'll need to replace the token by a vq
group representor. Is that ok?
I think it is as I like how using *domain here makes this patch
clearer, and adding something else will make this patch harder to
review.
>
> vduse_domain_sync_single_for_device(domain, dma_addr, size, dir);
> }
>
> -static void vduse_dev_sync_single_for_cpu(struct device *dev,
> +static void vduse_dev_sync_single_for_cpu(void *token,
> dma_addr_t dma_addr, size_t size,
> enum dma_data_direction dir)
> {
> - struct vduse_dev *vdev = dev_to_vduse(dev);
> - struct vduse_iova_domain *domain = vdev->domain;
> + struct vduse_iova_domain *domain = token;
>
> vduse_domain_sync_single_for_cpu(domain, dma_addr, size, dir);
> }
>
> -static dma_addr_t vduse_dev_map_page(struct device *dev, struct page *page,
> +static dma_addr_t vduse_dev_map_page(void *token, struct page *page,
> unsigned long offset, size_t size,
> enum dma_data_direction dir,
> unsigned long attrs)
> {
> - struct vduse_dev *vdev = dev_to_vduse(dev);
> - struct vduse_iova_domain *domain = vdev->domain;
> + struct vduse_iova_domain *domain = token;
>
> return vduse_domain_map_page(domain, page, offset, size, dir, attrs);
> }
>
> -static void vduse_dev_unmap_page(struct device *dev, dma_addr_t dma_addr,
> +static void vduse_dev_unmap_page(void *token, dma_addr_t dma_addr,
> size_t size, enum dma_data_direction dir,
> unsigned long attrs)
> {
> - struct vduse_dev *vdev = dev_to_vduse(dev);
> - struct vduse_iova_domain *domain = vdev->domain;
> + struct vduse_iova_domain *domain = token;
>
> return vduse_domain_unmap_page(domain, dma_addr, size, dir, attrs);
> }
>
> -static void *vduse_dev_alloc_coherent(struct device *dev, size_t size,
> - dma_addr_t *dma_addr, gfp_t flag,
> - unsigned long attrs)
> +static void *vduse_dev_alloc_coherent(void *token, size_t size,
> + dma_addr_t *dma_addr, gfp_t flag)
> {
> - struct vduse_dev *vdev = dev_to_vduse(dev);
> - struct vduse_iova_domain *domain = vdev->domain;
> + struct vduse_iova_domain *domain = token;
> unsigned long iova;
> void *addr;
>
> *dma_addr = DMA_MAPPING_ERROR;
> addr = vduse_domain_alloc_coherent(domain, size,
> - (dma_addr_t *)&iova, flag, attrs);
> + (dma_addr_t *)&iova, flag);
> if (!addr)
> return NULL;
>
> @@ -875,31 +870,45 @@ static void *vduse_dev_alloc_coherent(struct device *dev, size_t size,
> return addr;
> }
>
> -static void vduse_dev_free_coherent(struct device *dev, size_t size,
> - void *vaddr, dma_addr_t dma_addr,
> - unsigned long attrs)
> +static void vduse_dev_free_coherent(void *token, size_t size,
> + void *vaddr, dma_addr_t dma_addr,
> + unsigned long attrs)
> {
> - struct vduse_dev *vdev = dev_to_vduse(dev);
> - struct vduse_iova_domain *domain = vdev->domain;
> + struct vduse_iova_domain *domain = token;
>
> vduse_domain_free_coherent(domain, size, vaddr, dma_addr, attrs);
> }
>
> -static size_t vduse_dev_max_mapping_size(struct device *dev)
> +static bool vduse_dev_need_sync(void *token, dma_addr_t dma_addr)
> {
> - struct vduse_dev *vdev = dev_to_vduse(dev);
> - struct vduse_iova_domain *domain = vdev->domain;
> + struct vduse_iova_domain *domain = token;
> +
> + return dma_addr < domain->bounce_size;
> +}
> +
> +static int vduse_dev_mapping_error(void *token, dma_addr_t dma_addr)
> +{
> + if (unlikely(dma_addr == DMA_MAPPING_ERROR))
> + return -ENOMEM;
> + return 0;
> +}
> +
> +static size_t vduse_dev_max_mapping_size(void *token)
> +{
> + struct vduse_iova_domain *domain = token;
>
> return domain->bounce_size;
> }
>
> -static const struct dma_map_ops vduse_dev_dma_ops = {
> +static const struct virtio_map_ops vduse_map_ops = {
> .sync_single_for_device = vduse_dev_sync_single_for_device,
> .sync_single_for_cpu = vduse_dev_sync_single_for_cpu,
> .map_page = vduse_dev_map_page,
> .unmap_page = vduse_dev_unmap_page,
> .alloc = vduse_dev_alloc_coherent,
> .free = vduse_dev_free_coherent,
> + .need_sync = vduse_dev_need_sync,
> + .mapping_error = vduse_dev_mapping_error,
I think that adding these functions here is problematic,
In the case of mapping error vring_mapping_error only checks for
vdev->map, not for vdev->map->mapping_error:
static int vring_mapping_error(const struct vring_virtqueue *vq,
dma_addr_t addr)
{
struct virtio_device *vdev = vq->vq.vdev;
if (!vq->use_map_api)
return 0;
if (vdev->map)
return vdev->map->mapping_error(vring_mapping_token(vq), addr);
else
return dma_mapping_error(vring_dma_dev(vq), addr);
}
So we either add the check for the member or we define them from the beginning.
> .max_mapping_size = vduse_dev_max_mapping_size,
> };
>
> @@ -2003,27 +2012,18 @@ static struct vduse_mgmt_dev *vduse_mgmt;
> static int vduse_dev_init_vdpa(struct vduse_dev *dev, const char *name)
> {
> struct vduse_vdpa *vdev;
> - int ret;
>
> if (dev->vdev)
> return -EEXIST;
>
> vdev = vdpa_alloc_device(struct vduse_vdpa, vdpa, dev->dev,
> - &vduse_vdpa_config_ops, NULL,
> + &vduse_vdpa_config_ops, &vduse_map_ops,
> 1, 1, name, true);
> if (IS_ERR(vdev))
> return PTR_ERR(vdev);
>
> dev->vdev = vdev;
> vdev->dev = dev;
> - vdev->vdpa.dev.dma_mask = &vdev->vdpa.dev.coherent_dma_mask;
> - ret = dma_set_mask_and_coherent(&vdev->vdpa.dev, DMA_BIT_MASK(64));
> - if (ret) {
> - put_device(&vdev->vdpa.dev);
> - return ret;
> - }
> - set_dma_ops(&vdev->vdpa.dev, &vduse_dev_dma_ops);
> - vdev->vdpa.mapping_token.dma_dev = &vdev->vdpa.dev;
> vdev->vdpa.mdev = &vduse_mgmt->mgmt_dev;
>
> return 0;
> @@ -2056,6 +2056,7 @@ static int vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name,
> return -ENOMEM;
> }
>
> + dev->vdev->vdpa.mapping_token.token = dev->domain;
> ret = _vdpa_register_device(&dev->vdev->vdpa, dev->vq_num);
> if (ret) {
> put_device(&dev->vdev->vdpa.dev);
> --
> 2.31.1
>
Powered by blists - more mailing lists