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
