On Tue, 2008-12-23 at 23:36 +0800, Geoff Thorpe wrote: > 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?
I use OpenSSL cvs to track upstream version. And I use quilt to prepare the patch. > > +#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. OK. I will fix it. > 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 ...) I will rename the names to aesni. > 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))) I will do this. > 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. I will add it. Best Regards, Huang Ying
signature.asc
Description: This is a digitally signed message part
