Re: [edk2-devel] [PATCH v2 2/2] CryptoPkg/OpensslLib: Commit the auto-generated assembly files for X64

["Michael D Kinney" <michael.d.kinney@intel.com>]

Laszlo,

The current CryptoPkg DCS file with use of the CRYPTO_SERVICES define is cumbersome.

  #
  # Flavor of PEI, DXE, SMM modules to build.
  # Must be one of ALL, NONE, MIN_PEI, MIN_DXE_MIN_SMM.
  # Default is ALL that is used for package build verification.
  #   PACKAGE         - Package verification build of all components.  Null
  #                     versions of libraries are used to minimize build times.
  #   ALL             - Build PEIM, DXE, and SMM drivers.  Protocols and PPIs
  #                     publish all services.
  #   NONE            - Build PEIM, DXE, and SMM drivers.  Protocols and PPIs
  #                     publish no services.  Used to verify compiler/linker
  #                     optimizations are working correctly.
  #   MIN_PEI         - Build PEIM with PPI that publishes minimum required
  #                     services.
  #   MIN_DXE_MIN_SMM - Build DXE and SMM drivers with Protocols that publish
  #                     minimum required services.
  #
  DEFINE CRYPTO_SERVICES = PACKAGE

There is a known limitation for using structured PCDs in a module scope and 
that limitation is what resulted in the use of this define.  Bob Feng
has provided a BaseTools patch that attempts to address this limitation.

    https://edk2.groups.io/g/devel/message/63906

This patch is functional, but has one open issue around the PCD report.  Once
this issue is resolved we will be able to specify structured PCD field values
in the scope of a single module.  I have a branch that simplifies the DSC and
allows all flavors of the crypto modules to be built in a single invocation
of the build command.  There is more cleanup of the DSC possible, but I
wanted to share a quick test case for Bob's patch.

    https://github.com/mdkinney/edk2/tree/Bug_xxx_CryptoPkg_UseModuleScopedPcds

This feature supports both the generation of standard flavors of the crypto
modules that a platform could consume as a pre-built binary and also allows
a platform to choose their own profile by specifying the specific crypto APIs
needed in PEI, DXE, SMM when building crypto modules from sources.
  
