1. Patchew
  2. QEMU
  3. [PATCH v2 00/12] VIRTIO-IOMMU/VFIO: Don't assume 64b IOVA space
  4. View patch

[PATCH v2 05/12] virtio-iommu: Introduce per IOMMUDevice reserved regions

Eric Auger posted 12 patches 2 years, 5 months ago
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com>, Eduardo Habkost <eduardo@habkost.net>, Alex Williamson <alex.williamson@redhat.com>, "Cédric Le Goater" <clg@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>, Eric Auger <eric.auger@redhat.com>, Peter Xu <peterx@redhat.com>, David Hildenbrand <david@redhat.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>
There is a newer version of this series
[PATCH v2 05/12] virtio-iommu: Introduce per IOMMUDevice reserved regions
Posted by Eric Auger 2 years, 5 months ago 
For the time being the per device reserved regions are
just a duplicate of IOMMU wide reserved regions. Subsequent
patches will combine those with host reserved regions, if any.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 include/hw/virtio/virtio-iommu.h |  1 +
 hw/virtio/virtio-iommu.c         | 42 ++++++++++++++++++++++++++------
 2 files changed, 35 insertions(+), 8 deletions(-)

diff --git a/include/hw/virtio/virtio-iommu.h b/include/hw/virtio/virtio-iommu.h
index eea4564782..70b8ace34d 100644
--- a/include/hw/virtio/virtio-iommu.h
+++ b/include/hw/virtio/virtio-iommu.h
@@ -39,6 +39,7 @@ typedef struct IOMMUDevice {
     AddressSpace  as;
     MemoryRegion root;          /* The root container of the device */
     MemoryRegion bypass_mr;     /* The alias of shared memory MR */
+    GList *resv_regions;
 } IOMMUDevice;
 
 typedef struct IOMMUPciBus {
diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index 979cdb5648..ea359b586a 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -624,22 +624,48 @@ static int virtio_iommu_unmap(VirtIOIOMMU *s,
     return ret;
 }
 
+static int consolidate_resv_regions(IOMMUDevice *sdev)
+{
+    VirtIOIOMMU *s = sdev->viommu;
+    int i;
+
+    for (i = 0; i < s->nr_prop_resv_regions; i++) {
+        ReservedRegion *reg = g_new0(ReservedRegion, 1);
+
+        *reg = s->prop_resv_regions[i];
+        sdev->resv_regions = g_list_append(sdev->resv_regions, reg);
+    }
+    return 0;
+}
+
 static ssize_t virtio_iommu_fill_resv_mem_prop(VirtIOIOMMU *s, uint32_t ep,
                                                uint8_t *buf, size_t free)
 {
     struct virtio_iommu_probe_resv_mem prop = {};
     size_t size = sizeof(prop), length = size - sizeof(prop.head), total;
-    int i;
+    IOMMUDevice *sdev;
+    GList *l;
+    int ret;
 
-    total = size * s->nr_prop_resv_regions;
+    sdev = container_of(virtio_iommu_mr(s, ep), IOMMUDevice, iommu_mr);
+    if (!sdev) {
+        return -EINVAL;
+    }
 
+    ret = consolidate_resv_regions(sdev);
+    if (ret) {
+        return ret;
+    }
+
+    total = size * g_list_length(sdev->resv_regions);
     if (total > free) {
         return -ENOSPC;
     }
 
-    for (i = 0; i < s->nr_prop_resv_regions; i++) {
-        unsigned subtype = s->prop_resv_regions[i].type;
-        Range *range = &s->prop_resv_regions[i].range;
+    for (l = sdev->resv_regions; l; l = l->next) {
+        ReservedRegion *reg = l->data;
+        unsigned subtype = reg->type;
+        Range *range = &reg->range;
 
         assert(subtype == VIRTIO_IOMMU_RESV_MEM_T_RESERVED ||
                subtype == VIRTIO_IOMMU_RESV_MEM_T_MSI);
@@ -857,7 +883,7 @@ static IOMMUTLBEntry virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
     bool bypass_allowed;
     int granule;
     bool found;
-    int i;
+    GList *l;
 
     interval.low = addr;
     interval.high = addr + 1;
@@ -895,8 +921,8 @@ static IOMMUTLBEntry virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
         goto unlock;
     }
 
-    for (i = 0; i < s->nr_prop_resv_regions; i++) {
-        ReservedRegion *reg = &s->prop_resv_regions[i];
+    for (l = sdev->resv_regions; l; l = l->next) {
+        ReservedRegion *reg = l->data;
 
         if (range_contains(&reg->range, addr)) {
             switch (reg->type) {
-- 
2.41.0
Re: [PATCH v2 05/12] virtio-iommu: Introduce per IOMMUDevice reserved regions
Posted by Jean-Philippe Brucker 2 years, 4 months ago 
Hi Eric,

On Wed, Sep 13, 2023 at 10:01:40AM +0200, Eric Auger wrote:
> For the time being the per device reserved regions are
> just a duplicate of IOMMU wide reserved regions. Subsequent
> patches will combine those with host reserved regions, if any.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  include/hw/virtio/virtio-iommu.h |  1 +
>  hw/virtio/virtio-iommu.c         | 42 ++++++++++++++++++++++++++------
>  2 files changed, 35 insertions(+), 8 deletions(-)
> 
> diff --git a/include/hw/virtio/virtio-iommu.h b/include/hw/virtio/virtio-iommu.h
> index eea4564782..70b8ace34d 100644
> --- a/include/hw/virtio/virtio-iommu.h
> +++ b/include/hw/virtio/virtio-iommu.h
> @@ -39,6 +39,7 @@ typedef struct IOMMUDevice {
>      AddressSpace  as;
>      MemoryRegion root;          /* The root container of the device */
>      MemoryRegion bypass_mr;     /* The alias of shared memory MR */
> +    GList *resv_regions;
>  } IOMMUDevice;
>  
>  typedef struct IOMMUPciBus {
> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
> index 979cdb5648..ea359b586a 100644
> --- a/hw/virtio/virtio-iommu.c
> +++ b/hw/virtio/virtio-iommu.c
> @@ -624,22 +624,48 @@ static int virtio_iommu_unmap(VirtIOIOMMU *s,
>      return ret;
>  }
>  
> +static int consolidate_resv_regions(IOMMUDevice *sdev)
> +{
> +    VirtIOIOMMU *s = sdev->viommu;
> +    int i;
> +
> +    for (i = 0; i < s->nr_prop_resv_regions; i++) {
> +        ReservedRegion *reg = g_new0(ReservedRegion, 1);
> +
> +        *reg = s->prop_resv_regions[i];
> +        sdev->resv_regions = g_list_append(sdev->resv_regions, reg);
> +    }
> +    return 0;
> +}
> +
>  static ssize_t virtio_iommu_fill_resv_mem_prop(VirtIOIOMMU *s, uint32_t ep,
>                                                 uint8_t *buf, size_t free)
>  {
>      struct virtio_iommu_probe_resv_mem prop = {};
>      size_t size = sizeof(prop), length = size - sizeof(prop.head), total;
> -    int i;
> +    IOMMUDevice *sdev;
> +    GList *l;
> +    int ret;
>  
> -    total = size * s->nr_prop_resv_regions;
> +    sdev = container_of(virtio_iommu_mr(s, ep), IOMMUDevice, iommu_mr);
> +    if (!sdev) {
> +        return -EINVAL;
> +    }
>  
> +    ret = consolidate_resv_regions(sdev);
> +    if (ret) {
> +        return ret;
> +    }
> +
> +    total = size * g_list_length(sdev->resv_regions);
>      if (total > free) {
>          return -ENOSPC;
>      }
>  
> -    for (i = 0; i < s->nr_prop_resv_regions; i++) {
> -        unsigned subtype = s->prop_resv_regions[i].type;
> -        Range *range = &s->prop_resv_regions[i].range;
> +    for (l = sdev->resv_regions; l; l = l->next) {
> +        ReservedRegion *reg = l->data;
> +        unsigned subtype = reg->type;
> +        Range *range = &reg->range;
>  
>          assert(subtype == VIRTIO_IOMMU_RESV_MEM_T_RESERVED ||
>                 subtype == VIRTIO_IOMMU_RESV_MEM_T_MSI);
> @@ -857,7 +883,7 @@ static IOMMUTLBEntry virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
>      bool bypass_allowed;
>      int granule;
>      bool found;
> -    int i;
> +    GList *l;
>  
>      interval.low = addr;
>      interval.high = addr + 1;
> @@ -895,8 +921,8 @@ static IOMMUTLBEntry virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
>          goto unlock;
>      }
>  
> -    for (i = 0; i < s->nr_prop_resv_regions; i++) {
> -        ReservedRegion *reg = &s->prop_resv_regions[i];
> +    for (l = sdev->resv_regions; l; l = l->next) {
> +        ReservedRegion *reg = l->data;

This means translate() now only takes reserved regions into account after
the guest issues a probe request, which only happens if the guest actually
supports the probe feature. It may be better to build the list earlier
(like when creating the IOMMUDevice), and complete it in
set_iova_ranges(). I guess both could call consolidate() which would
rebuild the whole list, for example

Thanks,
Jean

>  
>          if (range_contains(&reg->range, addr)) {
>              switch (reg->type) {
> -- 
> 2.41.0
>
Re: [PATCH v2 05/12] virtio-iommu: Introduce per IOMMUDevice reserved regions
Posted by Eric Auger 2 years, 4 months ago 
Hi Jean,

On 9/29/23 17:52, Jean-Philippe Brucker wrote:
> Hi Eric,
>
> On Wed, Sep 13, 2023 at 10:01:40AM +0200, Eric Auger wrote:
>> For the time being the per device reserved regions are
>> just a duplicate of IOMMU wide reserved regions. Subsequent
>> patches will combine those with host reserved regions, if any.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  include/hw/virtio/virtio-iommu.h |  1 +
>>  hw/virtio/virtio-iommu.c         | 42 ++++++++++++++++++++++++++------
>>  2 files changed, 35 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/hw/virtio/virtio-iommu.h b/include/hw/virtio/virtio-iommu.h
>> index eea4564782..70b8ace34d 100644
>> --- a/include/hw/virtio/virtio-iommu.h
>> +++ b/include/hw/virtio/virtio-iommu.h
>> @@ -39,6 +39,7 @@ typedef struct IOMMUDevice {
>>      AddressSpace  as;
>>      MemoryRegion root;          /* The root container of the device */
>>      MemoryRegion bypass_mr;     /* The alias of shared memory MR */
>> +    GList *resv_regions;
>>  } IOMMUDevice;
>>  
>>  typedef struct IOMMUPciBus {
>> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
>> index 979cdb5648..ea359b586a 100644
>> --- a/hw/virtio/virtio-iommu.c
>> +++ b/hw/virtio/virtio-iommu.c
>> @@ -624,22 +624,48 @@ static int virtio_iommu_unmap(VirtIOIOMMU *s,
>>      return ret;
>>  }
>>  
>> +static int consolidate_resv_regions(IOMMUDevice *sdev)
>> +{
>> +    VirtIOIOMMU *s = sdev->viommu;
>> +    int i;
>> +
>> +    for (i = 0; i < s->nr_prop_resv_regions; i++) {
>> +        ReservedRegion *reg = g_new0(ReservedRegion, 1);
>> +
>> +        *reg = s->prop_resv_regions[i];
>> +        sdev->resv_regions = g_list_append(sdev->resv_regions, reg);
>> +    }
>> +    return 0;
>> +}
>> +
>>  static ssize_t virtio_iommu_fill_resv_mem_prop(VirtIOIOMMU *s, uint32_t ep,
>>                                                 uint8_t *buf, size_t free)
>>  {
>>      struct virtio_iommu_probe_resv_mem prop = {};
>>      size_t size = sizeof(prop), length = size - sizeof(prop.head), total;
>> -    int i;
>> +    IOMMUDevice *sdev;
>> +    GList *l;
>> +    int ret;
>>  
>> -    total = size * s->nr_prop_resv_regions;
>> +    sdev = container_of(virtio_iommu_mr(s, ep), IOMMUDevice, iommu_mr);
>> +    if (!sdev) {
>> +        return -EINVAL;
>> +    }
>>  
>> +    ret = consolidate_resv_regions(sdev);
>> +    if (ret) {
>> +        return ret;
>> +    }
>> +
>> +    total = size * g_list_length(sdev->resv_regions);
>>      if (total > free) {
>>          return -ENOSPC;
>>      }
>>  
>> -    for (i = 0; i < s->nr_prop_resv_regions; i++) {
>> -        unsigned subtype = s->prop_resv_regions[i].type;
>> -        Range *range = &s->prop_resv_regions[i].range;
>> +    for (l = sdev->resv_regions; l; l = l->next) {
>> +        ReservedRegion *reg = l->data;
>> +        unsigned subtype = reg->type;
>> +        Range *range = &reg->range;
>>  
>>          assert(subtype == VIRTIO_IOMMU_RESV_MEM_T_RESERVED ||
>>                 subtype == VIRTIO_IOMMU_RESV_MEM_T_MSI);
>> @@ -857,7 +883,7 @@ static IOMMUTLBEntry virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
>>      bool bypass_allowed;
>>      int granule;
>>      bool found;
>> -    int i;
>> +    GList *l;
>>  
>>      interval.low = addr;
>>      interval.high = addr + 1;
>> @@ -895,8 +921,8 @@ static IOMMUTLBEntry virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
>>          goto unlock;
>>      }
>>  
>> -    for (i = 0; i < s->nr_prop_resv_regions; i++) {
>> -        ReservedRegion *reg = &s->prop_resv_regions[i];
>> +    for (l = sdev->resv_regions; l; l = l->next) {
>> +        ReservedRegion *reg = l->data;
> This means translate() now only takes reserved regions into account after
> the guest issues a probe request, which only happens if the guest actually
> supports the probe feature. It may be better to build the list earlier
> (like when creating the IOMMUDevice), and complete it in
> set_iova_ranges(). I guess both could call consolidate() which would
> rebuild the whole list, for example

you're totally right, I completely missed that point. Will try to follow
your suggestion.

Thanks!

Eric
>
> Thanks,
> Jean
>
>>  
>>          if (range_contains(&reg->range, addr)) {
>>              switch (reg->type) {
>> -- 
>> 2.41.0
>>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.