[Top] [All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On 15/08/2019 14:51, Volodymyr Babchuk wrote:


Julien Grall writes:


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.

Okay, I see the point there.


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.

Yes, right. But, we can free space:

(banks + bootinfo.mem.nr_banks > NR_MEM_BANKS)
I guess you mean before the loop? If so, this is possible but then you will ignore the full node rather than trying to add as much regions as possible.
To give an exagerated example, imagine a the DT has a single node with NR_MEM_BANKS + 1. You will end up to not add any banks, so Xen will see no memory. This is not very ideal. 




So we need the check suggested by Stefano.

As I said earlier, it does not cover all corner cases. It will behave
differently, depending on ordering of entries in "reg" property (if we
allow zero-length regions). Yes, this is the user's problem, but I think
it is better to have consistent behavior even in case of user's fault.
Where did I say it cover all corner cases? As I said "If a DT is wrong then we should treat as such and shout at the user." 




But were saying, that it is error to have region with zero length. So,
instead of

  device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
  if ( !size )
      continue;

we need

  device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
  if ( !size )
      return -ENOENT; >
In this case, check suggested by Stefano will work fine, but it will be
redundant, because we can either do early check for free space in the
array, or just write


See above for the early check.


  if ( i < banks )
      return -ENOSPC;
This is another option for Stefano check. I don't particularly care on the check as long as it is correct. 



If we want array to be filled no mater what.

Anyways, I don't want to press on this anymore. I just wanted to share
my concerns.
You are preaching the converted. However, I have already pointed multiple times that we need to fill the array as much as possible. This is not a user fault but a Xen limitation. So I am not sure why you are pushing for an early check. 

Cheers,

--
Julien Grall

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

 

©2013 Xen Project, A Linux Foundation Collaborative Project. All Rights Reserved.
Linux Foundation is a registered trademark of The Linux Foundation.
Xen Project is a trademark of The Linux Foundation.

Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.