Re: [Qemu-devel] [PATCH v2 4/8] crypto: convert xts_tweak_encdec to use xts_uint128 type

[Daniel P . Berrangé]

On Tue, Oct 16, 2018 at 03:09:16PM +0200, Alberto Garcia wrote:
> On Tue 16 Oct 2018 12:09:14 PM CEST, Daniel P. Berrangé wrote:
> 
> > @@ -110,20 +111,34 @@ void xts_decrypt(const void *datactx,
> >      /* encrypt the iv */
> >      encfunc(tweakctx, XTS_BLOCK_SIZE, T.b, iv);
> >  
> > -    for (i = 0; i < lim; i++) {
> > -        xts_tweak_encdec(datactx, decfunc, src, dst, T.b);
> > -
> > -        src += XTS_BLOCK_SIZE;
> > -        dst += XTS_BLOCK_SIZE;
> > +    if (QEMU_PTR_IS_ALIGNED(src, sizeof(uint64_t)) &&
> > +        QEMU_PTR_IS_ALIGNED(dst, sizeof(uint64_t))) {
> > +        xts_uint128 *S = (xts_uint128 *)src;
> > +        xts_uint128 *D = (xts_uint128 *)dst;
> > +        for (i = 0; i < lim; i++, S++, D++) {
> > +            xts_tweak_encdec(datactx, decfunc, S, D, &T);
> > +        }
> > +    } else {
> > +        xts_uint128 S, D;
> > +
> > +        for (i = 0; i < lim; i++) {
> > +            memcpy(&S, src, XTS_BLOCK_SIZE);
> > +            xts_tweak_encdec(datactx, decfunc, &S, &D, &T);
> > +            memcpy(dst, &D, XTS_BLOCK_SIZE);
> > +            src += XTS_BLOCK_SIZE;
> > +            dst += XTS_BLOCK_SIZE;
> > +        }
> 
> The patch looks good to me, but a couple of comments:
> 
> - As far as I can see xts_tweak_encdec() works the same regardless of
>   whether src and dst point to the same address or not. As a matter of
>   fact both qcrypto_block_decrypt() and qcrypto_block_encrypt() do the
>   decryption and encryption in place, and as you can see the
>   qcrypto_cipher_*crypt() calls in crypto/block.c pass the same buffer
>   as input and output.
> 
>   So instead of having S and D you should be fine with just one of them.

Yes, I could do that in the 2nd loop.

> 
> - I think this is just a matter of style preference, but in the first
>   for loop you can remove the comma operator (i++, S++, D++) and use
>   S[i] and D[I] instead in the line after that. I'm fine if you prefer
>   the current style, though.

The syntax I used results in slightly more efficient asm code.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|