Re: [PATCH 00/12] Add SmiHandlerProfile feature

[Laszlo Ersek <lersek@redhat.com>]

On 02/08/17 18:57, Yao, Jiewen wrote:
> 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.

Sounds great, thank you!

> 
>  
> 
> 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);
> 
>   }
> 
> }

Perfect. Thank you for confirming!

> 
>  
> 
> 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'm very sorry to hear that, and I can't deny that using QEMU from the
command line is extremely hard. It's hard even for the people who work
on QEMU / use it on a daily basis.

The fact to keep in mind is that the QEMU command line is actually no
longer a human interface; it is a machine interface. The libvirt system
service sits on top of QEMU/KVM (and a few other hypervisors), and
manages the configuration and lifecycle of virtual machines.

In turn, there are both command line utilities and graphical tools that
connect to the libvirt service, and allow for much-much easier handling
of guests than the raw QEMU command line. So ultimately, it looks like this:

  helpful friendly GUI client  ------ libvirt service ---- QEMU --- KVM
  utility called "virt-manager"

The closer you sit to the right side, as a human / interactive user, the
more bumpy your ride will be.

In my own development environment, I *insist* on sitting at the leftmost
side.

In fact, for Windows desktop users, there's one more layer:

      [ WINDOWS DESKTOP ]
               |
               |
+-------[ LINUX SERVER ]-----------------------------------------------+
|                                                                      |
| helpful friendly GUI client  ----- libvirt service ---- QEMU --- KVM |
| utility called "virt-manager"                                        |
|                                                                      |
+----------------------------------------------------------------------+

In this scenario, you'd be using the "helpful friendly GUI client
utility called 'virt-manager'" that runs on the Linux server, getting
the GUI on your Windows desktop.

Last time around, I provided you with a QEMU command line, which is very
hard to adjust (for whatever reason). This time, I documented a setup so
that we can sit at the left side of the diagram.

> 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.

The setup is likely not *quick*, but it should be reasonably
*straightforward*, and the end result is *extremely* convenient to use.

I *really* want it to work out for you. If you hit *any* roadblocks,
please email me, on-list or off-list, as you prefer (or we can discuss
on the #edk2 IRC channel), and I'll make it my first priority to assist you.

I can log in every day around 09:30 in the UTC+01:00 time zone.

Thanks,
Laszlo

> 
>  
> 
> 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
>