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 AA9B421CEB151 for ; Wed, 25 Oct 2017 05:47:01 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 32316C059B62; Wed, 25 Oct 2017 12:50:46 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 32316C059B62 Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-213.rdu2.redhat.com [10.10.120.213]) by smtp.corp.redhat.com (Postfix) with ESMTP id D871A7A215; Wed, 25 Oct 2017 12:50:44 +0000 (UTC) To: "Wang, Jian J" , "edk2-devel@lists.01.org" Cc: "Yao, Jiewen" , "Dong, Eric" References: <20171023065022.1272-1-jian.j.wang@intel.com> <764523da-0d75-d4ac-3121-c8282ada3e8f@redhat.com> From: Laszlo Ersek Message-ID: <2367cd4c-d660-c848-acdf-c67fd94b4880@redhat.com> Date: Wed, 25 Oct 2017 14:50:43 +0200 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: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Wed, 25 Oct 2017 12:50:46 +0000 (UTC) Subject: Re: [PATCH] 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, 25 Oct 2017 12:47:01 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 10/25/17 03:33, Wang, Jian J wrote: > Hi Laszlo, > > Thanks for the feedback. I'd like to explain a bit more here first and > will update the commit message later. > > The multiple RT_CODE entries issue was reported by LUV test suite from > https://01.org/linux-uefi-validation > > You're right this issue is caused by my commit c1cab54ce57c, which > tried to fix the GCD issue in which you can't set paging related > memory attributes through GCD service API. The reasons are that GCD > will filter out all paging related memory attributes and also, the CPU > driver didn't sync the paging attributes back to GCD. Sorry I don't > know why GCD and cpu driver are implemented that way. > > My previous commit c1cab54ce57c fixed above issues but didn't make > sure that all memory blocks share the same capabilities because I just > added paging related memory capabilities to those memory block having > some paging attributes set. This will in turn cause more than one > RT_CODE entries in memory map because GCD reports memory to OS based > on the memory block capability. > > Why multiple RT_CODE matters? It's because that Linux kernel would > misplace the runtime service code segment and its data segment. What I > know is this Linux issue had been fixed. That's why recent Linux > distro won't encounter problems even if we report multiple RT_CODE > memory to kernel. Ah, OK, I remember now. The point is that separate entries in the UEFI memmap may be mapped by the OS to discontiguous virtual address ranges. However, if we take the UEFI memmap entries that belong to a single runtime DXE driver, and unintentionally split those entries up (by setting distinct capabilities for a subset of their pages), then the runtime driver will break, because the linear address range that the driver expects (from its original loading and relocation) will not be kept linear by the OS. > I'm sorry that I cannot find the specific version of kernel which has > such problem and I can't find any discussion related. Maybe Jiewen can > provide more detailed information. It would be really helpful if you guys could name a guest kernel version (or a GNU/Linux distro release) that is affected by this problem. Are there perhaps any conditions that this issue depends on, such as PCDs for example? In other words, is it possible that various edk2 platform settings (in the DSC) can mask this issue? Can you maybe present UEFI memmaps (dumped in the UEFI shell, or in Linux) that show the problem, compared to this fix? > This patch will make sure that all memory block share the same paging > capabilities. Because all memory are actually paged in current EDK2 > (at least IA processors), technically we're capable of setting any > page of memory to read-only and/or non-executable. I think this fix is > not only trying to avoid multiple RT_CODE memory map entries but also > trying to make sure the memory capabilities in GCD service to reflect > complete status of the real world. Are you saying that *any* firmware that carries commit c1cab54ce57c should also receive this patch? If that's the case, then some kind of "reproducer" would be really nice -- steps that you can run both with and without this patch, and the output or the behavior will show the difference. Thanks! Laszlo >> -----Original Message----- >> From: Laszlo Ersek [mailto:lersek@redhat.com] >> Sent: Tuesday, October 24, 2017 8:20 PM >> To: Wang, Jian J ; edk2-devel@lists.01.org >> Cc: Yao, Jiewen ; Dong, Eric >> Subject: Re: [edk2] [PATCH] UefiCpuPkg/CpuDxe: Fix multiple entries of >> RT_CODE in memory map >> >> On 10/23/17 08:50, Jian J Wang wrote: >>> 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. >>> >>> Cc: Eric Dong >>> Cc: Jiewen Yao >>> Contributed-under: TianoCore Contribution Agreement 1.1 >>> Signed-off-by: Jian J Wang >>> --- >>> UefiCpuPkg/CpuDxe/CpuPageTable.c | 14 +++++++++++--- >>> 1 file changed, 11 insertions(+), 3 deletions(-) >> >> Can you please explain in the commit message; what OSes are affected by >> this issue, to your knowledge? >> >> Also, the code being patched seems to originate from commit c1cab54ce57c >> ("UefiCpuPkg/CpuDxe: Fix out-of-sync issue in page attributes", >> 2017-09-16). I vaguely recall that this commit was related to your "page >> 0 protection" work. >> >> Can you please explain, in the commit message, under what circumstances >> (PCD settings etc) the issue arises, and how we end up with multiple >> RT_CODE entries in the memory map? >> >> (BTW, multiple RT_CODE entries in the memmap should be perfectly >> normal... So I'm extra curious about the OSes that are not compatible >> with that.) >> >> Thanks, >> Laszlo >> >>> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c >> b/UefiCpuPkg/CpuDxe/CpuPageTable.c >>> index d312eb66f8..0802464b9d 100644 >>> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c >>> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c >>> @@ -829,6 +829,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 +855,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 +862,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)); >>> >