Re: [Qemu-arm] [RFC v4 08/27] hw/vfio/common: Force nested if iommu requires it

[Auger Eric]

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 <address@hidden>
>>
>> ---
>>
>> 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,
>