[RFC v2] tag IOMMU-protected devices in dom0 fdt

[RFC v2] tag IOMMU-protected devices in dom0 fdt

[Sergiy Kibrik]

At the moment, dom0 can't distinguish which devices are protected by
IOMMU and which are not. In some cases, this can cause swiotlb bounce
buffer use for DMA addresses above 32 bits, which in turn can lead
to poor performance.

Previous discussions at [1,2] agreed upon passing info about IOMMU via
device tree. This series does that in a way consistent with existing iommu bindings.

[1] https://lore.kernel.org/all/1392913234-25429-1-git-send-email-julien.grall__16109.9684810781$1392913341$gmane$org@linaro.org/
[2] https://lore.kernel.org/all/cover.1633106362.git.roman_skakun@epam.com/

Sergiy Kibrik (2):
  xen/arm: introduce dummy iommu node for dom0
  arm/xen: don't use xen DMA ops when the device is protected

Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien@xen.org>
Cc: Roman Skakun <rm.skakun@gmail.com>
Cc: Oleksandr Tyshchenko <olekstysh@gmail.com>
Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Cc: Andrii Anisov <Andrii_Anisov@epam.com>
-- 
2.25.1

[XEN PATCH 1/1] xen/arm: introduce dummy iommu node for dom0

[Sergiy Kibrik]

Currently no IOMMU properties are exposed to dom0, thus kernel by default
assumes no protection and enables swiotlb-xen, which leads to costly and
unnecessary buffers bouncing.

To let kernel know which device is behing IOMMU and hence needs no swiotlb
services we introduce dummy xen-iommu node in FDT and link protected device
nodes to it, using here device tree iommu bindings.

Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
---
 xen/arch/arm/domain_build.c           | 44 +++++++++++++++++++++++++++
 xen/include/asm-arm/kernel.h          |  3 ++
 xen/include/public/device_tree_defs.h |  1 +
 3 files changed, 48 insertions(+)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 6cfc772e66..951ca0a0cb 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -623,6 +623,12 @@ static int __init write_properties(struct domain *d, struct kernel_info *kinfo,
         }
     }
 
+    if ( iommu_node && kinfo->phandle_iommu && dt_device_is_protected(node) )
+    {
+        res = fdt_property_cell(kinfo->fdt, "iommus", kinfo->phandle_iommu);
+        if ( res )
+            return res;
+    }
     return 0;
 }
 
@@ -948,6 +954,38 @@ static int __init make_cpus_node(const struct domain *d, void *fdt)
     return res;
 }
 
