Yeah, I think it's good. Move first, and fix later. :-)
Best Regards & Thanks, LONG, Qin -----Original Message----- From: El-Haj-Mahmoud, Samer [mailto:samer.el-haj-mahm...@hp.com] Sent: Tuesday, June 23, 2015 1:07 AM To: edk2-devel@lists.sourceforge.net Subject: Re: [edk2] [PATCH 1/2] SecurityPkg: Add RandLib from existing RngDxe code Thanks Qin and Scott. Since Jonathan's task is based on the existing EDK2 code, and involves code refactoring, and moving pieces of code from the DXE Driver to a library, I propose we proceed with that without making any modifications to the code. Once that patch is approved and checked in, we can then submit additional individual patches to address your feedback (removing the Intel vendor check, and fixing the IA32 assembly), as well as Mike's (moving rdrand to BaseLib, and using TimerLib for the timeout). Thanks, --Samer -----Original Message----- From: Scott Duplichan [mailto:sc...@notabs.org] Sent: Sunday, June 21, 2015 7:59 PM To: edk2-devel@lists.sourceforge.net Subject: Re: [edk2] [PATCH 1/2] SecurityPkg: Add RandLib from existing RngDxe code Long, Qin [mailto:qin.l...@intel.com] wrote: ]Sent: Saturday, June 20, 2015 01:45 PM ]To: edk2-devel@lists.sourceforge.net ]Subject: Re: [edk2] [PATCH 1/2] SecurityPkg: Add RandLib from existing RngDxe code ] ]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. No problem, thanks for explaining. That is what I suspected. ]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. Sure enough. I see it. While it looks like the Intel reference code is pretty well written overall, I would vote for removing the vendor check. >From what I can find, AMD family 16h processors are the only non-Intel processors that set bit 30 of the cupid function1 feature flags. AMD documents bit 30 as RDRAND instruction support, exactly as Intel does. If there is any incompatibility with AMD's implementation, it will be up to AMD engineers to fix it. Thanks, Scott ]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 ------------------------------------------------------------------------------ Monitor 25 network devices or servers for free with OpManager! OpManager is web-based network management software that monitors network devices and physical & virtual servers, alerts via email & sms for fault. Monitor 25 devices for free with no restriction. Download now http://ad.doubleclick.net/ddm/clk/292181274;119417398;o _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel ------------------------------------------------------------------------------ Monitor 25 network devices or servers for free with OpManager! OpManager is web-based network management software that monitors network devices and physical & virtual servers, alerts via email & sms for fault. Monitor 25 devices for free with no restriction. Download now http://ad.doubleclick.net/ddm/clk/292181274;119417398;o _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel ------------------------------------------------------------------------------ Monitor 25 network devices or servers for free with OpManager! OpManager is web-based network management software that monitors network devices and physical & virtual servers, alerts via email & sms for fault. Monitor 25 devices for free with no restriction. Download now http://ad.doubleclick.net/ddm/clk/292181274;119417398;o _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
