lkml.org 
[lkml]   [2017]   [Jan]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH v3 2/2] efi: efi_mem_reserve(): don't reserve through memblock after mm_init()
Date
Ard Biesheuvel <ard.biesheuvel@linaro.org> writes:

> On 6 January 2017 at 13:02, Nicolai Stange <nicstange@gmail.com> wrote:
>> Ard Biesheuvel <ard.biesheuvel@linaro.org> writes:
>>
>>> On 5 January 2017 at 12:51, Nicolai Stange <nicstange@gmail.com> wrote:
>>>> Before invoking the arch specific handler, efi_mem_reserve() reserves
>>>> the given memory region through memblock.
>>>>
>>>> efi_mem_reserve() can get called after mm_init() though -- through
>>>> efi_bgrt_init(), for example. After mm_init(), memblock is dead and should
>>>> not be used anymore.
>>>>
>>>> Let efi_mem_reserve() check whether memblock is dead and not do the
>>>> reservation if so. Emit a warning from the generic efi_arch mem_reserve()
>>>> in this case: if the architecture doesn't provide any other means of
>>>> registering the region as reserved, the operation would be a nop.
>>>>
>>>> Fixes: 4bc9f92e64c8 ("x86/efi-bgrt: Use efi_mem_reserve() to avoid copying image data")
>>>> Signed-off-by: Nicolai Stange <nicstange@gmail.com>
>>>> ---
>>>> Applicable to next-20170105.
>>>> No changes to v2.
>>>> Boot-tested on x86_64.
>>>>
>>>> drivers/firmware/efi/efi.c | 7 +++++--
>>>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
>>>> index 92914801e388..158a8df2f4af 100644
>>>> --- a/drivers/firmware/efi/efi.c
>>>> +++ b/drivers/firmware/efi/efi.c
>>>> @@ -403,7 +403,10 @@ u64 __init efi_mem_desc_end(efi_memory_desc_t *md)
>>>> return end;
>>>> }
>>>>
>>>> -void __init __weak efi_arch_mem_reserve(phys_addr_t addr, u64 size) {}
>>>> +void __init __weak efi_arch_mem_reserve(phys_addr_t addr, u64 size)
>>>> +{
>>>> + WARN(slab_is_available(), "efi_mem_reserve() has no effect");
>>>> +}
>>>>
>>>> /**
>>>> * efi_mem_reserve - Reserve an EFI memory region
>>>> @@ -419,7 +422,7 @@ void __init __weak efi_arch_mem_reserve(phys_addr_t addr, u64 size) {}
>>>> */
>>>> void __init efi_mem_reserve(phys_addr_t addr, u64 size)
>>>> {
>>>> - if (!memblock_is_region_reserved(addr, size))
>>>> + if (!slab_is_available() && !memblock_is_region_reserved(addr, size))
>>>> memblock_reserve(addr, size);
>>>>
>>
>> More context:
>>
>> /*
>> * Some architectures (x86) reserve all boot services ranges
>> * until efi_free_boot_services() because of buggy firmware
>> * implementations. This means the above memblock_reserve() is
>> * superfluous on x86 and instead what it needs to do is
>> * ensure the @start, @size is not freed.
>> */
>> efi_arch_mem_reserve(addr, size);
>> }
>>
>>
>>> I share Dave's concern: on x86, this will silently ignore the
>>> reservation if slab_is_available() returns true,
>>
>> AFAICS, x86 has got an efi_arch_mem_reserve() which doesn't ignore the
>> reservation at any stage.
>>
>
> Thanks for the clarification. But my concern is whether changing the
> EFI memory map is going to have any effect at this stage, i.e., after
> slab_is_available() returns true: haven't we already communicated to
> the kernel which RAM regions it may allocate from? How does it know
> the memory map has changed, and how do we ensure that it has not
> already allocated from the region we are reserving here?

Ah, I see what you mean. I think it works like this on x86:

All EFI_BOOT_SERVICES_* regions as reported by the firmware are marked
as reserved at memblock unconditionally through the early setup_arch()
=> efi_reserve_boot_services(). This prevents these from getting handed
over to the "normal" kernel MM until efi_free_boot_services()
gets called later on. The latter frees these EFI_BOOT_SERVICES_* regions,
but only if their EFI_MEMORY_RUNTIME flag is not set.

Now, efi_arch_mem_reserve() basically just sets the EFI_MEMORY_RUNTIME
flag, allowing the given region to survive beyond efi_free_boot_services().

Corrolary 1: any efi_mem_reserve() after efi_free_boot_services wouldn't
have any effect.

Corollary 2: anything handed to efi_arch_mem_reserve() must live within
some memory region which had been reported by firmware already.

Indeed, at its very top, there is

if (efi_mem_desc_lookup(addr, &md)) {
pr_err("Failed to lookup EFI memory descriptor for %pa\n", &addr);
return;
}

if (addr + size > md.phys_addr + (md.num_pages << EFI_PAGE_SHIFT)) {
pr_err("Region spans EFI memory descriptors, %pa\n", &addr);
return;
}

For further information, the comment at the x86's efi_arch_mem_reserve()
might be helpful.


I hope this is correct and helps.

Thanks,

Nicolai

\
 
 \ /
  Last update: 2017-01-06 18:47    [W:0.077 / U:0.900 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site