From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by mx.groups.io with SMTP id smtpd.web11.11259.1580404215205953210 for ; Thu, 30 Jan 2020 09:10:15 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.120, mailfrom: michael.d.kinney@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 30 Jan 2020 09:10:14 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,382,1574150400"; d="scan'208";a="428433125" Received: from orsmsx104.amr.corp.intel.com ([10.22.225.131]) by fmsmga005.fm.intel.com with ESMTP; 30 Jan 2020 09:10:14 -0800 Received: from orsmsx159.amr.corp.intel.com (10.22.240.24) by ORSMSX104.amr.corp.intel.com (10.22.225.131) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 30 Jan 2020 09:10:13 -0800 Received: from orsmsx113.amr.corp.intel.com ([169.254.9.57]) by ORSMSX159.amr.corp.intel.com ([169.254.11.41]) with mapi id 14.03.0439.000; Thu, 30 Jan 2020 09:10:13 -0800 From: "Michael D Kinney" To: Laszlo Ersek , "devel@edk2.groups.io" , "Kinney, Michael D" , Sean Brogan , "'Bret.Barkelew@microsoft.com'" , Matthew Carlson CC: "Wang, Jian J" , "Lu, XiaoyuX" , Ard Biesheuvel Subject: Re: [edk2-devel] [Patch 3/5] CryptoPkg/Driver: Add Crypto PEIM, DXE, and SMM modules Thread-Topic: [edk2-devel] [Patch 3/5] CryptoPkg/Driver: Add Crypto PEIM, DXE, and SMM modules Thread-Index: AQHV1zsMTCzYinc4L0OsZHFM9CHzc6gDwRSA//+pJXA= Date: Thu, 30 Jan 2020 17:10:12 +0000 Message-ID: References: <20200130070037.8516-1-michael.d.kinney@intel.com> <20200130070037.8516-4-michael.d.kinney@intel.com> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-originating-ip: [10.22.254.140] MIME-Version: 1.0 Return-Path: michael.d.kinney@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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=20 service that is disabled. Perhaps REPORT_STATUS_CODE would be a good option for a platform provide its own handler for that condition. =20 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=20 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=20 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=20 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 > Sent: Thursday, January 30, 2020 5:54 AM > To: devel@edk2.groups.io; Kinney, Michael D > > Cc: Wang, Jian J ; Lu, XiaoyuX > ; Ard Biesheuvel > > Subject: Re: [edk2-devel] [Patch 3/5] CryptoPkg/Driver: > Add Crypto PEIM, DXE, and SMM modules >=20 > On 01/30/20 08:00, Michael D Kinney wrote: > > https://bugzilla.tianocore.org/show_bug.cgi?id=3D2420 > > > > 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. >=20 > I wonder how platforms are expected to determine their > minimal PCD bitmaps. >=20 > 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. >=20 > 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. >=20 > 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. >=20 > 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. >=20 > 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? >=20 > 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. >=20 > 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. >=20 > (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.) >=20 > 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). >=20 > 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? >=20 > Thanks > Laszlo