Message-ID: <CALzav=cyDaiKbQfkjF_UUQ0PB6cAKZhnSqM3ZvodqqEe8kQEqw@mail.gmail.com>
Date: Tue, 21 Oct 2025 17:38:31 -0700
From: David Matlack <dmatlack@...gle.com>
To: Alex Mastro <amastro@...com>
Cc: Alex Williamson <alex@...zbot.org>, Alejandro Jimenez <alejandro.j.jimenez@...cle.com>, 
	Jason Gunthorpe <jgg@...pe.ca>, kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 0/3] vfio: handle DMA map/unmap up to the addressable limit

On Tue, Oct 21, 2025 at 12:13 PM Alex Mastro <amastro@...com> wrote:
>
> On Tue, Oct 21, 2025 at 09:31:59AM -0700, David Matlack wrote:
> > On Tue, Oct 21, 2025 at 9:26 AM Alex Mastro <amastro@...com> wrote:
> > > On Mon, Oct 20, 2025 at 03:36:33PM -0600, Alex Williamson wrote:
> > > > Should we also therefore expand the DMA mapping tests in
> > > > tools/testing/selftests/vfio to include an end of address space test?
> > >
> > > Yes. I will append such a commit to the end of the series in v5. Our VFIO tests
> > > are built on top of a hermetic rust wrapper library over VFIO ioctls, but they
> > > aren't quite ready to be open sourced yet.
> >
> > Feel free to reach out if you have any questions about writing or
> > running the VFIO selftests.
>
> Thanks David. I built and ran using below. I am not too familiar with
> kselftests, so open to tips.
>
> $ make LLVM=1 -j kselftest-install INSTALL_PATH=/tmp/kst TARGETS="vfio"
> $ VFIO_SELFTESTS_BDF=0000:05:00.0 /tmp/kst/run_kselftest.sh
>
> I added the following. Is this the right direction? Is multiple fixtures per
> file OK? Seems related enough to vfio_dma_mapping_test.c to keep together.

Adding a fixture to vfio_dma_mapping_test.c is what I was imagining as well.

Overall looks good, we can hash out the specifics in the patches if
you prefer. But I added some thoughts below.

>
> 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?

