Re: [PATCH 00/12] Add SmiHandlerProfile feature

[Laszlo Ersek <lersek@redhat.com>]

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>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Feng Tian <feng.tian@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jiewen Yao <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.inf
>  create mode 100644 MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.uni
>  create mode 100644 MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfoExtra.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.inf
>  create mode 100644 MdeModulePkg/Library/SmmSmiHandlerProfileLib/SmmSmiHandlerProfileLib.uni
>  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
>