[PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: optimize TLB flush operation

[PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: optimize TLB flush operation

[Jian J Wang]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1281

This optimization has two purpose:

  1. fix BZ#1281 which caused by flushing TLB for AP
  2. improve performance for SMM heap guard

The code change is simple: just flush TLB for current processor.

Since processor's (including AP) SMI entry code will always initialize
CR3, it looks like that there's no need to add extra code in AP handler,
called from SMI entry, to flush TLB again.

Let each processor itself guarantee the TLB integrity can improve memory
operations performance if Heap Guard is enabled. This has been proved
by CpuDxe driver. Please check following patches for details.

  41a9c3fd110bed93c4fdf088eea18412bb2dfcde
  0dbb0f1a5ce6a9ec5213c85e5d4244cf5b061417
    Stop flush TLB for APs (DXE) upon change

  199de89677deffffff30eda7ad17793b30042cce
    Let AP (DXE) flush TLB in its wake-up procedure

Tests:
  a. Verified that issue in BZ#1281 is gone
  b. Verified SMM heap guard works well on any processor
  c. OVMF boot (Fedora26, Ubuntu18.04, Windows 10)

Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 45 +++-------------------
 1 file changed, 6 insertions(+), 39 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
index 684b14dc28..e0bf0cd5ac 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
@@ -464,41 +464,6 @@ ConvertMemoryPageAttributes (
   return RETURN_SUCCESS;
 }
 
-/**
-  FlushTlb on current processor.
-
-  @param[in,out] Buffer  Pointer to private data buffer.
-**/
-VOID
-EFIAPI
-FlushTlbOnCurrentProcessor (
-  IN OUT VOID  *Buffer
-  )
-{
-  CpuFlushTlb ();
-}
-
-/**
-  FlushTlb for all processors.
-**/
-VOID
-FlushTlbForAll (
-  VOID
-  )
-{
-  UINTN       Index;
-
-  FlushTlbOnCurrentProcessor (NULL);
-
-  for (Index = 0; Index < gSmst->NumberOfCpus; Index++) {
-    if (Index != gSmst->CurrentlyExecutingCpu) {
-      // Force to start up AP in blocking mode,
-      SmmBlockingStartupThisAp (FlushTlbOnCurrentProcessor, Index, NULL);
-      // Do not check return status, because AP might not be present in some corner cases.
-    }
-  }
-}
-
 /**
   This function sets the attributes for the memory region specified by BaseAddress and
   Length from their current attributes to the attributes specified by Attributes.
@@ -538,9 +503,10 @@ SmmSetMemoryAttributesEx (
   if (!EFI_ERROR(Status)) {
     if (IsModified) {
       //
-      // Flush TLB as last step
+      // Flush TLB as last step. No need to do it for APs, which sould take care
+      // of it in the wake-up procedure.
       //
-      FlushTlbForAll();
+      CpuFlushTlb ();
     }
   }
 
@@ -586,9 +552,10 @@ SmmClearMemoryAttributesEx (
   if (!EFI_ERROR(Status)) {
     if (IsModified) {
       //
-      // Flush TLB as last step
+      // Flush TLB as last step. No need to do it for APs, which sould take care
+      // of it in the wake-up procedure.
       //
-      FlushTlbForAll();
+      CpuFlushTlb ();
     }
   }
 
-- 
2.16.2.windows.1

Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: optimize TLB flush operation

[Ni, Ruiyu]

On 10/26/2018 8:40 PM, Jian J Wang wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1281
> 
> This optimization has two purpose:
> 
>    1. fix BZ#1281 which caused by flushing TLB for AP
>    2. improve performance for SMM heap guard
> 
> The code change is simple: just flush TLB for current processor.
> 
> Since processor's (including AP) SMI entry code will always initialize
> CR3, it looks like that there's no need to add extra code in AP handler,
> called from SMI entry, to flush TLB again.
> 
> Let each processor itself guarantee the TLB integrity can improve memory
> operations performance if Heap Guard is enabled. This has been proved
> by CpuDxe driver. Please check following patches for details.
> 
>    41a9c3fd110bed93c4fdf088eea18412bb2dfcde
>    0dbb0f1a5ce6a9ec5213c85e5d4244cf5b061417
>      Stop flush TLB for APs (DXE) upon change
> 
>    199de89677deffffff30eda7ad17793b30042cce
>      Let AP (DXE) flush TLB in its wake-up procedure
> 
> Tests:
>    a. Verified that issue in BZ#1281 is gone
>    b. Verified SMM heap guard works well on any processor
>    c. OVMF boot (Fedora26, Ubuntu18.04, Windows 10)
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
>   UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 45 +++-------------------
>   1 file changed, 6 insertions(+), 39 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> index 684b14dc28..e0bf0cd5ac 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> @@ -464,41 +464,6 @@ ConvertMemoryPageAttributes (
>     return RETURN_SUCCESS;
>   }
>   
> -/**
> -  FlushTlb on current processor.
> -
> -  @param[in,out] Buffer  Pointer to private data buffer.
> -**/
> -VOID
> -EFIAPI
> -FlushTlbOnCurrentProcessor (
> -  IN OUT VOID  *Buffer
> -  )
> -{
> -  CpuFlushTlb ();
> -}
> -
> -/**
> -  FlushTlb for all processors.
> -**/
> -VOID
> -FlushTlbForAll (
> -  VOID
> -  )
> -{
> -  UINTN       Index;
> -
> -  FlushTlbOnCurrentProcessor (NULL);
> -
> -  for (Index = 0; Index < gSmst->NumberOfCpus; Index++) {
> -    if (Index != gSmst->CurrentlyExecutingCpu) {
> -      // Force to start up AP in blocking mode,
> -      SmmBlockingStartupThisAp (FlushTlbOnCurrentProcessor, Index, NULL);
> -      // Do not check return status, because AP might not be present in some corner cases.
> -    }
> -  }
> -}
> -
>   /**
>     This function sets the attributes for the memory region specified by BaseAddress and
>     Length from their current attributes to the attributes specified by Attributes.
> @@ -538,9 +503,10 @@ SmmSetMemoryAttributesEx (
>     if (!EFI_ERROR(Status)) {
>       if (IsModified) {
>         //
> -      // Flush TLB as last step
> +      // Flush TLB as last step. No need to do it for APs, which sould take care
> +      // of it in the wake-up procedure.
>         //
> -      FlushTlbForAll();
> +      CpuFlushTlb ();
>       }
>     }
>   
> @@ -586,9 +552,10 @@ SmmClearMemoryAttributesEx (
>     if (!EFI_ERROR(Status)) {
>       if (IsModified) {
>         //
> -      // Flush TLB as last step
> +      // Flush TLB as last step. No need to do it for APs, which sould take care
> +      // of it in the wake-up procedure.
>         //
> -      FlushTlbForAll();
> +      CpuFlushTlb ();
>       }
>     }
>   
> 

Jian,
I see you are using the same optimization as that in DXE MP to improve 
performance.
But considering AP already runs to ApHandler() when BSP calls 
SmmStartupAp(), this optimization cannot be used actually.

The original code is safe. With your changes, the AP's TLB is not 
flushed and may use out-of-date page table settings.

I think for the specific Bugzilla, it's caller's fault to use the wrong 
DebugLib instance. AP procedure shouldn't call any PI/UEFI interface.
We do not need to fix it.

-- 
Thanks,
Ray

Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: optimize TLB flush operation

[Wang, Jian J]

Ray,

Thanks for the explanations. I misunderstood the work flow here. So I'll drop this
patch and close the tracker.

Regards,
Jian


> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Monday, October 29, 2018 11:06 AM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; Yao,
> Jiewen <jiewen.yao@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: optimize TLB flush
> operation
> 
> On 10/26/2018 8:40 PM, Jian J Wang wrote:
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1281
> >
> > This optimization has two purpose:
> >
> >    1. fix BZ#1281 which caused by flushing TLB for AP
> >    2. improve performance for SMM heap guard
> >
> > The code change is simple: just flush TLB for current processor.
> >
> > Since processor's (including AP) SMI entry code will always initialize
> > CR3, it looks like that there's no need to add extra code in AP handler,
> > called from SMI entry, to flush TLB again.
> >
> > Let each processor itself guarantee the TLB integrity can improve memory
> > operations performance if Heap Guard is enabled. This has been proved
> > by CpuDxe driver. Please check following patches for details.
> >
> >    41a9c3fd110bed93c4fdf088eea18412bb2dfcde
> >    0dbb0f1a5ce6a9ec5213c85e5d4244cf5b061417
> >      Stop flush TLB for APs (DXE) upon change
> >
> >    199de89677deffffff30eda7ad17793b30042cce
> >      Let AP (DXE) flush TLB in its wake-up procedure
> >
> > Tests:
> >    a. Verified that issue in BZ#1281 is gone
> >    b. Verified SMM heap guard works well on any processor
> >    c. OVMF boot (Fedora26, Ubuntu18.04, Windows 10)
> >
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Star Zeng <star.zeng@intel.com>
> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> > ---
> >   UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 45 +++----
> ---------------
> >   1 file changed, 6 insertions(+), 39 deletions(-)
> >
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> > index 684b14dc28..e0bf0cd5ac 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> > @@ -464,41 +464,6 @@ ConvertMemoryPageAttributes (
> >     return RETURN_SUCCESS;
> >   }
> >
> > -/**
> > -  FlushTlb on current processor.
> > -
> > -  @param[in,out] Buffer  Pointer to private data buffer.
> > -**/
> > -VOID
> > -EFIAPI
> > -FlushTlbOnCurrentProcessor (
> > -  IN OUT VOID  *Buffer
> > -  )
> > -{
> > -  CpuFlushTlb ();
> > -}
> > -
> > -/**
> > -  FlushTlb for all processors.
> > -**/
> > -VOID
> > -FlushTlbForAll (
> > -  VOID
> > -  )
> > -{
> > -  UINTN       Index;
> > -
> > -  FlushTlbOnCurrentProcessor (NULL);
> > -
> > -  for (Index = 0; Index < gSmst->NumberOfCpus; Index++) {
> > -    if (Index != gSmst->CurrentlyExecutingCpu) {
> > -      // Force to start up AP in blocking mode,
> > -      SmmBlockingStartupThisAp (FlushTlbOnCurrentProcessor, Index, NULL);
> > -      // Do not check return status, because AP might not be present in some
> corner cases.
> > -    }
> > -  }
> > -}
> > -
> >   /**
> >     This function sets the attributes for the memory region specified by
> BaseAddress and
> >     Length from their current attributes to the attributes specified by Attributes.
> > @@ -538,9 +503,10 @@ SmmSetMemoryAttributesEx (
> >     if (!EFI_ERROR(Status)) {
> >       if (IsModified) {
> >         //
> > -      // Flush TLB as last step
> > +      // Flush TLB as last step. No need to do it for APs, which sould take care
> > +      // of it in the wake-up procedure.
> >         //
> > -      FlushTlbForAll();
> > +      CpuFlushTlb ();
> >       }
> >     }
> >
> > @@ -586,9 +552,10 @@ SmmClearMemoryAttributesEx (
> >     if (!EFI_ERROR(Status)) {
> >       if (IsModified) {
> >         //
> > -      // Flush TLB as last step
> > +      // Flush TLB as last step. No need to do it for APs, which sould take care
> > +      // of it in the wake-up procedure.
> >         //
> > -      FlushTlbForAll();
> > +      CpuFlushTlb ();
> >       }
> >     }
> >
> >
> 
> Jian,
> I see you are using the same optimization as that in DXE MP to improve
> performance.
> But considering AP already runs to ApHandler() when BSP calls
> SmmStartupAp(), this optimization cannot be used actually.
> 
> The original code is safe. With your changes, the AP's TLB is not
> flushed and may use out-of-date page table settings.
> 
> I think for the specific Bugzilla, it's caller's fault to use the wrong
> DebugLib instance. AP procedure shouldn't call any PI/UEFI interface.
> We do not need to fix it.
> 
> --
> Thanks,
> Ray