* [PATCH v4] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM
@ 2019-03-18 14:38 nkvangup
2019-03-20 16:31 ` Laszlo Ersek
0 siblings, 1 reply; 5+ messages in thread
From: nkvangup @ 2019-03-18 14:38 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.
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/MpService.c | 22 ++++++++++++++--------
UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 2 +-
2 files changed, 15 insertions(+), 9 deletions(-)
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
index 3b0b3b52ac..0c07b31c4f 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -28,6 +28,7 @@ UINTN mSemaphoreSize;
SPIN_LOCK *mPFLock = NULL;
SMM_CPU_SYNC_MODE mCpuSmmSyncMode;
BOOLEAN mMachineCheckSupported = FALSE;
+BOOLEAN mCpuSmmStaticPageTable = TRUE;
/**
Performs an atomic compare exchange operation to get semaphore.
@@ -1111,10 +1112,13 @@ SmiRendezvous (
ASSERT(CpuIndex < mMaxNumberOfCpus);
- //
- // Save Cr2 because Page Fault exception in SMM may override its value
- //
- Cr2 = AsmReadCr2 ();
+ if (!mCpuSmmStaticPageTable) {
+ //
+ // Save and restore Cr2 when using on-demand paging for above 4G memory because Page Fault
+ // exception in SMM may override its value
+ //
+ Cr2 = AsmReadCr2 ();
+ }
//
// Perform CPU specific entry hooks
@@ -1253,10 +1257,12 @@ SmiRendezvous (
Exit:
SmmCpuFeaturesRendezvousExit (CpuIndex);
- //
- // Restore Cr2
- //
- AsmWriteCr2 (Cr2);
+ if (!mCpuSmmStaticPageTable) {
+ //
+ // Restore Cr2
+ //
+ AsmWriteCr2 (Cr2);
+ }
}
/**
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
index 2c77cb47a4..e444b8a031 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
@@ -21,7 +21,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
LIST_ENTRY mPagePool = INITIALIZE_LIST_HEAD_VARIABLE (mPagePool);
BOOLEAN m1GPageTableSupport = FALSE;
-BOOLEAN mCpuSmmStaticPageTable;
+extern BOOLEAN mCpuSmmStaticPageTable;
/**
Disable CET.
--
2.16.2.windows.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v4] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM
2019-03-18 14:38 [PATCH v4] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM nkvangup
@ 2019-03-20 16:31 ` Laszlo Ersek
2019-03-20 17:27 ` Vanguput, Narendra K
0 siblings, 1 reply; 5+ messages in thread
From: Laszlo Ersek @ 2019-03-20 16:31 UTC (permalink / raw)
To: nkvangup, edk2-devel; +Cc: Yao Jiewen, Eric Dong
On 03/18/19 15:38, nkvangup wrote:
> 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.
>
> 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/MpService.c | 22 ++++++++++++++--------
> UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 2 +-
> 2 files changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> index 3b0b3b52ac..0c07b31c4f 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -28,6 +28,7 @@ UINTN mSemaphoreSize;
> SPIN_LOCK *mPFLock = NULL;
> SMM_CPU_SYNC_MODE mCpuSmmSyncMode;
> BOOLEAN mMachineCheckSupported = FALSE;
> +BOOLEAN mCpuSmmStaticPageTable = TRUE;
Hmmm. This change is a bit daring, but I think it could be valid.
- In the IA32 build, mCpuSmmStaticPageTable would never be modified, or
read, by *preexistent* code (because all that code is in X64/PageTbl.c).
And the new code, added by this patch, would (presumably) work fine,
with the initial TRUE value.
- In the X64 build, the preexistent code would never read the initial
value (which we now set to TRUE here), i.e. before overwriting the
variable from the PCD -- because that would mean a bug in the
preexistent code. (Well, unless that code relied on the zero initial
value of the variable).
(1) I think I'd like to defer on this to other UefiCpuPkg reviewers.
Honestly I find this style questionable. It makes me feel uncomfortable.
I'd prefer the new APIs with the separate IA32/X64 implementations that
I suggested in my v2 review. But if other reviewers like this one
better, I won't mind.
(After hearing their opinions, I'd attempt to find the time to
regression test the patch (or maybe v5), too.)
Assuming other reviewers prefer this approach over my suggestion, I have
some other comments:
>
> /**
> Performs an atomic compare exchange operation to get semaphore.
> @@ -1111,10 +1112,13 @@ SmiRendezvous (
>
> ASSERT(CpuIndex < mMaxNumberOfCpus);
>
> - //
> - // Save Cr2 because Page Fault exception in SMM may override its value
> - //
> - Cr2 = AsmReadCr2 ();
> + if (!mCpuSmmStaticPageTable) {
> + //
> + // Save and restore Cr2 when using on-demand paging for above 4G memory because Page Fault
> + // exception in SMM may override its value
> + //
> + Cr2 = AsmReadCr2 ();
> + }
(2) The indentation of the "if" is broken.
(3) Given that we're already using two comment lines, I'd suggest not
exceeding 80 characters per line.
>
> //
> // Perform CPU specific entry hooks
> @@ -1253,10 +1257,12 @@ SmiRendezvous (
>
> Exit:
> SmmCpuFeaturesRendezvousExit (CpuIndex);
> - //
> - // Restore Cr2
> - //
> - AsmWriteCr2 (Cr2);
> + if (!mCpuSmmStaticPageTable) {
(4) same as (2).
> + //
> + // Restore Cr2
> + //
> + AsmWriteCr2 (Cr2);
> + }
> }
>
> /**
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> index 2c77cb47a4..e444b8a031 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> @@ -21,7 +21,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>
> LIST_ENTRY mPagePool = INITIALIZE_LIST_HEAD_VARIABLE (mPagePool);
> BOOLEAN m1GPageTableSupport = FALSE;
> -BOOLEAN mCpuSmmStaticPageTable;
> +extern BOOLEAN mCpuSmmStaticPageTable;
(5) This is generally not great style, and it conflicts with the
existent code of this driver. Namely, declarations of variables with
file scope, static storage duration, and external linkage, should go
into "PiSmmCpuDxeSmm.h"-- we already got a bunch of them there.
Thanks
Laszlo
>
> /**
> Disable CET.
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v4] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM
2019-03-20 16:31 ` Laszlo Ersek
@ 2019-03-20 17:27 ` Vanguput, Narendra K
2019-03-21 0:56 ` Dong, Eric
0 siblings, 1 reply; 5+ messages in thread
From: Vanguput, Narendra K @ 2019-03-20 17:27 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel@lists.01.org
Cc: Yao, Jiewen, Dong, Eric, Chinnusamy, Rajkumar K, Ni, Ray
Hi Laszlo,
Thanks for your comments.
For your comment #1, My thoughts are when we add two functions (SaveCr2 & RestoreCr2). For IA32, it actually don't save and restore, simply returns. Later, it might be confusing unless if we know the background and gone through 64 bit supported code. And also its kind of adding more code while we have alternate solution.
In the proposed changes, I felt its straight forward and light changes needed.
Yes, I would like to hear from other reviewers too to take the right option.
For comments #2 & #4, Yes, I notified it, waiting to update along with other comments.
For comments #3 & #5, will consider them. Will adjust the no. characters and will move extern of mCpuSmmStaticPageTable to PiSmmCpuDxeSmm.h file.
Thanks,
Naren
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com]
Sent: Wednesday, March 20, 2019 10:01 PM
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>
Subject: Re: [edk2] [PATCH v4] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM
On 03/18/19 15:38, nkvangup wrote:
> 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.
>
> 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/MpService.c | 22 ++++++++++++++--------
> UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 2 +-
> 2 files changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> index 3b0b3b52ac..0c07b31c4f 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -28,6 +28,7 @@ UINTN mSemaphoreSize;
> SPIN_LOCK *mPFLock = NULL;
> SMM_CPU_SYNC_MODE mCpuSmmSyncMode;
> BOOLEAN mMachineCheckSupported = FALSE;
> +BOOLEAN mCpuSmmStaticPageTable = TRUE;
Hmmm. This change is a bit daring, but I think it could be valid.
- In the IA32 build, mCpuSmmStaticPageTable would never be modified, or read, by *preexistent* code (because all that code is in X64/PageTbl.c).
And the new code, added by this patch, would (presumably) work fine, with the initial TRUE value.
- In the X64 build, the preexistent code would never read the initial value (which we now set to TRUE here), i.e. before overwriting the variable from the PCD -- because that would mean a bug in the preexistent code. (Well, unless that code relied on the zero initial value of the variable).
(1) I think I'd like to defer on this to other UefiCpuPkg reviewers.
Honestly I find this style questionable. It makes me feel uncomfortable.
I'd prefer the new APIs with the separate IA32/X64 implementations that I suggested in my v2 review. But if other reviewers like this one better, I won't mind.
(After hearing their opinions, I'd attempt to find the time to regression test the patch (or maybe v5), too.)
Assuming other reviewers prefer this approach over my suggestion, I have some other comments:
>
> /**
> Performs an atomic compare exchange operation to get semaphore.
> @@ -1111,10 +1112,13 @@ SmiRendezvous (
>
> ASSERT(CpuIndex < mMaxNumberOfCpus);
>
> - //
> - // Save Cr2 because Page Fault exception in SMM may override its
> value
> - //
> - Cr2 = AsmReadCr2 ();
> + if (!mCpuSmmStaticPageTable) {
> + //
> + // Save and restore Cr2 when using on-demand paging for above 4G memory because Page Fault
> + // exception in SMM may override its value
> + //
> + Cr2 = AsmReadCr2 ();
> + }
(2) The indentation of the "if" is broken.
(3) Given that we're already using two comment lines, I'd suggest not exceeding 80 characters per line.
>
> //
> // Perform CPU specific entry hooks @@ -1253,10 +1257,12 @@
> SmiRendezvous (
>
> Exit:
> SmmCpuFeaturesRendezvousExit (CpuIndex);
> - //
> - // Restore Cr2
> - //
> - AsmWriteCr2 (Cr2);
> + if (!mCpuSmmStaticPageTable) {
(4) same as (2).
> + //
> + // Restore Cr2
> + //
> + AsmWriteCr2 (Cr2);
> + }
> }
>
> /**
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> index 2c77cb47a4..e444b8a031 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> @@ -21,7 +21,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>
> LIST_ENTRY mPagePool = INITIALIZE_LIST_HEAD_VARIABLE (mPagePool);
> BOOLEAN m1GPageTableSupport = FALSE;
> -BOOLEAN mCpuSmmStaticPageTable;
> +extern BOOLEAN mCpuSmmStaticPageTable;
(5) This is generally not great style, and it conflicts with the existent code of this driver. Namely, declarations of variables with file scope, static storage duration, and external linkage, should go into "PiSmmCpuDxeSmm.h"-- we already got a bunch of them there.
Thanks
Laszlo
>
> /**
> Disable CET.
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v4] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM
2019-03-20 17:27 ` Vanguput, Narendra K
@ 2019-03-21 0:56 ` Dong, Eric
2019-03-21 2:48 ` Vanguput, Narendra K
0 siblings, 1 reply; 5+ messages in thread
From: Dong, Eric @ 2019-03-21 0:56 UTC (permalink / raw)
To: Vanguput, Narendra K, Laszlo Ersek, edk2-devel@lists.01.org
Cc: Yao, Jiewen, Chinnusamy, Rajkumar K, Ni, Ray
Hi Naren,
I agree with Laszlo's comment for #1. I think separate functions for IA32/X64 are much clear than the current one. I think in current EDK2 codebase, many similar cases already exits.
Thanks,
Eric
> -----Original Message-----
> From: Vanguput, Narendra K
> Sent: Thursday, March 21, 2019 1:28 AM
> To: Laszlo Ersek <lersek@redhat.com>; edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>;
> Chinnusamy, Rajkumar K <rajkumar.k.chinnusamy@intel.com>; Ni, Ray
> <ray.ni@intel.com>
> Subject: RE: [edk2] [PATCH v4] UefiCpuPkg\CpuSmm: Save & restore CR2 on-
> demand paging in SMM
>
> Hi Laszlo,
>
> Thanks for your comments.
>
> For your comment #1, My thoughts are when we add two functions (SaveCr2
> & RestoreCr2). For IA32, it actually don't save and restore, simply returns.
> Later, it might be confusing unless if we know the background and gone
> through 64 bit supported code. And also its kind of adding more code while
> we have alternate solution.
> In the proposed changes, I felt its straight forward and light changes needed.
> Yes, I would like to hear from other reviewers too to take the right option.
>
> For comments #2 & #4, Yes, I notified it, waiting to update along with other
> comments.
>
> For comments #3 & #5, will consider them. Will adjust the no. characters and
> will move extern of mCpuSmmStaticPageTable to PiSmmCpuDxeSmm.h file.
>
> Thanks,
> Naren
>
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, March 20, 2019 10:01 PM
> 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>
> Subject: Re: [edk2] [PATCH v4] UefiCpuPkg\CpuSmm: Save & restore CR2 on-
> demand paging in SMM
>
> On 03/18/19 15:38, nkvangup wrote:
> > 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.
> >
> > 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/MpService.c | 22 ++++++++++++++-----
> ---
> > UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 2 +-
> > 2 files changed, 15 insertions(+), 9 deletions(-)
> >
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > index 3b0b3b52ac..0c07b31c4f 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > @@ -28,6 +28,7 @@ UINTN mSemaphoreSize;
> > SPIN_LOCK *mPFLock = NULL;
> > SMM_CPU_SYNC_MODE mCpuSmmSyncMode;
> > BOOLEAN mMachineCheckSupported = FALSE;
> > +BOOLEAN mCpuSmmStaticPageTable = TRUE;
>
> Hmmm. This change is a bit daring, but I think it could be valid.
>
> - In the IA32 build, mCpuSmmStaticPageTable would never be modified, or
> read, by *preexistent* code (because all that code is in X64/PageTbl.c).
> And the new code, added by this patch, would (presumably) work fine, with
> the initial TRUE value.
>
> - In the X64 build, the preexistent code would never read the initial value
> (which we now set to TRUE here), i.e. before overwriting the variable from
> the PCD -- because that would mean a bug in the preexistent code. (Well,
> unless that code relied on the zero initial value of the variable).
>
> (1) I think I'd like to defer on this to other UefiCpuPkg reviewers.
> Honestly I find this style questionable. It makes me feel uncomfortable.
> I'd prefer the new APIs with the separate IA32/X64 implementations that I
> suggested in my v2 review. But if other reviewers like this one better, I won't
> mind.
>
> (After hearing their opinions, I'd attempt to find the time to regression test
> the patch (or maybe v5), too.)
>
> Assuming other reviewers prefer this approach over my suggestion, I have
> some other comments:
>
> >
> > /**
> > Performs an atomic compare exchange operation to get semaphore.
> > @@ -1111,10 +1112,13 @@ SmiRendezvous (
> >
> > ASSERT(CpuIndex < mMaxNumberOfCpus);
> >
> > - //
> > - // Save Cr2 because Page Fault exception in SMM may override its
> > value
> > - //
> > - Cr2 = AsmReadCr2 ();
> > + if (!mCpuSmmStaticPageTable) {
> > + //
> > + // Save and restore Cr2 when using on-demand paging for above 4G
> memory because Page Fault
> > + // exception in SMM may override its value
> > + //
> > + Cr2 = AsmReadCr2 ();
> > + }
>
> (2) The indentation of the "if" is broken.
>
> (3) Given that we're already using two comment lines, I'd suggest not
> exceeding 80 characters per line.
>
> >
> > //
> > // Perform CPU specific entry hooks @@ -1253,10 +1257,12 @@
> > SmiRendezvous (
> >
> > Exit:
> > SmmCpuFeaturesRendezvousExit (CpuIndex);
> > - //
> > - // Restore Cr2
> > - //
> > - AsmWriteCr2 (Cr2);
> > + if (!mCpuSmmStaticPageTable) {
>
> (4) same as (2).
>
> > + //
> > + // Restore Cr2
> > + //
> > + AsmWriteCr2 (Cr2);
> > + }
> > }
> >
> > /**
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> > b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> > index 2c77cb47a4..e444b8a031 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> > @@ -21,7 +21,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF
> ANY KIND, EITHER EXPRESS OR IMPLIED.
> >
> > LIST_ENTRY mPagePool = INITIALIZE_LIST_HEAD_VARIABLE
> (mPagePool);
> > BOOLEAN m1GPageTableSupport = FALSE;
> > -BOOLEAN mCpuSmmStaticPageTable;
> > +extern BOOLEAN mCpuSmmStaticPageTable;
>
> (5) This is generally not great style, and it conflicts with the existent code of
> this driver. Namely, declarations of variables with file scope, static storage
> duration, and external linkage, should go into "PiSmmCpuDxeSmm.h"-- we
> already got a bunch of them there.
>
> Thanks
> Laszlo
>
> >
> > /**
> > Disable CET.
> >
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v4] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM
2019-03-21 0:56 ` Dong, Eric
@ 2019-03-21 2:48 ` Vanguput, Narendra K
0 siblings, 0 replies; 5+ messages in thread
From: Vanguput, Narendra K @ 2019-03-21 2:48 UTC (permalink / raw)
To: Dong, Eric, Laszlo Ersek, edk2-devel@lists.01.org
Cc: Yao, Jiewen, Chinnusamy, Rajkumar K, Ni, Ray
Thanks Eric!.
Will go as per the comment #1 suggested and update further in the code review.
Thanks,
Naren
> -----Original Message-----
> From: Dong, Eric
> Sent: Thursday, March 21, 2019 6:26 AM
> To: Vanguput, Narendra K <narendra.k.vanguput@intel.com>; Laszlo Ersek
> <lersek@redhat.com>; edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Chinnusamy, Rajkumar K
> <rajkumar.k.chinnusamy@intel.com>; Ni, Ray <ray.ni@intel.com>
> Subject: RE: [edk2] [PATCH v4] UefiCpuPkg\CpuSmm: Save & restore CR2 on-
> demand paging in SMM
>
> Hi Naren,
>
> I agree with Laszlo's comment for #1. I think separate functions for IA32/X64
> are much clear than the current one. I think in current EDK2 codebase, many
> similar cases already exits.
>
> Thanks,
> Eric
>
> > -----Original Message-----
> > From: Vanguput, Narendra K
> > Sent: Thursday, March 21, 2019 1:28 AM
> > To: Laszlo Ersek <lersek@redhat.com>; edk2-devel@lists.01.org
> > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric
> > <eric.dong@intel.com>; Chinnusamy, Rajkumar K
> > <rajkumar.k.chinnusamy@intel.com>; Ni, Ray <ray.ni@intel.com>
> > Subject: RE: [edk2] [PATCH v4] UefiCpuPkg\CpuSmm: Save & restore CR2
> > on- demand paging in SMM
> >
> > Hi Laszlo,
> >
> > Thanks for your comments.
> >
> > For your comment #1, My thoughts are when we add two functions
> > (SaveCr2 & RestoreCr2). For IA32, it actually don't save and restore, simply
> returns.
> > Later, it might be confusing unless if we know the background and gone
> > through 64 bit supported code. And also its kind of adding more code
> > while we have alternate solution.
> > In the proposed changes, I felt its straight forward and light changes needed.
> > Yes, I would like to hear from other reviewers too to take the right option.
> >
> > For comments #2 & #4, Yes, I notified it, waiting to update along with
> > other comments.
> >
> > For comments #3 & #5, will consider them. Will adjust the no.
> > characters and will move extern of mCpuSmmStaticPageTable to
> PiSmmCpuDxeSmm.h file.
> >
> > Thanks,
> > Naren
> >
> > -----Original Message-----
> > From: Laszlo Ersek [mailto:lersek@redhat.com]
> > Sent: Wednesday, March 20, 2019 10:01 PM
> > 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>
> > Subject: Re: [edk2] [PATCH v4] UefiCpuPkg\CpuSmm: Save & restore CR2
> > on- demand paging in SMM
> >
> > On 03/18/19 15:38, nkvangup wrote:
> > > 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.
> > >
> > > 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/MpService.c | 22 ++++++++++++++-----
> > ---
> > > UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 2 +-
> > > 2 files changed, 15 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > > b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > > index 3b0b3b52ac..0c07b31c4f 100644
> > > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > > @@ -28,6 +28,7 @@ UINTN mSemaphoreSize;
> > > SPIN_LOCK *mPFLock = NULL;
> > > SMM_CPU_SYNC_MODE mCpuSmmSyncMode;
> > > BOOLEAN mMachineCheckSupported = FALSE;
> > > +BOOLEAN mCpuSmmStaticPageTable = TRUE;
> >
> > Hmmm. This change is a bit daring, but I think it could be valid.
> >
> > - In the IA32 build, mCpuSmmStaticPageTable would never be modified,
> > or read, by *preexistent* code (because all that code is in X64/PageTbl.c).
> > And the new code, added by this patch, would (presumably) work fine,
> > with the initial TRUE value.
> >
> > - In the X64 build, the preexistent code would never read the initial
> > value (which we now set to TRUE here), i.e. before overwriting the
> > variable from the PCD -- because that would mean a bug in the
> > preexistent code. (Well, unless that code relied on the zero initial value of
> the variable).
> >
> > (1) I think I'd like to defer on this to other UefiCpuPkg reviewers.
> > Honestly I find this style questionable. It makes me feel uncomfortable.
> > I'd prefer the new APIs with the separate IA32/X64 implementations
> > that I suggested in my v2 review. But if other reviewers like this one
> > better, I won't mind.
> >
> > (After hearing their opinions, I'd attempt to find the time to
> > regression test the patch (or maybe v5), too.)
> >
> > Assuming other reviewers prefer this approach over my suggestion, I
> > have some other comments:
> >
> > >
> > > /**
> > > Performs an atomic compare exchange operation to get semaphore.
> > > @@ -1111,10 +1112,13 @@ SmiRendezvous (
> > >
> > > ASSERT(CpuIndex < mMaxNumberOfCpus);
> > >
> > > - //
> > > - // Save Cr2 because Page Fault exception in SMM may override its
> > > value
> > > - //
> > > - Cr2 = AsmReadCr2 ();
> > > + if (!mCpuSmmStaticPageTable) {
> > > + //
> > > + // Save and restore Cr2 when using on-demand paging for above
> > > + 4G
> > memory because Page Fault
> > > + // exception in SMM may override its value
> > > + //
> > > + Cr2 = AsmReadCr2 ();
> > > + }
> >
> > (2) The indentation of the "if" is broken.
> >
> > (3) Given that we're already using two comment lines, I'd suggest not
> > exceeding 80 characters per line.
> >
> > >
> > > //
> > > // Perform CPU specific entry hooks @@ -1253,10 +1257,12 @@
> > > SmiRendezvous (
> > >
> > > Exit:
> > > SmmCpuFeaturesRendezvousExit (CpuIndex);
> > > - //
> > > - // Restore Cr2
> > > - //
> > > - AsmWriteCr2 (Cr2);
> > > + if (!mCpuSmmStaticPageTable) {
> >
> > (4) same as (2).
> >
> > > + //
> > > + // Restore Cr2
> > > + //
> > > + AsmWriteCr2 (Cr2);
> > > + }
> > > }
> > >
> > > /**
> > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> > > b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> > > index 2c77cb47a4..e444b8a031 100644
> > > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> > > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> > > @@ -21,7 +21,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF
> > ANY KIND, EITHER EXPRESS OR IMPLIED.
> > >
> > > LIST_ENTRY mPagePool = INITIALIZE_LIST_HEAD_VARIABLE
> > (mPagePool);
> > > BOOLEAN m1GPageTableSupport = FALSE;
> > > -BOOLEAN mCpuSmmStaticPageTable;
> > > +extern BOOLEAN mCpuSmmStaticPageTable;
> >
> > (5) This is generally not great style, and it conflicts with the
> > existent code of this driver. Namely, declarations of variables with
> > file scope, static storage duration, and external linkage, should go
> > into "PiSmmCpuDxeSmm.h"-- we already got a bunch of them there.
> >
> > Thanks
> > Laszlo
> >
> > >
> > > /**
> > > Disable CET.
> > >
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-03-21 2:48 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-03-18 14:38 [PATCH v4] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM nkvangup
2019-03-20 16:31 ` Laszlo Ersek
2019-03-20 17:27 ` Vanguput, Narendra K
2019-03-21 0:56 ` Dong, Eric
2019-03-21 2:48 ` Vanguput, Narendra K
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox