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

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

2019-03-30 Thread 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-boun...@lists.01.org] 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

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 ();
> 
>//
>// 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/Pa

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

2019-03-29 Thread Ni, Ray

Yes. With below "Cr2 !=0" removed from final commit, Reviewed-by: Ray Ni 

> > +RestoreCr2 (
> > +  UINTN  Cr2
> > +  )
> > +{
> > +  if ((!mCpuSmmStaticPageTable) && (Cr2 != 0)) {
> > +AsmWriteCr2 (Cr2);
> > +  }
> > +}

> -Original Message-
> From: Vanguput, Narendra K 
> Sent: Friday, March 29, 2019 5:32 PM
> To: Ni, Ray ; 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
> 
> 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-
> de...@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 ();
> >
> >//
> >// Perform CPU specific entry hooks @@ -1253,10 +1255,11 @@
> > SmiRendezvous (
> >
> >  Exit:
> >SmmCpuFeaturesRendezvousExit (CpuIndex);
> > +
&

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 ();
> 
>//
>// 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 100

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

2019-03-28 Thread Ni, Ray

> +  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 ();
> 
>//
>// 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
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

2019-03-28 Thread nkvangup

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 ();
 
   //
   // 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

Re: [edk2] [PATCH v6] 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 v6] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM

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

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

5 matches

Site Navigation

Mail list logo

Footer information