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.or...@amd.com> >> --- >> 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