diff mbox series

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

Message ID 20230913080423.523953-8-eric.auger@redhat.com (mailing list archive)
State New, archived
Headers show
Series VIRTIO-IOMMU/VFIO: Don't assume 64b IOVA space | expand

Commit Message

Eric Auger Sept. 13, 2023, 8:01 a.m. UTC 
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(+)

Comments

Jean-Philippe Brucker Sept. 29, 2023, 4:15 p.m. UTC | #1 
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
>
Eric Auger Oct. 10, 2023, 2:36 p.m. UTC | #2 
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
>>
diff mbox series

Patch

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);
+
+    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 = {