Re: [PATCH 00/12] Add SmiHandlerProfile feature

["Yao, Jiewen" <jiewen.yao@intel.com>]

HI Laszlo
We have enabled OVMF KVM, and validated this patch.

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

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