[v4,2/5] memory: Add IOMMU_ATTR_HW_NESTED_PAGING IOMMU memory region attribute
diff mbox series

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

Commit Message

Eric Auger Aug. 22, 2019, 5:23 p.m. UTC
We introduce a new IOMMU Memory Region attribute,
IOMMU_ATTR_HW_NESTED_PAGING that tells whether the virtual
IOMMU relies on physical IOMMU HW nested paging capability
when protecting host assigned devices.

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>

---

v3 -> v4:
- s/IOMMU_ATTR_VFIO_NESTED/IOMMU_ATTR_HW_NESTED_PAGING
- add comments related to the existing attributes
- fix space after the cast
---
 hw/arm/smmuv3.c       | 12 ++++++++++++
 include/exec/memory.h |  8 +++++++-
 2 files changed, 19 insertions(+), 1 deletion(-)

Comments

Peter Maydell Aug. 27, 2019, 4:19 p.m. UTC | #1
On Thu, 22 Aug 2019 at 18:24, Eric Auger <eric.auger@redhat.com> wrote:
>
> We introduce a new IOMMU Memory Region attribute,
> IOMMU_ATTR_HW_NESTED_PAGING that tells whether the virtual
> IOMMU relies on physical IOMMU HW nested paging capability
> when protecting host assigned devices.

I'm still not really happy with the name of this attribute.
"IOMMU_ATTR_HW_NESTED_PAGING" sounds like it ought to mean
"true if this IOMMU supports/is using hardware nested paging". What
your commit message suggests it means is "true if this IOMMU
*needs* hardware nested paging", but there's no NEEDS in the
attribute name.

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

The code changes look good to me though.

thanks
-- PMM
Eric Auger Aug. 28, 2019, 6:24 a.m. UTC | #2
Hi Peter,

On 8/27/19 6:19 PM, Peter Maydell wrote:
> On Thu, 22 Aug 2019 at 18:24, Eric Auger <eric.auger@redhat.com> wrote:
>>
>> We introduce a new IOMMU Memory Region attribute,
>> IOMMU_ATTR_HW_NESTED_PAGING that tells whether the virtual
>> IOMMU relies on physical IOMMU HW nested paging capability
>> when protecting host assigned devices.
> 
> I'm still not really happy with the name of this attribute.
> "IOMMU_ATTR_HW_NESTED_PAGING" sounds like it ought to mean
> "true if this IOMMU supports/is using hardware nested paging". What
> your commit message suggests it means is "true if this IOMMU
> *needs* hardware nested paging", but there's no NEEDS in the
> attribute name.

OK I will respin and add "_NEED_".

Thanks

Eric
> 
>> 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>
> 
> The code changes look good to me though.
> 
> thanks
> -- PMM
>

Patch
diff mbox series

diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 2eaf07fb5f..a37e3da240 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_HW_NESTED_PAGING) {
+        *(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 ecca388e69..4342f16980 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -212,7 +212,13 @@  typedef struct MemoryRegionClass {
 
 
 enum IOMMUMemoryRegionAttr {
-    IOMMU_ATTR_SPAPR_TCE_FD
+    /* Retrieve an integer corresponding to the TCE file descriptor */
+    IOMMU_ATTR_SPAPR_TCE_FD,
+    /*
+     * Retrieve a boolean that indicates whether the virtual IOMMU relies
+     * on physical IOMMU HW nested paging to protect host assigned devices
+     */
+    IOMMU_ATTR_HW_NESTED_PAGING,
 };
 
 /**