[U-Boot] [BUG] x86: incorrect dram definition in dram_init_banksize().

[Alexander Graf]

On 08.01.18 12:31, Heinrich Schuchardt wrote:
> On 01/06/2018 11:51 PM, Heinrich Schuchardt wrote:
>> Function efi_add_known_memory uses the configured DRAM banks
>> (gd->bd->bi_dram) to define the memory that an EFI application may use.
>>
>> For qemu-x86_defconfig this will result in the first 1 MB of physical
>> memory being available. Here we find the BIOS, interrupt vectors and the
>> VGA memory (0xA0000-0xBFFFF).
>>
>> As a result grubia32.efi writes unknowingly to the video memory.
>>
>> For reference:
>> In function install_e820_map() we block
>> [ISA_START_ADDRESS, ISA_END_ADDRESS[.
>>
>> The problem seems to stem from file arch/x86/cpu/qemu/dram.c, function
>> dram_init_banksize():
>>
>> gd->bd->bi_dram[0].start = 0;
>> gd->bd->bi_dram[0].size = gd->ram_size;
>>
>> Probably exluding [ISA_START_ADDRESS, ISA_END_ADDRESS[ is not sufficient
>> as this does not protect interrupt vectors.
>>
>> Could you, please, provide reasonable values.
>>
>> Maybe a better idea would be to define reserved memory like the ones
>> that we find in the device trees, e.g.
>>
>>         reserved-memory {
>>                 #address-cells = <2>;
>>                 #size-cells = <2>;
>>                 ranges;
>>
>>                 /* 16 MiB reserved for Hardware ROM Firmware */
>>                 hwrom_reserved: hwrom at 0 {
>>                         reg = <0x0 0x0 0x0 0x1000000>;
>>                         no-map;
>>                 };
>>
>> But U-Boot does not yet support the concept of reserved memory.
> 
> This is how memory is reserved in arch/arm/mach-meson/board.c:
> 
> static void meson_board_add_reserved_memory(void *fdt, u64 start, u64 size)
> {
>        int ret;
> 
>        ret = fdt_add_mem_rsv(fdt, start, size);
>        if (ret)
>                printf("Could not reserve zone @ 0x%llx\n", start);
> 
>        if (IS_ENABLED(CONFIG_EFI_LOADER)) {
>                efi_add_memory_map(start,
>                       ALIGN(size, EFI_PAGE_SIZE) >> EFI_PAGE_SHIFT,
>                       EFI_RESERVED_MEMORY_TYPE, false);
>        }
> }
> 
> Similar code can be found in:
> arch/arm/cpu/armv8/fsl-layerscape/fdt.c
> board/raspberrypi/rpi/rpi.c
> 
> Maybe we can generalize this.

Yes, I think we need to make memory reservation in dt integrate better
with efi_loader. A good number of people are used to just reserve random
ranges in their DT and expect Linux not to make use of them - which then
breaks with EFI.

What really should happen is that bootefi at the stage when it detects a
working fdt passed in, goes through that fdt's memory reservations and
declares all of them as reserved regions in the efi memory map as well.
That way we coherently mirror all reservations into EFI land.

I originally didn't want to do that because the device tree reservations
might be for different firmware versions, but I'd much rather have a
working device with a few kb less RAM than one where 2 cores end up
fighting over pieces of RAM ;).

As for x86, I suppose we need to explicitly mark some regions as
reserved in the board file, as that doesn't really use DT?


Alex