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

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Julien,

On 26/06/2024 22:13, Julien Grall wrote:
> 
> 
> 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?
Stefano answered this question. Also, IMO this change is quite straightforward
and does not introduce any engineering doubt, so I'm not really sure if it needs
more testing.

> 
>> ---
>>   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).
Are you sure? AFAIR, libfdt returns NULL and -FDT_ERR_TRUNCATED as len if a 
string is non null terminated.
Also, let's suppose it is somehow not terminated. In that case how could libfdt 
set len to be > 0?
It needs to know where is the end of the string to calculate the length.

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

~Michal