Re: [edk2-devel] [PATCH] UefiCpuPkg/CpuMpPei: Don't write CR3 in ConvertMemoryPageToNotPresent

["Ni, Ray" <ray.ni@intel.com>]

Thanks,
Ray
> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Wednesday, January 10, 2024 7:57 PM
> To: Liu, Zhiguang <zhiguang.liu@intel.com>; devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>;
> Gerd Hoffmann <kraxel@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.ni@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Rahul Kumar <rahul1.kumar@intel.com>
> > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > Signed-off-by: Zhiguang Liu <zhiguang.liu@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.

> 
> Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113569): https://edk2.groups.io/g/devel/message/113569
Mute This Topic: https://groups.io/mt/103636435/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-