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

[Julien Grall <julien@xxxxxxx>]

Hi,

On 11/01/2022 11:26, 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@xxxxxxxx>
> ---
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Julien Grall <julien@xxxxxxx>
> Cc: Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>
> Cc: Andrii Anisov <Andrii_Anisov@xxxxxxxx>
>
>
> Changelog:
>
> v3: rebased over staging & remove redundand phandle_iommu attribute, discussion:
>         
> https://lists.xenproject.org/archives/html/xen-devel/2021-12/msg01753.html
>
> v2: re-use common iommu dt bindings to let guests know which devices are 
> protected:
>         
> https://lists.xenproject.org/archives/html/xen-devel/2021-10/msg00073.html
>
>   xen/arch/arm/domain_build.c           | 42 +++++++++++++++++++++++++++
>   xen/include/public/device_tree_defs.h |  1 +
>   2 files changed, 43 insertions(+)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 6931c022a2..b82ba72fac 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -845,6 +845,12 @@ static int __init write_properties(struct domain *d, 
> struct kernel_info *kinfo,
>           }
>       }
>   
> +    if ( iommu_node && is_iommu_enabled(d) && dt_device_is_protected(node) )
I think it should be sufficient to check dt_device_is_protected() 
because it is set it means that device behind an IOMMU known by Xen. So 
iommu_node will always be valid.
Furthermore, you can't assign to dom0 a device that was protected with 
enabling the IOMMU for the domain.
> +    {
> +        res = fdt_property_cell(kinfo->fdt, "iommus", GUEST_PHANDLE_IOMMU);
> +        if ( res )
> +            return res;
> +    }
>       return 0;
>   }
>   
> @@ -1479,6 +1485,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 ( !is_iommu_enabled(d) )
> +        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", GUEST_PHANDLE_IOMMU);
Please don't hardocode the phandle for the IOMMU. Instead we should use 
one for an IOMMU that is used by Xen.
This will reduce the risk to use a phandle that could be possibly used 
in the host Device-Tree.
Cheers,

--
Julien Grall