[U-Boot] [PATCH 1/1] efi_loader: fix bug in efi_add_known_memory

[Heinrich Schuchardt]

On 09/18/2017 11:04 PM, Alexander Graf wrote:
> 
> 
> On 18.09.17 12:56, Heinrich Schuchardt wrote:
>> In efi_add_known_memory the start address is correctly rounded
>> up to a mulitple of EFI_PAGE_SIZE.
>> By this rounding we may loose up to EFI_PAGE_MASK bytes.
>> The current code does not take this loss of available memory
>> into account when calculating the number of pages.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
>> ---
>>   lib/efi_loader/efi_memory.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
>> index c1a080e2e9..38b0e1d808 100644
>> --- a/lib/efi_loader/efi_memory.c
>> +++ b/lib/efi_loader/efi_memory.c
>> @@ -444,8 +444,14 @@ __weak void efi_add_known_memory(void)
>>           u64 ram_start = gd->bd->bi_dram[i].start;
>>           u64 ram_size = gd->bd->bi_dram[i].size;
>>           u64 start = (ram_start + EFI_PAGE_MASK) & ~EFI_PAGE_MASK;
>> -        u64 pages = (ram_size + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
>> +        u64 pages;
>>   +        if (ram_size <= EFI_PAGE_MASK)
>> +            continue;
> 
> Why? The old code accounted that as 1 page in use.
> 
>> +        pages = (ram_start + ram_size - start + EFI_PAGE_MASK) >>
>> +            EFI_PAGE_SHIFT;
> 
> Hmm, looking at that code it seems quite bogus to me. Imagine we pass in
> a start address of 0x1001. Then the code will move start to 0x2000.
> That's just plain wrong - the region should start at 0x1000, no?
> 


I have only looked at part of the usage.

The function is used to add available memory in the init function based
on the DRAM definition.

I have been looking only at the addition of available memory in the
context of Simon's attempt to get the Sandbox running with EFI.

Here we will have to add available memory that we create via malloc.
There is not guarantee that available memory added in the init function
is aligned to EFI_PAGE_SIZE. In this special case the current rounding
logic does not fit.

The function is used to mark used memory elsewhere.
In this case the current logic is correct.

For real machines I have not yet seen memory bank definitions that do
not use multiples of 4096.

Best regards

Heinrich

>> +        if (pages == 0)
>> +            continue;
> 
> Is this necessary? efi_add_memory_map() already checks for pages == 0.
> 
> 
> Alex
> 
> 
>>           efi_add_memory_map(start, pages, EFI_CONVENTIONAL_MEMORY,
>>                      false);
>>       }
>>
>