From patchwork Wed Jul 16 13:13:48 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mark Salter X-Patchwork-Id: 4567021 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork1.web.kernel.org (Postfix) with ESMTP id B30F49F295 for ; Wed, 16 Jul 2014 13:16:09 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id AF92B201B4 for ; Wed, 16 Jul 2014 13:16:08 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 9E02D20145 for ; Wed, 16 Jul 2014 13:16:07 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1X7P2L-0000w3-Sp; Wed, 16 Jul 2014 13:14:21 +0000 Received: from mx1.redhat.com ([209.132.183.28]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1X7P2I-0000mD-Mg for linux-arm-kernel@lists.infradead.org; Wed, 16 Jul 2014 13:14:19 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s6GDDnYI029031 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 16 Jul 2014 09:13:49 -0400 Received: from [10.3.113.32] (ovpn-113-32.phx2.redhat.com [10.3.113.32]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s6GDDmnK012026; Wed, 16 Jul 2014 09:13:48 -0400 Message-ID: <1405516428.25580.58.camel@deneb.redhat.com> Subject: Re: [PATCH] efi/arm64: efistub: don't abort if base of DRAM is occupied From: Mark Salter To: Leif Lindholm Date: Wed, 16 Jul 2014 09:13:48 -0400 In-Reply-To: <20140715152836.GX4179@bivouac.eciton.net> References: <1405351521-12010-1-git-send-email-ard.biesheuvel@linaro.org> <1405363248.25580.12.camel@deneb.redhat.com> <20140715100221.GU4179@bivouac.eciton.net> <1405429860.25580.25.camel@deneb.redhat.com> <20140715135418.GV4179@bivouac.eciton.net> <1405434206.25580.31.camel@deneb.redhat.com> <20140715144944.GW4179@bivouac.eciton.net> <1405436677.25580.34.camel@deneb.redhat.com> <20140715152836.GX4179@bivouac.eciton.net> Organization: Red Hat, Inc Mime-Version: 1.0 X-Scanned-By: MIMEDefang 2.68 on 10.5.11.26 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20140716_061418_799679_E44245FB X-CRM114-Status: GOOD ( 33.85 ) X-Spam-Score: -5.0 (-----) Cc: mark.rutland@arm.com, linux-efi@vger.kernel.org, Ard Biesheuvel , catalin.marinas@arm.com, roy.franz@linaro.org, matt.fleming@intel.com, linux-arm-kernel@lists.infradead.org X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Tue, 2014-07-15 at 16:28 +0100, Leif Lindholm wrote: > On Tue, Jul 15, 2014 at 11:04:37AM -0400, Mark Salter wrote: > > On Tue, 2014-07-15 at 15:49 +0100, Leif Lindholm wrote: > > > On Tue, Jul 15, 2014 at 10:23:26AM -0400, Mark Salter wrote: > > > > On Tue, 2014-07-15 at 14:54 +0100, Leif Lindholm wrote: > > > > > On Tue, Jul 15, 2014 at 09:11:00AM -0400, Mark Salter wrote: > > > > > > On Tue, 2014-07-15 at 11:02 +0100, Leif Lindholm wrote: > > > > > > > > @@ -273,6 +282,10 @@ static void __init free_boot_services(void) > > > > > > > > continue; > > > > > > > > } > > > > > > > > > > > > > > > > + /* Don't free anything below kernel */ > > > > > > > > + if (md->phys_addr < PHYS_OFFSET) > > > > > > > > + continue; > > > > > > > > + > > > > > > > > > > > > > > Is the spin table area really allocated as BOOT_SERVICES_*? > > > > > > > > > > > > No. It is EFI_RESERVED_TYPE. But if UEFI is allowed below the kernel, > > > > > > then there could be BS code/data memory which we'd want to ignore. > > > > > > > > > > Well, if it is boot service code/data - then there is no need for us > > > > > to keep it around after ExitBootServices(). > > > > > > > > One would think, but EFI has proven to be less than strictly compliant > > > > in that regard in the past. I'm inclined to keep the boot services > > > > around until after SetVirtualAddressMap just in case. > > > > > > But the function you add this clause to will still throw away all boot > > > services code/data regions - just with this modification it skips > > > those that happen to lie lower in the address space than the kernel. > > Returning to the actual code we are discussing here: > The hunk above has no bearing on whether boot services regions are > generally unmapped or not. It only filters explicitly those boot > services regions that happen to be lower in memory than the kernel, > and keep them around for the duration of the system. It doesn't filter them to keep them around, it filters them to avoid calling free_bootmem_late() with an invalid address. If there are UEFI regions below the kernel, we don't want to call memblock_reserve() or free_bootmem_late() for them. > > > > > > > (And I do agree with Mark R here - let's not work around bugs that > > > don't exist yet.) > > > > > > > I'm not sure if they still exist or not, but on Foundation, I saw a > > crash in SetVirtualAddressMap unless I kept BS regions around. > > For the topic of keeping boot services code around: > I did also see issues with not keeping boot services regions on v7 - > ages ago. I have not seen it this year, and I _really_ want to see if > any such issues resurface. My view is that a problem has been seen in the past with tianocore for arm64. There is no harm in delaying the freeing of BS regions. The memory becomes usable for general kernel use at early_initcall time. This issue has also been seen with x86 firmware and some of those same vendors will be providing arm64 firmware. The problem isn't reproducible now, but I'm not sure if there was a bug fix for it or if it just went underground for some reason. Kernel boot may succeed by chance if some needed BS memory isn't reused by kernel. > So post-3.16 I would quite like to see the > call to free_boot_services() moved earlier to flush out any such > issues before we see large-scale deployments. > You can just get rid of it altogether: diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c index 453b7f8..06b59d9 100644 --- a/arch/arm64/kernel/efi.c +++ b/arch/arm64/kernel/efi.c @@ -177,9 +177,7 @@ static __init void reserve_regions(void) if (is_normal_ram(md)) early_init_dt_add_memory_arch(paddr, size); - if (is_reserve_region(md) || - md->type == EFI_BOOT_SERVICES_CODE || - md->type == EFI_BOOT_SERVICES_DATA) { + if (is_reserve_region(md)) { memblock_reserve(paddr, size); if (uefi_debug) pr_cont("*"); @@ -191,122 +189,6 @@ static __init void reserve_regions(void) } -static u64 __init free_one_region(u64 start, u64 end) -{ - u64 size = end - start; - - if (uefi_debug) - pr_info(" EFI freeing: 0x%012llx-0x%012llx\n", start, end - 1); - - free_bootmem_late(start, size); - return size; -} - -static u64 __init free_region(u64 start, u64 end) -{ - u64 map_start, map_end, total = 0; - - if (end <= start) - return total; - - map_start = (u64)memmap.phys_map; - map_end = PAGE_ALIGN(map_start + (memmap.map_end - memmap.map)); - map_start &= PAGE_MASK; - - if (start < map_end && end > map_start) { - /* region overlaps UEFI memmap */ - if (start < map_start) - total += free_one_region(start, map_start); - - if (map_end < end) - total += free_one_region(map_end, end); - } else - total += free_one_region(start, end); - - return total; -} - -static void __init free_boot_services(void) -{ - u64 total_freed = 0; - u64 keep_end, free_start, free_end; - efi_memory_desc_t *md; - - /* - * If kernel uses larger pages than UEFI, we have to be careful - * not to inadvertantly free memory we want to keep if there is - * overlap at the kernel page size alignment. We do not want to - * free is_reserve_region() memory nor the UEFI memmap itself. - * - * The memory map is sorted, so we keep track of the end of - * any previous region we want to keep, remember any region - * we want to free and defer freeing it until we encounter - * the next region we want to keep. This way, before freeing - * it, we can clip it as needed to avoid freeing memory we - * want to keep for UEFI. - */ - - keep_end = 0; - free_start = 0; - - for_each_efi_memory_desc(&memmap, md) { - u64 paddr, npages, size; - - if (is_reserve_region(md)) { - /* - * We don't want to free any memory from this region. - */ - if (free_start) { - /* adjust free_end then free region */ - if (free_end > md->phys_addr) - free_end -= PAGE_SIZE; - total_freed += free_region(free_start, free_end); - free_start = 0; - } - keep_end = md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT); - continue; - } - - if (md->type != EFI_BOOT_SERVICES_CODE && - md->type != EFI_BOOT_SERVICES_DATA) { - /* no need to free this region */ - continue; - } - - /* - * We want to free memory from this region. - */ - paddr = md->phys_addr; - npages = md->num_pages; - memrange_efi_to_native(&paddr, &npages); - size = npages << PAGE_SHIFT; - - if (free_start) { - if (paddr <= free_end) - free_end = paddr + size; - else { - total_freed += free_region(free_start, free_end); - free_start = paddr; - free_end = paddr + size; - } - } else { - free_start = paddr; - free_end = paddr + size; - } - if (free_start < keep_end) { - free_start += PAGE_SIZE; - if (free_start >= free_end) - free_start = 0; - } - } - if (free_start) - total_freed += free_region(free_start, free_end); - - if (total_freed) - pr_info("Freed 0x%llx bytes of EFI boot services memory", - total_freed); -} - void __init efi_init(void) { struct efi_fdt_params params; @@ -439,8 +321,6 @@ static int __init arm64_enter_virtual_mode(void) kfree(virtmap); - free_boot_services(); - if (status != EFI_SUCCESS) { pr_err("Failed to set EFI virtual address map! [%lx]\n", status);