Re: [PATCH 00/12] Add SmiHandlerProfile feature

[Laszlo Ersek <lersek@redhat.com>]

On 02/21/17 08:06, Yao, Jiewen wrote:
> HI Laszlo
> 
> We have enabled OVMF KVM, and validated this patch.

Fantastic!!! Thank you for making the effort!

> There is no impact to OVMF. Fedora Guest boot and S3 passes.

Hooray! :)

Thanks!
Laszlo

> 
>  
> 
> Thank you
> 
> Yao Jiewen
> 
>  
> 
>  
> 
> *From:*Yao, Jiewen
> *Sent:* Thursday, February 9, 2017 1:58 AM
> *To:* Yao, Jiewen <jiewen.yao@intel.com>; edk2-devel@ml01.01.org
> *Cc:* Kinney, Michael D <michael.d.kinney@intel.com>; Justen, Jordan L
> <jordan.l.justen@intel.com>; Tian, Feng <feng.tian@intel.com>; Zeng,
> Star <star.zeng@intel.com>; Gao, Liming <liming.gao@intel.com>; Laszlo
> Ersek (lersek@redhat.com) <lersek@redhat.com>
> *Subject:* RE: [edk2] [PATCH 00/12] Add SmiHandlerProfile feature
> 
>  
> 
> Hi Laszlo
> 
> Thanks for the comment.
> 
>  
> 
> To clarify something:
> 
>  
> 
> 1)       I did not enable SMI handler profile for OVMF because I notice
> the OVMF just use very few SMI handlers, and it does not have
> SmmChildDispatcher. Just like what you mentioned.
> 
>  
> 
> 2)       I forget to mention: I did regression test for OVMF IA32 and
> IA32X64 with default SmiHandlerProfile is DISABLED.
> 
> I boot to UEFI SHELL only, and I do not enter OS.
> 
> I think the risk is very low, because if SMI handler profile is
> disabled, there is no code logic change.
> 
>  
> 
> VOID
> 
> SmmCoreInitializeSmiHandlerProfile (
> 
>   VOID
> 
>   )
> 
> {
> 
>   EFI_STATUS  Status;
> 
>   VOID        *Registration;
> 
>   EFI_HANDLE  Handle;
> 
>  
> 
>   if((PcdGet8 (PcdSmiHandlerProfilePropertyMask) & 0x1) != 0) {
> 
>     InsertTailList (&mRootSmiEntryList, &mRootSmiEntry.AllEntries);
> 
>  
> 
>     Status = gSmst->SmmRegisterProtocolNotify (
> 
>                       &gEfiSmmReadyToLockProtocolGuid,
> 
>                       SmmReadyToLockInSmiHandlerProfile,
> 
>                       &Registration
> 
>                       );
> 
>     ASSERT_EFI_ERROR (Status);
> 
>  
> 
>     Handle = NULL;
> 
>     Status = gSmst->SmmInstallProtocolInterface (
> 
>                       &Handle,
> 
>                       &gSmiHandlerProfileGuid,
> 
>                       EFI_NATIVE_INTERFACE,
> 
>                       &mSmiHandlerProfile
> 
>                       );
> 
>     ASSERT_EFI_ERROR (Status);
> 
>   }
> 
> }
> 
>  
> 
> I do understand your concern, because OVMF has some special feature
> which might be different with real hardware.
> 
>  
> 
> At same time, I have to mention that, it is very hard to setup KVM in
> Linux. I did try to follow your description before on NX.
> 
> I spent one week, but still fail to launch the final image on my side, I
> have no idea on what is wrong. That was a horrible experience.
> 
>  
> 
> I will have a try to follow your new document.
> 
> If it can be setup successfully, easily and fortunately, I do want to
> run it.
> 
>  
> 
> Thank you
> 
> Yao Jiewen
> 
>  
> 
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
> Sent: Wednesday, February 8, 2017 9:34 AM
> To: Yao, Jiewen <jiewen.yao@intel.com
> <mailto:jiewen.yao@intel.com>>; edk2-devel@ml01.01.org
> <mailto:edk2-devel@ml01.01.org>
> Cc: Kinney, Michael D <michael.d.kinney@intel.com
> <mailto:michael.d.kinney@intel.com>>; Justen, Jordan L <jordan.l.justen@intel.com
> <mailto:jordan.l.justen@intel.com>>; Tian, Feng <feng.tian@intel.com
> <mailto:feng.tian@intel.com>>; Zeng, Star <star.zeng@intel.com
> <mailto:star.zeng@intel.com>>; Gao, Liming <liming.gao@intel.com
> <mailto:liming.gao@intel.com>>
> Subject: Re: [edk2] [PATCH 00/12] Add SmiHandlerProfile feature
> 
> CC Jordan
> 
> On 02/08/17 17:30, Jiewen Yao wrote:
>> This series patch add SMI handler profile.
>> 
>> The purpose of SMI handler profile is to add the capability to dump 
>> all SMI handlers produced by the firmware in a given boot.
>> The SMI handlers here include
>> 1) Root SMI handlers registered with SMST->SmiHandlerRegister by SmmCore.
>> 2) GUID SMI handlers registered with SMST->SmiHandlerRegister by SmmCore.
>> 3) Hardware SMI handlers registered with 
>> SMM_XXX_DISPATCH_PROTOCOL->Register
>> by SmmChildDispatcher module.
>> 
>> The final log is an XML format log, including all SMM image name, all 
>> SMI handlers name, type, location (source file and line number), and 
>> the caller who registeres the handler.
>> 
>> We enabled Quark platform to show how to add support in SmmChildDispatcher.
> 
> Thanks for the CC.
> 
> This module seems quite large, and OVMF (to my knowledge) doesn't include any special handlers. (Nothing from category (3), and I couldn't say if anything from category (2)).
> 
> Do you think this module would be useful to enable in OVMF, conditionally, with -D SMI_HANDLER_PROFILE_ENABLE?
> 
> Beyond the question of its utility to OVMF developers and users, requiring a separate UEFI app to dump the profile, presumably from the UEFI shell, is not really flexible for OVMF. We can't / don't build applications into the firmware image (beyond the shell and UiApp), so users would have to build a virtual disk image with this app, or we'd have to provide scripts for the same.
> 
> For now, I believe we don't wish to enable this in OVMF.
> 
> If you CC'd me for regression-testing PiSmmCore, as it is built into OVMF, with the default PcdSmiHandlerProfilePropertyMask=0 value, then I respectfully point you to the following article:
> 
> https://github.com/tianocore/tianocore.github.io/wiki/Testing-SMM-with-QEMU,-KVM-and-libvirt
> 
> Please set up a KVM environment as documented there, and adopt it for regular SMM testing :) My strong hope is that, by adopting KVM as a regular test platform, Intel developers won't only catch OVMF regressions before they post patches, but they should also see their own productivity increase.
> 
> I assume it must be much easier to click "Play" in the Virt-Manager GUI and to boot to an OS, on top of the new firmware, in seconds, than to flash firmware to a physical board. It does not replace physical board testing, but should catch a number of errors more gracefully.
> 
> The test cases I wrote up include UEFI variable testing and ACPI S3. The guest OSes covered are Fedora 25 and Windows 10. Please give it a look.
> 
> Thanks!
> Laszlo
> 
>> 
>> NOTE: This SMI handler profile is a *DEBUG* feature only, to help 
>> platform developer or test engineer to audit the SMI handlers. Please 
>> do not include it in the final *RELEASE* image.
>> 
>> Cc: Michael D Kinney <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>>
>> Cc: Liming Gao <liming.gao@intel.com <mailto:liming.gao@intel.com>>
>> Cc: Feng Tian <feng.tian@intel.com <mailto:feng.tian@intel.com>>
>> Cc: Star Zeng <star.zeng@intel.com <mailto:star.zeng@intel.com>>
>> Cc: Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Jiewen Yao <jiewen.yao@intel.com <mailto:jiewen.yao@intel.com>>
>> 
>> Jiewen Yao (12):
>>   MdePkg/Include: Add SmiHandlerProfileLib.h
>>   MdePkg/SmiHandlerProfileLibNull: Add NULL instance.
>>   MdePkg/dsc: add SmiHandlerProfileLib to dsc.
>>   MdeModulePkg/include: Add SmiHandlerProfile header file.
>>   MdeModulePkg/dec: Add PcdSmiHandlerProfilePropertyMask.
>>   MdeModulePkg/SmmSmiHandlerProfileLib: Add SMM instance.
>>   MdeModulePkg/PiSmmCore: Add SmiHandlerProfile support.
>>   MdeModulePkg/App: Add SmiHandlerProfile dump app.
>>   MdeModulePkg/dsc: add SmiHandlerProfile to dsc.
>>   BaseTool/Script: Add SmiHandleProfile OS tool to get symbol.
>>   QuarkSocPkg/SmmChildDispatch: Add SmiHandlerProfile support.
>>   QuarkPlatformPkg: enable SmiHandlerProfile.
>> 
>>  BaseTools/Scripts/SmiHandlerProfileSymbolGen.py                                |  351 ++++++
>>  MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.c         |  685 +++++++++++
>>  MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.inf       |   65 +
>>  MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.uni       |   22 +
>>  MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfoExtra.uni  |   19 +
>>  MdeModulePkg/Core/PiSmmCore/PiSmmCore.c                                        |    4 +-
>>  MdeModulePkg/Core/PiSmmCore/PiSmmCore.h                                        |   89 +-
>>  MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf                                      |   17 +-
>>  MdeModulePkg/Core/PiSmmCore/Smi.c                                              |   46 +-
>>  MdeModulePkg/Core/PiSmmCore/SmiHandlerProfile.c                                | 1261 ++++++++++++++++++++
>>  MdeModulePkg/Include/Guid/SmiHandlerProfile.h                                  |  177 +++
>>  MdeModulePkg/Library/SmmSmiHandlerProfileLib/SmmSmiHandlerProfileLib.c         |  106 ++
>>  MdeModulePkg/Library/SmmSmiHandlerProfileLib/SmmSmiHandlerProfileLib.inf       |   46 +
>>  MdeModulePkg/Library/SmmSmiHandlerProfileLib/SmmSmiHandlerProfileLib.uni       |   21 +
>>  MdeModulePkg/MdeModulePkg.dec                                                  |   11 +-
>>  MdeModulePkg/MdeModulePkg.dsc                                                  |    2 +
>>  MdePkg/Include/Library/SmiHandlerProfileLib.h                                  |   81 ++
>>  MdePkg/Library/SmiHandlerProfileLibNull/SmiHandlerProfileLibNull.c             |   72 ++
>>  MdePkg/Library/SmiHandlerProfileLibNull/SmiHandlerProfileLibNull.inf           |   36 +
>>  MdePkg/Library/SmiHandlerProfileLibNull/SmiHandlerProfileLibNull.uni           |   21 +
>>  MdePkg/MdePkg.dec                                                              |    5 +-
>>  MdePkg/MdePkg.dsc                                                              |    3 +-
>>  QuarkPlatformPkg/Quark.dsc                                                     |   16 +-
>>  QuarkPlatformPkg/Quark.fdf                                                     |    3 +-
>>  QuarkPlatformPkg/QuarkMin.dsc                                                  |    5 +-
>>  QuarkPlatformPkg/Readme.md                                                     |   29 +-
>>  QuarkSocPkg/QuarkNorthCluster/Smm/DxeSmm/QncSmmDispatcher/QNCSmmCore.c         |   19 +-
>>  QuarkSocPkg/QuarkNorthCluster/Smm/DxeSmm/QncSmmDispatcher/QNCSmmDispatcher.inf |    3 +-
>>  28 files changed, 3159 insertions(+), 56 deletions(-)  create mode 
>> 100644 BaseTools/Scripts/SmiHandlerProfileSymbolGen.py
>>  create mode 100644 
>> MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.c
>>  create mode 100644 
>> MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.i
>> nf  create mode 100644 
>> MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.u
>> ni  create mode 100644 
>> MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfoEx
>> tra.uni  create mode 100644 
>> MdeModulePkg/Core/PiSmmCore/SmiHandlerProfile.c
>>  create mode 100644 MdeModulePkg/Include/Guid/SmiHandlerProfile.h
>>  create mode 100644 
>> MdeModulePkg/Library/SmmSmiHandlerProfileLib/SmmSmiHandlerProfileLib.c
>>  create mode 100644 
>> MdeModulePkg/Library/SmmSmiHandlerProfileLib/SmmSmiHandlerProfileLib.i
>> nf  create mode 100644 
>> MdeModulePkg/Library/SmmSmiHandlerProfileLib/SmmSmiHandlerProfileLib.u
>> ni  create mode 100644 MdePkg/Include/Library/SmiHandlerProfileLib.h
>>  create mode 100644 
>> MdePkg/Library/SmiHandlerProfileLibNull/SmiHandlerProfileLibNull.c
>>  create mode 100644 
>> MdePkg/Library/SmiHandlerProfileLibNull/SmiHandlerProfileLibNull.inf
>>  create mode 100644 
>> MdePkg/Library/SmiHandlerProfileLibNull/SmiHandlerProfileLibNull.uni
>> 
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org <mailto:edk2-devel@lists.01.org>
> https://lists.01.org/mailman/listinfo/edk2-devel
>