diff mbox series

[RFC,v4,08/27] hw/vfio/common: Force nested if iommu requires it

Message ID 20190527114203.2762-9-eric.auger@redhat.com
State New
Headers show
Series vSMMUv3/pSMMUv3 2 stage VFIO integration | expand

Commit Message

Eric Auger May 27, 2019, 11:41 a.m. UTC 
In case we detect the address space is translated by
a virtual IOMMU which requires nested stages, let's set up
the container with the VFIO_TYPE1_NESTING_IOMMU iommu_type.

Signed-off-by: Eric Auger <eric.auger@redhat.com>

---

v2 -> v3:
- add "nested only is selected if requested by @force_nested"
  comment in this patch
---
 hw/vfio/common.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

Comments

Peter Xu May 28, 2019, 2:47 a.m. UTC | #1 
On Mon, May 27, 2019 at 01:41:44PM +0200, Eric Auger wrote:
> In case we detect the address space is translated by
> a virtual IOMMU which requires nested stages, let's set up
> the container with the VFIO_TYPE1_NESTING_IOMMU iommu_type.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> ---
> 
> v2 -> v3:
> - add "nested only is selected if requested by @force_nested"
>   comment in this patch
> ---
>  hw/vfio/common.c | 27 +++++++++++++++++++++++----
>  1 file changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 1f1deff360..99ade21056 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -1136,14 +1136,19 @@ static void vfio_put_address_space(VFIOAddressSpace *space)
>   * vfio_get_iommu_type - selects the richest iommu_type (v2 first)
>   */
>  static int vfio_get_iommu_type(VFIOContainer *container,
> +                               bool force_nested,
>                                 Error **errp)
>  {
> -    int iommu_types[] = { VFIO_TYPE1v2_IOMMU, VFIO_TYPE1_IOMMU,
> +    int iommu_types[] = { VFIO_TYPE1_NESTING_IOMMU,
> +                          VFIO_TYPE1v2_IOMMU, VFIO_TYPE1_IOMMU,
>                            VFIO_SPAPR_TCE_v2_IOMMU, VFIO_SPAPR_TCE_IOMMU };
>      int i;
>  
>      for (i = 0; i < ARRAY_SIZE(iommu_types); i++) {
>          if (ioctl(container->fd, VFIO_CHECK_EXTENSION, iommu_types[i])) {
> +            if (iommu_types[i] == VFIO_TYPE1_NESTING_IOMMU && !force_nested) {

If force_nested==true and if the kernel does not support
VFIO_TYPE1_NESTING_IOMMU, we will still return other iommu types?
That seems to not match with what "force" mean here.

What I feel like is that we want an "iommu_nest_types[]" which only
contains VFIO_TYPE1_NESTING_IOMMU.  Then:

        if (nested) {
                target_types = iommu_nest_types;
        } else {
                target_types = iommu_types;
        }

        foreach (target_types)
                ...

        return -EINVAL;

Might be clearer?  Then we can drop [2] below since we'll fail earlier
at [1].

> +                continue;
> +            }
>              return iommu_types[i];
>          }
>      }
> @@ -1152,11 +1157,11 @@ static int vfio_get_iommu_type(VFIOContainer *container,
>  }
>  
>  static int vfio_init_container(VFIOContainer *container, int group_fd,
> -                               Error **errp)
> +                               bool force_nested, Error **errp)
>  {
>      int iommu_type, ret;
>  
> -    iommu_type = vfio_get_iommu_type(container, errp);
> +    iommu_type = vfio_get_iommu_type(container, force_nested, errp);
>      if (iommu_type < 0) {
>          return iommu_type;

[1]

>      }
> @@ -1192,6 +1197,14 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
>      VFIOContainer *container;
>      int ret, fd;
>      VFIOAddressSpace *space;
> +    IOMMUMemoryRegion *iommu_mr;
> +    bool force_nested = false;
> +
> +    if (as != &address_space_memory && memory_region_is_iommu(as->root)) {
> +        iommu_mr = IOMMU_MEMORY_REGION(as->root);
> +        memory_region_iommu_get_attr(iommu_mr, IOMMU_ATTR_VFIO_NESTED,
> +                                     (void *)&force_nested);
> +    }
>  
>      space = vfio_get_address_space(as);
>  
> @@ -1252,12 +1265,18 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
>      QLIST_INIT(&container->giommu_list);
>      QLIST_INIT(&container->hostwin_list);
>  
> -    ret = vfio_init_container(container, group->fd, errp);
> +    ret = vfio_init_container(container, group->fd, force_nested, errp);
>      if (ret) {
>          goto free_container_exit;
>      }
>  
> +    if (force_nested && container->iommu_type != VFIO_TYPE1_NESTING_IOMMU) {
> +            error_setg(errp, "nested mode requested by the virtual IOMMU "
> +                       "but not supported by the vfio iommu");
> +    }

[2]

> +
>      switch (container->iommu_type) {
> +    case VFIO_TYPE1_NESTING_IOMMU:
>      case VFIO_TYPE1v2_IOMMU:
>      case VFIO_TYPE1_IOMMU:
>      {
> -- 
> 2.20.1
> 

Regards,
Eric Auger May 28, 2019, 12:51 p.m. UTC | #2 
Hi Peter,

On 5/28/19 4:47 AM, Peter Xu wrote:
> On Mon, May 27, 2019 at 01:41:44PM +0200, Eric Auger wrote:
>> In case we detect the address space is translated by
>> a virtual IOMMU which requires nested stages, let's set up
>> the container with the VFIO_TYPE1_NESTING_IOMMU iommu_type.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>
>> ---
>>
>> v2 -> v3:
>> - add "nested only is selected if requested by @force_nested"
>>   comment in this patch
>> ---
>>  hw/vfio/common.c | 27 +++++++++++++++++++++++----
>>  1 file changed, 23 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 1f1deff360..99ade21056 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -1136,14 +1136,19 @@ static void vfio_put_address_space(VFIOAddressSpace *space)
>>   * vfio_get_iommu_type - selects the richest iommu_type (v2 first)
>>   */
>>  static int vfio_get_iommu_type(VFIOContainer *container,
>> +                               bool force_nested,
>>                                 Error **errp)
>>  {
>> -    int iommu_types[] = { VFIO_TYPE1v2_IOMMU, VFIO_TYPE1_IOMMU,
>> +    int iommu_types[] = { VFIO_TYPE1_NESTING_IOMMU,
>> +                          VFIO_TYPE1v2_IOMMU, VFIO_TYPE1_IOMMU,
>>                            VFIO_SPAPR_TCE_v2_IOMMU, VFIO_SPAPR_TCE_IOMMU };
>>      int i;
>>  
>>      for (i = 0; i < ARRAY_SIZE(iommu_types); i++) {
>>          if (ioctl(container->fd, VFIO_CHECK_EXTENSION, iommu_types[i])) {
>> +            if (iommu_types[i] == VFIO_TYPE1_NESTING_IOMMU && !force_nested) {
> 
> If force_nested==true and if the kernel does not support
> VFIO_TYPE1_NESTING_IOMMU, we will still return other iommu types?
> That seems to not match with what "force" mean here.
> 
> What I feel like is that we want an "iommu_nest_types[]" which only
> contains VFIO_TYPE1_NESTING_IOMMU.  Then:
> 
>         if (nested) {
>                 target_types = iommu_nest_types;
>         } else {
>                 target_types = iommu_types;
>         }
> 
>         foreach (target_types)
>                 ...
> 
>         return -EINVAL;
> 
> Might be clearer?  Then we can drop [2] below since we'll fail earlier
> at [1].

agreed. I can fail immediately in case the nested mode was requested and
not supported. This will be clearer.

Thanks!


Eric
> 
>> +                continue;
>> +            }
>>              return iommu_types[i];
>>          }
>>      }
>> @@ -1152,11 +1157,11 @@ static int vfio_get_iommu_type(VFIOContainer *container,
>>  }
>>  
>>  static int vfio_init_container(VFIOContainer *container, int group_fd,
>> -                               Error **errp)
>> +                               bool force_nested, Error **errp)
>>  {
>>      int iommu_type, ret;
>>  
>> -    iommu_type = vfio_get_iommu_type(container, errp);
>> +    iommu_type = vfio_get_iommu_type(container, force_nested, errp);
>>      if (iommu_type < 0) {
>>          return iommu_type;
> 
> [1]
> 
>>      }
>> @@ -1192,6 +1197,14 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
>>      VFIOContainer *container;
>>      int ret, fd;
>>      VFIOAddressSpace *space;
>> +    IOMMUMemoryRegion *iommu_mr;
>> +    bool force_nested = false;
>> +
>> +    if (as != &address_space_memory && memory_region_is_iommu(as->root)) {
>> +        iommu_mr = IOMMU_MEMORY_REGION(as->root);
>> +        memory_region_iommu_get_attr(iommu_mr, IOMMU_ATTR_VFIO_NESTED,
>> +                                     (void *)&force_nested);
>> +    }
>>  
>>      space = vfio_get_address_space(as);
>>  
>> @@ -1252,12 +1265,18 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
>>      QLIST_INIT(&container->giommu_list);
>>      QLIST_INIT(&container->hostwin_list);
>>  
>> -    ret = vfio_init_container(container, group->fd, errp);
>> +    ret = vfio_init_container(container, group->fd, force_nested, errp);
>>      if (ret) {
>>          goto free_container_exit;
>>      }
>>  
>> +    if (force_nested && container->iommu_type != VFIO_TYPE1_NESTING_IOMMU) {
>> +            error_setg(errp, "nested mode requested by the virtual IOMMU "
>> +                       "but not supported by the vfio iommu");
>> +    }
> 
> [2]
> 
>> +
>>      switch (container->iommu_type) {
>> +    case VFIO_TYPE1_NESTING_IOMMU:
>>      case VFIO_TYPE1v2_IOMMU:
>>      case VFIO_TYPE1_IOMMU:
>>      {
>> -- 
>> 2.20.1
>>
> 
> Regards,
>
diff mbox series

Patch

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 1f1deff360..99ade21056 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1136,14 +1136,19 @@  static void vfio_put_address_space(VFIOAddressSpace *space)
  * vfio_get_iommu_type - selects the richest iommu_type (v2 first)
  */
 static int vfio_get_iommu_type(VFIOContainer *container,
+                               bool force_nested,
                                Error **errp)
 {
-    int iommu_types[] = { VFIO_TYPE1v2_IOMMU, VFIO_TYPE1_IOMMU,
+    int iommu_types[] = { VFIO_TYPE1_NESTING_IOMMU,
+                          VFIO_TYPE1v2_IOMMU, VFIO_TYPE1_IOMMU,
                           VFIO_SPAPR_TCE_v2_IOMMU, VFIO_SPAPR_TCE_IOMMU };
     int i;
 
     for (i = 0; i < ARRAY_SIZE(iommu_types); i++) {
         if (ioctl(container->fd, VFIO_CHECK_EXTENSION, iommu_types[i])) {
+            if (iommu_types[i] == VFIO_TYPE1_NESTING_IOMMU && !force_nested) {
+                continue;
+            }
             return iommu_types[i];
         }
     }
@@ -1152,11 +1157,11 @@  static int vfio_get_iommu_type(VFIOContainer *container,
 }
 
 static int vfio_init_container(VFIOContainer *container, int group_fd,
-                               Error **errp)
+                               bool force_nested, Error **errp)
 {
     int iommu_type, ret;
 
-    iommu_type = vfio_get_iommu_type(container, errp);
+    iommu_type = vfio_get_iommu_type(container, force_nested, errp);
     if (iommu_type < 0) {
         return iommu_type;
     }
@@ -1192,6 +1197,14 @@  static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
     VFIOContainer *container;
     int ret, fd;
     VFIOAddressSpace *space;
+    IOMMUMemoryRegion *iommu_mr;
+    bool force_nested = false;
+
+    if (as != &address_space_memory && memory_region_is_iommu(as->root)) {
+        iommu_mr = IOMMU_MEMORY_REGION(as->root);
+        memory_region_iommu_get_attr(iommu_mr, IOMMU_ATTR_VFIO_NESTED,
+                                     (void *)&force_nested);
+    }
 
     space = vfio_get_address_space(as);
 
@@ -1252,12 +1265,18 @@  static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
     QLIST_INIT(&container->giommu_list);
     QLIST_INIT(&container->hostwin_list);
 
-    ret = vfio_init_container(container, group->fd, errp);
+    ret = vfio_init_container(container, group->fd, force_nested, errp);
     if (ret) {
         goto free_container_exit;
     }
 
+    if (force_nested && container->iommu_type != VFIO_TYPE1_NESTING_IOMMU) {
+            error_setg(errp, "nested mode requested by the virtual IOMMU "
+                       "but not supported by the vfio iommu");
+    }
+
     switch (container->iommu_type) {
+    case VFIO_TYPE1_NESTING_IOMMU:
     case VFIO_TYPE1v2_IOMMU:
     case VFIO_TYPE1_IOMMU:
     {