| 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: <CACGkMEtYq9XnhNzdmD+=d1k8RStMcMP-Ljo_j1qfz6Us2wOXdQ@mail.gmail.com>
Date: Thu, 14 Aug 2025 11:33:41 +0800
From: Jason Wang <jasowang@...hat.com>
To: Eugenio Perez Martin <eperezma@...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 5:03 PM Eugenio Perez Martin
<eperezma@...hat.com> wrote:
>
> 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
>
Let me fix that.
> > 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?
It should be 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.
Yes.
>
> >
> > 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.
I'm not sure I will get here. I can add a comment to explain that all
map ops are mandated not optional.
Thanks
>
> > .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