Re: [edk2-devel] CryptoPkg host test broken due to smoketest for RDRAND

["Li, Yi" <yi1.li@intel.com>]

> 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.

Sure I will follow this rule future.
There is a critical Mbedtls bug based on this patch (blocked build) so I merged this patch quickly to unblock CI.
https://github.com/tianocore/edk2/pull/5773


Thanks,
Yi

-----Original Message-----
From: Yao, Jiewen <jiewen.yao@intel.com> 
Sent: Saturday, June 15, 2024 12:45 AM
To: Ard Biesheuvel <ardb@kernel.org>
Cc: Li, Yi1 <yi1.li@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; devel@edk2.groups.io; Hou, Wenxing <wenxing.hou@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Pedro Falcato <pedro.falcato@gmail.com>
Subject: RE: [edk2-devel] CryptoPkg host test broken due to smoketest for RDRAND


> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> Sent: Saturday, June 15, 2024 12:14 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>
> Cc: Li, Yi1 <yi1.li@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; 
> devel@edk2.groups.io; Hou, Wenxing <wenxing.hou@intel.com>; Kinney, 
> Michael D <michael.d.kinney@intel.com>; Pedro Falcato 
> <pedro.falcato@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.yao@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;
}


> 
> >
> > 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.







-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119582): https://edk2.groups.io/g/devel/message/119582
Mute This Topic: https://groups.io/mt/106666288/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-