+static int __init make_iommu_node(const struct domain *d,
+                                  const struct kernel_info *kinfo)
+{
+    const char compat[] = "xen,iommu-el2-v1";
+    int res;
+
+    if ( !kinfo->phandle_iommu )
+        return 0;
+
+    dt_dprintk("Create iommu node\n");
+
+    res = fdt_begin_node(kinfo->fdt, "xen-iommu");
+    if ( res )
+        return res;
+
+    res = fdt_property(kinfo->fdt, "compatible", compat, sizeof(compat));
+    if ( res )
+        return res;
+
+    res = fdt_property_cell(kinfo->fdt, "#iommu-cells", 0);
+    if ( res )
+        return res;
+
+    res = fdt_property_cell(kinfo->fdt, "phandle", kinfo->phandle_iommu);
+
+    res = fdt_end_node(kinfo->fdt);
+    if ( res )
+        return res;
+
+    return res;
+}
+
 static int __init make_gic_node(const struct domain *d, void *fdt,
                                 const struct dt_device_node *node)
 {
@@ -1584,6 +1622,10 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo,
         if ( res )
             return res;
 
+        res = make_iommu_node(d, kinfo);
+        if ( res )
+            return res;
+
         res = make_memory_node(d, kinfo->fdt, addrcells, sizecells, &kinfo->mem);
         if ( res )
             return res;
@@ -2177,6 +2219,8 @@ static int __init prepare_dtb_hwdom(struct domain *d, struct kernel_info *kinfo)
     ASSERT(dt_host && (dt_host->sibling == NULL));
 
     kinfo->phandle_gic = dt_interrupt_controller->phandle;
+    if ( is_iommu_enabled(d) )
+        kinfo->phandle_iommu = GUEST_PHANDLE_IOMMU;
     fdt = device_tree_flattened;
 
     new_size = fdt_totalsize(fdt) + DOM0_FDT_EXTRA_SIZE;
diff --git a/xen/include/asm-arm/kernel.h b/xen/include/asm-arm/kernel.h
index 874aa108a7..efe09cd1e0 100644
--- a/xen/include/asm-arm/kernel.h
+++ b/xen/include/asm-arm/kernel.h
@@ -39,6 +39,9 @@ struct kernel_info {
     /* GIC phandle */
     uint32_t phandle_gic;
 
+    /* dummy iommu phandle */
+    uint32_t phandle_iommu;
+
     /* loader to use for this kernel */
     void (*load)(struct kernel_info *info);
     /* loader specific state */
diff --git a/xen/include/public/device_tree_defs.h b/xen/include/public/device_tree_defs.h
index 209d43de3f..df58944bd0 100644
--- a/xen/include/public/device_tree_defs.h
+++ b/xen/include/public/device_tree_defs.h
@@ -7,6 +7,7 @@
  * onwards. Reserve a high value for the GIC phandle.
  */
 #define GUEST_PHANDLE_GIC (65000)
+#define GUEST_PHANDLE_IOMMU (GUEST_PHANDLE_GIC + 1)
 
 #define GUEST_ROOT_ADDRESS_CELLS 2
 #define GUEST_ROOT_SIZE_CELLS 2
-- 
2.25.1

[PATCH 1/1] arm/xen: don't use xen DMA ops when the device is protected by an IOMMU

[Sergiy Kibrik]

Only Xen is able to know if a device can safely avoid to use xen-swiotlb.
However since Xen links FDT nodes of protected devices to special dummy
xen-iommu node we can use that information to decide whether
xen-swiotlb is needed.

Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
---
 arch/arm/mm/dma-mapping.c   | 2 +-
 arch/arm/xen/enlighten.c    | 9 +++++++++
 arch/arm64/mm/dma-mapping.c | 2 +-
 include/xen/swiotlb-xen.h   | 1 +
 4 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index c4b8df2ad328..fc875dd16e0e 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -2280,7 +2280,7 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 	set_dma_ops(dev, dma_ops);
 
 #ifdef CONFIG_XEN
-	if (xen_initial_domain())
+	if (xen_initial_domain() && !xen_is_protected_device(dev))
 		dev->dma_ops = &xen_swiotlb_dma_ops;
 #endif
 	dev->archdata.dma_ops_setup = true;
diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 49f566ad9acb..b36659238db3 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -66,6 +66,15 @@ static __read_mostly unsigned int xen_events_irq;
 uint32_t xen_start_flags;
 EXPORT_SYMBOL(xen_start_flags);
 
+bool xen_is_protected_device(struct device *dev)
+{
+	struct fwnode_handle *fwnode =
+		fwnode_find_reference(dev_fwnode(dev), "iommus", 0) ;
+	if (IS_ERR(fwnode))
+		return false;
+	return of_device_is_compatible(to_of_node(fwnode), "xen,iommu-el2-v1");
+}
+
 int xen_unmap_domain_gfn_range(struct vm_area_struct *vma,
 			       int nr, struct page **pages)
 {
diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index 93e87b287556..68248e72e052 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -53,7 +53,7 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 		iommu_setup_dma_ops(dev, dma_base, size);
 
 #ifdef CONFIG_XEN
-	if (xen_initial_domain())
+	if (xen_initial_domain() && !xen_is_protected_device(dev))
 		dev->dma_ops = &xen_swiotlb_dma_ops;
 #endif
 }
diff --git a/include/xen/swiotlb-xen.h b/include/xen/swiotlb-xen.h
index d5eaf9d682b8..00b2782430fb 100644
--- a/include/xen/swiotlb-xen.h
+++ b/include/xen/swiotlb-xen.h
@@ -8,6 +8,7 @@ void xen_dma_sync_for_cpu(struct device *dev, dma_addr_t handle,
 			  size_t size, enum dma_data_direction dir);
 void xen_dma_sync_for_device(struct device *dev, dma_addr_t handle,
 			     size_t size, enum dma_data_direction dir);
+bool xen_is_protected_device(struct device *dev);
 
 extern int xen_swiotlb_init(int verbose, bool early);
 extern const struct dma_map_ops xen_swiotlb_dma_ops;
-- 
2.25.1

Re: [XEN PATCH 1/1] xen/arm: introduce dummy iommu node for dom0

[Stefano Stabellini]

On Wed, 22 Dec 2021, Sergiy Kibrik wrote:
> Currently no IOMMU properties are exposed to dom0, thus kernel by default
> assumes no protection and enables swiotlb-xen, which leads to costly and
> unnecessary buffers bouncing.
> 
> To let kernel know which device is behing IOMMU and hence needs no swiotlb
> services we introduce dummy xen-iommu node in FDT and link protected device
> nodes to it, using here device tree iommu bindings.
> 
> Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>

Hi Sergiy,

Thanks for the patch, I like how simple it is. FYI the patch doesn't
apply cleanly to staging any longer, you might need to rebase it.


> ---
>  xen/arch/arm/domain_build.c           | 44 +++++++++++++++++++++++++++
>  xen/include/asm-arm/kernel.h          |  3 ++
>  xen/include/public/device_tree_defs.h |  1 +
>  3 files changed, 48 insertions(+)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 6cfc772e66..951ca0a0cb 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -623,6 +623,12 @@ static int __init write_properties(struct domain *d, struct kernel_info *kinfo,
>          }
>      }
>  
> +    if ( iommu_node && kinfo->phandle_iommu && dt_device_is_protected(node) )
> +    {
> +        res = fdt_property_cell(kinfo->fdt, "iommus", kinfo->phandle_iommu);
> +        if ( res )
> +            return res;
> +    }
>      return 0;
>  }
>  
> @@ -948,6 +954,38 @@ static int __init make_cpus_node(const struct domain *d, void *fdt)
>      return res;
>  }
>  
> +static int __init make_iommu_node(const struct domain *d,
> +                                  const struct kernel_info *kinfo)
> +{
> +    const char compat[] = "xen,iommu-el2-v1";
> +    int res;
> +
> +    if ( !kinfo->phandle_iommu )
> +        return 0;
> +
> +    dt_dprintk("Create iommu node\n");
> +
> +    res = fdt_begin_node(kinfo->fdt, "xen-iommu");
> +    if ( res )
> +        return res;
> +
> +    res = fdt_property(kinfo->fdt, "compatible", compat, sizeof(compat));
> +    if ( res )
> +        return res;
> +
> +    res = fdt_property_cell(kinfo->fdt, "#iommu-cells", 0);
> +    if ( res )
> +        return res;
> +
> +    res = fdt_property_cell(kinfo->fdt, "phandle", kinfo->phandle_iommu);
> +
> +    res = fdt_end_node(kinfo->fdt);
> +    if ( res )
> +        return res;
> +
> +    return res;
> +}
> +
>  static int __init make_gic_node(const struct domain *d, void *fdt,
>                                  const struct dt_device_node *node)
>  {
> @@ -1584,6 +1622,10 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo,
>          if ( res )
>              return res;
>  
> +        res = make_iommu_node(d, kinfo);
> +        if ( res )
> +            return res;
> +
>          res = make_memory_node(d, kinfo->fdt, addrcells, sizecells, &kinfo->mem);
>          if ( res )
>              return res;
> @@ -2177,6 +2219,8 @@ static int __init prepare_dtb_hwdom(struct domain *d, struct kernel_info *kinfo)
>      ASSERT(dt_host && (dt_host->sibling == NULL));
>  
>      kinfo->phandle_gic = dt_interrupt_controller->phandle;
> +    if ( is_iommu_enabled(d) )
> +        kinfo->phandle_iommu = GUEST_PHANDLE_IOMMU;

It doesn't look like we need to save GUEST_PHANDLE_IOMMU under kinfo.
GUEST_PHANDLE_IOMMU is static. Instead we can just:

if ( !is_iommu_enabled(d) )
    return 0;

at the beginning of make_iommu_node. Same for write_properties, we can
just skip the kinfo->phandle_iommu check and use GUEST_PHANDLE_IOMMU in
fdt_property_cell.





>      fdt = device_tree_flattened;
>  
>      new_size = fdt_totalsize(fdt) + DOM0_FDT_EXTRA_SIZE;
> diff --git a/xen/include/asm-arm/kernel.h b/xen/include/asm-arm/kernel.h
> index 874aa108a7..efe09cd1e0 100644
> --- a/xen/include/asm-arm/kernel.h
> +++ b/xen/include/asm-arm/kernel.h
> @@ -39,6 +39,9 @@ struct kernel_info {
>      /* GIC phandle */
>      uint32_t phandle_gic;
>  
> +    /* dummy iommu phandle */
> +    uint32_t phandle_iommu;
> +
>      /* loader to use for this kernel */
>      void (*load)(struct kernel_info *info);
>      /* loader specific state */
> diff --git a/xen/include/public/device_tree_defs.h b/xen/include/public/device_tree_defs.h
> index 209d43de3f..df58944bd0 100644
> --- a/xen/include/public/device_tree_defs.h
> +++ b/xen/include/public/device_tree_defs.h
> @@ -7,6 +7,7 @@
>   * onwards. Reserve a high value for the GIC phandle.
>   */
>  #define GUEST_PHANDLE_GIC (65000)
> +#define GUEST_PHANDLE_IOMMU (GUEST_PHANDLE_GIC + 1)
>  
>  #define GUEST_ROOT_ADDRESS_CELLS 2
>  #define GUEST_ROOT_SIZE_CELLS 2
> -- 
> 2.25.1
> 
> 

Re: [PATCH 1/1] arm/xen: don't use xen DMA ops when the device is protected by an IOMMU

[Stefano Stabellini]

On Wed, 22 Dec 2021, Sergiy Kibrik wrote:
> Only Xen is able to know if a device can safely avoid to use xen-swiotlb.
> However since Xen links FDT nodes of protected devices to special dummy
> xen-iommu node we can use that information to decide whether
> xen-swiotlb is needed.
> 
> Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
> ---
>  arch/arm/mm/dma-mapping.c   | 2 +-
>  arch/arm/xen/enlighten.c    | 9 +++++++++
>  arch/arm64/mm/dma-mapping.c | 2 +-
>  include/xen/swiotlb-xen.h   | 1 +
>  4 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index c4b8df2ad328..fc875dd16e0e 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -2280,7 +2280,7 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
>  	set_dma_ops(dev, dma_ops);
>  
>  #ifdef CONFIG_XEN
> -	if (xen_initial_domain())
> +	if (xen_initial_domain() && !xen_is_protected_device(dev))
>  		dev->dma_ops = &xen_swiotlb_dma_ops;
>  #endif
>  	dev->archdata.dma_ops_setup = true;
> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> index 49f566ad9acb..b36659238db3 100644
> --- a/arch/arm/xen/enlighten.c
> +++ b/arch/arm/xen/enlighten.c
> @@ -66,6 +66,15 @@ static __read_mostly unsigned int xen_events_irq;
>  uint32_t xen_start_flags;
>  EXPORT_SYMBOL(xen_start_flags);
>  
> +bool xen_is_protected_device(struct device *dev)
> +{
> +	struct fwnode_handle *fwnode =
> +		fwnode_find_reference(dev_fwnode(dev), "iommus", 0) ;
> +	if (IS_ERR(fwnode))
> +		return false;
> +	return of_device_is_compatible(to_of_node(fwnode), "xen,iommu-el2-v1");
> +}

We need to add a description of the "xen,iommu-el2-v1" compatible node
under Documentation/devicetree/bindings. Maybe it could be added to
Documentation/devicetree/bindings/arm/xen.txt, but it could also be its
own new file.


>  int xen_unmap_domain_gfn_range(struct vm_area_struct *vma,
>  			       int nr, struct page **pages)
>  {
> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
> index 93e87b287556..68248e72e052 100644
> --- a/arch/arm64/mm/dma-mapping.c
> +++ b/arch/arm64/mm/dma-mapping.c
> @@ -53,7 +53,7 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
>  		iommu_setup_dma_ops(dev, dma_base, size);
>  
>  #ifdef CONFIG_XEN
> -	if (xen_initial_domain())
> +	if (xen_initial_domain() && !xen_is_protected_device(dev))
>  		dev->dma_ops = &xen_swiotlb_dma_ops;

This patch needs to be rebased on the latest master. You'll see that now
we have a more sophisticated xen_swiotlb_detect(), instead of the simple
xen_initial_domain() we used to have. Still, xen_swiotlb_detect() is
global, not per device, so I think this change would still apply as is,
resulting in:

if (xen_swiotlb_detect() && !xen_is_protected_device(dev))
    dev->dma_ops = &xen_swiotlb_dma_ops;


>  #endif
>  }
> diff --git a/include/xen/swiotlb-xen.h b/include/xen/swiotlb-xen.h
> index d5eaf9d682b8..00b2782430fb 100644
> --- a/include/xen/swiotlb-xen.h
> +++ b/include/xen/swiotlb-xen.h
> @@ -8,6 +8,7 @@ void xen_dma_sync_for_cpu(struct device *dev, dma_addr_t handle,
>  			  size_t size, enum dma_data_direction dir);
>  void xen_dma_sync_for_device(struct device *dev, dma_addr_t handle,
>  			     size_t size, enum dma_data_direction dir);
> +bool xen_is_protected_device(struct device *dev);
>  
>  extern int xen_swiotlb_init(int verbose, bool early);
>  extern const struct dma_map_ops xen_swiotlb_dma_ops;