[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 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;
+    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

  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     }
  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.

Repairing any wrong inputs should be done on best efforts.


Julien Grall

Xen-devel mailing list



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