Re: [PATCH v5 4/5] vfio: selftests: update DMA map/unmap helpers to support more test kinds

Alex Mastro
2025-10-27T23:58:30Z

On Mon, Oct 27, 2025 at 11:37:10PM +0000, David Matlack wrote:
> On 2025-10-27 10:33 AM, Alex Mastro wrote:
> > Add __vfio_pci_dma_* helpers which return -errno from the underlying
> > ioctls.
> >
> > Add __vfio_pci_dma_unmap_all to test more unmapping code paths. Add an
> > out unmapped arg to report the unmapped byte size.
>
> nit: Please append () to function names in commit messages and comments
> (e.g. "Add __vfio_pci_dma_unmap_all() to test ..."). It helps make it
> obvious you are referring to a function.

Ack

> > The existing vfio_pci_dma_* functions, which are intended for happy-path
> > usage (assert on failure) are now thin wrappers on top of the
> > double-underscore helpers.
> >
> > Signed-off-by: Alex Mastro <amastro@fb.com>
>
> Aside from the commit message and the braces nits,

Thanks David. The nits are easy enough to fix.

> Reviewed-by: David Matlack <dmatlack@google.com>
>
> > @@ -152,10 +153,13 @@ static void vfio_iommu_dma_map(struct vfio_pci_device *device,
> > .size = region->size,
> > };
> >
> > - ioctl_assert(device->container_fd, VFIO_IOMMU_MAP_DMA, &args);
> > + if (ioctl(device->container_fd, VFIO_IOMMU_MAP_DMA, &args))
> > + return -errno;
>
> Interesting. I was imagining this would would return whatever ioctl()
> returned and then the caller could check errno if it wanted to. But I
> actually like this better, since it simplifies the assertions at the
> caller (like in your next patch).

Yea, I was also worried about errno clobbering up the stack from the ioctl.
The reason for -errno was to keep error values out of band of >= 0 ioctl return
values (e.g. if we ever need to do similar for ioctls which return fds)

> > +int __vfio_pci_dma_unmap_all(struct vfio_pci_device *device, u64 *unmapped)
> > +{
> > + int ret;
> > + struct vfio_dma_region *curr, *next;
> > +
> > + if (device->iommufd)
> > + ret = iommufd_dma_unmap(device->iommufd, 0, UINT64_MAX,
> > + device->ioas_id, unmapped);
>
> This reminds me, I need to get rid of INVALID_IOVA in vfio_util.h.
>
> __to_iova() can just return int for success/error and pass the iova up
> to the caller via parameter.
>
> > + else
> > + ret = vfio_iommu_dma_unmap(device->container_fd, 0, 0,
> > + VFIO_DMA_UNMAP_FLAG_ALL, unmapped);
> > +
> > + if (ret)
> > + return ret;
> > +
> > + list_for_each_entry_safe(curr, next, &device->dma_regions, link) {
> > + list_del_init(&curr->link);
> > + }
>
> nit: No need for {} for single-line loop.

Ack