Re: [PATCH v4 19/22] xen/arm: vIOMMU: Modify the partial device tree for dom0less

[Oleksandr Tyshchenko <olekstysh@gmail.com>]

On 10/7/26 03:07, Milan Djokic wrote:

Hello Milan

> From: Rahul Singh <rahul.singh@arm.com>
> 
> 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 configure 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@arm.com>
> Signed-off-by: Milan Djokic <milan_djokic@epam.com>
> ---
>   xen/common/device-tree/dom0less-build.c | 40 ++++++++++++++++++++++++-
>   1 file changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/common/device-tree/dom0less-build.c b/xen/common/device-tree/dom0less-build.c
> index fcbeb8adbd..7280bcf72b 100644
> --- a/xen/common/device-tree/dom0less-build.c
> +++ b/xen/common/device-tree/dom0less-build.c
> @@ -331,7 +331,44 @@ 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,
> +#ifdef CONFIG_ARM_VIRTUAL_IOMMU
> +static void __init 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);
> +    if ( !prop_c )
> +        return;
> +
> +    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;
> +}
> +#else
> +    static inline void __init modify_pfdt_node(void *pfdt, int nodeoff)
> +    {
> +        return;
> +    }
> +#endif
> +
> +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)
> @@ -357,6 +394,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);

I might miss something, but ^^^ returns void, so allocation or vSID 
assigment failures (viommu_allocate_vsid() is added by next commit 
#20/22) are ignored. So the guest might be created with the host phandle 
and SID in its device tree or with a half-updated device tree. The IOMMU 
may (will?) not work and nobody is told. Should not we return the error 
here?

Also looks like the each rewritten node also leaks prop_c.



>           rc = scan_pfdt_node(kinfo, pfdt, node_next, address_cells, size_cells,
>                               scan_passthrough_prop);
>           if ( rc )