Best regards,

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
> Sent: Wednesday, August 19, 2020 3:44 AM
> To: devel@edk2.groups.io; spbrogan@outlook.com; Kinney, Michael D <michael.d.kinney@intel.com>; Wang, Jian J
> <jian.j.wang@intel.com>; Zurcher, Christopher J <christopher.j.zurcher@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
> Cc: Lu, XiaoyuX <xiaoyux.lu@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: Re: [edk2-devel] [PATCH v2 2/2] CryptoPkg/OpensslLib: Commit the auto-generated assembly files for X64
> 
> Hi,
> 
> On 08/18/20 23:33, Sean wrote:
> > Mike,
> >
> > I am not technically a basetool maintainer but as an active user/dev in
> > basetools, i would be opposed to bringing in perl as an edk2 dependency.
> > Also introducing another language is counter to the goal of aligning on
> > python and improving the python used within edk2.  From my perspective
> > the openssl config case isn't strong enough to counter the above goal.
> > In fact as you know we are trying to change the paradigm for
> > Crypto/OpenSSL with the Crypto Driver
> > (https://github.com/tianocore/edk2/tree/master/CryptoPkg/Driver) and
> > BaseCryptLibOnProtocolPpi
> > (https://github.com/tianocore/edk2/tree/master/CryptoPkg/Library/BaseCryptLibOnProtocolPpi)
> > work so that everyday development doesn't need to compile openssl in
> > their edk2 builds.
> 
> Here I'd only like to comment on this one aspect (= build OpenSSL as a
> statically linked library vs. as a crypto service driver / PEIM).
> 
> Recently I tried to evaluate the crypto driver for OVMF. I started with
> the PEI phase. The configuration interface (the PCD) is baroque, *BUT*
> that is a direct consequence of OpenSSL offering a huge range of
> interfaces. So no complaints about the config interface.
> 
> I also reviewed the CryptoPkg.dsc "pre-sets" (i.e., CRYPTO_SERVICES
> being one of PACKAGE / ALL / NONE / MIN_PEI / MIN_DXE_MIN_SMM). I had
> two problems:
> 
> - These pre-sets are supremely suitable for a platform that is composed
> of multiple build runs; that is, build the crypto PEIM, build the DXE /
> SMM protocol drivers, package up the resultant binaries, and *then*
> build the actual platforms (which will then include the crypto service
> drivers in *binary* form). On the other hand, the pre-sets are not
> useful to a platform that is supposed to be built in a single-shot.
> Importantly, I'm not saying that the pre-sets are *detrimental* to such
> platforms -- they aren't. It's just that the pre-sets target a different
> use case.
> 
> - The other problem I had was the one that we had discussed when the
> crypto service driver was being introduced. Namely, selecting the
> OpenSSL interfaces (interface families) that the platform actually consumes.
> 
> Now, I carefully tracked down the modules in OVMF that needed crypto
> support, by *not* resolving SmmCryptLib, RuntimeCryptLib, TlsLib in
> general [LibraryClasses] sections in the OVMF DSC files. Then I re-added
> those lib-class resolutions as module-scoped <LibraryClasses> overrides
> to the actual modules that needed them.
> 
> However, I didn't know how to even *begin* evaluating the specific "API
> needs" of the modules identified thusly. On a Windows or Linux OS, when
> you have a dynamically linked executable, and it doesn't find a symbol
> in a shared library, you get a nice error message, and the application
> doesn't start. On the other hand, if a crypto protocol call fails in SMM
> because we missed a feature bit in the config PCD, the results are
> somewhat less user-friendly.
> 
> The expression "minimum required services" in CryptoPkg.dsc seems
> relevant, but it didn't convince me that it would cover everything
> needed by -- for example -- VariableSmm, VariableRuntimeDxe, and TlsDxe.
> 
> So, given that I couldn't construct a "tight profile", I started my
> investigation (for OVMF's PEI phase) by including the crypto service
> PEIM with *all* interfaces enabled.
> 
> This would be restricted to "TPM_ENABLE", because only that is when
> OVMF's PEI phase needs crypto -- due to including the various TPM1 and
> TPM2 PEIMs.
> 
> So basically this check would replace the statically linked -- and
> accordingly trimmed! -- "thick" OpenSSL library copies in the TPM1/PTM2
> PEIMs, with the thin wrapper lib
> (BaseCryptLibOnProtocolPpi/PeiCryptLib.inf) *plus* the full-blown crypto
> service PEIM.
> 
> The result was a *violent* size explosion in PEIFV; at least in the
> NOOPT build. Before:
> 
> > PEIFV [64%Full] 917504 total, 592456 used, 325048 free
> 
> after:
> 
> >   the required fv image size 0x132968 exceeds the set fv image size
> > 0xe0000
> 
> The PEIFV footprint more than doubled, from 592,456 bytes to 1,255,784
> bytes.
> 
> I gave up there. Until the "crypto profile" construction is not
> automated for platforms, somehow, I don't know how I could maintain OVMF
> consuming the crypto service PEIMs/drivers.
> 
> (I wonder if we should maintain a "required crypto services" bitmap for
> each individual PEIM / DXE / SMM driver inside edk2. And then, when a
> platform includes any one such PEIM or driver, they'd know to "OR" the
> bitmap for that particular module into their platform PCD setting.)
> 
> > So I support leaving it as is which means if you have to change
> > something in openssl config you deal with it and a special one off.
> 
> (OK, I guess I can comment on this too, after all.)
> 
> I agree.
> 
> While perl is readily available on Linux build hosts, I remember:
> - accommodating the python3 BaseTools requirements on my RHEL7 laptop,
> - (almost) bumping the NASM version so we could compile the VMGEXIT
> instruction for IA32,
> - the python virtual environment discussions for running CI locally.
> 
> So I agree that new build dependencies should be avoided as much as
> possible.
> 
> Thanks
> Laszlo
> 
> 
>