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

[Eric Auger <eric.auger@redhat.com>]

Hi Jean,
On 9/29/23 18:15, Jean-Philippe Brucker wrote:
> 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 added a warning if the set_iova is called after the probe

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