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 <michael.d.kin...@intel.com> >> --- > >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
