From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S967985AbdAFT2u (ORCPT ); Fri, 6 Jan 2017 14:28:50 -0500 Received: from mail-io0-f179.google.com ([209.85.223.179]:33457 "EHLO mail-io0-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965836AbdAFT2m (ORCPT ); Fri, 6 Jan 2017 14:28:42 -0500 MIME-Version: 1.0 In-Reply-To: <87showm682.fsf@gmail.com> References: <20170105125130.2815-1-nicstange@gmail.com> <20170105125130.2815-2-nicstange@gmail.com> <87wpe8mjdk.fsf@gmail.com> <87showm682.fsf@gmail.com> From: Ard Biesheuvel Date: Fri, 6 Jan 2017 19:28:40 +0000 Message-ID: Subject: Re: [PATCH v3 2/2] efi: efi_mem_reserve(): don't reserve through memblock after mm_init() To: Nicolai Stange Cc: Ingo Molnar , Matt Fleming , Thomas Gleixner , "H. Peter Anvin" , "x86@kernel.org" , =?UTF-8?Q?Mika_Penttil=C3=A4?= , Dan Williams , Dave Young , "linux-efi@vger.kernel.org" , "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 6 January 2017 at 17:46, Nicolai Stange wrote: > Ard Biesheuvel writes: > >> On 6 January 2017 at 13:02, Nicolai Stange wrote: >>> Ard Biesheuvel writes: >>> >>>> On 5 January 2017 at 12:51, Nicolai Stange 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 >>>>> --- >>>>> 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. > This is my point exactly. But it appears efi_free_boot_services() occurs much later than I thought, and so there is a sizabe time window where SLAB is up but reservations can still be made. But we don't check whether efi_free_boot_services() has been called. Another problem is that we never check that the reservation is covered by a BootServicesData region, which are the only ones that are guaranteed to be retained up to this point. > Corollary 2: anything handed to efi_arch_mem_reserve() must live within > some memory region which had been reported by firmware already. > Yes, but the EFI memory map describes all of RAM, so this is not an issue by itself. The issue is that the region must have been covered by a BootServicesCode or BootServicesData region > 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