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

[Auger Eric]

Hi Alex, Peter,

On 8/8/19 5:07 AM, Alex Williamson wrote:
> 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,

I replaced it by IOMMU_ATTR_HW_NESTED_PAGING instead.

Thanks

Eric


> 
> Alex
>