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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113527): https://edk2.groups.io/g/devel/message/113527 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] -=-=-=-=-=-=-=-=-=-=-=-
