Sounds good, I will try it.
Thanks,
Yi
-----Original Message-----
From: Kinney, Michael D <michael.d.kin...@intel.com>
Sent: Saturday, June 15, 2024 12:58 PM
To: Li, Yi1 <yi1...@intel.com>; Ard Biesheuvel <a...@kernel.org>; Yao, Jiewen
<jiewen....@intel.com>
Cc: Gerd Hoffmann <kra...@redhat.com>; devel@edk2.groups.io; Hou, Wenxing
<wenxing....@intel.com>; Pedro Falcato <pedro.falc...@gmail.com>; Kinney,
Michael D <michael.d.kin...@intel.com>
Subject: RE: [edk2-devel] CryptoPkg host test broken due to smoketest for RDRAND
If the host test was updated to use GoogleTest/GoogleMock, then the call to
AsmCpuid() could be mocked instead of calling the version of BaseLib that is
safe to use from host envs. Then all the code paths can be tested properly.
Mike
> -----Original Message-----
> From: Li, Yi1 <yi1...@intel.com>
> Sent: Friday, June 14, 2024 9:55 PM
> To: Ard Biesheuvel <a...@kernel.org>; Yao, Jiewen
> <jiewen....@intel.com>
> Cc: 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
>
> 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 (#119587): https://edk2.groups.io/g/devel/message/119587
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]
-=-=-=-=-=-=-=-=-=-=-=-
