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

[oleksii.kurochko@xxxxxxxxx]

On Wed, 2024-06-26 at 10:04 +0200, 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.
So if we have a warning/panic and there is no random behaviour, I think
that it would be better to postpone until 4.20 branching.

~ Oleksii

> ---
>  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") )
> +        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") )