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 <ying.hu...@intel.com>
>
> ---
> 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 openssl-dev@openssl.org
Automated List Manager majord...@openssl.org
