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

["Laszlo Ersek" <lersek@redhat.com>]

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



-=-=-=-=-=-=-=-=-=-=-=-
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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-