Re: [Xen-devel] [PATCH v5 2/7] xen/arm: make process_memory_node a device_tree_node_func

[Julien Grall <julien.grall@xxxxxxx>]

Hi Volodymyr,

On 15/08/2019 13:14, Volodymyr Babchuk wrote:
> Julien Grall writes:
>
> > On 15/08/2019 12:24, Julien Grall wrote:
> > > Hi Volodymyr,
> > >
> > > On 15/08/2019 12:20, Volodymyr Babchuk wrote:
> > > > Hi Stefano,
> > > >
> > > > Stefano Stabellini writes:
> > > >
> > > > > On Tue, 13 Aug 2019, Volodymyr Babchuk wrote:
> > > > > > > @@ -162,6 +156,10 @@ static void __init
> > > > > > > process_memory_node(const void *fdt, int node,
> > > > > > >   bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size;
> > > > > > >   bootinfo.mem.nr_banks++;
> > > > > > >   }
> > > > > > > +
> > > > > > > + if ( bootinfo.mem.nr_banks == NR_MEM_BANKS )
> > > > > > > + return -ENOSPC;
> > > > > > Are you sure that this logic is correct?
> > > > > >
> > > > > > For example, if NR_MEM_BANKS is 1, and we have exactly one memory node
> > > > > > in device tree, this function will fail. But it should not. I think you
> > > > > > want this condition: bootinfo.mem.nr_banks > NR_MEM_BANKS
> > > > > You are right, if NR_MEM_BANKS is 1 and we have 1 memory node in device
> > > > > tree the code would return an error while actually it is normal.
> > > > >
> > > > > I think the right check would be:
> > > > >
> > > > >   if ( i < banks && bootinfo.mem.nr_banks == NR_MEM_BANKS )
> > > > >   return -ENOSPC;
> > > > Actually, this does not cover all corner cases. Here is the resulting
> > > > code:
> > > >
> > > >   150 for ( i = 0; i < banks && bootinfo.mem.nr_banks < NR_MEM_BANKS; i++ )
> > > >   151 {
> > > >   152 device_tree_get_reg(&cell, address_cells, size_cells,
> > > > &start, &size);
> > > >   153 if ( !size )
> > > >   154 continue;
> > > >   155 bootinfo.mem.bank[bootinfo.mem.nr_banks].start = start;
> > > >   156 bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size;
> > > >   157 bootinfo.mem.nr_banks++;
> > > >   158 }
> > > >   159
> > > >   160 if ( i < banks && bootinfo.mem.nr_banks == NR_MEM_BANKS )
> > > >   161 return -ENOSPC;
> > > >
> > > > Lines 153-154 cause the issue.
> > > >
> > > > Imagine that NR_MEM_BANKS = 1 and we have two memory nodes in device
> > > > tree with. Nodes have sizes 0 and 1024. Your code will work as
> > > > intended. At the end of loop we will have banks = 2, i = 2 and
> > > > bootinfo.mem.nr_banks = 1.
> > > >
> > > > But if we switch order of memory nodes, so first one will be with size
> > > > 1024 and second one with size 0, your code will return -ENOSPC, because
> > > > we'll have banks = 2, i = 1, bootinfo.mem.nr_banks = 1.
> > > >
> > > > I think, right solution will be to scan all nodes to count nodes
> > > > with size > 0. And then - either return an error or do second loop to
> > > > fill bootinfo.mem.bank[].
> > > To be honest, a memory with size 0 is an error in the DT
> > > provided. So I would not care too much if Xen is not working as
> > > intended.
> > >
> > > If we want to fix this, then we should bail out as we do for missing
> > > 'regs' and invalid 'address-cells'/'size-cells'.
> > I send this too early. I forgot to mention that I would not be happy
> > with parsing the Device-Tree twice just for benefits of wrong DT. If a
> > DT is wrong then we should treat as such and shout at the user.
> Fair enough. But then at line 154 we need to return an error, instead of
> continuing the iterations. And in this case we can simplify the error
> check to (banks > NR_MEM_BANKS).
I am afraid this would not be correct. It is allowed to have multiple memory 
nodes in the device-tree. This function only deal with one node at the times.
In particular banks is the number of regions described in the node. With the 
check you suggest, you would only catch the case where a node contain more banks 
than supported. It does not tell you whether there are enough space left in 
mem.bank[...] to cater the regions described by the node.
So we need the check suggested by Stefano.

Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel