Scott, Yes, there are apparent bugs in these IA32 version assembly codes. And looks they were duplicated from X64 codes with simple replacement. And yes, it's me. I only validated X64 platform at that time, and missed any double-check on IA32 codes.
For Intel CPU check, the reference comes from the DRNG software implementation guide (https://software.intel.com/sites/default/files/managed/4d/91/DRNG_Software_Implementation_Guide_2.0.pdf). I am not sure why this kind of checking was still placed in sample code. I am fine to remove those vendor checking if ECX feature flag is enough. Best Regards & Thanks, LONG, Qin -----Original Message----- From: Scott Duplichan [mailto:sc...@notabs.org] Sent: Saturday, June 20, 2015 10:25 AM To: edk2-devel@lists.sourceforge.net Subject: Re: [edk2] [PATCH 1/2] SecurityPkg: Add RandLib from existing RngDxe code El-Haj-Mahmoud, Samer [mailto:samer.el-haj-mahm...@hp.com] wrote: ]Sent: Friday, June 19, 2015 04:12 PM ]To: edk2-devel@lists.sourceforge.net ]Subject: Re: [edk2] [PATCH 1/2] SecurityPkg: Add RandLib from existing RngDxe code ] ]Scott, ] ]The code refactoring the Jonathan did is simply moving the existing code from RngDxe to the new RandLib. He ]did not change any of the existing code, so your comments apply to the existing EDK2 implementation. Hello Samer, OK, I see. I diffed without applying patch 2 of 2. On the other hand, there is that annoying principle of 'you touch it you own it'. Anyway, either I am misreading the code, or the 32-bit build will corrupt memory by writing the return value of asm functions to the wrong address. ]With Jonathan's change, it is possible for others to provide multiple implementations of RandLib to support ]any processors (including non-Intel CPUs that support rdrand, as well as other processors that may not ]necessarily have a similar instruction). RDRAND works like many other x86 instruction set options. A processor reports support for the instruction with CPUID function 1 ecx feature flags. An unnecessary check for vendor=Intel can be viewed as an "Artificial Performance Impairment" to non-Intel processors. The Intel C compiler used to place the same unneeded vendor=Intel check in binaries it generated. On November 11, 2009, Intel agreed to stop doing this: http://download.intel.com/pressroom/legal/AMD_settlement_agreement.pdf As for making an AMD version of this code, I don't think it would get past code review because it would be an exact duplicate of the existing code (except for the vendor=Intel check). EDK2 doesn't check vendor=Intel anywhere else, even though it checks other cupid feature flags. For example, ArchDebugSupport.c determines support for debug extensions using a cpuid feature flag check and no cpuid vendor check. Same for MtrrLib.c when it checks for mtrr support. Thanks, Scott ]Thanks, ]--Samer ------------------------------------------------------------------------------ _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel ------------------------------------------------------------------------------ _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
