Re: [RFC][PATCH 2/4] AES assembler implementation for x86_64
On Mon, Apr 18, 2005 at 12:34:59PM +0200, Andreas Steinmetz wrote:
> Denis Vlasenko wrote:
>
> > OTOH, if _exactly the same file_ exist in i384 arch, then
> > you should not duplicate it at all. Find a way to use one file
> > for both arches.
I haven't looked at the patches yet, but if it were possible to
share code such as gen_tab then it would be very nice indeed.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[EMAIL PROTECTED]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC][PATCH 2/4] AES assembler implementation for x86_64
Denis Vlasenko wrote:
> On Monday 18 April 2005 12:01, Andreas Steinmetz wrote:
>
>>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 [EMAIL PROTECTED]
>>>
>>>
>>>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
>
>
> "It's a cut-n-paste" is not a good argument here. You
> are adding a _new file_ with your patch, it's okay to clean
> it up while doing this. IOW: do not dup the mess.
>
> OTOH, if _exactly the same file_ exist in i384 arch, then
> you should not duplicate it at all. Find a way to use one file
> for both arches.
>
> Note that this is only my view, I can be wrong.
> --
> vda
>
I'll wait for Herbert Xu's review and his opinion on this.
--
Andreas Steinmetz SPAMmers use [EMAIL PROTECTED]
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC][PATCH 2/4] AES assembler implementation for x86_64
On Monday 18 April 2005 12:01, Andreas Steinmetz wrote:
> 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 [EMAIL PROTECTED]
> >
> >
> > 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
"It's a cut-n-paste" is not a good argument here. You
are adding a _new file_ with your patch, it's okay to clean
it up while doing this. IOW: do not dup the mess.
OTOH, if _exactly the same file_ exist in i384 arch, then
you should not duplicate it at all. Find a way to use one file
for both arches.
Note that this is only my view, I can be wrong.
--
vda
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC][PATCH 2/4] AES assembler implementation for x86_64
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 [EMAIL PROTECTED]
>
>
> 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.
--
Andreas Steinmetz SPAMmers use [EMAIL PROTECTED]
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC][PATCH 2/4] AES assembler implementation for x86_64
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 [EMAIL PROTECTED]
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.
--
Andreas Steinmetz SPAMmers use [EMAIL PROTECTED]
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC][PATCH 2/4] AES assembler implementation for x86_64
On Monday 18 April 2005 12:01, Andreas Steinmetz wrote:
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 [EMAIL PROTECTED]
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
It's a cut-n-paste is not a good argument here. You
are adding a _new file_ with your patch, it's okay to clean
it up while doing this. IOW: do not dup the mess.
OTOH, if _exactly the same file_ exist in i384 arch, then
you should not duplicate it at all. Find a way to use one file
for both arches.
Note that this is only my view, I can be wrong.
--
vda
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC][PATCH 2/4] AES assembler implementation for x86_64
Denis Vlasenko wrote:
On Monday 18 April 2005 12:01, Andreas Steinmetz wrote:
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 [EMAIL PROTECTED]
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
It's a cut-n-paste is not a good argument here. You
are adding a _new file_ with your patch, it's okay to clean
it up while doing this. IOW: do not dup the mess.
OTOH, if _exactly the same file_ exist in i384 arch, then
you should not duplicate it at all. Find a way to use one file
for both arches.
Note that this is only my view, I can be wrong.
--
vda
I'll wait for Herbert Xu's review and his opinion on this.
--
Andreas Steinmetz SPAMmers use [EMAIL PROTECTED]
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC][PATCH 2/4] AES assembler implementation for x86_64
On Mon, Apr 18, 2005 at 12:34:59PM +0200, Andreas Steinmetz wrote:
Denis Vlasenko wrote:
OTOH, if _exactly the same file_ exist in i384 arch, then
you should not duplicate it at all. Find a way to use one file
for both arches.
I haven't looked at the patches yet, but if it were possible to
share code such as gen_tab then it would be very nice indeed.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmVHI~} [EMAIL PROTECTED]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
