* [PATCH v6] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM
@ 2019-03-29 4:58 nkvangup
2019-03-29 5:10 ` Ni, Ray
2019-03-30 14:55 ` Zeng, Star
0 siblings, 2 replies; 5+ messages in thread
From: nkvangup @ 2019-03-29 4:58 UTC (permalink / raw)
To: edk2-devel
Cc: Vanguput Narendra K, Eric Dong, Ray Ni, Laszlo Ersek, Yao Jiewen
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1593
For every SMI occurrence, save and restore CR2 register only when SMM
on-demand paging support is enabled in 64 bit operation mode.
This is not a bug but to have better improvement of code.
Patch5 is updated with separate functions for Save and Restore of CR2
based on review feedback.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Vanguput Narendra K <narendra.k.vanguput@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Yao Jiewen <jiewen.yao@intel.com>
---
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 26 ++++++++++++++++++++++++++
UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 9 ++++++---
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 22 ++++++++++++++++++++++
UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 30 ++++++++++++++++++++++++++++++
4 files changed, 84 insertions(+), 3 deletions(-)
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
index b734a1ea8c..af96e42982 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
@@ -316,3 +316,29 @@ SetPageTableAttributes (
return ;
}
+
+/**
+ This function returns with no action for 32 bit.
+
+ @param[in] *Cr2 Pointer to variable to hold CR2 register value
+**/
+VOID
+SaveCr2 (
+ UINTN *Cr2
+ )
+{
+ return ;
+}
+
+/**
+ This function returns with no action for 32 bit.
+
+ @param[in] Cr2 Value to write into CR2 register
+**/
+VOID
+RestoreCr2 (
+ UINTN Cr2
+ )
+{
+ return ;
+}
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
index 3b0b3b52ac..ce70f77709 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -1112,9 +1112,11 @@ SmiRendezvous (
ASSERT(CpuIndex < mMaxNumberOfCpus);
//
- // Save Cr2 because Page Fault exception in SMM may override its value
+ // Save Cr2 because Page Fault exception in SMM may override its value,
+ // when using on-demand paging for above 4G memory.
//
- Cr2 = AsmReadCr2 ();
+ Cr2 = 0;
+ SaveCr2 (&Cr2);
//
// Perform CPU specific entry hooks
@@ -1253,10 +1255,11 @@ SmiRendezvous (
Exit:
SmmCpuFeaturesRendezvousExit (CpuIndex);
+
//
// Restore Cr2
//
- AsmWriteCr2 (Cr2);
+ RestoreCr2 (Cr2);
}
/**
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
index 84efb22981..c9d147c8a1 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
@@ -1243,4 +1243,26 @@ EFIAPI
PiSmmCpuSmiEntryFixupAddress (
);
+/**
+ This function reads CR2 register when on-demand paging is enabled
+ for 64 bit and no action for 32 bit.
+
+ @param[in] *Cr2 Pointer to variable to hold CR2 register value
+**/
+VOID
+SaveCr2 (
+ 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 (
+ UINTN Cr2
+ );
+
#endif
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
index 2c77cb47a4..6cb44fbbe5 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
@@ -1053,3 +1053,33 @@ SetPageTableAttributes (
return ;
}
+
+/**
+ This function reads CR2 register when on-demand paging is enabled
+
+ @param[in] *Cr2 Pointer to variable to hold CR2 register value
+**/
+VOID
+SaveCr2 (
+ 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 (
+ UINTN Cr2
+ )
+{
+ if ((!mCpuSmmStaticPageTable) && (Cr2 != 0)) {
+ AsmWriteCr2 (Cr2);
+ }
+}
--
2.16.2.windows.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v6] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM
2019-03-29 4:58 [PATCH v6] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM nkvangup
@ 2019-03-29 5:10 ` Ni, Ray
2019-03-29 9:31 ` Vanguput, Narendra K
2019-03-30 14:55 ` Zeng, Star
1 sibling, 1 reply; 5+ messages in thread
From: Ni, Ray @ 2019-03-29 5:10 UTC (permalink / raw)
To: Vanguput, Narendra K, edk2-devel@lists.01.org
Cc: Yao, Jiewen, Dong, Eric, Laszlo Ersek
> + if ((!mCpuSmmStaticPageTable) && (Cr2 != 0)) {
Can the "Cr2 != 0" be removed?
> -----Original Message-----
> From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of
> nkvangup
> Sent: Friday, March 29, 2019 12:58 PM
> To: edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>;
> Laszlo Ersek <lersek@redhat.com>
> Subject: [edk2] [PATCH v6] UefiCpuPkg\CpuSmm: Save & restore CR2 on-
> demand paging in SMM
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1593
>
> For every SMI occurrence, save and restore CR2 register only when SMM on-
> demand paging support is enabled in 64 bit operation mode.
> This is not a bug but to have better improvement of code.
>
> Patch5 is updated with separate functions for Save and Restore of CR2 based
> on review feedback.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Vanguput Narendra K <narendra.k.vanguput@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Yao Jiewen <jiewen.yao@intel.com>
> ---
> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 26
> ++++++++++++++++++++++++++
> UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 9 ++++++---
> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 22
> ++++++++++++++++++++++
> UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 30
> ++++++++++++++++++++++++++++++
> 4 files changed, 84 insertions(+), 3 deletions(-)
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> index b734a1ea8c..af96e42982 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> @@ -316,3 +316,29 @@ SetPageTableAttributes (
>
> return ;
> }
> +
> +/**
> + This function returns with no action for 32 bit.
> +
> + @param[in] *Cr2 Pointer to variable to hold CR2 register value **/
> +VOID
> +SaveCr2 (
> + UINTN *Cr2
> + )
> +{
> + return ;
> +}
> +
> +/**
> + This function returns with no action for 32 bit.
> +
> + @param[in] Cr2 Value to write into CR2 register **/ VOID
> +RestoreCr2 (
> + UINTN Cr2
> + )
> +{
> + return ;
> +}
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> index 3b0b3b52ac..ce70f77709 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -1112,9 +1112,11 @@ SmiRendezvous (
> ASSERT(CpuIndex < mMaxNumberOfCpus);
>
> //
> - // Save Cr2 because Page Fault exception in SMM may override its value
> + // Save Cr2 because Page Fault exception in SMM may override its
> + value, // when using on-demand paging for above 4G memory.
> //
> - Cr2 = AsmReadCr2 ();
> + Cr2 = 0;
> + SaveCr2 (&Cr2);
>
> //
> // Perform CPU specific entry hooks
> @@ -1253,10 +1255,11 @@ SmiRendezvous (
>
> Exit:
> SmmCpuFeaturesRendezvousExit (CpuIndex);
> +
> //
> // Restore Cr2
> //
> - AsmWriteCr2 (Cr2);
> + RestoreCr2 (Cr2);
> }
>
> /**
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> index 84efb22981..c9d147c8a1 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> @@ -1243,4 +1243,26 @@ EFIAPI
> PiSmmCpuSmiEntryFixupAddress (
> );
>
> +/**
> + This function reads CR2 register when on-demand paging is enabled
> + for 64 bit and no action for 32 bit.
> +
> + @param[in] *Cr2 Pointer to variable to hold CR2 register value **/
> +VOID
> +SaveCr2 (
> + 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 (
> + UINTN Cr2
> + );
> +
> #endif
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> index 2c77cb47a4..6cb44fbbe5 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> @@ -1053,3 +1053,33 @@ SetPageTableAttributes (
>
> return ;
> }
> +
> +/**
> + This function reads CR2 register when on-demand paging is enabled
> +
> + @param[in] *Cr2 Pointer to variable to hold CR2 register value **/
> +VOID
> +SaveCr2 (
> + 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 (
> + UINTN Cr2
> + )
> +{
> + if ((!mCpuSmmStaticPageTable) && (Cr2 != 0)) {
> + AsmWriteCr2 (Cr2);
> + }
> +}
> --
> 2.16.2.windows.1
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v6] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM
2019-03-29 5:10 ` Ni, Ray
@ 2019-03-29 9:31 ` Vanguput, Narendra K
2019-03-29 9:36 ` Ni, Ray
0 siblings, 1 reply; 5+ messages in thread
From: Vanguput, Narendra K @ 2019-03-29 9:31 UTC (permalink / raw)
To: Ni, Ray, edk2-devel@lists.01.org; +Cc: Yao, Jiewen, Dong, Eric, Laszlo Ersek
Hi Ray,
While programming, I thought this cannot be 0 as in SDM, it says page-fault linear address.
>> CR2 - Contains the page-fault linear address (the linear address that caused a page fault).
So added a check for 0.
Now as the function is changed like program into CR2 register based on input parameter and checking for 0 is up to caller of this function. And also we don't need to require for checking 0 means, will remove it. Please confirm.
Thanks,
Naren
-----Original Message-----
From: Ni, Ray
Sent: Friday, March 29, 2019 10:40 AM
To: Vanguput, Narendra K <narendra.k.vanguput@intel.com>; edk2-devel@lists.01.org
Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>
Subject: RE: [edk2] [PATCH v6] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM
> + if ((!mCpuSmmStaticPageTable) && (Cr2 != 0)) {
Can the "Cr2 != 0" be removed?
> -----Original Message-----
> From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of
> nkvangup
> Sent: Friday, March 29, 2019 12:58 PM
> To: edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric
> <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: [edk2] [PATCH v6] UefiCpuPkg\CpuSmm: Save & restore CR2 on-
> demand paging in SMM
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1593
>
> For every SMI occurrence, save and restore CR2 register only when SMM
> on- demand paging support is enabled in 64 bit operation mode.
> This is not a bug but to have better improvement of code.
>
> Patch5 is updated with separate functions for Save and Restore of CR2
> based on review feedback.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Vanguput Narendra K <narendra.k.vanguput@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Yao Jiewen <jiewen.yao@intel.com>
> ---
> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 26
> ++++++++++++++++++++++++++
> UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 9 ++++++---
> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 22
> ++++++++++++++++++++++
> UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 30
> ++++++++++++++++++++++++++++++
> 4 files changed, 84 insertions(+), 3 deletions(-)
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> index b734a1ea8c..af96e42982 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> @@ -316,3 +316,29 @@ SetPageTableAttributes (
>
> return ;
> }
> +
> +/**
> + This function returns with no action for 32 bit.
> +
> + @param[in] *Cr2 Pointer to variable to hold CR2 register value
> +**/ VOID
> +SaveCr2 (
> + UINTN *Cr2
> + )
> +{
> + return ;
> +}
> +
> +/**
> + This function returns with no action for 32 bit.
> +
> + @param[in] Cr2 Value to write into CR2 register **/ VOID
> +RestoreCr2 (
> + UINTN Cr2
> + )
> +{
> + return ;
> +}
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> index 3b0b3b52ac..ce70f77709 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -1112,9 +1112,11 @@ SmiRendezvous (
> ASSERT(CpuIndex < mMaxNumberOfCpus);
>
> //
> - // Save Cr2 because Page Fault exception in SMM may override its
> value
> + // Save Cr2 because Page Fault exception in SMM may override its
> + value, // when using on-demand paging for above 4G memory.
> //
> - Cr2 = AsmReadCr2 ();
> + Cr2 = 0;
> + SaveCr2 (&Cr2);
>
> //
> // Perform CPU specific entry hooks @@ -1253,10 +1255,11 @@
> SmiRendezvous (
>
> Exit:
> SmmCpuFeaturesRendezvousExit (CpuIndex);
> +
> //
> // Restore Cr2
> //
> - AsmWriteCr2 (Cr2);
> + RestoreCr2 (Cr2);
> }
>
> /**
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> index 84efb22981..c9d147c8a1 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> @@ -1243,4 +1243,26 @@ EFIAPI
> PiSmmCpuSmiEntryFixupAddress (
> );
>
> +/**
> + This function reads CR2 register when on-demand paging is enabled
> + for 64 bit and no action for 32 bit.
> +
> + @param[in] *Cr2 Pointer to variable to hold CR2 register value
> +**/ VOID
> +SaveCr2 (
> + 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 (
> + UINTN Cr2
> + );
> +
> #endif
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> index 2c77cb47a4..6cb44fbbe5 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> @@ -1053,3 +1053,33 @@ SetPageTableAttributes (
>
> return ;
> }
> +
> +/**
> + This function reads CR2 register when on-demand paging is enabled
> +
> + @param[in] *Cr2 Pointer to variable to hold CR2 register value
> +**/ VOID
> +SaveCr2 (
> + 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 (
> + UINTN Cr2
> + )
> +{
> + if ((!mCpuSmmStaticPageTable) && (Cr2 != 0)) {
> + AsmWriteCr2 (Cr2);
> + }
> +}
> --
> 2.16.2.windows.1
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v6] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM
2019-03-29 9:31 ` Vanguput, Narendra K
@ 2019-03-29 9:36 ` Ni, Ray
0 siblings, 0 replies; 5+ messages in thread
From: Ni, Ray @ 2019-03-29 9:36 UTC (permalink / raw)
To: Vanguput, Narendra K, edk2-devel@lists.01.org
Cc: Yao, Jiewen, Dong, Eric, Laszlo Ersek
Yes. With below "Cr2 !=0" removed from final commit, Reviewed-by: Ray Ni <ray.ni@intel.com>
> > +RestoreCr2 (
> > + UINTN Cr2
> > + )
> > +{
> > + if ((!mCpuSmmStaticPageTable) && (Cr2 != 0)) {
> > + AsmWriteCr2 (Cr2);
> > + }
> > +}
> -----Original Message-----
> From: Vanguput, Narendra K <narendra.k.vanguput@intel.com>
> Sent: Friday, March 29, 2019 5:32 PM
> To: Ni, Ray <ray.ni@intel.com>; edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>;
> Laszlo Ersek <lersek@redhat.com>
> Subject: RE: [edk2] [PATCH v6] UefiCpuPkg\CpuSmm: Save & restore CR2 on-
> demand paging in SMM
>
> Hi Ray,
>
> While programming, I thought this cannot be 0 as in SDM, it says page-fault
> linear address.
> >> CR2 - Contains the page-fault linear address (the linear address that
> caused a page fault).
>
> So added a check for 0.
>
> Now as the function is changed like program into CR2 register based on input
> parameter and checking for 0 is up to caller of this function. And also we
> don't need to require for checking 0 means, will remove it. Please confirm.
>
> Thanks,
> Naren
>
> -----Original Message-----
> From: Ni, Ray
> Sent: Friday, March 29, 2019 10:40 AM
> To: Vanguput, Narendra K <narendra.k.vanguput@intel.com>; edk2-
> devel@lists.01.org
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>;
> Laszlo Ersek <lersek@redhat.com>
> Subject: RE: [edk2] [PATCH v6] UefiCpuPkg\CpuSmm: Save & restore CR2 on-
> demand paging in SMM
>
> > + if ((!mCpuSmmStaticPageTable) && (Cr2 != 0)) {
>
> Can the "Cr2 != 0" be removed?
>
> > -----Original Message-----
> > From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of
> > nkvangup
> > Sent: Friday, March 29, 2019 12:58 PM
> > To: edk2-devel@lists.01.org
> > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric
> > <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>
> > Subject: [edk2] [PATCH v6] UefiCpuPkg\CpuSmm: Save & restore CR2 on-
> > demand paging in SMM
> >
> > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1593
> >
> > For every SMI occurrence, save and restore CR2 register only when SMM
> > on- demand paging support is enabled in 64 bit operation mode.
> > This is not a bug but to have better improvement of code.
> >
> > Patch5 is updated with separate functions for Save and Restore of CR2
> > based on review feedback.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Vanguput Narendra K <narendra.k.vanguput@intel.com>
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Yao Jiewen <jiewen.yao@intel.com>
> > ---
> > UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 26
> > ++++++++++++++++++++++++++
> > UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 9 ++++++---
> > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 22
> > ++++++++++++++++++++++
> > UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 30
> > ++++++++++++++++++++++++++++++
> > 4 files changed, 84 insertions(+), 3 deletions(-)
> >
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> > b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> > index b734a1ea8c..af96e42982 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> > @@ -316,3 +316,29 @@ SetPageTableAttributes (
> >
> > return ;
> > }
> > +
> > +/**
> > + This function returns with no action for 32 bit.
> > +
> > + @param[in] *Cr2 Pointer to variable to hold CR2 register value
> > +**/ VOID
> > +SaveCr2 (
> > + UINTN *Cr2
> > + )
> > +{
> > + return ;
> > +}
> > +
> > +/**
> > + This function returns with no action for 32 bit.
> > +
> > + @param[in] Cr2 Value to write into CR2 register **/ VOID
> > +RestoreCr2 (
> > + UINTN Cr2
> > + )
> > +{
> > + return ;
> > +}
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > index 3b0b3b52ac..ce70f77709 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > @@ -1112,9 +1112,11 @@ SmiRendezvous (
> > ASSERT(CpuIndex < mMaxNumberOfCpus);
> >
> > //
> > - // Save Cr2 because Page Fault exception in SMM may override its
> > value
> > + // Save Cr2 because Page Fault exception in SMM may override its
> > + value, // when using on-demand paging for above 4G memory.
> > //
> > - Cr2 = AsmReadCr2 ();
> > + Cr2 = 0;
> > + SaveCr2 (&Cr2);
> >
> > //
> > // Perform CPU specific entry hooks @@ -1253,10 +1255,11 @@
> > SmiRendezvous (
> >
> > Exit:
> > SmmCpuFeaturesRendezvousExit (CpuIndex);
> > +
> > //
> > // Restore Cr2
> > //
> > - AsmWriteCr2 (Cr2);
> > + RestoreCr2 (Cr2);
> > }
> >
> > /**
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> > b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> > index 84efb22981..c9d147c8a1 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> > @@ -1243,4 +1243,26 @@ EFIAPI
> > PiSmmCpuSmiEntryFixupAddress (
> > );
> >
> > +/**
> > + This function reads CR2 register when on-demand paging is enabled
> > + for 64 bit and no action for 32 bit.
> > +
> > + @param[in] *Cr2 Pointer to variable to hold CR2 register value
> > +**/ VOID
> > +SaveCr2 (
> > + 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 (
> > + UINTN Cr2
> > + );
> > +
> > #endif
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> > b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> > index 2c77cb47a4..6cb44fbbe5 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> > @@ -1053,3 +1053,33 @@ SetPageTableAttributes (
> >
> > return ;
> > }
> > +
> > +/**
> > + This function reads CR2 register when on-demand paging is enabled
> > +
> > + @param[in] *Cr2 Pointer to variable to hold CR2 register value
> > +**/ VOID
> > +SaveCr2 (
> > + 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 (
> > + UINTN Cr2
> > + )
> > +{
> > + if ((!mCpuSmmStaticPageTable) && (Cr2 != 0)) {
> > + AsmWriteCr2 (Cr2);
> > + }
> > +}
> > --
> > 2.16.2.windows.1
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v6] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM
2019-03-29 4:58 [PATCH v6] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM nkvangup
2019-03-29 5:10 ` Ni, Ray
@ 2019-03-30 14:55 ` Zeng, Star
1 sibling, 0 replies; 5+ messages in thread
From: Zeng, Star @ 2019-03-30 14:55 UTC (permalink / raw)
To: Vanguput, Narendra K, edk2-devel@lists.01.org
Cc: Yao, Jiewen, Dong, Eric, Laszlo Ersek, Zeng, Star
Not comment to functionality. But just some observations about code style.
Please check them inline.
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> nkvangup
> Sent: Friday, March 29, 2019 12:58 PM
> To: edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>;
> Laszlo Ersek <lersek@redhat.com>
> Subject: [edk2] [PATCH v6] UefiCpuPkg\CpuSmm: Save & restore CR2 on-
> demand paging in SMM
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1593
>
> For every SMI occurrence, save and restore CR2 register only when SMM
> on-demand paging support is enabled in 64 bit operation mode.
> This is not a bug but to have better improvement of code.
>
> Patch5 is updated with separate functions for Save and Restore of CR2
> based on review feedback.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Vanguput Narendra K <narendra.k.vanguput@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Yao Jiewen <jiewen.yao@intel.com>
> ---
> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 26
> ++++++++++++++++++++++++++
> UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 9 ++++++---
> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 22
> ++++++++++++++++++++++
> UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 30
> ++++++++++++++++++++++++++++++
> 4 files changed, 84 insertions(+), 3 deletions(-)
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> index b734a1ea8c..af96e42982 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> @@ -316,3 +316,29 @@ SetPageTableAttributes (
>
> return ;
> }
> +
> +/**
> + This function returns with no action for 32 bit.
> +
> + @param[in] *Cr2 Pointer to variable to hold CR2 register value
It will be more accurate with "@param[out]" according to https://edk2-docs.gitbooks.io/edk-ii-c-coding-standards-specification/content/5_source_files/57_c_programming.html#table-9-parameter-modifiers.
Same comment is also applied to other implementation or prototype in header file.
> +**/
> +VOID
> +SaveCr2 (
> + UINTN *Cr2
Align with the comments above, it will be better with "OUT UINTN *Cr2"
Same comment is also applied to other implementation or prototype in header file.
> + )
> +{
> + return ;
> +}
> +
> +/**
> + This function returns with no action for 32 bit.
> +
> + @param[in] Cr2 Value to write into CR2 register
> +**/
> +VOID
> +RestoreCr2 (
> + UINTN Cr2
It will be better with "IN UINTN Cr2".
Same comment is also applied to other implementation or prototype in header file.
Thanks,
Star
> + )
> +{
> + return ;
> +}
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> index 3b0b3b52ac..ce70f77709 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -1112,9 +1112,11 @@ SmiRendezvous (
> ASSERT(CpuIndex < mMaxNumberOfCpus);
>
> //
> - // Save Cr2 because Page Fault exception in SMM may override its value
> + // Save Cr2 because Page Fault exception in SMM may override its value,
> + // when using on-demand paging for above 4G memory.
> //
> - Cr2 = AsmReadCr2 ();
> + Cr2 = 0;
> + SaveCr2 (&Cr2);
>
> //
> // Perform CPU specific entry hooks
> @@ -1253,10 +1255,11 @@ SmiRendezvous (
>
> Exit:
> SmmCpuFeaturesRendezvousExit (CpuIndex);
> +
> //
> // Restore Cr2
> //
> - AsmWriteCr2 (Cr2);
> + RestoreCr2 (Cr2);
> }
>
> /**
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> index 84efb22981..c9d147c8a1 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> @@ -1243,4 +1243,26 @@ EFIAPI
> PiSmmCpuSmiEntryFixupAddress (
> );
>
> +/**
> + This function reads CR2 register when on-demand paging is enabled
> + for 64 bit and no action for 32 bit.
> +
> + @param[in] *Cr2 Pointer to variable to hold CR2 register value
> +**/
> +VOID
> +SaveCr2 (
> + 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 (
> + UINTN Cr2
> + );
> +
> #endif
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> index 2c77cb47a4..6cb44fbbe5 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> @@ -1053,3 +1053,33 @@ SetPageTableAttributes (
>
> return ;
> }
> +
> +/**
> + This function reads CR2 register when on-demand paging is enabled
> +
> + @param[in] *Cr2 Pointer to variable to hold CR2 register value
> +**/
> +VOID
> +SaveCr2 (
> + 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 (
> + UINTN Cr2
> + )
> +{
> + if ((!mCpuSmmStaticPageTable) && (Cr2 != 0)) {
> + AsmWriteCr2 (Cr2);
> + }
> +}
> --
> 2.16.2.windows.1
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-03-30 14:55 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-03-29 4:58 [PATCH v6] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM nkvangup
2019-03-29 5:10 ` Ni, Ray
2019-03-29 9:31 ` Vanguput, Narendra K
2019-03-29 9:36 ` Ni, Ray
2019-03-30 14:55 ` Zeng, Star
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox