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

[Julien Grall <julien.grall@arm.com>]

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.

Repairing any wrong inputs should be done on best efforts.

Cheers,

-- 
Julien Grall

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