On Fri, 14 Jun 2024 at 18:45, Yao, Jiewen <[email protected]> wrote:
>
>
> > -----Original Message-----
> > From: Ard Biesheuvel <[email protected]>
> > Sent: Saturday, June 15, 2024 12:14 AM
> > To: Yao, Jiewen <[email protected]>
> > Cc: Li, Yi1 <[email protected]>; Gerd Hoffmann <[email protected]>;
> > [email protected]; Hou, Wenxing <[email protected]>; Kinney, Michael
> > D <[email protected]>; Pedro Falcato <[email protected]>
> > Subject: Re: [edk2-devel] CryptoPkg host test broken due to smoketest for
> > RDRAND
> >
> > On Fri, 14 Jun 2024 at 18:09, Yao, Jiewen <[email protected]> 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 (#119579): https://edk2.groups.io/g/devel/message/119579
Mute This Topic: https://groups.io/mt/106666288/21656
Group Owner: [email protected]
Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]]
-=-=-=-=-=-=-=-=-=-=-=-
