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

David Matlack
2025-10-27T23:37:16Z

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@fb.com>

Aside from the commit message and the braces nits,

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

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