From: "Vanguput, Narendra K" <narendra.k.vanguput@intel.com>
To: "Dong, Eric" <eric.dong@intel.com>,
Laszlo Ersek <lersek@redhat.com>,
"edk2-devel@lists.01.org" <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: [PATCH v4] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM
Date: Thu, 21 Mar 2019 02:48:42 +0000 [thread overview]
Message-ID: <020B34E8430BB544AB9E0330B597780A664DD68C@BGSMSX102.gar.corp.intel.com> (raw)
In-Reply-To: <ED077930C258884BBCB450DB737E662259DD94F8@shsmsx102.ccr.corp.intel.com>
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.
> > >
prev parent reply other threads:[~2019-03-21 2:48 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
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 message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=020B34E8430BB544AB9E0330B597780A664DD68C@BGSMSX102.gar.corp.intel.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox