[edk2-devel] [PATCH 2/3] UefiCpuPkg/PiSmmCpu: Refine CR2 save/restore logic
Laszlo Ersek
lersek at redhat.com
Mon Jul 29 11:42:06 UTC 2019
On 07/27/19 05:28, Ni, Ray wrote:
> Because IsStaticPageTableEnabled() is added for both IA32 and x64
> build, the CR2 save/restore logic can be refined:
> 1. Remove arch specific SaveCr2() / RestoreCr2() implementation;
> 2. Conditionally save and restore CR2 in SmiRendezvous().
>
> Signed-off-by: Ray Ni <ray.ni at intel.com>
> Cc: Eric Dong <eric.dong at intel.com>
> Cc: Laszlo Ersek <lersek at redhat.com>
> Cc: Vanguput Narendra K <narendra.k.vanguput at intel.com>
> ---
> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 25 -------------------
> UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 8 ++++--
> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 22 ----------------
> UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 29 ----------------------
> 4 files changed, 6 insertions(+), 78 deletions(-)
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> index 2a9af4b77d..cae23d6d1d 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> @@ -327,28 +327,3 @@ SetPageTableAttributes (
> return ;
> }
>
> -/**
> - This function returns with no action for 32 bit.
> -
> - @param[out] *Cr2 Pointer to variable to hold CR2 register value.
> -**/
> -VOID
> -SaveCr2 (
> - OUT UINTN *Cr2
> - )
> -{
> - return ;
> -}
> -
> -/**
> - This function returns with no action for 32 bit.
> -
> - @param[in] Cr2 Value to write into CR2 register.
> -**/
> -VOID
> -RestoreCr2 (
> - IN UINTN Cr2
> - )
> -{
> - return ;
> -}
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> index ef16997547..5d0124b368 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -1575,7 +1575,9 @@ SmiRendezvous (
> // when using on-demand paging for above 4G memory.
> //
> Cr2 = 0;
> - SaveCr2 (&Cr2);
> + if (!IsStaticPageTableEnabled ()) {
> + Cr2 = AsmReadCr2 ();
> + }
>
> //
> // Call the user register Startup function first.
So, because this patch is supposed to only refactor / simplify the code,
it should not change behavior.
But, because in patch#1 we return FALSE for IA32, the condition above
will evaluate to TRUE. And so we will massage CR2 (= fault address),
even though the IA32 build shouldn't do that (and doesn't do it, at the
moment).
This should be fixed by returning constant TRUE from
IsStaticPageTableEnabled(), in patch#1, on IA32.
(Note: in the message of commit d47b85a621ad ("Revert
"UefiCpuPkg/PiSmmCpu: Allow SMM access-out when static paging is OFF"",
2019-07-26), I wrote that "The IA32 implementation should return a
constant value". I didn't say either "constant TRUE" or "constant
FALSE". And that's because I couldn't know the right value, without
actually looking at the code. Determining the correct IA32 value was out
of scope for the revert.)
More below:
> @@ -1725,7 +1727,9 @@ Exit:
> //
> // Restore Cr2
> //
> - RestoreCr2 (Cr2);
> + if (!IsStaticPageTableEnabled ()) {
> + AsmWriteCr2 (Cr2);
> + }
> }
>
> /**
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> index 14b7676c16..5a97733def 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> @@ -1282,28 +1282,6 @@ IsStaticPageTableEnabled (
> )
> ;
>
> -/**
> - This function reads CR2 register when on-demand paging is enabled
> - for 64 bit and no action for 32 bit.
> -
> - @param[out] *Cr2 Pointer to variable to hold CR2 register value.
> -**/
> -VOID
> -SaveCr2 (
> - OUT UINTN *Cr2
> - );
> -
> -/**
> - This function writes into CR2 register when on-demand paging is enabled
> - for 64 bit and no action for 32 bit.
> -
> - @param[in] Cr2 Value to write into CR2 register.
> -**/
> -VOID
> -RestoreCr2 (
> - IN UINTN Cr2
> - );
> -
> /**
> Schedule a procedure to run on the specified CPU.
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> index 18e3f9e08d..8259b01a95 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> @@ -1209,32 +1209,3 @@ SetPageTableAttributes (
> return ;
> }
>
> -/**
> - This function reads CR2 register when on-demand paging is enabled.
> -
> - @param[out] *Cr2 Pointer to variable to hold CR2 register value.
> -**/
> -VOID
> -SaveCr2 (
> - OUT UINTN *Cr2
> - )
> -{
> - if (!mCpuSmmStaticPageTable) {
> - *Cr2 = AsmReadCr2 ();
> - }
> -}
> -
> -/**
> - This function restores CR2 register when on-demand paging is enabled.
> -
> - @param[in] Cr2 Value to write into CR2 register.
> -**/
> -VOID
> -RestoreCr2 (
> - IN UINTN Cr2
> - )
> -{
> - if (!mCpuSmmStaticPageTable) {
> - AsmWriteCr2 (Cr2);
> - }
> -}
>
For this patch:
Reviewed-by: Laszlo Ersek <lersek at redhat.com>
Thanks
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#44516): https://edk2.groups.io/g/devel/message/44516
Mute This Topic: https://groups.io/mt/32616002/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list