On 1/11/24 03:08, Ni, Ray wrote: > > > Thanks, > Ray >> -----Original Message----- >> From: Laszlo Ersek <ler...@redhat.com> >> Sent: Wednesday, January 10, 2024 7:57 PM >> To: Liu, Zhiguang <zhiguang....@intel.com>; devel@edk2.groups.io >> Cc: Ni, Ray <ray...@intel.com>; Kumar, Rahul R <rahul.r.ku...@intel.com>; >> Gerd Hoffmann <kra...@redhat.com> >> Subject: Re: [PATCH] UefiCpuPkg/CpuMpPei: Don't write CR3 in >> ConvertMemoryPageToNotPresent >> >> On 1/10/24 06:43, Zhiguang Liu wrote: >>> After ConvertMemoryPageToNotPresent, there is always a flush TLB >>> function. So, to improve performance, there is no need to write CR3 >>> inside ConvertMemoryPageToNotPresent >>> >>> Cc: Ray Ni <ray...@intel.com> >>> Cc: Laszlo Ersek <ler...@redhat.com> >>> Cc: Rahul Kumar <rahul1.ku...@intel.com> >>> Cc: Gerd Hoffmann <kra...@redhat.com> >>> Signed-off-by: Zhiguang Liu <zhiguang....@intel.com> >>> --- >>> UefiCpuPkg/CpuMpPei/CpuPaging.c | 1 - >>> 1 file changed, 1 deletion(-) >>> >>> diff --git a/UefiCpuPkg/CpuMpPei/CpuPaging.c >> b/UefiCpuPkg/CpuMpPei/CpuPaging.c >>> index 15c7015fb8..c6894458f7 100644 >>> --- a/UefiCpuPkg/CpuMpPei/CpuPaging.c >>> +++ b/UefiCpuPkg/CpuMpPei/CpuPaging.c >>> @@ -167,7 +167,6 @@ ConvertMemoryPageToNotPresent ( >>> } >>> >>> ASSERT_EFI_ERROR (Status); >>> - AsmWriteCr3 (PageTable); >>> return Status; >>> } >>> >> >> (1) I mostly understand the point that the commit message makes, but the >> message is not clear enough. The real point is that we have two >> ConvertMemoryPageToNotPresent() calls altogether, and each of those >> takes place in a *loop*, and each of those loops is followed by a >> CpuFlushTlb() call. >> >> The loops matter. If there were no loops, then we might be motivated to >> choose a different solution (for example, to move centralize the >> CpuFlushTlb() calls *inside* ConvertMemoryPageToNotPresent(), or >> something similar). >> >> So, please update the commit message; mention the loops. >> >> (2) I can't easily see why this change is actually correct. IIRC, >> writing CR3 has a "side effect" of flushing the TLB. But obviously, >> that's not the *only* effect of writing CR3. You could say that >> CpuFlushTlb() performs a *strict subset* of what AsmWriteCr3() performs. >> Therefore, in order to replace AsmWriteCr3() with just CpuFlushTlb(), >> you need to demonstrate that the functionality that now is *not* going >> to be done has always been superfluous. In more direct terms, you need >> to show that the AsmWriteCr3() write call that's being removed here >> never actuall changes the *value* of CR3. >> >> And I cannot show that myself very easily. >> ConvertMemoryPageToNotPresent(). In ConvertMemoryPageToNotPresent(), >> "PageTable" is first set from AsmReadCr3(), then passed twice to >> PageTableMap() by reference (!), and then it is written back to CR3. If >> at least one of those PageTableMap() calls change "PageTable", then the >> AsmWriteCr3() call at the end that's now being removed actually changes >> the value of CR3, and then the patch would be wrong. >> >> It's possible that PageTableMap() never changes the value of >> "PageTable", but it must be proved, and the evidence should be included >> in the commit message. >> >> (3) Furthermore, with the patch applied, ConvertMemoryPageToNotPresent() >> will no longer flush the TLB itself -- that will always be left to the >> caller. This new caller responsibility should be documented in the >> leading comment of ConvertMemoryPageToNotPresent(). We already have a >> paragraph there starting with "Caller should make sure..." >> >> Sorry for making such a big deal out of this, but such simple-looking >> changes can sometimes case obscure (and rarely occurring) bugs down the >> road. If you already have evidence that CR3 does not change here, that's >> great, but then please don't think it's "obvious"; just go ahead and >> document it. > > Laszlo, > Happy to see these comments! All make sense! > > PageTableMap() only modifies the PageTable root pointer when creating from > zero. > Since there is only one instance of the PageTableLib, I think we could update > the > PageTableLib API comments to explicitly mention that. So point (2) will be > resolved.
That should work, thanks! Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113597): https://edk2.groups.io/g/devel/message/113597 Mute This Topic: https://groups.io/mt/103636435/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
