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.w...@intel.com>; edk2-devel@lists.01.org
> Cc: Laszlo Ersek <ler...@redhat.com>; Yao, Jiewen <jiewen....@intel.com>;
> Dong, Eric <eric.d...@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...@intel.com>
> > Cc: Jiewen Yao <jiewen....@intel.com>
> > Cc: Eric Dong <eric.d...@intel.com>
> > Cc: Laszlo Ersek <ler...@redhat.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jian J Wang <jian.j.w...@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...@intel.com>
>
>
> > CpuFlushTlb();
> > - SyncMemoryPageAttributesAp (SyncCpuFlushTlb);
> > }
> > }
> >
> >
>
>
> --
> Thanks,
> Ray
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
