Laszlo,
According to RFC discussion, using PCD here is mainly for optimization purpose.
So I
think we should limit the PCD type to just FixedAtBuild. Then there's no problem
for modules linking this library.
Regards,
Jian
> -----Original Message-----
> From: Laszlo Ersek <ler...@redhat.com>
> Sent: Wednesday, February 05, 2020 7:00 PM
> To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kin...@intel.com>
> Cc: Sukerkar, Amol N <amol.n.suker...@intel.com>; Yao, Jiewen
> <jiewen....@intel.com>; Wang, Jian J <jian.j.w...@intel.com>
> Subject: Re: [edk2-devel] [Patch v10 2/2] CryptoPkg/BaseHashApiLib: Implement
> Unified Hash Calculation API
>
> Hi,
>
> sorry I'm late to this discussion. I'd only like to mention a potential
> future improvement:
>
> On 02/04/20 00:35, Michael D Kinney wrote:
>
> > +[PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
> > + ## This PCD indicates the HASH algorithm to calculate hash of data
> > + # Based on the value set, the required algorithm is chosen to calculate
> > + # the hash of data.<BR>
> > + # The default hashing algorithm for BaseHashApiLib is set to SHA256.<BR>
> > + # 0x00000001 - MD4.<BR>
> > + # 0x00000002 - MD5.<BR>
> > + # 0x00000003 - SHA1.<BR>
> > + # 0x00000004 - SHA256.<BR>
> > + # 0x00000005 - SHA384.<BR>
> > + # 0x00000006 - SHA512.<BR>
> > + # 0x00000007 - SM3_256.<BR>
> > + # @Prompt Set policy for hashing unsigned image for Secure Boot.
> > + # @ValidRange 0x80000001 | 0x00000001 - 0x00000007
> > +
> gEfiCryptoPkgTokenSpaceGuid.PcdHashApiLibPolicy|0x04|UINT8|0x00000001
> > +
>
> The platform may choose to make this PCD dynamic or dynamicEx. That's
> good. But:
>
> > +UINTN
> > +EFIAPI
> > +HashApiGetContextSize (
> > + VOID
> > + )
> > +{
> > + switch (PcdGet8 (PcdHashApiLibPolicy)) {
> > + case HASH_API_ALGO_MD4:
> > + return Md4GetContextSize ();
> > + break;
>
> we have direct PcdGet8() calls in the lib API implementations. And:
>
> > +[Defines]
> > + INF_VERSION = 0x00010005
> > + BASE_NAME = BaseHashApiLib
> > + MODULE_UNI_FILE = BaseHashApiLib.uni
> > + FILE_GUID = B1E566DD-DE7C-4F04-BDA0-B1295D3BE927
> > + MODULE_TYPE = BASE
> > + VERSION_STRING = 1.0
> > + LIBRARY_CLASS = BaseHashApiLib
>
> [...]
>
> > +[Pcd]
> > + gEfiCryptoPkgTokenSpaceGuid.PcdHashApiLibPolicy ## CONSUMES
>
> The lib class is not restricted to any particular firmware phase, or
> module type.
>
> This suggests that the lib instance is usable in DXE runtime drivers or
> SMM drivers. If the serives are called outside of the entry point
> functions, the dynamic PCD fetches would be a problem, I think.
>
> So the idea here would be to create a minimal separate INF file + C file
> for runtime applications (runtime DXE and SMM drivers), and there a
> constructor function could run PcdGet8(), and stash the value in a
> global variable.
>
> Alternatively, if this is overkill, we could improve safety by restricting
>
> LIBRARY_CLASS = BaseHashApiLib|<module_type> <module_type> ...
>
> to every module type except runtime DXE drivers and SMM drivers.
>
> Thanks
> Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#53825): https://edk2.groups.io/g/devel/message/53825
Mute This Topic: https://groups.io/mt/70960524/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
