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

["Laszlo Ersek" <lersek@redhat.com>]

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/SharedCryptoPkg
> 
> 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.PcdCryptoServiceFamilyEnable
> 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