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

[oleksii.kurochko@xxxxxxxxx]

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