Re: [PATCH v4 0/3] vfio: handle DMA map/unmap up to the addressable limit

[Alex Mastro <amastro@fb.com>]

Thanks David -- this is good feedback. Will roll these suggestions into v5.

On Tue, Oct 21, 2025 at 05:38:31PM -0700, David Matlack wrote:
> On Tue, Oct 21, 2025 at 12:13 PM Alex Mastro <amastro@fb.com> wrote:
> > I updated the *_unmap function signatures to return the count of bytes unmapped,
> > since that is part of the test pass criteria. Also added unmap_all flavors,
> > since those exercise different code paths than range-based unmap.
> 
> When you send, can you introduce these in a separate commit and update
> the existing test function in vfio_dma_mapping_test.c to assert on it?

SGTM

> > +#undef FIXTURE_VARIANT_ADD_IOMMU_MODE
> 
> I think this can/should go just after the
> FIXTURE_VARIANT_ADD_ALL_IOMMU_MODES(); statement. The same below.

Ack.

> > +       unmapped = vfio_pci_dma_unmap_all(self->device);
> > +       ASSERT_EQ(unmapped, size);
> 
> The unmap_all test should probably be in a separate TEST_F. You can
> put the struct vfio_dma_region in the FIXTURE and initialize it in the
> FIXTURE_SETUP() to reduce code duplication.
> > +}

Make sense.

> Would it be useful to add negative map/unmap tests as well? If so we'd
> need a way to plumb the return value of the ioctl up to the caller so
> you can assert that it failed, which will conflict with returning the
> amount of unmapped bytes.

Testing negative cases would be useful. Not sure about the mechanics yet.

> 
> Maybe we should make unmapped an output parameter like so?
> 
> int __vfio_pci_dma_map(struct vfio_pci_device *device,
>         struct vfio_dma_region *region);
> 
> void vfio_pci_dma_map(struct vfio_pci_device *device,
>         struct vfio_dma_region *region);
> 
> int __vfio_pci_dma_unmap(struct vfio_pci_device *device,
>         struct vfio_dma_region *region, u64 *unmapped);
> 
> void vfio_pci_dma_unmap(struct vfio_pci_device *device,
>         struct vfio_dma_region *region, u64 *unmapped);
> 
> int __vfio_pci_dma_unmap_all(struct vfio_pci_device *device, u64 *unmapped);
> void vfio_pci_dma_unmap_all(struct vfio_pci_device *device, u64 *unmapped);
> 
> unmapped can be optional and callers that don't care can pass in NULL.
> It'll be a little gross though to see NULL on all the unmap calls
> though... Maybe unmapped can be restricted to __vfio_pci_dma_unmap().
> So something like this:
> 
> int __vfio_pci_dma_unmap(struct vfio_pci_device *device,
>         struct vfio_dma_region *region, u64 *unmapped);
> 
> void vfio_pci_dma_unmap(struct vfio_pci_device *device,
>         struct vfio_dma_region *region);

I'll put some thought into this and propose something in v5.