>
> Relevant test output:
>
> # #  RUN           vfio_dma_map_limit_test.vfio_type1_iommu.end_of_address_space ...
> # Mapped HVA 0x7f6638222000 (size 0x1000) at IOVA 0xfffffffffffff000
> # Mapped HVA 0x7f6638222000 (size 0x1000) at IOVA 0xfffffffffffff000
> # #            OK  vfio_dma_map_limit_test.vfio_type1_iommu.end_of_address_space
> # ok 16 vfio_dma_map_limit_test.vfio_type1_iommu.end_of_address_space
> # #  RUN           vfio_dma_map_limit_test.vfio_type1v2_iommu.end_of_address_space ...
> # Mapped HVA 0x7f6638222000 (size 0x1000) at IOVA 0xfffffffffffff000
> # Mapped HVA 0x7f6638222000 (size 0x1000) at IOVA 0xfffffffffffff000
> # #            OK  vfio_dma_map_limit_test.vfio_type1v2_iommu.end_of_address_space
> # ok 17 vfio_dma_map_limit_test.vfio_type1v2_iommu.end_of_address_space
> # #  RUN           vfio_dma_map_limit_test.iommufd_compat_type1.end_of_address_space ...
> # Mapped HVA 0x7f6638222000 (size 0x1000) at IOVA 0xfffffffffffff000
> # Mapped HVA 0x7f6638222000 (size 0x1000) at IOVA 0xfffffffffffff000
> # #            OK  vfio_dma_map_limit_test.iommufd_compat_type1.end_of_address_space
> # ok 18 vfio_dma_map_limit_test.iommufd_compat_type1.end_of_address_space
> # #  RUN           vfio_dma_map_limit_test.iommufd_compat_type1v2.end_of_address_space ...
> # Mapped HVA 0x7f6638222000 (size 0x1000) at IOVA 0xfffffffffffff000
> # Mapped HVA 0x7f6638222000 (size 0x1000) at IOVA 0xfffffffffffff000
> # #            OK  vfio_dma_map_limit_test.iommufd_compat_type1v2.end_of_address_space
> # ok 19 vfio_dma_map_limit_test.iommufd_compat_type1v2.end_of_address_space
> # #  RUN           vfio_dma_map_limit_test.iommufd.end_of_address_space ...
> # Mapped HVA 0x7f6638222000 (size 0x1000) at IOVA 0xfffffffffffff000
> # Mapped HVA 0x7f6638222000 (size 0x1000) at IOVA 0xfffffffffffff000
>
> diff --git a/tools/testing/selftests/vfio/lib/include/vfio_util.h b/tools/testing/selftests/vfio/lib/include/vfio_util.h
> index ed31606e01b7..8e9d40845ccc 100644
> --- a/tools/testing/selftests/vfio/lib/include/vfio_util.h
> +++ b/tools/testing/selftests/vfio/lib/include/vfio_util.h
> @@ -208,8 +208,9 @@ void vfio_pci_device_reset(struct vfio_pci_device *device);
>
>  void vfio_pci_dma_map(struct vfio_pci_device *device,
>                       struct vfio_dma_region *region);
> -void vfio_pci_dma_unmap(struct vfio_pci_device *device,
> -                       struct vfio_dma_region *region);
> +u64 vfio_pci_dma_unmap(struct vfio_pci_device *device,
> +                       struct vfio_dma_region *region);
> +u64 vfio_pci_dma_unmap_all(struct vfio_pci_device *device);
>
>  void vfio_pci_config_access(struct vfio_pci_device *device, bool write,
>                             size_t config, size_t size, void *data);
> diff --git a/tools/testing/selftests/vfio/lib/vfio_pci_device.c b/tools/testing/selftests/vfio/lib/vfio_pci_device.c
> index 0921b2451ba5..f5ae68a7df9c 100644
> --- a/tools/testing/selftests/vfio/lib/vfio_pci_device.c
> +++ b/tools/testing/selftests/vfio/lib/vfio_pci_device.c
> @@ -183,7 +183,7 @@ void vfio_pci_dma_map(struct vfio_pci_device *device,
>         list_add(&region->link, &device->dma_regions);
>  }
>
> -static void vfio_iommu_dma_unmap(struct vfio_pci_device *device,
> +static u64 vfio_iommu_dma_unmap(struct vfio_pci_device *device,
>                                  struct vfio_dma_region *region)
>  {
>         struct vfio_iommu_type1_dma_unmap args = {
> @@ -193,9 +193,25 @@ static void vfio_iommu_dma_unmap(struct vfio_pci_device *device,
>         };
>
>         ioctl_assert(device->container_fd, VFIO_IOMMU_UNMAP_DMA, &args);
> +
> +       return args.size;
> +}
> +
> +static u64 vfio_iommu_dma_unmap_all(struct vfio_pci_device *device)
> +{
> +       struct vfio_iommu_type1_dma_unmap args = {
> +               .argsz = sizeof(args),
> +               .iova = 0,
> +               .size = 0,
> +               .flags = VFIO_DMA_UNMAP_FLAG_ALL,
> +       };
> +
> +       ioctl_assert(device->container_fd, VFIO_IOMMU_UNMAP_DMA, &args);
> +
> +       return args.size;
>  }
>
> -static void iommufd_dma_unmap(struct vfio_pci_device *device,
> +static u64 iommufd_dma_unmap(struct vfio_pci_device *device,
>                               struct vfio_dma_region *region)
>  {
>         struct iommu_ioas_unmap args = {
> @@ -206,17 +222,54 @@ static void iommufd_dma_unmap(struct vfio_pci_device *device,
>         };
>
>         ioctl_assert(device->iommufd, IOMMU_IOAS_UNMAP, &args);
> +
> +       return args.length;
> +}
> +
> +static u64 iommufd_dma_unmap_all(struct vfio_pci_device *device)
> +{
> +       struct iommu_ioas_unmap args = {
> +               .size = sizeof(args),
> +               .iova = 0,
> +               .length = UINT64_MAX,
> +               .ioas_id = device->ioas_id,
> +       };
> +
> +       ioctl_assert(device->iommufd, IOMMU_IOAS_UNMAP, &args);
> +
> +       return args.length;
>  }
>
> -void vfio_pci_dma_unmap(struct vfio_pci_device *device,
> +u64 vfio_pci_dma_unmap(struct vfio_pci_device *device,
>                         struct vfio_dma_region *region)
>  {
> +       u64 unmapped;
> +
>         if (device->iommufd)
> -               iommufd_dma_unmap(device, region);
> +               unmapped = iommufd_dma_unmap(device, region);
>         else
> -               vfio_iommu_dma_unmap(device, region);
> +               unmapped = vfio_iommu_dma_unmap(device, region);
>
>         list_del(&region->link);
> +
> +       return unmapped;
> +}
> +
> +u64 vfio_pci_dma_unmap_all(struct vfio_pci_device *device)
> +{
> +       u64 unmapped;
> +       struct vfio_dma_region *curr, *next;
> +
> +       if (device->iommufd)
> +               unmapped = iommufd_dma_unmap_all(device);
> +       else
> +               unmapped = vfio_iommu_dma_unmap_all(device);
> +
> +       list_for_each_entry_safe(curr, next, &device->dma_regions, link) {
> +               list_del(&curr->link);
> +       }
> +
> +       return unmapped;
>  }
>
>  static void vfio_pci_region_get(struct vfio_pci_device *device, int index,
> diff --git a/tools/testing/selftests/vfio/vfio_dma_mapping_test.c b/tools/testing/selftests/vfio/vfio_dma_mapping_test.c
> index ab19c54a774d..e908c1fe7103 100644
> --- a/tools/testing/selftests/vfio/vfio_dma_mapping_test.c
> +++ b/tools/testing/selftests/vfio/vfio_dma_mapping_test.c
> @@ -122,6 +122,8 @@ FIXTURE_TEARDOWN(vfio_dma_mapping_test)
>         vfio_pci_device_cleanup(self->device);
>  }
>
> +#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.

> +
>  TEST_F(vfio_dma_mapping_test, dma_map_unmap)
>  {
>         const u64 size = variant->size ?: getpagesize();
> @@ -192,6 +194,61 @@ TEST_F(vfio_dma_mapping_test, dma_map_unmap)
>         ASSERT_TRUE(!munmap(region.vaddr, size));
>  }
>
> +FIXTURE(vfio_dma_map_limit_test) {
> +       struct vfio_pci_device *device;
> +};
> +
> +FIXTURE_VARIANT(vfio_dma_map_limit_test) {
> +       const char *iommu_mode;
> +};
> +
> +#define FIXTURE_VARIANT_ADD_IOMMU_MODE(_iommu_mode)                            \
> +FIXTURE_VARIANT_ADD(vfio_dma_map_limit_test, _iommu_mode) {                    \
> +       .iommu_mode = #_iommu_mode,                                            \
> +}
> +
> +FIXTURE_VARIANT_ADD_ALL_IOMMU_MODES();
> +
> +FIXTURE_SETUP(vfio_dma_map_limit_test)
> +{
> +       self->device = vfio_pci_device_init(device_bdf, variant->iommu_mode);
> +}
> +
> +FIXTURE_TEARDOWN(vfio_dma_map_limit_test)
> +{
> +       vfio_pci_device_cleanup(self->device);
> +}
> +
> +#undef FIXTURE_VARIANT_ADD_IOMMU_MODE
> +
> +TEST_F(vfio_dma_map_limit_test, end_of_address_space)
> +{
> +       struct vfio_dma_region region = {};
> +       u64 size = getpagesize();
> +       u64 unmapped;
> +
> +       region.vaddr = mmap(NULL, size, PROT_READ | PROT_WRITE,
> +                           MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
> +       ASSERT_NE(region.vaddr, MAP_FAILED);
> +
> +       region.iova = ~(iova_t)0 & ~(size - 1);
> +       region.size = size;
> +
> +       vfio_pci_dma_map(self->device, &region);
> +       printf("Mapped HVA %p (size 0x%lx) at IOVA 0x%lx\n", region.vaddr, size, region.iova);
> +       ASSERT_EQ(region.iova, to_iova(self->device, region.vaddr));
> +
> +       unmapped = vfio_pci_dma_unmap(self->device, &region);
> +       ASSERT_EQ(unmapped, size);
> +
> +       vfio_pci_dma_map(self->device, &region);
> +       printf("Mapped HVA %p (size 0x%lx) at IOVA 0x%lx\n", region.vaddr, size, region.iova);
> +       ASSERT_EQ(region.iova, to_iova(self->device, region.vaddr));
> +
> +       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.
> +}

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.

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

Powered by blists - more mailing lists