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

[Peter Xu]

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].

> +                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,

-- 
Peter Xu