from:"Vanguput, Narendra K"

Re: [edk2] [PATCH v4] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM

2019-03-20 Thread Vanguput, Narendra K

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:ler...@redhat.com] 
Sent: Wednesday, March 20, 2019 10:01 PM
To: Vanguput, Narendra K ; 
edk2-devel@lists.01.org
Cc: Yao, Jiewen ; Dong, Eric 
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 
> Cc: Eric Dong 
> Cc: Ray Ni 
> Cc: Laszlo Ersek 
> Cc: Yao Jiewen 
> ---
>  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.
> @@ -,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 A

Re: [edk2] [PATCH v4] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM

2019-03-20 Thread Vanguput, Narendra K

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 ; Laszlo Ersek
> ; edk2-devel@lists.01.org
> Cc: Yao, Jiewen ; Chinnusamy, Rajkumar K
> ; Ni, Ray 
> 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 ; edk2-devel@lists.01.org
> > Cc: Yao, Jiewen ; Dong, Eric
> > ; Chinnusamy, Rajkumar K
> > ; Ni, Ray 
> > 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:ler...@redhat.com]
> > Sent: Wednesday, March 20, 2019 10:01 PM
> > To: Vanguput, Narendra K ; edk2-
> > de...@lists.01.org
> > Cc: Yao, Jiewen ; Dong, Eric
> > 
> > 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 
> > > Cc: Eric Dong 
> > > Cc: Ray Ni 
> > > Cc: Laszlo Ersek 
> > > Cc: Yao Jiewen 
> > > ---
> > >  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. I

Re: [edk2] [PATCH v5] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM

2019-03-28 Thread Vanguput, Narendra K

Thanks Mike for the comments!

I updated and send as PATCH v6.
Please review.

Thanks,
Narendra

