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: 33729 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-pa0-f69.google.com (mail-pa0-f69.google.com [209.85.220.69]) by ip-10-151-82-157.ec2.internal (Postfix) with ESMTPS id 1425820792 for ; Wed, 16 Jul 2014 13:15:29 +0000 (UTC) Received: by mail-pa0-f69.google.com with SMTP id kx10sf6544851pab.8 for ; Wed, 16 Jul 2014 06:15:29 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:delivered-to:message-id:subject:from:to:date :in-reply-to:references:organization:mime-version:cc:precedence :list-id:list-unsubscribe:list-archive:list-post:list-help :list-subscribe:sender:errors-to:x-original-sender :x-original-authentication-results:mailing-list:content-type :content-transfer-encoding; bh=qbTXV85Q+OcPJrJY/4ZjaXRolrJLaN7zwhlhELgppps=; b=nGqsXGNi+ARMxr0jzNjFZtK0RZT91Hb2peV23ts6jxhRn7vT4oe7MLiov+lOJHyfJ7 8pJIAsEl5ThG2L3+A13b0tFJaDFNj04TlW+f1r+bt7lBAzOX0RcyT+oRp9B8GEoTzvNH mjjv/GkwYIZN1ICWByxN2PqZhM0aTpTClf8oF64VUzBKqp/l/19DFhWLgPhjVtSZM4Ou ZUQJj8IQRaha2DIrBBB2U10AwKisHoFSqfndm8wkQOD/y3imibK07/VUil7vMzGpG7kZ ccaLssclmqR3qKAQPiiQMZl2CsIdQr8NKJMyWhpYdqA4vT9AQUw4rlai0ip/amOf3wyk HuRw== X-Gm-Message-State: ALoCoQkwnjOmnJEIT/zfzi3c9HUUUweC/a9PZSNJq6cHJT4RqINDx9U0zlKa8xgzgt6hGw/RP5Vn X-Received: by 10.66.124.199 with SMTP id mk7mr1990905pab.15.1405516529240; Wed, 16 Jul 2014 06:15:29 -0700 (PDT) X-BeenThere: patchwork-forward@linaro.org Received: by 10.140.32.38 with SMTP id g35ls376176qgg.1.gmail; Wed, 16 Jul 2014 06:15:29 -0700 (PDT) X-Received: by 10.52.119.179 with SMTP id kv19mr24397073vdb.3.1405516529129; Wed, 16 Jul 2014 06:15:29 -0700 (PDT) Received: from mail-vc0-f180.google.com (mail-vc0-f180.google.com [209.85.220.180]) by mx.google.com with ESMTPS id sq3si7980263vec.65.2014.07.16.06.15.29 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Wed, 16 Jul 2014 06:15:29 -0700 (PDT) Received-SPF: pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.220.180 as permitted sender) client-ip=209.85.220.180; Received: by mail-vc0-f180.google.com with SMTP id ij19so1561345vcb.39 for ; Wed, 16 Jul 2014 06:15:29 -0700 (PDT) X-Received: by 10.52.244.81 with SMTP id xe17mr24358586vdc.24.1405516529028; Wed, 16 Jul 2014 06:15:29 -0700 (PDT) X-Forwarded-To: patchwork-forward@linaro.org X-Forwarded-For: patch@linaro.org patchwork-forward@linaro.org Delivered-To: patch@linaro.org Received: by 10.221.37.5 with SMTP id tc5csp297638vcb; Wed, 16 Jul 2014 06:15:28 -0700 (PDT) X-Received: by 10.68.112.225 with SMTP id it1mr29861591pbb.23.1405516527735; Wed, 16 Jul 2014 06:15:27 -0700 (PDT) Received: from bombadil.infradead.org (bombadil.infradead.org. [2001:1868:205::9]) by mx.google.com with ESMTPS id yc8si7587960pab.212.2014.07.16.06.15.27 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 16 Jul 2014 06:15:27 -0700 (PDT) Received-SPF: none (google.com: linux-arm-kernel-bounces+patch=linaro.org@lists.infradead.org does not designate permitted sender hosts) client-ip=2001:1868:205::9; 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 (-----) X-Spam-Report: SpamAssassin version 3.4.0 on bombadil.infradead.org summary: Content analysis details: (-5.0 points) pts rule name description ---- ---------------------- -------------------------------------------------- -5.0 RCVD_IN_DNSWL_HI RBL: Sender listed at http://www.dnswl.org/, high trust [209.132.183.28 listed in list.dnswl.org] -0.0 SPF_HELO_PASS SPF: HELO matches SPF record -0.0 RP_MATCHES_RCVD Envelope sender domain matches handover relay domain -0.0 SPF_PASS SPF: sender matches SPF record 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+patch=linaro.org@lists.infradead.org X-Removed-Original-Auth: Dkim didn't pass. X-Original-Sender: msalter@redhat.com X-Original-Authentication-Results: mx.google.com; spf=pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.220.180 as permitted sender) smtp.mail=patch+caf_=patchwork-forward=linaro.org@linaro.org Mailing-list: list patchwork-forward@linaro.org; contact patchwork-forward+owners@linaro.org X-Google-Group-Id: 836684582541 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);