> 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....@intel.com> Sent: Saturday, June 15, 2024 12:45 AM To: Ard Biesheuvel <a...@kernel.org> 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 > -----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; } > > > > > 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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
