diff mbox series

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

Message ID 20230913080423.523953-6-eric.auger@redhat.com
State New
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 
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(-)

Comments

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

Patch

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