Re: [RFC PATCH 21/21] xen/arm: vIOMMU: Modify the partial device tree for dom0less

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Rahul,

On 01/12/2022 17:02, Rahul Singh wrote:
> 
> 
> To configure IOMMU in guest for passthrough devices, user will need to
> copy the unmodified "iommus" property from host device tree to partial
> device tree. To enable the dom0 linux kernel to confiure the IOMMU
> correctly replace the phandle in partial device tree with virtual
> IOMMU phandle when "iommus" property is set.
> 
> Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx>
> ---
>  xen/arch/arm/domain_build.c | 31 ++++++++++++++++++++++++++++++-
>  1 file changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 7cd99a6771..afb3e76409 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -3235,7 +3235,35 @@ static int __init handle_prop_pfdt(struct kernel_info 
> *kinfo,
>      return ( propoff != -FDT_ERR_NOTFOUND ) ? propoff : 0;
>  }
> 
> -static int __init scan_pfdt_node(struct kernel_info *kinfo, const void *pfdt,
> +static void modify_pfdt_node(void *pfdt, int nodeoff)
> +{
> +    int proplen, i, rc;
> +    const fdt32_t *prop;
> +    fdt32_t *prop_c;
> +
> +    prop = fdt_getprop(pfdt, nodeoff, "iommus", &proplen);
> +    if ( !prop )
> +        return;
> +
> +    prop_c = xzalloc_bytes(proplen);
> +
> +    for ( i = 0; i < proplen / 8; ++i )
> +    {
> +        prop_c[i * 2] = cpu_to_fdt32(GUEST_PHANDLE_VSMMUV3);
> +        prop_c[i * 2 + 1] = prop[i * 2 + 1];
> +    }
> +
> +    rc = fdt_setprop(pfdt, nodeoff, "iommus", prop_c, proplen);
> +    if ( rc )
> +    {
> +        dprintk(XENLOG_ERR, "Can't set the iommus property in partial FDT");
> +        return;
> +    }
> +
> +    return;
> +}
> +
> +static int __init scan_pfdt_node(struct kernel_info *kinfo, void *pfdt,
>                                   int nodeoff,
>                                   uint32_t address_cells, uint32_t size_cells,
>                                   bool scan_passthrough_prop)
> @@ -3261,6 +3289,7 @@ static int __init scan_pfdt_node(struct kernel_info 
> *kinfo, const void *pfdt,
>      node_next = fdt_first_subnode(pfdt, nodeoff);
>      while ( node_next > 0 )
>      {
> +        modify_pfdt_node(pfdt, node_next);
You do not protect this call in any way and there is no check inside it whether 
viommu is enabled or not.
This means that even with viommu disabled, if a user copies the iommus property 
to partial dtb
(which is understandable as the flow is to copy almost everything), you will 
perform the phandle replacement.
I do not think we should do that (no need for an extra code).

>          scan_pfdt_node(kinfo, pfdt, node_next, address_cells, size_cells,
>                         scan_passthrough_prop);
>          node_next = fdt_next_subnode(pfdt, node_next);
> --
> 2.25.1
> 
> 

~Michal