From patchwork Thu Jan 15 08:12:24 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ard Biesheuvel X-Patchwork-Id: 43143 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-la0-f69.google.com (mail-la0-f69.google.com [209.85.215.69]) by ip-10-151-82-157.ec2.internal (Postfix) with ESMTPS id D1943240BA for ; Thu, 15 Jan 2015 08:12:47 +0000 (UTC) Received: by mail-la0-f69.google.com with SMTP id gd6sf7143343lab.0 for ; Thu, 15 Jan 2015 00:12:46 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:delivered-to:mime-version:in-reply-to:references :date:message-id:from:to:cc:subject:precedence:reply-to:list-id :list-unsubscribe:list-archive:list-post:list-help:list-subscribe :content-type:content-transfer-encoding:errors-to:x-original-sender :x-original-authentication-results:mailing-list; bh=4pCoztUGjyUdIs4hvmMv55g4+i21+Uokyj43pqfE9dE=; b=Wf8meQMJZYEDDrb09nLSi8Uf1cMwF0OUEeDwNuJshYRGHG6SsTr7eTMNskWHm1DjIB 3zla1LUA++9dW+vz8gA4d1EiNHLXjg3IhCCakk1TeYGstSBJ4EV+0OsWevyHIwAylM0W BGaeaVyQB+2j6qEouhUSzXQ9ys/jKgqXs3lnP0BG5rG5a7/Jx5iVAXeL8F4SjrZ/aQB6 ZncIHrWBMVlpnPscSvxN9KgN1zSKj1j7ptzjJ3TNmJL7WCar9LvXg+EpSajjEa/vMLKo jKcKgPnltNuSUjy+AbPz4oB8fdimozZv3AEwQpCJKDgbXD7YK0hB8OwWDg5U2oc4ix/8 CafQ== X-Gm-Message-State: ALoCoQkjarWGYjK8yDdsh+SVG+i4ruhr54xCrOy7soHv42bMhPaFH2TcFvMBgPx+rfZ1znwf48GT X-Received: by 10.112.84.226 with SMTP id c2mr1033599lbz.5.1421309566752; Thu, 15 Jan 2015 00:12:46 -0800 (PST) X-BeenThere: patchwork-forward@linaro.org Received: by 10.152.6.130 with SMTP id b2ls174789laa.27.gmail; Thu, 15 Jan 2015 00:12:46 -0800 (PST) X-Received: by 10.112.225.166 with SMTP id rl6mr8150806lbc.58.1421309566609; Thu, 15 Jan 2015 00:12:46 -0800 (PST) Received: from mail-lb0-f181.google.com (mail-lb0-f181.google.com. [209.85.217.181]) by mx.google.com with ESMTPS id jd8si661453lbc.99.2015.01.15.00.12.46 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Thu, 15 Jan 2015 00:12:46 -0800 (PST) Received-SPF: pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.217.181 as permitted sender) client-ip=209.85.217.181; Received: by mail-lb0-f181.google.com with SMTP id u14so2471443lbd.12 for ; Thu, 15 Jan 2015 00:12:46 -0800 (PST) X-Received: by 10.152.197.5 with SMTP id iq5mr8324686lac.6.1421309566119; Thu, 15 Jan 2015 00:12:46 -0800 (PST) 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.112.9.200 with SMTP id c8csp1952980lbb; Thu, 15 Jan 2015 00:12:45 -0800 (PST) X-Received: by 10.50.117.68 with SMTP id kc4mr9005976igb.25.1421309564568; Thu, 15 Jan 2015 00:12:44 -0800 (PST) Received: from lists.sourceforge.net (lists.sourceforge.net. [216.34.181.88]) by mx.google.com with ESMTPS id t7si3916349igh.3.2015.01.15.00.12.43 (version=TLSv1 cipher=RC4-SHA bits=128/128); Thu, 15 Jan 2015 00:12:44 -0800 (PST) Received-SPF: pass (google.com: domain of edk2-devel-bounces@lists.sourceforge.net designates 216.34.181.88 as permitted sender) client-ip=216.34.181.88; Received: from localhost ([127.0.0.1] helo=sfs-ml-2.v29.ch3.sourceforge.com) by sfs-ml-2.v29.ch3.sourceforge.com with esmtp (Exim 4.76) (envelope-from ) id 1YBfXe-0005sr-SI; Thu, 15 Jan 2015 08:12:34 +0000 Received: from sog-mx-1.v43.ch3.sourceforge.com ([172.29.43.191] helo=mx.sourceforge.net) by sfs-ml-2.v29.ch3.sourceforge.com with esmtp (Exim 4.76) (envelope-from ) id 1YBfXd-0005sl-3j for edk2-devel@lists.sourceforge.net; Thu, 15 Jan 2015 08:12:33 +0000 Received-SPF: pass (sog-mx-1.v43.ch3.sourceforge.com: domain of linaro.org designates 209.85.215.52 as permitted sender) client-ip=209.85.215.52; envelope-from=ard.biesheuvel@linaro.org; helo=mail-la0-f52.google.com; Received: from mail-la0-f52.google.com ([209.85.215.52]) by sog-mx-1.v43.ch3.sourceforge.com with esmtps (TLSv1:RC4-SHA:128) (Exim 4.76) id 1YBfXb-0002uK-3T for edk2-devel@lists.sourceforge.net; Thu, 15 Jan 2015 08:12:33 +0000 Received: by mail-la0-f52.google.com with SMTP id hs14so12275491lab.11 for ; Thu, 15 Jan 2015 00:12:24 -0800 (PST) MIME-Version: 1.0 X-Received: by 10.112.169.34 with SMTP id ab2mr8226326lbc.77.1421309544613; Thu, 15 Jan 2015 00:12:24 -0800 (PST) Received: by 10.112.12.36 with HTTP; Thu, 15 Jan 2015 00:12:24 -0800 (PST) In-Reply-To: <7F1BAD85ADEA444D97065A60D2E97EE501CCA592@SHSMSX101.ccr.corp.intel.com> References: <1421161997-19958-1-git-send-email-ard.biesheuvel@linaro.org> <54B54499.50808@redhat.com> <7F1BAD85ADEA444D97065A60D2E97EE501CCA592@SHSMSX101.ccr.corp.intel.com> Date: Thu, 15 Jan 2015 08:12:24 +0000 Message-ID: From: Ard Biesheuvel To: "Tian, Feng" X-Spam-Score: -1.5 (-) X-Spam-Report: Spam Filtering performed by mx.sourceforge.net. See http://spamassassin.org/tag/ for more details. -1.5 SPF_CHECK_PASS SPF reports sender host as permitted sender for sender-domain -0.0 SPF_PASS SPF: sender matches SPF record X-Headers-End: 1YBfXb-0002uK-3T Cc: "edk2-devel@lists.sourceforge.net" Subject: Re: [edk2] [PATCH] DxeCore: set EFI_MEMORY_RUNTIME where appropriate on internal memory map X-BeenThere: edk2-devel@lists.sourceforge.net X-Mailman-Version: 2.1.9 Precedence: list Reply-To: edk2-devel@lists.sourceforge.net List-Id: List-Unsubscribe: , List-Archive: List-Post: , List-Help: , List-Subscribe: , Errors-To: edk2-devel-bounces@lists.sourceforge.net X-Removed-Original-Auth: Dkim didn't pass. X-Original-Sender: ard.biesheuvel@linaro.org X-Original-Authentication-Results: mx.google.com; spf=pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.217.181 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 15 January 2015 at 07:25, Tian, Feng wrote: > Hi, Ard > > Do you see any real impact of this issue when you tried to change runtime region allocation strategy? > Yes. On 64-bit ARM (AArch64), the OS can decide to use 64 KB pages instead of 4 KB pages. As an optimization, I tried to change the allocation strategy for runtime regions in Tianocore so that they are always 64 KB aligned multiple of 64 KB, even if UEFI's native page size is 4 KB. The default allocation can remain at 4 KB, as UEFI itself runs with 4 KB pages regardless. With the following patch applied ---->8---- /// For genric EFI machines make the default allocations 4K aligned ---->8---- I only get a partial result: AllocatePages () will adhere to the larger alignment, but AllocatePool () ignores it. This is a separate issues that needs to be addressed in Pool.c But more importantly, the checks in CoreTerminateMemoryMap() do not detect this at all, hence my patch. > I just did a quick search for possible updates on Attribute field, please see the calling flow: > gBS->SetMemorySpaceCapabilities() -> CoreUpdateMemoryAttributes() -> CoreConvertPagesEx() -> CoreAddRange () -> InsertTailList () > > it means Attribute field may be set to EFI_MEMORY_RUNTIME by caller, am I right? > Yes, it may be set by the caller, but it is clearly not the intention of the sanity check in CoreTerminateMemoryMap() to only detect regions whose attributes where modified by SetMemorySpaceCapabilities() [which only has one caller in the entire code base] The intention is obviously to make ExitBootServices () fail if *any* of the runtime regions do not adhere to EFI_ACPI_RUNTIME_PAGE_ALLOCATION_ALIGNMENT, and that is quite clearly broken. Regards, Ard. > -----Original Message----- > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] > Sent: Wednesday, January 14, 2015 00:19 > To: edk2-devel@lists.sourceforge.net > Subject: Re: [edk2] [PATCH] DxeCore: set EFI_MEMORY_RUNTIME where appropriate on internal memory map > > On 13 January 2015 at 16:15, Laszlo Ersek wrote: >> On 01/13/15 16:13, Ard Biesheuvel wrote: >>> The function CoreTerminateMemoryMap() performs some final sanity >>> checks on the runtime regions in the memory map before allowing >>> ExitBootServices() to complete. Unfortunately, it does so by testing >>> the EFI_MEMORY_RUNTIME bit in the Attribute field, which is never set anywhere in the code. >>> >>> Fix it by setting the bit where appropriate in CoreAddRange(). >>> >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Signed-off-by: Ard Biesheuvel >>> --- >>> MdeModulePkg/Core/Dxe/Mem/Page.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c >>> b/MdeModulePkg/Core/Dxe/Mem/Page.c >>> index fa84e2612526..3abf934933d8 100644 >>> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c >>> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c >>> @@ -240,6 +240,10 @@ CoreAddRange ( >>> } >>> } >>> >>> + if (mMemoryTypeStatistics[Type].Runtime) { >>> + Attribute |= EFI_MEMORY_RUNTIME; } >>> + >>> // >>> // Add descriptor >>> // >>> >> >> I think the check that you imply in CoreTerminateMemoryMap() is indeed >> dead code. But, I don't see how your patch would make it less dead. :) >> >> mMemoryTypeStatistics is a static array. Elements in that array never change their Runtime fields. >> >> The CoreGetMemoryMap() function uses the Runtime field to set the EFI_MEMORY_RUNTIME bit for various types of memory (EfiRuntimeServicesCode, EfiRuntimeServicesData, EfiPalCode), but that bit is set only in the map built for the caller, never in the internal map. >> >> When CoreTerminateMemoryMap() looks at the internal attribute bits, I agree that EFI_MEMORY_RUNTIME is never set, so that check is dead. However, the first check within that condition looks for at the type directly, EfiACPIReclaimMemory and EfiACPIMemoryNVS. Even if the EFI_MEMORY_RUNTIME bit had fired, this check would remain dead, because for these two memory types the Runtime bit is never set in the mMemoryTypeStatistics. >> >> ... Aha! You're not aiming at the first check here. You are looking at the start & end alignments. >> > > Correct. I am looking into changing the allocation strategy for runtime regions so they are aligned multiples of 64k (to accommodate OSes running with 64k pages) > >> In that case though, I don't think we should change the invariant >> >> the internal attributes never set EFI_MEMORY_RUNTIME >> >> Instead, I think CoreTerminateMemoryMap() should replicate the check in seen CoreGetMemoryMap(). Something like: >> >>> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c >>> b/MdeModulePkg/Core/Dxe/Mem/Page.c >>> index fa84e26..d9e2075 100644 >>> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c >>> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c >>> @@ -1790,12 +1790,9 @@ CoreTerminateMemoryMap ( >>> >>> for (Link = gMemoryMap.ForwardLink; Link != &gMemoryMap; Link = Link->ForwardLink) { >>> Entry = CR(Link, MEMORY_MAP, Link, MEMORY_MAP_SIGNATURE); >>> - if ((Entry->Attribute & EFI_MEMORY_RUNTIME) != 0) { >>> - if (Entry->Type == EfiACPIReclaimMemory || Entry->Type == EfiACPIMemoryNVS) { >>> - DEBUG((DEBUG_ERROR | DEBUG_PAGE, "ExitBootServices: ACPI memory entry has RUNTIME attribute set.\n")); >>> - Status = EFI_INVALID_PARAMETER; >>> - goto Done; >>> - } >>> + if (mMemoryTypeStatistics[Entry->Type].Runtime) { >>> + ASSERT (Entry->Type != EfiACPIReclaimMemory); >>> + ASSERT (Entry->Type != EfiACPIMemoryNVS); >>> if ((Entry->Start & (EFI_ACPI_RUNTIME_PAGE_ALLOCATION_ALIGNMENT - 1)) != 0) { >>> DEBUG((DEBUG_ERROR | DEBUG_PAGE, "ExitBootServices: A RUNTIME memory entry is not on a proper alignment.\n")); >>> Status = EFI_INVALID_PARAMETER; >> >> Of course I have no jurisdiction in MdeModulePkg/Core/, so I'm just >> theorizing :) Still, >> >> the internal attributes never set EFI_MEMORY_RUNTIME >> >> looks more like a deliberate invariant than an oversight. >> > > OK, I am fine with that as well. In fact, I don't care deeply about whether or not this check is performed at all, but the current situation is a bit awkward. > > -- > Ard. > > ------------------------------------------------------------------------------ > New Year. New Location. New Benefits. New Data Center in Ashburn, VA. > GigeNET is offering a free month of service with a new server in Ashburn. > Choose from 2 high performing configs, both with 100TB of bandwidth. > Higher redundancy.Lower latency.Increased capacity.Completely compliant. > http://p.sf.net/sfu/gigenet > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/edk2-devel ------------------------------------------------------------------------------ New Year. New Location. New Benefits. New Data Center in Ashburn, VA. GigeNET is offering a free month of service with a new server in Ashburn. Choose from 2 high performing configs, both with 100TB of bandwidth. Higher redundancy.Lower latency.Increased capacity.Completely compliant. http://p.sf.net/sfu/gigenet Acked-by: Ard Biesheuvel diff --git a/MdeModulePkg/Core/Dxe/Mem/Imem.h b/MdeModulePkg/Core/Dxe/Mem/Imem.h index d09ff3c5220f..41204c8cf412 100644 --- a/MdeModulePkg/Core/Dxe/Mem/Imem.h +++ b/MdeModulePkg/Core/Dxe/Mem/Imem.h @@ -22,6 +22,14 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. #define EFI_ACPI_RUNTIME_PAGE_ALLOCATION_ALIGNMENT (EFI_PAGE_SIZE * 2) #define DEFAULT_PAGE_ALLOCATION (EFI_PAGE_SIZE * 2) +#elif defined (MDE_CPU_AARCH64) +/// +/// OSes on 64-bit ARM may run with 64k pages, so align the runtime +/// regions to 64k as well +/// +#define EFI_ACPI_RUNTIME_PAGE_ALLOCATION_ALIGNMENT (EFI_PAGE_SIZE * 16) +#define DEFAULT_PAGE_ALLOCATION (EFI_PAGE_SIZE) + #else ///