Re: [PATCH v2 07/12] virtio-iommu: Implement set_iova_ranges() callback

[Jean-Philippe Brucker]

On Wed, Sep 13, 2023 at 10:01:42AM +0200, Eric Auger wrote:
> The implementation populates the array of per IOMMUDevice
> host reserved regions.
> 
> It is forbidden to have conflicting sets of host IOVA ranges
> to be applied onto the same IOMMU MR (implied by different
> host devices).
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> ---
> 
> v1 -> v2:
> - Forbid conflicting sets of host resv regions
> ---
>  include/hw/virtio/virtio-iommu.h |  2 ++
>  hw/virtio/virtio-iommu.c         | 48 ++++++++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+)
> 
> diff --git a/include/hw/virtio/virtio-iommu.h 
> b/include/hw/virtio/virtio-iommu.h
> index 70b8ace34d..31b69c8261 100644
> --- a/include/hw/virtio/virtio-iommu.h
> +++ b/include/hw/virtio/virtio-iommu.h
> @@ -40,6 +40,8 @@ typedef struct IOMMUDevice {
>      MemoryRegion root;          /* The root container of the device */
>      MemoryRegion bypass_mr;     /* The alias of shared memory MR */
>      GList *resv_regions;
> +    Range *host_resv_regions;
> +    uint32_t nr_host_resv_regions;
>  } IOMMUDevice;
>  
>  typedef struct IOMMUPciBus {
> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
> index ea359b586a..ed2df5116f 100644
> --- a/hw/virtio/virtio-iommu.c
> +++ b/hw/virtio/virtio-iommu.c
> @@ -20,6 +20,7 @@
>  #include "qemu/osdep.h"
>  #include "qemu/log.h"
>  #include "qemu/iov.h"
> +#include "qemu/range.h"
>  #include "exec/target_page.h"
>  #include "hw/qdev-properties.h"
>  #include "hw/virtio/virtio.h"
> @@ -1158,6 +1159,52 @@ static int 
> virtio_iommu_set_page_size_mask(IOMMUMemoryRegion *mr,
>      return 0;
>  }
>  
> +static int virtio_iommu_set_iova_ranges(IOMMUMemoryRegion *mr,
> +                                        uint32_t nr_ranges,
> +                                        struct Range *iova_ranges,
> +                                        Error **errp)
> +{
> +    IOMMUDevice *sdev = container_of(mr, IOMMUDevice, iommu_mr);
> +    uint32_t nr_host_resv_regions;
> +    Range *host_resv_regions;
> +    int ret = -EINVAL;
> +
> +    if (!nr_ranges) {
> +        return 0;
> +    }
> +
> +    if (sdev->host_resv_regions) {
> +        range_inverse_array(nr_ranges, iova_ranges,
> +                            &nr_host_resv_regions, &host_resv_regions,
> +                            0, UINT64_MAX);
> +        if (nr_host_resv_regions != sdev->nr_host_resv_regions) {
> +            goto error;
> +        }
> +        for (int i = 0; i < nr_host_resv_regions; i++) {
> +            Range *new = &host_resv_regions[i];
> +            Range *existing = &sdev->host_resv_regions[i];
> +
> +            if (!range_contains_range(existing, new)) {
> +                goto error;
> +            }
> +        }
> +        ret = 0;
> +        goto out;
> +    }
> +
> +    range_inverse_array(nr_ranges, iova_ranges,
> +                        &sdev->nr_host_resv_regions, 
> &sdev->host_resv_regions,
> +                        0, UINT64_MAX);

Can set_iova_ranges() only be called for the first time before the guest
has had a chance to issue a probe request?  Maybe we could add a
sanity-check that the guest hasn't issued a probe request yet, since we
can't notify about updated reserved regions.

I'm probably misremembering because I thought Linux set up IOMMU contexts
(including probe requests) before enabling DMA master in PCI which cause
QEMU VFIO to issue these calls. I'll double check.

Thanks,
Jean

> +
> +    return 0;
> +error:
> +    error_setg(errp, "IOMMU mr=%s Conflicting host reserved regions set!",
> +               mr->parent_obj.name);
> +out:
> +    g_free(host_resv_regions);
> +    return ret;
> +}
> +
>  static void virtio_iommu_system_reset(void *opaque)
>  {
>      VirtIOIOMMU *s = opaque;
> @@ -1453,6 +1500,7 @@ static void 
> virtio_iommu_memory_region_class_init(ObjectClass *klass,
>      imrc->replay = virtio_iommu_replay;
>      imrc->notify_flag_changed = virtio_iommu_notify_flag_changed;
>      imrc->iommu_set_page_size_mask = virtio_iommu_set_page_size_mask;
> +    imrc->iommu_set_iova_ranges = virtio_iommu_set_iova_ranges;
>  }
>  
>  static const TypeInfo virtio_iommu_info = {
> -- 
> 2.41.0
>