I agree with your proposal. Great suggestion. I will create v2 later. -----Original Message----- From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo Ersek Sent: Thursday, November 10, 2016 4:51 PM To: Fan, Jeff; edk2-de...@ml01.01.org Cc: Kinney, Michael D; Paolo Bonzini; Yao, Jiewen Subject: Re: [edk2] [PATCH 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Put AP into safe hlt-loop code on S3 path
Before I test this, I have one question / suggestion: On 11/10/16 07:07, Jeff Fan wrote: > On S3 path, we will wake up APs to restore CPU context in > PiSmmCpuDxeSmm driver. However, we place AP in hlt-loop under 1MB > space borrowed after CPU restoring CPU contexts. > In case, one NMI or SMI happens, APs may exit from hlt state and > execute the instruction after HLT instruction. But the code under 1MB > is no longer safe at that time. > > This fix is to allocate one ACPI NVS range to place the AP hlt-loop > code. When CPU finished restoration CPU contexts, AP will execute in this > ACPI NVS range. > > https://bugzilla.tianocore.org/show_bug.cgi?id=216 > > Reported-by: Laszlo Ersek <ler...@redhat.com> > Cc: Laszlo Ersek <ler...@redhat.com> > Cc: Paolo Bonzini <pbonz...@redhat.com> > Cc: Jiewen Yao <jiewen....@intel.com> > Cc: Michael D Kinney <michael.d.kin...@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Jeff Fan <jeff....@intel.com> > --- > UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 31 > +++++++++++++++++++++++++++ > UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c | 25 +++++++++++++++++++++ > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 13 +++++++++++ > UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c | 26 > ++++++++++++++++++++++ > 4 files changed, 95 insertions(+) > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c > b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c > index 6a798ef..6f290b8 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c > @@ -77,6 +77,13 @@ SMM_S3_RESUME_STATE *mSmmS3ResumeState = NULL; > > BOOLEAN mAcpiS3Enable = TRUE; > > +UINT8 *mApHltLoopCode = NULL; > +UINT8 mApHltLoopCodeTemplate[] = { > + 0xFA, // cli > + 0xF4, // hlt > + 0xEB, 0xFC // jmp $-2 > + }; > + > /** > Get MSR spin lock by MSR index. > > @@ -376,6 +383,8 @@ MPRendezvousProcedure ( > CPU_REGISTER_TABLE *RegisterTableList; > UINT32 InitApicId; > UINTN Index; > + UINT32 TopOfStack; > + UINT8 Stack[128]; > > ProgramVirtualWireMode (); > DisableLvtInterrupts (); > @@ -393,6 +402,13 @@ MPRendezvousProcedure ( > // Count down the number with lock mechanism. > // > InterlockedDecrement (&mNumberToFinish); > + > + // > + // Place AP into the safe code > + // > + TopOfStack = (UINT32) (UINTN) Stack + sizeof (Stack); Here I suggest to append the following assignment: TopOfStack &= ~(UINT32) (CPU_STACK_ALIGNMENT - 1); This will clear the least significant bits of TopOfStack, wasting at most (CPU_STACK_ALIGNMENT - 1) bytes from the Stack array. However, the stack will be aligned correctly. Not sure if it's necessary, but the patch reminds me of commit f98f5ec304ec ("S3Resume2Pei: align return stacks explicitly"). What do you think? Thanks! Laszlo > + CopyMem ((VOID *) (UINTN) mApHltLoopCode, mApHltLoopCodeTemplate, > + sizeof (mApHltLoopCodeTemplate)); TransferApToSafeState ((UINT32) > + (UINTN) mApHltLoopCode, TopOfStack); > } > > /** > @@ -731,6 +747,8 @@ InitSmmS3ResumeState ( > VOID *GuidHob; > EFI_SMRAM_DESCRIPTOR *SmramDescriptor; > SMM_S3_RESUME_STATE *SmmS3ResumeState; > + EFI_PHYSICAL_ADDRESS Address; > + EFI_STATUS Status; > > if (!mAcpiS3Enable) { > return; > @@ -773,6 +791,19 @@ InitSmmS3ResumeState ( > // Patch SmmS3ResumeState->SmmS3Cr3 > // > InitSmmS3Cr3 (); > + > + // > + // Allocate safe memory in ACPI NVS for AP to execute hlt loop on > + S3 path // Address = BASE_4GB - 1; Status = gBS->AllocatePages ( > + AllocateMaxAddress, > + EfiACPIMemoryNVS, > + EFI_SIZE_TO_PAGES (sizeof (mApHltLoopCodeTemplate)), > + &Address > + ); > + ASSERT_EFI_ERROR (Status); > + mApHltLoopCode = (UINT8 *) (UINTN) Address; > } > > /** > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c > b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c > index 545b534..3e99aab 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c > @@ -94,3 +94,28 @@ InitGdt ( > *GdtStepSize = GdtTableStepSize; > return GdtTssTables; > } > + > +/** > + Transfer AP to safe hlt-loop after it finished restore CPU features on S3 > patch. > + > + @param[in] ApHltLoopCode The 32-bit address of the safe hlt-loop > function > + @param[in] TopOfStack A pointer to the new stack to use for the > ApHltLoopCode. > + > +**/ > +VOID > +TransferApToSafeState ( > + IN UINT32 ApHltLoopCode, > + IN UINT32 TopOfStack > + ) > +{ > + SwitchStack ( > + (SWITCH_STACK_ENTRY_POINT) (UINTN) ApHltLoopCode, > + NULL, > + NULL, > + (VOID *) (UINTN) TopOfStack > + ); > + // > + // It should never reach here > + // > + ASSERT (FALSE); > +} > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > index 9b119c8..82fa5a6 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > @@ -825,4 +825,17 @@ GetAcpiS3EnableFlag ( > VOID > ); > > +/** > + Transfer AP to safe hlt-loop after it finished restore CPU features on S3 > patch. > + > + @param[in] ApHltLoopCode The 32-bit address of the safe hlt-loop > function > + @param[in] TopOfStack A pointer to the new stack to use for the > ApHltLoopCode. > + > +**/ > +VOID > +TransferApToSafeState ( > + IN UINT32 ApHltLoopCode, > + IN UINT32 TopOfStack > + ); > + > #endif > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c > b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c > index b53aa45..c05dec7 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c > @@ -68,3 +68,29 @@ InitGdt ( > *GdtStepSize = GdtTableStepSize; > return GdtTssTables; > } > +/** > + Transfer AP to safe hlt-loop after it finished restore CPU features on S3 > patch. > + > + @param[in] ApHltLoopCode The 32-bit address of the safe hlt-loop > function > + @param[in] TopOfStack A pointer to the new stack to use for the > ApHltLoopCode. > + > +**/ > +VOID > +TransferApToSafeState ( > + IN UINT32 ApHltLoopCode, > + IN UINT32 TopOfStack > + ) > +{ > + SwitchStack ( > + (SWITCH_STACK_ENTRY_POINT) (UINTN) ApHltLoopCode, > + NULL, > + NULL, > + (VOID *) (UINTN) TopOfStack > + ); > + // > + // It should never reach here > + // > + ASSERT (FALSE); > +} > + > + > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
