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

["Wang, Jian J" <jian.j.wang@intel.com>]

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