Re: [PATCH for-4.19(?)] xen/arm: bootfdt: Fix device tree memory node probing

[Julien Grall <julien@xxxxxxx>]

Hi Michal,

On 26/06/2024 09:04, Michal Orzel wrote:
> Memory node probing is done as part of early_scan_node() that is called
> for each node with depth >= 1 (root node is at depth 0). According to
> Devicetree Specification v0.4, chapter 3.4, /memory node can only exists
> as a top level node. However, Xen incorrectly considers all the nodes with
> unit node name "memory" as RAM. This buggy behavior can result in a
> failure if there are other nodes in the device tree (at depth >= 2) with
> "memory" as unit node name. An example can be a "memory@xxx" node under
> /reserved-memory. Fix it by introducing device_tree_is_memory_node() to
> perform all the required checks to assess if a node is a proper /memory
> node.
>
> Fixes: 3e99c95ba1c8 ("arm, device tree: parse the DTB for RAM location and 
> size")
> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
> ---
> 4.19: This patch is fixing a possible early boot Xen failure (before main
> console is initialized). In my case it results in a warning "Shattering
> superpage is not supported" and panic "Unable to setup the directmap mappings".
>
> If this is too late for this patch to go in, we can backport it after the tree
> re-opens.
The code looks correct to me, but I am not sure about merging it to 4.19.

This is not a new bug (in fact has been there since pretty much Xen on 
Arm was created) and we haven't seen any report until today. So in some 
way it would be best to merge it after 4.19 so it can get more testing.
In the other hand, I guess this will block you. Is this a new platform? 
Is it available?
> ---
>   xen/arch/arm/bootfdt.c | 28 +++++++++++++++++++++++++++-
>   1 file changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index 6e060111d95b..23c968e6955d 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -83,6 +83,32 @@ static bool __init device_tree_node_compatible(const void 
> *fdt, int node,
>       return false;
>   }
>   
> +/*
> + * Check if a node is a proper /memory node according to Devicetree
> + * Specification v0.4, chapter 3.4.
> + */
> +static bool __init device_tree_is_memory_node(const void *fdt, int node,
> +                                              int depth)
> +{
> +    const char *type;
> +    int len;
> +
> +    if ( depth != 1 )
> +        return false;
> +
> +    if ( !device_tree_node_matches(fdt, node, "memory") )
> +        return false;
> +
> +    type = fdt_getprop(fdt, node, "device_type", &len);
> +    if ( !type )
> +        return false;
> +
> +    if ( (len <= 0) || strcmp(type, "memory") )
I would consider to use strncmp() to avoid relying on the property to be 
well-formed (i.e. nul-terminated).
> +        return false;
> +
> +    return true;
> +}
> +
>   void __init device_tree_get_reg(const __be32 **cell, uint32_t address_cells,
>                                   uint32_t size_cells, paddr_t *start,
>                                   paddr_t *size)
> @@ -448,7 +474,7 @@ static int __init early_scan_node(const void *fdt,
>        * populated. So we should skip the parsing.
>        */
>       if ( !efi_enabled(EFI_BOOT) &&
> -         device_tree_node_matches(fdt, node, "memory") )
> +         device_tree_is_memory_node(fdt, node, depth) )
>           rc = process_memory_node(fdt, node, name, depth,
>                                    address_cells, size_cells, 
> bootinfo_get_mem());
>       else if ( depth == 1 && !dt_node_cmp(name, "reserved-memory") )
Cheers,

--
Julien Grall