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 <[email protected]>; [email protected] > Cc: Dong, Eric <[email protected]>; Laszlo Ersek <[email protected]>; Yao, > Jiewen <[email protected]>; Zeng, Star <[email protected]> > 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 <[email protected]> > > Cc: Laszlo Ersek <[email protected]> > > Cc: Jiewen Yao <[email protected]> > > Cc: Star Zeng <[email protected]> > > Cc: Ruiyu Ni <[email protected]> > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Jian J Wang <[email protected]> > > --- > > 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 _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
