| 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: <CAJaqyWcZpX6XeXqTgBxyGnZWmnoR39juaOM=_owcNAGuDhBTCQ@mail.gmail.com>
Date: Thu, 14 Aug 2025 08:42:57 +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 Thu, Aug 14, 2025 at 5:33 AM Jason Wang <jasowang@...hat.com> wrote:
>
> 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.
>
Ok I thought there was a possibility for virtio core to try calling
vdev->map->mapping_error in previous patches. Now I see that
vduse_map_ops is actually created in this patch, so that's not
possible. My bad!
Powered by blists - more mailing lists