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

[Alex Mastro <amastro@fb.com>]

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