> -Original Message-
> From: Kinney, Michael D
> Sent: Saturday, March 23, 2019 12:44 AM
> To: Vanguput, Narendra K ; edk2-
> de...@lists.01.org; Kinney, Michael D 
> Cc: Yao, Jiewen ; Dong, Eric ;
> Laszlo Ersek 
> Subject: RE: [edk2] [PATCH v5] UefiCpuPkg\CpuSmm: Save & restore CR2 on-
> demand paging in SMM
> 
> Hi Narendra,
> 
> With this implementation, you have moved the save/restore location to a
> module global variable.  The name should be prefixed with 'm' to make that
> clear.
> 
>   mCr2
> 
> I do not think using a module global is MP safe.
> 
> The current implementation uses a local on the stack that is MP safe because
> each CPU has its own stack.
> 
> Mike
> 
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org]
> > On Behalf Of nkvangup
> > Sent: Friday, March 22, 2019 11:50 AM
> > To: edk2-devel@lists.01.org
> > Cc: Yao, Jiewen ; Dong, Eric
> > ; Laszlo Ersek 
> > Subject: [edk2] [PATCH v5] 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
> > 
> > Cc: Eric Dong 
> > Cc: Ray Ni 
> > Cc: Laszlo Ersek 
> > Cc: Yao Jiewen 
> > ---
> >  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c   | 22
> > ++
> >  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c  |  9 +---
> > -
> >  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 16
> > 
> >  UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c| 28
> > 
> >  4 files changed, 71 insertions(+), 4 deletions(-)
> >
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> > b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> > index b734a1ea8c..3750332ca8 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> > @@ -316,3 +316,25 @@ SetPageTableAttributes (
> >
> >return ;
> >  }
> > +
> > +/**
> > +  This function returns with no action for 32 bit.
> > +**/
> > +VOID
> > +SaveCr2 (
> > +  VOID
> > +  )
> > +{
> > +// Do Nothing
> > +}
> > +
> > +/**
> > +  This function returns with no action for 32 bit.
> > +**/
> > +VOID
> > +RestoreCr2 (
> > +  VOID
> > +  )
> > +{
> > +// Do Nothing
> > +}
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > index 3b0b3b52ac..6a5736a3eb 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > @@ -1107,14 +1107,14 @@ SmiRendezvous (
> >BOOLEANIsBsp;
> >BOOLEANBspInProgress;
> >UINTN  Index;
> > -  UINTN  Cr2;
> >
> >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 ();
> > +  SaveCr2 ();
> >
> >//
> >// Perform CPU specific entry hooks @@ -1253,10 +1253,11 @@
> > SmiRendezvous (
> >
> >  Exit:
> >SmmCpuFeaturesRendezvousExit (CpuIndex);
> > +
> >//
> >// Restore Cr2
> >//
> > -  AsmWriteCr2 (Cr2);
> > +  RestoreCr2 ();
> >  }
> >
> >  /**
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> > b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> > index 84efb22981..71a8c13960 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> > @@ -1243,4 +1243,20 @@ EFIAPI
> >  PiSmmCpuSmiEntryFixupAddress (
> >   );
> >
> > +/**
> > +  This function saves CR2 regis

Re: [edk2] [PATCH v6] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM

2019-03-29 Thread Vanguput, Narendra K

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 ; 
edk2-devel@lists.01.org
Cc: Yao, Jiewen ; Dong, Eric ; 
Laszlo Ersek 
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  On Behalf Of 
> nkvangup
> Sent: Friday, March 29, 2019 12:58 PM
> To: edk2-devel@lists.01.org
> Cc: Yao, Jiewen ; Dong, Eric 
> ; Laszlo Ersek 
> 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 
> Cc: Eric Dong 
> Cc: Ray Ni 
> Cc: Laszlo Ersek 
> Cc: Yao Jiewen 
> ---
>  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..6cb44

Re: [edk2] [PATCH v8] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM

2019-03-31 Thread Vanguput, Narendra K

Hi Nate, Andrew Fish,

Paging is enabled. Here the point is SMM using Static page table Vs On-Demand 
Paging.
SMM always builds static page table for 32 bit. Only for 64 bit, there is a PCD 
flag to control whether to use Static Page Table or On-Demand Paging.
Below is the PCD flag details copied from 'UefiCpuPkg.dec' file
[
  ## Indicates if SMM uses static page table.
  #  If enabled, SMM will not use on-demand paging. SMM will build static page 
table for all memory.
  #  This flag only impacts X64 build, because SMM always builds static page 
table for IA32.
  #  It could not be enabled at the same time with SMM profile feature 
(PcdCpuSmmProfileEnable).
  #  It could not be enabled also at the same time with heap guard feature for 
SMM
  #  (PcdHeapGuardPropertyMask in MdeModulePkg).
  #   TRUE  - SMM uses static page table for all memory.
  #   FALSE - SMM uses static page table for below 4G memory and use on-demand 
paging for above 4G memory.
  # @Prompt Use static page table for all memory in SMM.
  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStaticPageTable|TRUE|BOOLEAN|0x3213210D
]

Storing and restoring CR2 register is only required for On-Demand Paging when 
its in 64 bit. So the changes are related to that.

For the Comment #2,
Initially it was like that (I think in Patch 4), but based on review comments 
and to make clear implementation changed to use APIs. 

Hope I have answered your questions.

Thanks,
Naren

> -Original Message-
> From: af...@apple.com [mailto:af...@apple.com]
> Sent: Saturday, March 30, 2019 3:08 AM
> To: Desimone, Nathaniel L 
> Cc: Vanguput, Narendra K ; edk2-
> de...@lists.01.org; Laszlo Ersek ; Yao, Jiewen
> ; Dong, Eric 
> Subject: Re: [edk2] [PATCH v8] UefiCpuPkg\CpuSmm: Save & restore CR2 on-
> demand paging in SMM
> 
> 
> 
> > On Mar 29, 2019, at 2:22 PM, Desimone, Nathaniel L
>  wrote:
> >
> > 1. Why would you do this for 64 bit but not 32 bit?
> 
> Is paging enabled on 32-bit, it is required for Long mode?
> 
> Also I'm not clear why it is an enhancement given you could take a periodic
> SMM in the kernels page fault handler and trashing CR2 seems bad.  Maybe
> there is some behavior I'm missing?
> 
> I'm not sure how big an issue this is but if SMM is modifying CR2 it is 
> leaking
> information about SMM operations outside of SMM.
> 
> Thanks,
> 
> Andrew Fish
> 
> > 2. Why don't you add the if statement to MpService.c instead of spreading it
> to PageTbl.c?
> > 3. What is the reason for this anyway? Adding the conditional is probably
> more execution time than just reading CR2 always.
> >
> > Thanks,
> > Nate
> >
> > -Original Message-
> > From: edk2-devel  On Behalf Of
> > nkvangup
> > Sent: Friday, March 29, 2019 8:45 AM
> > To: edk2-devel@lists.01.org
> > Cc: Yao, Jiewen ; Dong, Eric
> > ; Laszlo Ersek 
> > Subject: [edk2] [PATCH v8] 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.
> >
> > Patch6 - Removed Global Cr2 instead used function parameter
> >
> > Patch7 - Removed checking Cr2 with 0 as per feedback
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Vanguput Narendra K
> > <mailto:narendra.k.vangu...@intel.com>
> > Cc: Eric Dong <mailto:eric.d...@intel.com>
> > Cc: Ray Ni <mailto:ray...@intel.com>
> > Cc: Laszlo Ersek <mailto:ler...@redhat.com>
> > Cc: Yao Jiewen <mailto:jiewen@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..d3f62ed806 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> > @@ -316,3 +316,29 @@ SetPageTableAttributes (
> >
> >   return ;
> > }
> > +
> > +/**
> > +  This function returns

Re: [edk2] [PATCH v9] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM

2019-04-01 Thread Vanguput, Narendra K

Hi Nate,

The 'PcdCpuSmmStaticPageTable' is only used in the X64 version of PageTbl.c  
that's why I updated only for 64 bit. SMM always builds static page table for 
IA32.
Please refer my previous mail for more details.

Thanks,
Narendra

> -Original Message-
> From: Desimone, Nathaniel L
> Sent: Tuesday, April 2, 2019 5:57 AM
> To: Vanguput, Narendra K ; edk2-
> de...@lists.01.org
> Cc: Yao, Jiewen ; Dong, Eric ;
> Laszlo Ersek 
> Subject: RE: [edk2] [PATCH v9] UefiCpuPkg\CpuSmm: Save & restore CR2 on-
> demand paging in SMM
> 
> This patch seems to only add the IN/OUT decorators on function parameters,
> which is a good change. However, it does not address any of my previous
> comments:
> 
> 1. Why would you do this for 64 bit but not 32 bit?
> 2. Why don't you add the if statement to MpService.c instead of spreading it
> to PageTbl.c?
> 3. What is the reason for this anyway? Adding the conditional is probably
> more execution time than just reading CR2 always.
> 
> I also share Andrew's concern that in the case of a periodic SMI happening
> during OS runtime, there is nothing preventing the handler of the periodic
> SMI from clobbering the value of CR2, which could potentially cause kernel
> panics once we return back from SMM to the OS. I am not aware of any
> periodic SMIs in OVMF, so I don't believe OVMF testing will catch these type
> of issues. I consider not doing the save/restore in the 32 bit SMM to be
> dangerous, especially since all recent platforms that I can think of don't 
> use 32
> bit SMM anymore, so any bug(s) introduced may go unnoticed for a long time.
> 
> Thanks,
> Nate
> 
> -Original Message-
> From: edk2-devel  On Behalf Of nkvangup
> Sent: Monday, April 1, 2019 1:16 AM
> To: edk2-devel@lists.01.org
> Cc: Yao, Jiewen ; Dong, Eric ;
> Laszlo Ersek 
> Subject: [edk2] [PATCH v9] 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.
> 
> Patch6 - Removed Global Cr2 instead used function parameter.
> 
> Patch7 - Removed checking Cr2 with 0 as per feedback.
> 
> Patch8 and 9 - Aligned with EDK2 Coding style.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Vanguput Narendra K 
> Cc: Eric Dong 
> Cc: Ray Ni 
> Cc: Laszlo Ersek 
> Cc: Yao Jiewen 
> ---
>  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..d1e146a70c 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[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 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);
&

Re: [edk2] [PATCH v4] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM

Re: [edk2] [PATCH v4] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM

Re: [edk2] [PATCH v5] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM

Re: [edk2] [PATCH v6] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM

Re: [edk2] [PATCH v8] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM

Re: [edk2] [PATCH v9] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM

6 matches

Site Navigation

Mail list logo

Footer information