Re: [PATCH 2/2] UefiCpuPkg/CpuDxe: remove all code to flush TLB for APs

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

Right. I'll update the comments.

Regards,
Jian


> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Friday, January 26, 2018 5:14 PM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>;
> Dong, Eric <eric.dong@intel.com>
> Subject: Re: [edk2] [PATCH 2/2] UefiCpuPkg/CpuDxe: remove all code to flush
> TLB for APs
> 
> On 1/26/2018 5:03 PM, Jian J Wang wrote:
> > The reason doing this is that we found that calling StartupAllAps() to
> > flush TLB for all APs in CpuDxe driver after changing page attributes
> > will spend a lot of time to complete. If there are many page attributes
> > update requests, the whole system performance will be slowed down
> > explicitly, including any shell command and UI operation.
> >
> > The solution is removing the flush operation for AP in CpuDxe driver
> > and let AP flush TLB after woken up.
> >
> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> > ---
> >   UefiCpuPkg/CpuDxe/CpuPageTable.c | 85 +++-------------------------------------
> >   1 file changed, 5 insertions(+), 80 deletions(-)
> >
> > diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > index a33ac5519e..a5bf0dfe28 100644
> > --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > @@ -89,70 +89,6 @@ PAGE_ATTRIBUTE_TABLE mPageAttributeTable[] = {
> >
> >   PAGE_TABLE_POOL   *mPageTablePool = NULL;
> >
> > -/**
> > -  Enable write protection function for AP.
> > -
> > -  @param[in,out] Buffer  The pointer to private data buffer.
> > -**/
> > -VOID
> > -EFIAPI
> > -SyncCpuEnableWriteProtection (
> > -  IN OUT VOID *Buffer
> > -  )
> > -{
> > -  AsmWriteCr0 (AsmReadCr0 () | BIT16);
> > -}
> > -
> > -/**
> > -  CpuFlushTlb function for AP.
> > -
> > -  @param[in,out] Buffer  The pointer to private data buffer.
> > -**/
> > -VOID
> > -EFIAPI
> > -SyncCpuFlushTlb (
> > -  IN OUT VOID *Buffer
> > -  )
> > -{
> > -  CpuFlushTlb();
> > -}
> > -
> > -/**
> > -  Sync memory page attributes for AP.
> > -
> > -  @param[in] Procedure            A pointer to the function to be run on enabled
> APs of
> > -                                  the system.
> > -**/
> > -VOID
> > -SyncMemoryPageAttributesAp (
> > -  IN EFI_AP_PROCEDURE            Procedure
> > -  )
> > -{
> > -  EFI_STATUS                Status;
> > -  EFI_MP_SERVICES_PROTOCOL  *MpService;
> > -
> > -  Status = gBS->LocateProtocol (
> > -                  &gEfiMpServiceProtocolGuid,
> > -                  NULL,
> > -                  (VOID **)&MpService
> > -                  );
> > -  //
> > -  // Synchronize the update with all APs
> > -  //
> > -  if (!EFI_ERROR (Status)) {
> > -    Status = MpService->StartupAllAPs (
> > -                          MpService,          // This
> > -                          Procedure,          // Procedure
> > -                          FALSE,              // SingleThread
> > -                          NULL,               // WaitEvent
> > -                          0,                  // TimeoutInMicrosecsond
> > -                          NULL,               // ProcedureArgument
> > -                          NULL                // FailedCpuList
> > -                          );
> > -    ASSERT (Status == EFI_SUCCESS || Status == EFI_NOT_STARTED || Status
> == EFI_NOT_READY);
> > -  }
> > -}
> > -
> >   /**
> >     Return current paging context.
> >
> > @@ -574,20 +510,6 @@ IsReadOnlyPageWriteProtected (
> >     return ((AsmReadCr0 () & BIT16) != 0);
> >   }
> >
> > -/**
> > -  Disable write protection function for AP.
> > -
> > -  @param[in,out] Buffer  The pointer to private data buffer.
> > -**/
> > -VOID
> > -EFIAPI
> > -SyncCpuDisableWriteProtection (
> > -  IN OUT VOID *Buffer
> > -  )
> > -{
> > -  AsmWriteCr0 (AsmReadCr0() & ~BIT16);
> > -}
> > -
> >   /**
> >    Disable Write Protect on pages marked as read-only.
> >   **/
> > @@ -835,10 +757,13 @@ AssignMemoryPageAttributes (
> >     if (!EFI_ERROR(Status)) {
> >       if ((PagingContext == NULL) && IsModified) {
> >         //
> > -      // Flush TLB as last step
> > +      // Flush TLB as last step.
> > +      //
> > +      // Note: Don't flush TLB for APs here. It will take a lot of time to
> > +      // complete, and then slow down boot performance of the whole system
> > +      // if page attributes are requested frequently to update.
> >         //
> 
> Code change looks good. But comments look like we skip the sync due to
> performance. In fact, sync is unnecessary.
> How about comments like below (refine as you need):
> No need to flush TLB for APs here because:
> 1. when APs wake up from hlt, AP initialization code always sets CR3
> 2. when APs wake up from mwait/run loop, patch
>     *UefiCpuPkg/MpInitLib: force flushing TLB for AP in mwait loop mode*
>     sets CR3.
> 
> With the comments refine, Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
> 
> 
> >         CpuFlushTlb();
> > -      SyncMemoryPageAttributesAp (SyncCpuFlushTlb);
> >       }
> >     }
> >
> >
> 
> 
> --
> Thanks,
> Ray