Re: [RFC][PATCH 2/4] AES assembler implementation for x86_64

Andreas Steinmetz
2005-04-18T09:02:59Z

Denis Vlasenko wrote:
> On Sunday 17 April 2005 22:20, Andreas Steinmetz wrote:
>
>>The attached patch contains Gladman's in-kernel code for key schedule
>>and table generation modified to fit to my assembler implementation,
>>--
>>Andreas Steinmetz SPAMmers use robotrap@domdv.de
>
>
> Patch contains a mix of several coding styles:
>
> +/*
> + * #define byte(x, nr) ((unsigned char)((x) >> (nr*8)))
> + */
> +inline static u8
> +byte(const u32 x, const unsigned n)
> +{
> + return x >> (n << 3);
> +}
>
> what does const do here?

Taken 'as is' from current kernel sources, i,e, crypto/aes.c

>
> +static inline u32 ror32(u32 word, unsigned int shift)
> +{
> + return (word >> shift) | (word << (32 - shift));
> +}
> +
> +static inline u8 __init
> +f_mult (u8 a, u8 b)
> +{
> + u8 aa = log_tab[a], cc = aa + log_tab[b];
> +
> + return pow_tab[cc + (cc < aa ? 1 : 0)];
> +}
>
> Can you stick to either
> type f()
> or
> type
> f()
> style, but not both at once?

As above.

>
> +#define ls_box(x) \
> + ( aes_fl_tab[0][byte(x, 0)] ^ \
> + aes_fl_tab[1][byte(x, 1)] ^ \
> + aes_fl_tab[2][byte(x, 2)] ^ \
> + aes_fl_tab[3][byte(x, 3)] )
>
> +#define star_x(x) (((x) & 0x7f7f7f7f) << 1) ^ ((((x) & 0x80808080) >> 7) * 0x1b)
>
> You used inlines for complex function-like calls above, why not here?

As above.

>
> +#define imix_col(y,x) \
> + u = star_x(x); \
> + v = star_x(u); \
> + w = star_x(v); \
> + t = w ^ (x); \
> + (y) = u ^ v ^ w; \
> + (y) ^= ror32(u ^ t, 8) ^ \
> + ror32(v ^ t, 16) ^ \
> + ror32(t,24)
>
> this #define is bad, bad, BAD. Imagine: if(...) imix_col(a,b);
> Also I'm not sure that usage of "hidden" params (u,v,w,t) is ok.

As above.

> --
> vda
>

The thing is I didn't want to modify the existing source code of
crpto/aes.c except where necessary.