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

[David Matlack]

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.

> 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@xxxxxx>

Aside from the commit message and the braces nits,

  Reviewed-by: David Matlack <dmatlack@xxxxxxxxxx>

> @@ -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).

> +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.