Re: [edk2-devel] [Patch 3/5] CryptoPkg/Driver: Add Crypto PEIM, DXE, and SMM modules

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

Hi Laszlo,

Great feedback.  The reason I chose to use a Structured PCD
is to provide maximum flexibility.  I agree we should review
the behavior in RELEASE builds when a call is made to a 
service that is disabled.  Perhaps REPORT_STATUS_CODE would
be a good option for a platform provide its own handler for
that condition.
 
Platforms have a few choices for use of BaseCryptLib and
TlsLib services:

1) No changes from today.  Continue to statically link them
   into modules.  As modules change their usage, the linker
   takes care of everything.

2) Switch to the dynamic services through Protocols/PPIs
   and each platform is responsible for their own analysis
   and setting the Structured PCD for the PEI, DXE, SMM
   module.  I agree some additional guidance and/or tools
   may be required for a platform to have high confidence 
   in their Structured PCD settings.  Of course build time
   detection is preferred over runtime detection.

3) EDK II community defines a small number of profiles
   for the PEI, DXE, SMM versions of these modules and
   publishes released binaries for each profile.  Platforms
   integrate the published binary modules.  This may 
   provide more services than are required for some platforms
   but has the advantage of using a released binary from
   the EDK II project and reducing build times.  Once again,
   some guidance and/or tools may be required to help the
   EDK II community determine the right number of profiles
   and the services enabled in each profile.

I agree with your summary of the risks associated with the 
use of dynamic services.  Each platform will have to make
an assessment of those risks and choose to use this feature
or not.  Adding this feature to the EDK II will allow
platforms to try the feature on dev/debug builds first and
decide if they want to use the static or dynamic services for
their release builds.  That may also result in creative ideas
on how to get accurate accounting on service usage and work
towards a build time detection mechanism to know exactly what
services are used.

Best regards,

Mike

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Thursday, January 30, 2020 5:54 AM
> To: devel@edk2.groups.io; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX
> <xiaoyux.lu@intel.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>
> Subject: Re: [edk2-devel] [Patch 3/5] CryptoPkg/Driver:
> Add Crypto PEIM, DXE, and SMM modules
> 
> On 01/30/20 08:00, Michael D Kinney wrote:
> > https://bugzilla.tianocore.org/show_bug.cgi?id=2420
> >
> > Based on the following package with changes to merge
> into
> > CryptoPkg.
> >
> >
> https://github.com/microsoft/mu_plus/tree/dev/201908/Sh
> aredCryptoPkg
> >
> > Add the CryptoPei, CryptoDxe, and CryptoSmm modules
> that produce
> > EDK II Crypto Protocols/PPIs that provide the same
> services as
> > the BaseCryptLib class.
> >
> > In order to optimize the size of CryptoPei,
> CryptoDxe, and
> > CryptoSmm modules for a specific platform, the
> FixedAtBuild
> > PCD
> gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnabl
> e
> > is used to determine if a specific service is enabled
> or
> > disabled.  If a service is enabled, then a call is
> made to
> > the BaseCryptLib service.  If the service is
> disabled, then
> > a DEBUG() message and ASSERT() are performed and a
> default
> > return value is returned.  This provides simple
> detection
> > of a service that is disabled but is used by another
> module
> > when DEBUG()/ASSERT() macros are enabled.
> >
> > The use of a FixedAtBuild PCD is required so the
> compiler
> > and linker know each services enable/disable setting
> at
> > build time and allows disabled services to be
> optimized away.
> 
> I wonder how platforms are expected to determine their
> minimal PCD bitmaps.
> 
> In the static linking case, the linker sees all sites
> that (a) call the
> crypto functions, (b) take the addresses of crypto
> functions. Therefore
> the linker can eliminate unused functions with
> certainty.
> 
> If we switch to dynamic linking, we save space between
> independent
> modules, but we will not easily know what crypto
> functions are globally
> used (and, un-used) between all modules.
> 
> Dynamic coverage testing seems risky. If we miss
> something, then in a
> RELEASE build, the missing function will not even crash
> the system, but
> pretend some action and return a default value (IIUC).
> That might allow
> the issue to spread out, and cause symptoms at distant
> locations. (This
> sounds really dangerous for crypto, so I think I'd
> prefer a hard crash,
> on the spot). Either way, safety requirements appear to
> conflict with
> space saving goals.
> 
> I tried to find a method for statically deriving the
> necessary set of
> functions (basically, ask the linker or the object
> files about the
> *wrapper* crypto APIs that are actually called), but I
> came up empty.
> 
> And then there's the other direction too -- we don't
> just want to enable
> a service that we initially missed (and caught with an
> ASSERT() in a
> DEBUG build), we also want to disable a service that is
> not used
> *anymore*, after e.g. removing a module from a firmware
> build, or after
> slimming down a module. How will we notice such
> opportunities for
> "trimming" the protocol?
> 
> I think the static PCD is a great mechanism, but we
> need some automated
> way ("policy") too, for calculating the PCD for a given
> platform.
> 
> This is not to say that the series should not go in as-
> is -- it's huge,
> and I can't offer a review better than this mere email,
> so I'm not
> trying to say anything like go/no-go. It's just that in
> OVMF I wouldn't
> even attempt to come up with the right bitmaps, I'd
> just set "give me
> everything" for safety -- and that might actually
> consume more space in
> the flash than statically linking just a handful (?) of
> functions into 5
> modules.
> 
> (To clarify: by "we", I don't mean "Red Hat", I mean
> the upstream
> community. I.e. the above is not a vendor-side
> evaluation, but a general
> platform perspective. OVMF is used just as an example
> that I'm familiar
> with.)
> 
> With the DevicePathLib and PrintLib instances that
> defer to a protocol,
> I think the *proportions* are a bit different. I feel
> it's not difficult
> for a platform firmware to put most of the
> DevicePathLib / PrintLib APIs
> to use (i.e. the duplication due to static linking may
> be significant),
> plus the protocol implementations themselves can be
> "complete", because
> they are not huge (i.e., even if the protocols are
> provided at some net
> loss in space, the loss is not serious).
> 
> Again: this is not a review either way, I'm just
> asking: how is a
> platform supposed to calculate the minimal service
> bitmaps for the PCD?
> 
> Thanks
> Laszlo