On Tuesday 23 December 2008 02:01:38 Huang Ying wrote: > This patch adds support to Intel AES-NI instruction set for x86_64 > platform.
Cool. I'm relying on Andy to provide a more thorough review than my quick scan - I don't do perl-asm :-) In particular, I haven't tried patching and building this. (Andy, let me know if you need any off-platform testing - I presume not, but ...) Quick comment: > Signed-off-by: Huang Ying <[email protected]> > > --- > crypto/engine/Makefile | 11 > crypto/engine/eng_all.c | 3 > crypto/engine/eng_intel.c | 589 ++++++++++++++++++++++++++ > crypto/engine/eng_intel_asm.pl | 918 > +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 1519 > insertions(+), 2 deletions(-) Are you using git to prepare this patch, and if so, which git repo+branch are you tracking? > +#define INTEL_AES_MIN_ALIGN 16 > +#define ALIGN(x,a) (((unsigned long)(x)+(a)-1)&(~((a)-1))) > +#define INTEL_AES_ALIGN(x) ALIGN(x,INTEL_AES_MIN_ALIGN) You don't seem to need the ALIGN() macro anywhere, just INTEL_AES_ALIGN(), so I'd personally prefer it if you didn't use "ALIGN" as this is tempting fate with respect to possible symbol conflicts. Also, if you have no philosophical objection, I think the file and symbol naming should be based on the interface rather than the manufacturer (particularly for "intel", who provide lots of h/w and interfaces that have nothing to do with AES-NI). Perhaps eng_aesni.c rather than eng_intel.c. If it's absolutely certain no other manufacturer will support the same instructions in the future, we could live with eng_intel_aesni.c, but it still needs to be clear that the engine targets the "AES-NI" interface rather than (any) "intel" interface. (I don't want to handle support questions from x86 noobs who presume the "eng_intel.c" engine accelerates any intel cpu ...) As your use of INTEL_AES_ALIGN() was always to cast it to a pointer, please also rephrase the macro to not need casting every time; #define AESNI_MIN_ALIGN 16 #define AESNI_ALIGN(x) \ (void *)(((unsigned long)(x) + AESNI_MIN_ALIGN - 1) & \ (~(unsigned long)(AESNI_MIN_ALIGN - 1))) Finally - did you omit a patch to engine.h? Your changes to eng_all.c include a call to ENGINE_load_intel_aes_ni(), which is in eng_intel.c, but this doen't appear to be declared in any header. Cheers, Geoff -- Un terrien, c'est un singe avec des clefs de char... ______________________________________________________________________ OpenSSL Project http://www.openssl.org Development Mailing List [email protected] Automated List Manager [email protected]
