Re: [edk2] [PATCH 6/7] UefiCpiPkg: Add PiSmmCpuDxeSmm module
Hi Laszlo, A couple responses below. Mike >-Original Message- >From: Laszlo Ersek [mailto:ler...@redhat.com] >Sent: Thursday, October 08, 2015 11:37 AM >To: Kinney, Michael D >Cc: edk2-de...@ml01.01.org >Subject: Re: [edk2] [PATCH 6/7] UefiCpiPkg: Add PiSmmCpuDxeSmm module > >Hi Mike, > >On 10/08/15 02:54, Kinney, Michael D wrote: >> Hi Laszlo, >> >> Thanks for the feedback! >> >> Please let me know if I missed any of your questions. > >I'm very happy with your answers, thank you for them. > >I'll comment on S3 below: > >> Mike >> >>> -Original Message- >>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of >>> Laszlo Ersek >>> Sent: Wednesday, October 07, 2015 10:32 AM >>> To: Kinney, Michael D >>> Cc: edk2-de...@ml01.01.org >>> Subject: Re: [edk2] [PATCH 6/7] UefiCpiPkg: Add PiSmmCpuDxeSmm >module >>> >>> On 10/05/15 01:57, Michael Kinney wrote: >>>> Add module that initializes a CPU for the SMM envirnment and >>>> installs the first level SMI handler. This module along with the >>>> SMM IPL and SMM Core provide the services required for >>>> DXE_SMM_DRIVERS to register hardware and software SMI handlers. >>>> >>>> CPU specific features are abstracted through the SmmCpuFeaturesLib >>>> >>>> Platform specific features are abstracted through the >>>> SmmCpuPlatformHookLib >>>> >>>> Several PCDs are added to enable/disable features and configure >>>> settings for the PiSmmCpuDxeSmm module >>>> >>>> Contributed-under: TianoCore Contribution Agreement 1.0 >>>> Signed-off-by: Michael Kinney >>> >>> (I'm snipping the patch because it is extremely long.) >>> >>> I'm going through my "QuarkPort" patches, evaluating for each one if I >>> should keep it, drop it, or adapt it. >>> >>> * [PATCH 27/58] OvmfPkg: import PCDs from >>> Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg >>> http://thread.gmane.org/gmane.comp.bios.edk2.devel/329/focus=350 >>> >>> Going through all the PCDs I had to import there, and checking if each >>> was present in this patch too, I found the following difference: >>> >>> PcdCpuSmmUncacheCpuSyncData is absent from this patch. In my >>> "QuarkPort", the PCD is used by the InitializeMpServiceData() function >>> only, gating a call to SetCacheability(). And that call to >>> SetCacheability() is the *only* such call. >>> >>> Now, this patch provides SetCacheability(), but no calls are made to >>> it, ever (in accordance with the fact that the PCD that would control >>> the call is also absent). >>> >>> (1) Therefore I recommend to delete the SetCacheability() function >>> from this patch. >> >> I agree. I will remove that function. I missed that function when I removed >> PcdCpuSmmUncacheCpuSyncData. >> >>> >>> (2) I have a question wrt. PcdCpuSmmEnableBspElection. In the Quark >>> package, the "IA32FamilyCpuBasePkg.dsc" file overrides the default >>> TRUE value (from the .dec) for this PCD, with FALSE. >>> >>> The .dec default is similarly TRUE here. In my "QuarkPort" for >>> OVMF, I flipped the PCD to FALSE as well. Does that make sense? >>> Should I stick with that override, when rebasing the OVMF SMM >>> series on top of this series? I don't know why this PCD matters. >>> What are the benefits vs. drawbacks of dynamic BSP-for-SMI >>> election? >> >> For OVMF you will likely not see any difference in behavior. >> This PCD allows a platform to provide PlatformSmmBspElection() >> in a platform specific SmmCpuPlatformHookLib instance to >> decide which CPU gets elected to be the BSP in each SMI. >> >> The SmmCpuPlatformHookLibNull always returns >> EFI_NOT_READY for that function, which makes the >> module behave the same as the PCD being set to FALSE. >> >> The default is TRUE, so the platform lib is always called, >> so a platform developer can implement the hook >> function and does not have to also change a PCD >> setting for the hook function to be active. >> >> A platform that wants to eliminate the call to the >> hook function all together can set the PCD to FALSE. >> >> So for OVMF, I think it makes sense to set this PCD to >> FAL
Re: [edk2] [PATCH 6/7] UefiCpiPkg: Add PiSmmCpuDxeSmm module
On 10/08/15 23:22, Kinney, Michael D wrote: > Hi Laszlo, > > A couple responses below. Thanks much (again!); I think I can rework CpuS3DataDxe based on your advice: > > Mike > >> -Original Message- >> From: Laszlo Ersek [mailto:ler...@redhat.com] >> Sent: Thursday, October 08, 2015 11:37 AM >> To: Kinney, Michael D >> Cc: edk2-de...@ml01.01.org >> Subject: Re: [edk2] [PATCH 6/7] UefiCpiPkg: Add PiSmmCpuDxeSmm module [snip] >> Let my try to go through the fields of ACPI_CPU_DATA again, and see how >> my CpuS3DataDxe could interace with the PiSmmCpuDxeSmm driver in this >> patch: >> >> - APState was one that I removed from both drivers. We've discussed >> this above -- even keeping it wouldn't cause CpuS3DataDxe problems. >> >> - StartupVector is a strange field. In Quark, CpuMpDxe allocated it and >> populated it, and PiSmmCpuDxeSmm overwrote its contents during S3 >> resume (which was fine), but not with the original contents saved to, >> and restored from, SMRAM, but with its *own* code. >> >> IOW, PiSmmCpuDxeSmm only relied, at S3 resume, on the reserved >> buffer's *allocation*, which had been made by CpuMpDxe previously >> (= during boot). For that reason in CpuS3DataDxe, I didn't bother to >> fill in the buffer at all, just to allocate it. > > Correct. The requirement is to allocate a 4KB aligned buffer that is > 4KB is size in usable memory below 1MB. > >> >> *Plus* I added an ASSERT() to PiSmmCpuDxeSmm (function >> PrepareApStartupVector()) about the size of the pre-allocated buffer >> (constant 4KB) being big enough for the actual communications area, >> and the executable code, that PiSmmCpuDxeSmm would copy into it. >> >> As far as I can see, in the PrepareApStartupVector() function in your >> patch 6/7, this silent size assumption (i.e., without an explicit >> assert) still exist. Am I correct? > > Yes. I agree that is a bad assumption. We either need to state that > 4KB is always enough, or we need a way for the module that allocates > the buffer to know the size of the buffer required. > >> >> This is not necessarily a bug, but it should be spelled out as one of >> the requirements between CpuS3DataDxe and PiSmmCpuDxeSmm, so that >> the >> former can accommodate the latter -- and the latter should preferably >> *enforce* that somehow. >> > > Agreed For now I'll just stick with the current allocation in CpuS3DataDxe. > >> - The (a) GdtrProfile, IdtrProfile, ApMachineCheckHandlerBase, >> ApMachineCheckHandlerSize; (b) StackAddress and StackSize; and >> (c) NumberOfCpus fields were filled in in three more patches. I >> assume the CpuS3DataDxe code would work with *this* PiSmmCpuDxeSmm >> too. >> >> Now, the trouble starts with the following fields: >> >> - The MTRR settings (in the MtrrTable field) are saved in a delayed >> manner, that is, not when CpuS3DataDxe collects the other data. This >> is why I had to write patches for UefiCpuPkg/CpuDxe too, so that it >> would reflect the most recent MTRR settings to a spot where >> PiSmmCpuDxeSmm could find them at SMM-ready-to-lock. Again I assume >> the (minimal) code currently in CpuS3DataDxe would work with *this* >> PiSmmCpuDxeSmm, however the functionality ultimately depends on >> UefiCpuPkg/CpuDxe changes too. > > You could add an event to CpuS3DataDxe that save MTRR settings > Into the structure at end of DXE. On simple platforms, there may > Not be any different in MTRR settings between the time the CPU driver > runs and the end of DXE. In that case, the MTRR settings can be > captured when the CPU driver runs and fills in the structure. Saving MTRR settings and End-of-DXE is a great idea! OVMF's BDS triggers End-of-DXE not much before it kicks DxeSmmReadyToLock too (which in turn gets reflected as SmmReadyToLock to SMM drivers, and then PiSmmCpuDxeSmm fetches and stashes ACPI_CPU_DATA into SMRAM), and I think it's reasonable *not* to expect MTRR changes between those two actions, within BDS. With regard to my other UefiCpuPkg/CpuDxe patches, I think I'll keep some of them, independently: - "broadcast MTRR changes to APs" - "sync MTRR settings to APs at MP startup" - "provide EFI_MP_SERVICES_PROTOCOL when there's no AP" because they seem useful. (I'll have to adapt them to your patches of course.) >> - PreSmmInitRegisterTable. The handling of this field in Quark's >> PiSmmCpuDxeSmm had a security bug (the registers weren't stashed in >> SMRAM between boot and S3 re
Re: [edk2] [PATCH 6/7] UefiCpiPkg: Add PiSmmCpuDxeSmm module
Mike, On 10/05/15 01:57, Michael Kinney wrote: > Add module that initializes a CPU for the SMM envirnment and > installs the first level SMI handler. This module along with the > SMM IPL and SMM Core provide the services required for > DXE_SMM_DRIVERS to register hardware and software SMI handlers. > > CPU specific features are abstracted through the SmmCpuFeaturesLib > > Platform specific features are abstracted through the > SmmCpuPlatformHookLib > > Several PCDs are added to enable/disable features and configure > settings for the PiSmmCpuDxeSmm module > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Michael Kinney > --- I found that this version of PiSmmCpuDxeSmm depends on CpuExceptionHandlerLib (Quark's doesn't). The following interfaces from the library class are called: - InitializeCpuExceptionHandlers() - RegisterCpuInterruptHandler() I think this makes sense. The call sites are: PiCpuSmmEntry InitializeSmmIdt InitializeCpuExceptionHandlers SmmRestoreCpu InitializeCpuExceptionHandlers SmmRegisterExceptionHandler == mSmmCpuService.RegisterExceptionHandler RegisterCpuInterruptHandler What I don't understand though is why the CpuExceptionHandlerLib class is resolved to a NULL instance, in "UefiCpuPkg/UefiCpuPkg.dsc": CpuExceptionHandlerLib|MdeModulePkg/Library/CpuExceptionHandlerLibNull/CpuExceptionHandlerLibNull.inf OVMF uses the following non-null instances (for the appropriate module types): - UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf - UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf and as far as I remember, these exception handler library instances print, by default, those fault information dumps (to the serial port) that are very helpful for debugging incorrect pointer references and the like. So why doesn't this module use the corresponding SMM driver instance: - UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf and why does it use a NULL instance instead? I realize that this module has its own fault info dumping facility (called SmiPFHandler(), specific to Ia32 vs. X64), similarly to the version in Quark. That handler calls DumpModuleInfoByIp(), which should do what I like to have -- helpful location info. However, SmiPFHandler() is registered with SmmRegisterExceptionHandler() -- since PcdCpuSmmProfileEnable defaults to FALSE --, and that call ultimately ends up in CpuExceptionHandlerLib. See the third call tree excerpt at the top -- in total, we have: SmmInitPageTable() SmmRegisterExceptionHandler(SmiPFHandler) RegisterCpuInterruptHandler(SmiPFHandler) // implemented by CpuExceptionHandlerLib Now, given that the CpuExceptionHandlerLib instance is the Null one here (according to UefiCpuPkg/UefiCpuPkg.dsc), this PF handler will actually *not* be installed. Is that your intent? I don't think so. Your patch doesn't seem to affect the CpuExceptionHandlerLib resolutions in any way; therefore I believe this is an omission in the patch. And, I think it doesn't only affect PiSmmCpuDxeSmm, but also SecCore (which you added in patch 4/7 of this series). On the other hand... I can see a preexistent client of CpuExceptionHandlerLib in UefiCpuPkg: CpuDxe. (Of type DXE_DRIVER.) Since the default Null resolution affects that module too, I'm now inclined to think that maybe this is all intentional *within* UefiCpuPkg. Either way, I'll go ahead and resolve CpuExceptionHandlerLib to SmmCpuExceptionHandlerLib, for DXE_SMM_DRIVER modules, in OVMF. That should be consistent with the current resolutions we have for other (SEC / PEIM / DXE_DRIVER / ...) modules already. Thanks! Laszlo ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 6/7] UefiCpiPkg: Add PiSmmCpuDxeSmm module
Laszlo, Thanks for the feedback. I agree I need to update UefiCpuPkg DSC file. Mike >-Original Message- >From: Laszlo Ersek [mailto:ler...@redhat.com] >Sent: Friday, October 09, 2015 6:57 AM >To: Kinney, Michael D >Cc: edk2-de...@ml01.01.org >Subject: Re: [edk2] [PATCH 6/7] UefiCpiPkg: Add PiSmmCpuDxeSmm module > >Mike, > >On 10/05/15 01:57, Michael Kinney wrote: >> Add module that initializes a CPU for the SMM envirnment and >> installs the first level SMI handler. This module along with the >> SMM IPL and SMM Core provide the services required for >> DXE_SMM_DRIVERS to register hardware and software SMI handlers. >> >> CPU specific features are abstracted through the SmmCpuFeaturesLib >> >> Platform specific features are abstracted through the >> SmmCpuPlatformHookLib >> >> Several PCDs are added to enable/disable features and configure >> settings for the PiSmmCpuDxeSmm module >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Michael Kinney >> --- > >I found that this version of PiSmmCpuDxeSmm depends on >CpuExceptionHandlerLib (Quark's doesn't). > >The following interfaces from the library class are called: >- InitializeCpuExceptionHandlers() >- RegisterCpuInterruptHandler() > >I think this makes sense. The call sites are: > > PiCpuSmmEntry >InitializeSmmIdt > InitializeCpuExceptionHandlers > > SmmRestoreCpu >InitializeCpuExceptionHandlers > > SmmRegisterExceptionHandler == >mSmmCpuService.RegisterExceptionHandler >RegisterCpuInterruptHandler > >What I don't understand though is why the CpuExceptionHandlerLib class >is resolved to a NULL instance, in "UefiCpuPkg/UefiCpuPkg.dsc": > > >CpuExceptionHandlerLib|MdeModulePkg/Library/CpuExceptionHandlerLibNul >l/CpuExceptionHandlerLibNull.inf > >OVMF uses the following non-null instances (for the appropriate module >types): > >- >UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.i >nf >- >UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf > >and as far as I remember, these exception handler library instances >print, by default, those fault information dumps (to the serial port) >that are very helpful for debugging incorrect pointer references and the >like. > >So why doesn't this module use the corresponding SMM driver instance: > >- >UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.in >f > >and why does it use a NULL instance instead? Good point. The DSC file in the UefiCpuPkg is there to test the build of the Modules/libraries, so the specific lib instance from another package is not critical. I agree it makes sense to put in the preferred lib mappings is possible So I will update the DSC file for UefiCpuPkg to use the correct CpuExceptionHandlerLib for each module type. > >I realize that this module has its own fault info dumping facility >(called SmiPFHandler(), specific to Ia32 vs. X64), similarly to the >version in Quark. That handler calls DumpModuleInfoByIp(), which should >do what I like to have -- helpful location info. > >However, SmiPFHandler() is registered with SmmRegisterExceptionHandler() >-- since PcdCpuSmmProfileEnable defaults to FALSE --, and that call >ultimately ends up in CpuExceptionHandlerLib. See the third call tree >excerpt at the top -- in total, we have: > >SmmInitPageTable() > SmmRegisterExceptionHandler(SmiPFHandler) >RegisterCpuInterruptHandler(SmiPFHandler) > // implemented by CpuExceptionHandlerLib > >Now, given that the CpuExceptionHandlerLib instance is the Null one here >(according to UefiCpuPkg/UefiCpuPkg.dsc), this PF handler will actually >*not* be installed. Is that your intent? > >I don't think so. Your patch doesn't seem to affect the >CpuExceptionHandlerLib resolutions in any way; therefore I believe this >is an omission in the patch. > >And, I think it doesn't only affect PiSmmCpuDxeSmm, but also SecCore >(which you added in patch 4/7 of this series). > >On the other hand... I can see a preexistent client of >CpuExceptionHandlerLib in UefiCpuPkg: CpuDxe. (Of type DXE_DRIVER.) >Since the default Null resolution affects that module too, I'm now >inclined to think that maybe this is all intentional *within* UefiCpuPkg. > >Either way, I'll go ahead and resolve CpuExceptionHandlerLib to >SmmCpuExceptionHandlerLib, for DXE_SMM_DRIVER modules, in OVMF. That >should be consistent with the current resolutions we have for other (SEC >/ PEIM / DXE_DRIVER / ...) modules already. Yes. A platform specific DSC must use the right lib mapping based on module type. > >Thanks! >Laszlo ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 6/7] UefiCpiPkg: Add PiSmmCpuDxeSmm module
On 05/10/2015 01:57, Michael Kinney wrote: > Add module that initializes a CPU for the SMM envirnment and > installs the first level SMI handler. This module along with the > SMM IPL and SMM Core provide the services required for > DXE_SMM_DRIVERS to register hardware and software SMI handlers. > > CPU specific features are abstracted through the SmmCpuFeaturesLib > > Platform specific features are abstracted through the > SmmCpuPlatformHookLib > > Several PCDs are added to enable/disable features and configure > settings for the PiSmmCpuDxeSmm module > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Michael Kinney Hi Michael, I'm happy to report the first bug! :) InitPaging() is setting a page directory entry before initializing the corresponding page table. This works on real hardware (including KVM), but the TLB of QEMU's emulation mode is different (possibly it has different associativity, I don't really know) so at some point execution goes to nowhere's land. The fix is really simple: diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c index 9463e97..6ee9256 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c @@ -555,12 +555,12 @@ InitPaging ( Pt = AllocatePages (1); ASSERT (Pt != NULL); - *Pte = (UINTN)Pt | IA32_PG_RW | IA32_PG_P; - // Split it - for (Level4 = 0; Level4 < SIZE_4KB / sizeof(*Pt); Level4++, Pt++) { -*Pt = Address + ((Level4 << 12) | IA32_PG_RW | IA32_PG_P); + for (Level4 = 0; Level4 < SIZE_4KB / sizeof(*Pt); Level4++) { +Pt[Level4] = Address + ((Level4 << 12) | IA32_PG_RW | IA32_PG_P); } // end for PT + + *Pte = (UINTN)Pt | IA32_PG_RW | IA32_PG_P; } // end if IsAddressSplit } // end for PTE } // end for PDE Thanks, Paolo ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 6/7] UefiCpiPkg: Add PiSmmCpuDxeSmm module
On 2015-10-12 09:23:28, Paolo Bonzini wrote: > On 05/10/2015 01:57, Michael Kinney wrote: > > Add module that initializes a CPU for the SMM envirnment and > > installs the first level SMI handler. This module along with the > > SMM IPL and SMM Core provide the services required for > > DXE_SMM_DRIVERS to register hardware and software SMI handlers. > > > > CPU specific features are abstracted through the SmmCpuFeaturesLib > > > > Platform specific features are abstracted through the > > SmmCpuPlatformHookLib > > > > Several PCDs are added to enable/disable features and configure > > settings for the PiSmmCpuDxeSmm module > > > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off-by: Michael Kinney > > Hi Michael, > > I'm happy to report the first bug! :) > > InitPaging() is setting a page directory entry before initializing the > corresponding page table. This works on real hardware (including KVM), > but the TLB of QEMU's emulation mode is different (possibly it has > different associativity, I don't really know) so at some point > execution goes to nowhere's land. > > The fix is really simple: Can you submit your patch following? https://raw.githubusercontent.com/tianocore/edk2/master/MdePkg/Contributions.txt The key missing part being Contributed-under and Signed-off-by... So, maybe just reply adding something like this, so Mike can easily fold it into his patch's commit message: [your-email: What you changed] Contributed-under: Signed-off-by: -Jordan > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c > b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c > index 9463e97..6ee9256 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c > @@ -555,12 +555,12 @@ InitPaging ( >Pt = AllocatePages (1); >ASSERT (Pt != NULL); > > - *Pte = (UINTN)Pt | IA32_PG_RW | IA32_PG_P; > - >// Split it > - for (Level4 = 0; Level4 < SIZE_4KB / sizeof(*Pt); Level4++, > Pt++) { > -*Pt = Address + ((Level4 << 12) | IA32_PG_RW | IA32_PG_P); > + for (Level4 = 0; Level4 < SIZE_4KB / sizeof(*Pt); Level4++) { > +Pt[Level4] = Address + ((Level4 << 12) | IA32_PG_RW | > IA32_PG_P); >} // end for PT > + > + *Pte = (UINTN)Pt | IA32_PG_RW | IA32_PG_P; > } // end if IsAddressSplit >} // end for PTE > } // end for PDE > > Thanks, > > Paolo > ___ > 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
Re: [edk2] [PATCH 6/7] UefiCpiPkg: Add PiSmmCpuDxeSmm module
On 12/10/2015 18:23, Paolo Bonzini wrote: > > > On 05/10/2015 01:57, Michael Kinney wrote: >> Add module that initializes a CPU for the SMM envirnment and >> installs the first level SMI handler. This module along with the >> SMM IPL and SMM Core provide the services required for >> DXE_SMM_DRIVERS to register hardware and software SMI handlers. >> >> CPU specific features are abstracted through the SmmCpuFeaturesLib >> >> Platform specific features are abstracted through the >> SmmCpuPlatformHookLib >> >> Several PCDs are added to enable/disable features and configure >> settings for the PiSmmCpuDxeSmm module >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Michael Kinney > > Hi Michael, > > I'm happy to report the first bug! :) > > InitPaging() is setting a page directory entry before initializing the > corresponding page table. This works on real hardware (including KVM), > but the TLB of QEMU's emulation mode is different (possibly it has > different associativity, I don't really know) so at some point > execution goes to nowhere's land. > > The fix is really simple: As suggested by Jordan, here's the patch again but with all the standard signoffs. [pbonz...@redhat.com: InitPaging: prepare PT before filling in PDE] Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Paolo Bonzini diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c index 9463e97..6ee9256 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c @@ -555,12 +555,12 @@ InitPaging ( Pt = AllocatePages (1); ASSERT (Pt != NULL); - *Pte = (UINTN)Pt | IA32_PG_RW | IA32_PG_P; - // Split it - for (Level4 = 0; Level4 < SIZE_4KB / sizeof(*Pt); Level4++, Pt++) { -*Pt = Address + ((Level4 << 12) | IA32_PG_RW | IA32_PG_P); + for (Level4 = 0; Level4 < SIZE_4KB / sizeof(*Pt); Level4++) { +Pt[Level4] = Address + ((Level4 << 12) | IA32_PG_RW | IA32_PG_P); } // end for PT + + *Pte = (UINTN)Pt | IA32_PG_RW | IA32_PG_P; } // end if IsAddressSplit } // end for PTE } // end for PDE ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 6/7] UefiCpiPkg: Add PiSmmCpuDxeSmm module
On 10/13/15 12:32, Paolo Bonzini wrote: > > > On 12/10/2015 18:23, Paolo Bonzini wrote: >> >> >> On 05/10/2015 01:57, Michael Kinney wrote: >>> Add module that initializes a CPU for the SMM envirnment and >>> installs the first level SMI handler. This module along with the >>> SMM IPL and SMM Core provide the services required for >>> DXE_SMM_DRIVERS to register hardware and software SMI handlers. >>> >>> CPU specific features are abstracted through the SmmCpuFeaturesLib >>> >>> Platform specific features are abstracted through the >>> SmmCpuPlatformHookLib >>> >>> Several PCDs are added to enable/disable features and configure >>> settings for the PiSmmCpuDxeSmm module >>> >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Signed-off-by: Michael Kinney >> >> Hi Michael, >> >> I'm happy to report the first bug! :) >> >> InitPaging() is setting a page directory entry before initializing the >> corresponding page table. This works on real hardware (including KVM), >> but the TLB of QEMU's emulation mode is different (possibly it has >> different associativity, I don't really know) so at some point >> execution goes to nowhere's land. >> >> The fix is really simple: > > As suggested by Jordan, here's the patch again but with all the > standard signoffs. > > [pbonz...@redhat.com: InitPaging: prepare PT before filling in PDE] > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Paolo Bonzini > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c > b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c > index 9463e97..6ee9256 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c > @@ -555,12 +555,12 @@ InitPaging ( >Pt = AllocatePages (1); >ASSERT (Pt != NULL); > > - *Pte = (UINTN)Pt | IA32_PG_RW | IA32_PG_P; > - >// Split it > - for (Level4 = 0; Level4 < SIZE_4KB / sizeof(*Pt); Level4++, Pt++) { > -*Pt = Address + ((Level4 << 12) | IA32_PG_RW | IA32_PG_P); > + for (Level4 = 0; Level4 < SIZE_4KB / sizeof(*Pt); Level4++) { > +Pt[Level4] = Address + ((Level4 << 12) | IA32_PG_RW | IA32_PG_P); >} // end for PT > + > + *Pte = (UINTN)Pt | IA32_PG_RW | IA32_PG_P; > } // end if IsAddressSplit >} // end for PTE > } // end for PDE > ___ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel > For now I'm picking this up as well, as a separate patch, between Mike's original series and my upcoming v3. Thanks Laszlo ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 6/7] UefiCpiPkg: Add PiSmmCpuDxeSmm module
On 10/05/15 01:57, Michael Kinney wrote: > Add module that initializes a CPU for the SMM envirnment and > installs the first level SMI handler. This module along with the > SMM IPL and SMM Core provide the services required for > DXE_SMM_DRIVERS to register hardware and software SMI handlers. > > CPU specific features are abstracted through the SmmCpuFeaturesLib > > Platform specific features are abstracted through the > SmmCpuPlatformHookLib > > Several PCDs are added to enable/disable features and configure > settings for the PiSmmCpuDxeSmm module > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Michael Kinney (I'm snipping the patch because it is extremely long.) I'm going through my "QuarkPort" patches, evaluating for each one if I should keep it, drop it, or adapt it. * [PATCH 27/58] OvmfPkg: import PCDs from Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg http://thread.gmane.org/gmane.comp.bios.edk2.devel/329/focus=350 Going through all the PCDs I had to import there, and checking if each was present in this patch too, I found the following difference: PcdCpuSmmUncacheCpuSyncData is absent from this patch. In my "QuarkPort", the PCD is used by the InitializeMpServiceData() function only, gating a call to SetCacheability(). And that call to SetCacheability() is the *only* such call. Now, this patch provides SetCacheability(), but no calls are made to it, ever (in accordance with the fact that the PCD that would control the call is also absent). (1) Therefore I recommend to delete the SetCacheability() function from this patch. (2) I have a question wrt. PcdCpuSmmEnableBspElection. In the Quark package, the "IA32FamilyCpuBasePkg.dsc" file overrides the default TRUE value (from the .dec) for this PCD, with FALSE. The .dec default is similarly TRUE here. In my "QuarkPort" for OVMF, I flipped the PCD to FALSE as well. Does that make sense? Should I stick with that override, when rebasing the OVMF SMM series on top of this series? I don't know why this PCD matters. What are the benefits vs. drawbacks of dynamic BSP-for-SMI election? * [PATCH 28/58] OvmfPkg: import three protocols from Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg http://thread.gmane.org/gmane.comp.bios.edk2.devel/329/focus=351 (3) This patch doesn't install gSmmCpuSyncProtocolGuid or gSmmCpuSync2ProtocolGuid in PiCpuSmmEntry(), whereas Quark does. I don't know what those protocols good for, but can you please summarize why this patch doesn't need them? * [PATCH 32/58] OvmfPkg: QuarkPort: drop ACPI_CPU_DATA.APState http://thread.gmane.org/gmane.comp.bios.edk2.devel/329/focus=362 (4) Patch 5/7 in this series introduces the ACPI_CPU_DATA structure. That structure has a field APState. I think it should be dropped, it is never used in 6/7. * [PATCH 33/58] OvmfPkg: add skeleton QuarkPort/CpuS3DataDxe http://thread.gmane.org/gmane.comp.bios.edk2.devel/329/focus=357 This patch in my SMM series (and a few later ones that relate to CpuS3DataDxe) make me ask my most important question here. The PiSmmCpuDxeSmm module in this series -- same as in Quark -- only *consumes* PcdCpuS3DataAddress, to save the ACPI_CPU_DATA structure, collected by "some driver", into SMRAM. In Quark, PiSmmCpuDxeSmm insisted on this, therefore I had to extract the "collection" portions of CpuMpDxe, and I named it CpuS3DataDxe. However, in this patch, PiSmmCpuDxeSmm seems to be able to handle a zero PcdCpuS3DataAddress: - in SmmReadyToLockEventNotify(), on normal boot, no collected data is saved into SMRAM, - in SmmRestoreCpu(), at S3 resume, the restoration of said data is also skipped. My questions are: (5) Why is this safe? For OVMF's "QuarkPort", identifying and porting the required subset of CpuMpDxe (under the name CpuS3DataDxe), in order to populate PcdCpuS3DataAddress and ACPI_CPU_DATA, was a *major* headache. (Each field in that structure required separate analysis.) If PcdCpuS3DataAddress doesn't get populated though, then none of the ACPI_CPU_DATA fields are restored at S3 resume either, in EarlyInitializeCpu() and InitializeCpu(). (6) When you tested this series on physical hardware, did a different driver (not included in this series) exist there in the firmware, populating PcdCpuS3DataAddress, and the pointed-to ACPI_CPU_DATA structure? Did you test S3? - If you think it is safe to leave PcdCpuS3DataAddress and the pointed-to ACPI_CPU_DATA struct empty (and you tested such a config), then I'll just drop my CpuS3DataDxe driver. Meaning [PATCH 33/58] referenced above, plus *all* of these patches (note that there's one UefiCpuPkg/CpuDxe patch among them): [PATCH 34/58] OvmfPkg: QuarkPort/CpuS3DataDxe: fill in ACPI_CPU_DATA.StartupVector [PATCH 35/58] OvmfPkg: QuarkPor
Re: [edk2] [PATCH 6/7] UefiCpiPkg: Add PiSmmCpuDxeSmm module
Hi Laszlo, Thanks for the feedback! Please let me know if I missed any of your questions. Mike >-Original Message- >From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of >Laszlo Ersek >Sent: Wednesday, October 07, 2015 10:32 AM >To: Kinney, Michael D >Cc: edk2-de...@ml01.01.org >Subject: Re: [edk2] [PATCH 6/7] UefiCpiPkg: Add PiSmmCpuDxeSmm module > >On 10/05/15 01:57, Michael Kinney wrote: >> Add module that initializes a CPU for the SMM envirnment and >> installs the first level SMI handler. This module along with the >> SMM IPL and SMM Core provide the services required for >> DXE_SMM_DRIVERS to register hardware and software SMI handlers. >> >> CPU specific features are abstracted through the SmmCpuFeaturesLib >> >> Platform specific features are abstracted through the >> SmmCpuPlatformHookLib >> >> Several PCDs are added to enable/disable features and configure >> settings for the PiSmmCpuDxeSmm module >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Michael Kinney > >(I'm snipping the patch because it is extremely long.) > >I'm going through my "QuarkPort" patches, evaluating for each one if I >should keep it, drop it, or adapt it. > >* [PATCH 27/58] OvmfPkg: import PCDs from > Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg > http://thread.gmane.org/gmane.comp.bios.edk2.devel/329/focus=350 > > Going through all the PCDs I had to import there, and checking if each > was present in this patch too, I found the following difference: > > PcdCpuSmmUncacheCpuSyncData is absent from this patch. In my > "QuarkPort", the PCD is used by the InitializeMpServiceData() function > only, gating a call to SetCacheability(). And that call to > SetCacheability() is the *only* such call. > > Now, this patch provides SetCacheability(), but no calls are made to > it, ever (in accordance with the fact that the PCD that would control > the call is also absent). > > (1) Therefore I recommend to delete the SetCacheability() function > from this patch. I agree. I will remove that function. I missed that function when I removed PcdCpuSmmUncacheCpuSyncData. > > (2) I have a question wrt. PcdCpuSmmEnableBspElection. In the Quark > package, the "IA32FamilyCpuBasePkg.dsc" file overrides the default > TRUE value (from the .dec) for this PCD, with FALSE. > > The .dec default is similarly TRUE here. In my "QuarkPort" for > OVMF, I flipped the PCD to FALSE as well. Does that make sense? > Should I stick with that override, when rebasing the OVMF SMM > series on top of this series? I don't know why this PCD matters. > What are the benefits vs. drawbacks of dynamic BSP-for-SMI > election? For OVMF you will likely not see any difference in behavior. This PCD allows a platform to provide PlatformSmmBspElection() in a platform specific SmmCpuPlatformHookLib instance to decide which CPU gets elected to be the BSP in each SMI. The SmmCpuPlatformHookLibNull always returns EFI_NOT_READY for that function, which makes the module behave the same as the PCD being set to FALSE. The default is TRUE, so the platform lib is always called, so a platform developer can implement the hook function and does not have to also change a PCD setting for the hook function to be active. A platform that wants to eliminate the call to the hook function all together can set the PCD to FALSE. So for OVMF, I think it makes sense to set this PCD to FALSE in the DSC file. > >* [PATCH 28/58] OvmfPkg: import three protocols from > Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg > http://thread.gmane.org/gmane.comp.bios.edk2.devel/329/focus=351 > > (3) This patch doesn't install gSmmCpuSyncProtocolGuid or > gSmmCpuSync2ProtocolGuid in PiCpuSmmEntry(), whereas Quark does. > > I don't know what those protocols good for, but can you please > summarize why this patch doesn't need them? I did not find any consumers of the Sync protocols, so I removed them. > >* [PATCH 32/58] OvmfPkg: QuarkPort: drop ACPI_CPU_DATA.APState > http://thread.gmane.org/gmane.comp.bios.edk2.devel/329/focus=362 > > (4) Patch 5/7 in this series introduces the ACPI_CPU_DATA structure. > That structure has a field APState. I think it should be dropped, > it is never used in 6/7. I agree it is not used. However, this structure is shared between modules through a buffer address in a PCD, and I am concerned that there may be other modules that depend on this layout. I will need to do more investigation before I can consider removing it. > >* [PATCH 33/58] OvmfPkg: add skeleton QuarkPort/CpuS3Dat
Re: [edk2] [PATCH 6/7] UefiCpiPkg: Add PiSmmCpuDxeSmm module
Hi Mike, On 10/08/15 02:54, Kinney, Michael D wrote: > Hi Laszlo, > > Thanks for the feedback! > > Please let me know if I missed any of your questions. I'm very happy with your answers, thank you for them. I'll comment on S3 below: > Mike > >> -Original Message- >> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of >> Laszlo Ersek >> Sent: Wednesday, October 07, 2015 10:32 AM >> To: Kinney, Michael D >> Cc: edk2-de...@ml01.01.org >> Subject: Re: [edk2] [PATCH 6/7] UefiCpiPkg: Add PiSmmCpuDxeSmm module >> >> On 10/05/15 01:57, Michael Kinney wrote: >>> Add module that initializes a CPU for the SMM envirnment and >>> installs the first level SMI handler. This module along with the >>> SMM IPL and SMM Core provide the services required for >>> DXE_SMM_DRIVERS to register hardware and software SMI handlers. >>> >>> CPU specific features are abstracted through the SmmCpuFeaturesLib >>> >>> Platform specific features are abstracted through the >>> SmmCpuPlatformHookLib >>> >>> Several PCDs are added to enable/disable features and configure >>> settings for the PiSmmCpuDxeSmm module >>> >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Signed-off-by: Michael Kinney >> >> (I'm snipping the patch because it is extremely long.) >> >> I'm going through my "QuarkPort" patches, evaluating for each one if I >> should keep it, drop it, or adapt it. >> >> * [PATCH 27/58] OvmfPkg: import PCDs from >> Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg >> http://thread.gmane.org/gmane.comp.bios.edk2.devel/329/focus=350 >> >> Going through all the PCDs I had to import there, and checking if each >> was present in this patch too, I found the following difference: >> >> PcdCpuSmmUncacheCpuSyncData is absent from this patch. In my >> "QuarkPort", the PCD is used by the InitializeMpServiceData() function >> only, gating a call to SetCacheability(). And that call to >> SetCacheability() is the *only* such call. >> >> Now, this patch provides SetCacheability(), but no calls are made to >> it, ever (in accordance with the fact that the PCD that would control >> the call is also absent). >> >> (1) Therefore I recommend to delete the SetCacheability() function >> from this patch. > > I agree. I will remove that function. I missed that function when I removed > PcdCpuSmmUncacheCpuSyncData. > >> >> (2) I have a question wrt. PcdCpuSmmEnableBspElection. In the Quark >> package, the "IA32FamilyCpuBasePkg.dsc" file overrides the default >> TRUE value (from the .dec) for this PCD, with FALSE. >> >> The .dec default is similarly TRUE here. In my "QuarkPort" for >> OVMF, I flipped the PCD to FALSE as well. Does that make sense? >> Should I stick with that override, when rebasing the OVMF SMM >> series on top of this series? I don't know why this PCD matters. >> What are the benefits vs. drawbacks of dynamic BSP-for-SMI >> election? > > For OVMF you will likely not see any difference in behavior. > This PCD allows a platform to provide PlatformSmmBspElection() > in a platform specific SmmCpuPlatformHookLib instance to > decide which CPU gets elected to be the BSP in each SMI. > > The SmmCpuPlatformHookLibNull always returns > EFI_NOT_READY for that function, which makes the > module behave the same as the PCD being set to FALSE. > > The default is TRUE, so the platform lib is always called, > so a platform developer can implement the hook > function and does not have to also change a PCD > setting for the hook function to be active. > > A platform that wants to eliminate the call to the > hook function all together can set the PCD to FALSE. > > So for OVMF, I think it makes sense to set this PCD to > FALSE in the DSC file. > >> >> * [PATCH 28/58] OvmfPkg: import three protocols from >> Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg >> http://thread.gmane.org/gmane.comp.bios.edk2.devel/329/focus=351 >> >> (3) This patch doesn't install gSmmCpuSyncProtocolGuid or >> gSmmCpuSync2ProtocolGuid in PiCpuSmmEntry(), whereas Quark does. >> >> I don't know what those protocols good for, but can you please >> summarize why this patch doesn't need them? > > I did not find any consumers of the Sync protocols, so I removed them. > >> >> * [PATCH 32/58] OvmfPkg: Quark