Re: [Qemu-devel] [PATCH-for-4.2 v3 2/5] memory: Add IOMMU_ATTR_VFIO_NESTED IOMMU memory region attribute

[Alex Williamson]

On Thu, 11 Jul 2019 08:18:54 +0200
Eric Auger <address@hidden> wrote:

> We introduce a new IOMMU Memory Region attribute,
> IOMMU_ATTR_VFIO_NESTED that tells whether the virtual IOMMU
> requires HW nested paging for VFIO integration.
> 
> Current Intel virtual IOMMU device supports "Caching
> Mode" and does not require 2 stages at physical level to be
> integrated with VFIO. However SMMUv3 does not implement such
> "caching mode" and requires to use HW nested paging.
> 
> As such SMMUv3 is the first IOMMU device to advertise this
> attribute.
> 
> Signed-off-by: Eric Auger <address@hidden>
> ---
>  hw/arm/smmuv3.c       | 12 ++++++++++++
>  include/exec/memory.h |  3 ++-
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index e96d5beb9a..384c02cb91 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -1490,6 +1490,17 @@ static void 
> smmuv3_notify_flag_changed(IOMMUMemoryRegion *iommu,
>      }
>  }
>  
> +static int smmuv3_get_attr(IOMMUMemoryRegion *iommu,
> +                           enum IOMMUMemoryRegionAttr attr,
> +                           void *data)
> +{
> +    if (attr == IOMMU_ATTR_VFIO_NESTED) {
> +        *(bool *) data = true;
> +        return 0;
> +    }
> +    return -EINVAL;
> +}
> +
>  static void smmuv3_iommu_memory_region_class_init(ObjectClass *klass,
>                                                    void *data)
>  {
> @@ -1497,6 +1508,7 @@ static void 
> smmuv3_iommu_memory_region_class_init(ObjectClass *klass,
>  
>      imrc->translate = smmuv3_translate;
>      imrc->notify_flag_changed = smmuv3_notify_flag_changed;
> +    imrc->get_attr = smmuv3_get_attr;
>  }
>  
>  static const TypeInfo smmuv3_type_info = {
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index a078cd033f..e477a630a8 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -204,7 +204,8 @@ struct MemoryRegionOps {
>  };
>  
>  enum IOMMUMemoryRegionAttr {
> -    IOMMU_ATTR_SPAPR_TCE_FD
> +    IOMMU_ATTR_SPAPR_TCE_FD,
> +    IOMMU_ATTR_VFIO_NESTED,
>  };
>  
>  /**

Why VFIO_NESTED vs simply NESTED?  I figure any time we need to include
"VFIO" in the descriptions of something, we're probably not describing
the requirement correctly and it just becomes a meaningless tag that
gets ignored outside of VFIO related things.  If we're trying to
describe an IOMMU MemoryRegion that supports dynamic faulting rather
than requiring a replay to pre-populate it, then simply define that
semantic rather than hand waving some vfio specific interaction.
Thanks,

Alex