[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH for-4.19(?)] xen/arm: bootfdt: Fix device tree memory node probing
On Thu, 2024-06-27 at 11:32 +0100, Julien Grall wrote: > > > On 27/06/2024 08:40, Michal Orzel wrote: > > 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. > > At this stage of the release we should think whether the bug is > critical > enough (rather than the risk is low enough) to be merged. IMHO, it is > not because this has been there for the past 12 years... > > But if we focus on the "riskness". We are rightly changing an > interface > which possibly someone may have (bogusly) relied on. So there is a > lowish risk that you may end up to break use-case late in the release > (we are a couple of weeks away) for use-case that never worked in the > past 12 years. > > We should also think what the worse that can happen if there is a bug > in > your series. The worse is we would not be able to boot on already > supported platform. This would be quite a bad regression this late in > the release. For Device-Tree parsing, I don't think it is enough to > just > test on just a handful of platforms this late in the release. > > Which is why to me the answer to "It should be in 4.19" is not > straightforward. If we merge post 4.19, then we give the chance to > people to test, update & adjust their setup if needed. > > Anyway, ultimately this is Oleksii's decision as the release manager. I agree with Julien, it would be better to postpone this patch series until 4.20 branching. ~ Oleksii > > [...] > > > > > +/* > > > > + * 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. > > I can't find such code in path. Do you have any pointer? > > > Also, let's suppose it is somehow not terminated. In that case how > > could libfdt set len to be > 0? > > The FDT will store the length of the property otherwise you would not > be > able to encode property that are just a list of cells (i.e. numbers). > > > It needs to know where is the end of the string to calculate the > > length. > > For the name and the description, it is unclear to why would > fdt_getprop() would only work for string property. > > Cheers, >
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |