[PATCH-for-4.2,v3,2/5] memory: Add IOMMU_ATTR_VFIO_NESTED IOMMU memory region attribute
diff mbox series

Message ID 20190711061857.13086-3-eric.auger@redhat.com
State New
Headers show
Series
  • ARM SMMUv3: Fix spurious notification errors and assert with vfio
Related show

Commit Message

Eric Auger July 11, 2019, 6:18 a.m. UTC
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 <eric.auger@redhat.com>
---
 hw/arm/smmuv3.c       | 12 ++++++++++++
 include/exec/memory.h |  3 ++-
 2 files changed, 14 insertions(+), 1 deletion(-)

Comments

Peter Maydell Aug. 5, 2019, 2:47 p.m. UTC | #1
On Thu, 11 Jul 2019 at 07:19, Eric Auger <eric.auger@redhat.com> 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.

I'm not sure the name of the attribute really captures
the intention here, though I don't have any better
suggestions. Maybe IOMMU_ATTR_VFIO_NEEDS_HW_NESTED_PAGING ?

> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  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;

I'm surprised checkpatch doesn't warn about the space after
the cast here.

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

Could we have a comment documenting what the attribute's meaning
and semantics are, please? (including what the type is that the
data pointer is expected to point to, ie 'bool'.)

(We ought also to document the spapr-specific attribute...)

thanks
-- PMM
Alex Williamson Aug. 8, 2019, 3:07 a.m. UTC | #2
On Thu, 11 Jul 2019 08:18:54 +0200
Eric Auger <eric.auger@redhat.com> 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 <eric.auger@redhat.com>
> ---
>  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
Eric Auger Aug. 22, 2019, 3:15 p.m. UTC | #3
Hi Alex, Peter,

On 8/8/19 5:07 AM, Alex Williamson wrote:
> On Thu, 11 Jul 2019 08:18:54 +0200
> Eric Auger <eric.auger@redhat.com> 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 <eric.auger@redhat.com>
>> ---
>>  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
>

Patch
diff mbox series

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,
 };
 
 /**