Hi Jiewen,
Currently Host lib using a dummy AsmCpuid implementation:
BaseLib\X86UnitTestHost.c
AsmCpuid -> UnitTestHostBaseLibAsmCpuid -> Return all zero (BIT30 of ECX
hardcode to 1 after change of Gerd)
Did you mean prefer to use real AsmCpuid func in Host?
Or only use cpuid to check RdRand bit and set it.
Regards,
Yi
-----Original Message-----
From: Ard Biesheuvel <a...@kernel.org>
Sent: Saturday, June 15, 2024 1:16 AM
To: Yao, Jiewen <jiewen....@intel.com>
Cc: Li, Yi1 <yi1...@intel.com>; Gerd Hoffmann <kra...@redhat.com>;
devel@edk2.groups.io; Hou, Wenxing <wenxing....@intel.com>; Kinney, Michael D
<michael.d.kin...@intel.com>; Pedro Falcato <pedro.falc...@gmail.com>
Subject: Re: [edk2-devel] CryptoPkg host test broken due to smoketest for RDRAND
On Fri, 14 Jun 2024 at 18:45, Yao, Jiewen <jiewen....@intel.com> wrote:
>
>
> > -----Original Message-----
> > From: Ard Biesheuvel <a...@kernel.org>
> > Sent: Saturday, June 15, 2024 12:14 AM
> > To: Yao, Jiewen <jiewen....@intel.com>
> > Cc: Li, Yi1 <yi1...@intel.com>; Gerd Hoffmann <kra...@redhat.com>;
> > devel@edk2.groups.io; Hou, Wenxing <wenxing....@intel.com>; Kinney,
> > Michael D <michael.d.kin...@intel.com>; Pedro Falcato
> > <pedro.falc...@gmail.com>
> > Subject: Re: [edk2-devel] CryptoPkg host test broken due to
> > smoketest for RDRAND
> >
> > On Fri, 14 Jun 2024 at 18:09, Yao, Jiewen <jiewen....@intel.com> wrote:
> > >
> > > Hey
> > > This PR seems just a workaround.
> > >
> > > I don't feel it is right solution to hardcode BIT30.
> > > What if the host platform does not have such capability? You will
> > > get failure
> > later.
> > >
> >
> > Agreed. But that was already the case: RngLib assumed that RDRAND
> > was implemented without checking CPUID at all, and so the code was
> > already broken on systems without RDRAND.
>
> [Jiewen] Sorry, I don’t understand your comment. " implemented without
> checking CPUID at all "
>
> See below code. It does use CPUID to check the capability.
>
> EFI_STATUS
> EFIAPI
> BaseRngLibConstructor (
> VOID
> )
> {
> UINT32 RegEcx;
>
> //
> // Determine RDRAND support by examining bit 30 of the ECX register
> returned by
> // CPUID. A value of 1 indicates that processor support RDRAND instruction.
> //
> AsmCpuid (1, 0, 0, &RegEcx, 0);
>
> mRdRandSupported = ((RegEcx & RDRAND_MASK) == RDRAND_MASK);
>
> if (mRdRandSupported) {
> mRdRandSupported = TestRdRand ();
> }
>
> return EFI_SUCCESS;
> }
>
>
See commit 9301e5644cef5a5234f71b178373dd508cabb9ee
The old code had
+BOOLEAN
+EFIAPI
+ArchIsRngSupported (
+ VOID
+ )
+{
+ /*
+ Existing software depends on this always returning TRUE, so for
+ now hard-code it.
+
+ return mRdRandSupported;
+ */
+ return TRUE;
+}
> >
> > >
> > > To fix this function, can we call real CPUID instruction to return real
> > > value?
> > >
> >
> > That would be better. But this change just restores the old behavior.
> > And on top of that, Yi Li already merged it.
>
> [Jiewen] I don’t think it is right to merge it without thorough review.
>
> I think we need follow 24 hour rule.
> Any patch requires at least 24 hours before merge, to give people chance to
> review and feedback.
>
Agreed.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119585): https://edk2.groups.io/g/devel/message/119585
Mute This Topic: https://groups.io/mt/106666288/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
