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 EA77121CEB151 for ; Thu, 26 Oct 2017 03:03:58 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B8C7FDA303; Thu, 26 Oct 2017 10:07:43 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com B8C7FDA303 Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-110.rdu2.redhat.com [10.10.120.110]) by smtp.corp.redhat.com (Postfix) with ESMTP id 9B7785C542; Thu, 26 Oct 2017 10:07:41 +0000 (UTC) To: Jian J Wang , edk2-devel@lists.01.org Cc: Jiewen Yao , Eric Dong References: <20171023065022.1272-1-jian.j.wang@intel.com> From: Laszlo Ersek Message-ID: Date: Thu, 26 Oct 2017 12:07:40 +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: <20171023065022.1272-1-jian.j.wang@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Thu, 26 Oct 2017 10:07:43 +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: Thu, 26 Oct 2017 10:03:59 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Hello Jian, 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(-) Thank you again for the explanation elsewhere in this thread. I filed the following TianoCore Bugzilla entry about this issue, and assigned it to you: https://bugzilla.tianocore.org/show_bug.cgi?id=753 Can you please read the BZ, and add corrections (in further comments) if you think any such are necessary? I suggest that the BZ reference be included in the commit message. (If there is no v2 necessary for this patch, then Eric can do that as well, when he commits / pushes your patch.) I think the patch is good, but I have one technical question below: > 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); > + This logic -- i.e. the addition of the EFI_MEMORY_PAGETYPE_MASK capabilities -- will be applied to *all* GCD memory space map entries that have a type different from "EfiGcdMemoryTypeNonExistent". I wonder if that's a good idea -- for example I don't think it makes much sense for "EfiGcdMemoryTypeMemoryMappedIo". How about the following alternatives: (1) *Either* restrict this capability addition to EfiGcdMemoryTypeSystemMemory (and maybe some other GCD types as well?), (2) *or*, remove this change, and: > 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)); > keep the SetMemorySpaceCapabilities() call here, but use the following arguments instead: MemorySpaceMap[Index].BaseAddress MemorySpaceMap[Index].Length This will ensure that the capabilities are changed on the *entire* containing GCD memory space entry, and no entry splitting will take place. Yes, it is possible that the SetMemorySpaceCapabilities() function will be invoked multiple times, on the same GCD memory space entry, but that's not a problem, IMO. The Capabilities value (bitmask) should be the exact same. In fact, you could set Capabilities just before the inner loop, and then only *grow* it in the inner loop. Something like this: > diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c > index d312eb66f87c..5a0eb2900cd5 100644 > --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c > +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c > @@ -829,6 +829,7 @@ RefreshGcdMemoryAttributesFromPaging ( > // Sync real page attributes to GCD > BaseAddress = MemorySpaceMap[Index].BaseAddress; > MemorySpaceLength = MemorySpaceMap[Index].Length; > + Capabilities = MemorySpaceMap[Index].Capabilities; > while (MemorySpaceLength > 0) { > if (PageLength == 0) { > PageEntry = GetPageTableEntry (&PagingContext, BaseAddress, &PageAttribute); > @@ -846,7 +847,7 @@ 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; > + Capabilities |= Attributes; > } else { > DoUpdate = FALSE; > } > @@ -854,8 +855,20 @@ RefreshGcdMemoryAttributesFromPaging ( > > Length = MIN (PageLength, MemorySpaceLength); > if (DoUpdate) { > - gDS->SetMemorySpaceCapabilities (BaseAddress, Length, Capabilities); > - gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes); > + Status = gDS->SetMemorySpaceCapabilities ( > + MemorySpaceMap[Index].BaseAddress, > + MemorySpaceMap[Index].Length, > + Capabilities > + ); > + ASSERT_EFI_ERROR (Status); > + > + 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)); What do you think? Meta comment: can you please CC me on the next version of the patch (if you send one)? Looks like I'm now a Reviewer for UefiCpuPkg :) , I just have to commit the patch for "Maintainers.txt". In addition, if you send a v2, please locate the web link for it in the mailing list archive: https://lists.01.org/pipermail/edk2-devel/2017-October/thread.html and add the link to the Bugzilla, in a new comment -- this way someone who looks at the bugzilla can see all the versions and the discussion threads. Thanks! Laszlo