From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id BA93120355211 for ; Wed, 8 Nov 2017 05:21:07 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id EA7D946289; Wed, 8 Nov 2017 13:25:07 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com EA7D946289 Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-142.rdu2.redhat.com [10.10.120.142]) by smtp.corp.redhat.com (Postfix) with ESMTP id 7C1976BF9E; Wed, 8 Nov 2017 13:25:06 +0000 (UTC) To: "Zeng, Star" , "Wang, Jian J" , "edk2-devel@lists.01.org" Cc: "Yao, Jiewen" , "Dong, Eric" References: <20171103005729.7856-1-jian.j.wang@intel.com> <0C09AFA07DD0434D9E2A0C6AEB0483103B9B20B5@shsmsx102.ccr.corp.intel.com> <0C09AFA07DD0434D9E2A0C6AEB0483103B9B2DB6@shsmsx102.ccr.corp.intel.com> From: Laszlo Ersek Message-ID: Date: Wed, 8 Nov 2017 14:25:05 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <0C09AFA07DD0434D9E2A0C6AEB0483103B9B2DB6@shsmsx102.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Wed, 08 Nov 2017 13:25:08 +0000 (UTC) Subject: Re: [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 08 Nov 2017 13:21:07 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 11/08/17 04:13, Zeng, Star wrote: > https://bugzilla.tianocore.org/show_bug.cgi?id=763 is submitted to track this. I'd like to ask a question about the impact of BZ#763: Regarding BZ#753 ("UEFI memory map regression (runtime code entry splitting) introduced by c1cab54ce57c"), we seem to have agreed that any firmware that has commit c1cab54ce57c will need the fix for BZ#753. Otherwise the OS may get an invalid UEFI memory map, and if the OS is not specifically prepared for that, it can crash. My question is if the issue described in BZ#763 is similarly serious; i.e., whether any firmware that has commit 14dde9e903bb ("MdeModulePkg/Core: Fix out-of-sync issue in GCD", 2017-09-19) should urgently get the fix for BZ#763. In other words, does BZ#763 have a direct OS-level impact that needs to be fixed quickly? Thanks! Laszlo > -----Original Message----- > From: Wang, Jian J > Sent: Tuesday, November 7, 2017 8:55 AM > To: Zeng, Star ; edk2-devel@lists.01.org > Cc: Laszlo Ersek ; Yao, Jiewen ; Dong, Eric > Subject: RE: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map > > Thanks for the review. And I agree that GCD.SetMemoryAttributes should be used all the time in DxeCore. Let's fix it in another patch. > >> -----Original Message----- >> From: Zeng, Star >> Sent: Monday, November 06, 2017 5:16 PM >> To: Wang, Jian J ; edk2-devel@lists.01.org >> Cc: Laszlo Ersek ; Yao, Jiewen >> ; Dong, Eric ; Zeng, Star >> >> Subject: RE: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries >> of RT_CODE in memory map >> >> I am ok to this code approach. >> >> Acknowledged-by: Star Zeng >> >> Besides, I think except GCD services, DxeCore should not call gCpu- >>> SetMemoryAttributes() directly, for example >>> ApplyMemoryProtectionPolicy(), it >> should have no assumption of the CPU code (to set capabilities), and >> call GCD setcapabilities first, and then call GCD setattributes since >> 14dde9e903bb9a719ebb8f3381da72b19509bc36 "MdeModulePkg/Core: Fix out- >> of-sync issue in GCD", otherwise there may be mismatch of page >> attributes between GCD and gCPU after >> RefreshGcdMemoryAttributesFromPaging() by the calling gCpu->SetMemoryAttributes() in ApplyMemoryProtectionPolicy(). >> >> Anyway, that could be in a separated patch. :) >> >> Thanks, >> Star >> -----Original Message----- >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of >> Jian J Wang >> Sent: Friday, November 3, 2017 8:57 AM >> To: edk2-devel@lists.01.org >> Cc: Laszlo Ersek ; Yao, Jiewen >> ; Dong, Eric >> Subject: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of >> RT_CODE in memory map >> >>> v2 >>> a. Fix an issue which will cause setting capability failure if size is smaller >>> than a page. >> >> More than one entry of RT_CODE memory might cause boot problem for >> some old OSs. This patch will fix this issue to keep OS compatibility as much as possible. >> >> More detailed information, please refer to >> https://bugzilla.tianocore.org/show_bug.cgi?id=753 >> >> Cc: Eric Dong >> Cc: Jiewen Yao >> Cc: Laszlo Ersek >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Jian J Wang >> --- >> UefiCpuPkg/CpuDxe/CpuPageTable.c | 18 ++++++++++++++---- >> 1 file changed, 14 insertions(+), 4 deletions(-) >> >> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c >> b/UefiCpuPkg/CpuDxe/CpuPageTable.c >> index d312eb66f8..4a7827ebc9 100644 >> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c >> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c >> @@ -809,7 +809,9 @@ RefreshGcdMemoryAttributesFromPaging ( >> PageLength = 0; >> >> for (Index = 0; Index < NumberOfDescriptors; Index++) { >> - if (MemorySpaceMap[Index].GcdMemoryType == >> EfiGcdMemoryTypeNonExistent) { >> + if (MemorySpaceMap[Index].GcdMemoryType == >> EfiGcdMemoryTypeNonExistent >> + || (MemorySpaceMap[Index].BaseAddress & EFI_PAGE_MASK) != 0 >> + || (MemorySpaceMap[Index].Length & EFI_PAGE_MASK) != 0) { >> continue; >> } >> >> @@ -829,6 +831,15 @@ RefreshGcdMemoryAttributesFromPaging ( >> // Sync real page attributes to GCD >> BaseAddress = MemorySpaceMap[Index].BaseAddress; >> MemorySpaceLength = MemorySpaceMap[Index].Length; >> + Capabilities = MemorySpaceMap[Index].Capabilities | >> + EFI_MEMORY_PAGETYPE_MASK; >> + Status = gDS->SetMemorySpaceCapabilities ( >> + BaseAddress, >> + MemorySpaceLength, >> + Capabilities >> + ); >> + ASSERT_EFI_ERROR (Status); >> + >> while (MemorySpaceLength > 0) { >> if (PageLength == 0) { >> PageEntry = GetPageTableEntry (&PagingContext, BaseAddress, >> &PageAttribute); @@ -846,7 +857,6 @@ >> RefreshGcdMemoryAttributesFromPaging ( >> if (Attributes != (MemorySpaceMap[Index].Attributes & >> EFI_MEMORY_PAGETYPE_MASK)) { >> DoUpdate = TRUE; >> Attributes |= (MemorySpaceMap[Index].Attributes & >> ~EFI_MEMORY_PAGETYPE_MASK); >> - Capabilities = Attributes | MemorySpaceMap[Index].Capabilities; >> } else { >> DoUpdate = FALSE; >> } >> @@ -854,8 +864,8 @@ RefreshGcdMemoryAttributesFromPaging ( >> >> Length = MIN (PageLength, MemorySpaceLength); >> if (DoUpdate) { >> - gDS->SetMemorySpaceCapabilities (BaseAddress, Length, Capabilities); >> - gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes); >> + Status = gDS->SetMemorySpaceAttributes (BaseAddress, Length, >> Attributes); >> + ASSERT_EFI_ERROR (Status); >> DEBUG ((DEBUG_INFO, "Update memory space attribute: [%02d] >> %016lx - %016lx (%08lx -> %08lx)\r\n", >> Index, BaseAddress, BaseAddress + Length - 1, >> MemorySpaceMap[Index].Attributes, >> Attributes)); >> -- >> 2.14.1.windows.1 >> >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.01.org >> https://lists.01.org/mailman/listinfo/edk2-devel