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

[Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>]

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[].

>
> (That's because it is impossible to get to nr_banks > NR_MEM_BANKS.)
Yes, this is my mistake.

--
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel