Re: [Qemu-devel] [PATCH] target-arm: add support for v8 AES instructions

[Ard Biesheuvel]

On 2 December 2013 17:06, Peter Maydell <address@hidden> wrote:
> On 6 November 2013 14:21, Ard Biesheuvel <address@hidden> wrote:
>> This adds support for the AESE/AESD/AESMC/AESIMC instructions that
>> are available on some v8 implementations of Aarch32.
>>
>> Signed-off-by: Ard Biesheuvel <address@hidden>
>
> Hi; thanks for this patch. I have a few minor review comments,
> but it looks pretty good.
>

Thanks.

> (Do you have any plans to do the remaining instructions
> (SHA1, SHA256, VMULL)? Not a requirement, but it would be
> nice to know for planning purposes :-))
>

Unlikely for CRC and poly64 multiply, as they have no use in crypto.
The SHA* instructions perhaps, but this is not planned, and even the
OpenSSL work itself that I am doing is put on hold at the moment so
for planning purposes it is better to assume not. I would be happy to
help out with reviewing, though, if someone else picks this up in the
mean time.

>> ---
>>  target-arm/Makefile.objs   |   1 +
>>  target-arm/cpu.c           |   1 +
>>  target-arm/cpu.h           |   1 +
>>  target-arm/crypto_helper.c | 172 
>> +++++++++++++++++++++++++++++++++++++++++++++
>>  target-arm/helper.h        |   3 +
>>  target-arm/translate.c     |  18 +++++
>>  6 files changed, 196 insertions(+)
>>  create mode 100644 target-arm/crypto_helper.c
>>
>> diff --git a/target-arm/Makefile.objs b/target-arm/Makefile.objs
>> index 2d9f77f..6840f65 100644
>> --- a/target-arm/Makefile.objs
>> +++ b/target-arm/Makefile.objs
>> @@ -5,3 +5,4 @@ obj-$(CONFIG_NO_KVM) += kvm-stub.o
>>  obj-y += translate.o op_helper.o helper.o cpu.o
>>  obj-y += neon_helper.o iwmmxt_helper.o
>>  obj-y += gdbstub.o
>> +obj-y += crypto_helper.o
>> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
>> index f0ed62f..4ba3246 100644
>> --- a/target-arm/cpu.c
>> +++ b/target-arm/cpu.c
>> @@ -169,6 +169,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error 
>> **errp)
>>          set_feature(env, ARM_FEATURE_V7);
>>          set_feature(env, ARM_FEATURE_ARM_DIV);
>>          set_feature(env, ARM_FEATURE_LPAE);
>> +        set_feature(env, ARM_FEATURE_V8_AES);
>>      }
>>      if (arm_feature(env, ARM_FEATURE_V7)) {
>>          set_feature(env, ARM_FEATURE_VAPA);
>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>> index a44d56f..8e4593b 100644
>> --- a/target-arm/cpu.h
>> +++ b/target-arm/cpu.h
>> @@ -395,6 +395,7 @@ enum arm_features {
>>      ARM_FEATURE_LPAE, /* has Large Physical Address Extension */
>>      ARM_FEATURE_V8,
>>      ARM_FEATURE_TRUSTZONE, /* TrustZone Security Extensions. */
>> +    ARM_FEATURE_V8_AES, /* implements AES part of v8 Crypto Extensions */
>
> It looks like you've maybe based this on the wrong tree? I get
> conflicts trying to apply it to qemu upstream master because
> master doesn't have ARM_FEATURE_TRUSTZONE.
>

OK, I will rebase on top of upstream master, I probably used a tree
from some Linaro repo.

>>  };
>>
>>  static inline int arm_feature(CPUARMState *env, int feature)
>> diff --git a/target-arm/crypto_helper.c b/target-arm/crypto_helper.c
>> new file mode 100644
>> index 0000000..f4b633f
>> --- /dev/null
>> +++ b/target-arm/crypto_helper.c
>> @@ -0,0 +1,172 @@
>> +/*
>> + * crypto_helper.c - emulate v8 Crypto Extensions instructions
>> + *
>> + * Copyright (C) 2013 Linaro Ltd <address@hidden>
>> + *
>> + * This library is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU Lesser General Public
>> + * License as published by the Free Software Foundation; either
>> + * version 2 of the License, or (at your option) any later version.
>> + */
>> +
>> +#include <stdlib.h>
>> +
>> +#include "cpu.h"
>> +#include "exec/exec-all.h"
>> +#include "helper.h"
>> +
>> +union AES_STATE {
>> +    uint8_t    bytes[16];
>> +    uint64_t   l[2];
>> +};
>> +
>> +static void add_sub_shift(union AES_STATE *st, union AES_STATE *rk, int 
>> inv);
>> +static void mix_columns(union AES_STATE *out, union AES_STATE *in);
>> +static void inv_mix_columns_pre(union AES_STATE *out);
>> +
>> +void HELPER(crypto_aese)(CPUARMState *env, uint32_t rd, uint32_t rm, 
>> uint32_t e)
>
> "e" is rather cryptic as a parameter name...
>

OK, will change that

>> +{
>> +    union AES_STATE sm = { .l = {
>> +        float64_val(env->vfp.regs[rm]),
>> +        float64_val(env->vfp.regs[rm + 1])
>> +    } };
>> +    union AES_STATE sd = { .l = {
>> +        float64_val(env->vfp.regs[rd]),
>> +        float64_val(env->vfp.regs[rd + 1])
>> +    } };
>> +
>> +    add_sub_shift(&sd, &sm, e);
>> +
>> +    env->vfp.regs[rd] = make_float64(sd.l[0]);
>> +    env->vfp.regs[rd + 1] = make_float64(sd.l[1]);
>> +}
>> +
>> +void HELPER(crypto_aesmc)(CPUARMState *env, uint32_t rd, uint32_t rm, 
>> uint32_t e)
>
> Line over 80 characters (scripts/checkpatch.pl will catch this sort of nit).
> "e" is still cryptic, and even less applicable here :-)
>

OK.

>> +{
>> +    union AES_STATE sd, sm = { .l = {
>> +        float64_val(env->vfp.regs[rm]),
>> +        float64_val(env->vfp.regs[rm + 1])
>> +    } };
[...]
>> +        st->bytes[i] = sbox[inv][rk->bytes[permute[inv][i]]];
>> +    }
>> +}
>
> I'm going to have to take the correctness of the crypto magic
> on trust :-)
>
> (I should be able to do a cross-test against a reference
> implementation later this week I think.)
>

Please do. I know Steve Capper has the Fast Model plugin, and he has
tested my kernel version before, but it would be good to check
explicitly against the Aarch32 versions of the instructions.

>> diff --git a/target-arm/helper.h b/target-arm/helper.h
>> index 63ae13a..a42f550 100644
>> --- a/target-arm/helper.h
>> +++ b/target-arm/helper.h
>> @@ -458,4 +458,7 @@ DEF_HELPER_3(neon_qzip8, void, env, i32, i32)
>>  DEF_HELPER_3(neon_qzip16, void, env, i32, i32)
>>  DEF_HELPER_3(neon_qzip32, void, env, i32, i32)
>>
>> +DEF_HELPER_4(crypto_aese, void, env, i32, i32, i32)
>> +DEF_HELPER_4(crypto_aesmc, void, env, i32, i32, i32)
>> +
>>  #include "exec/def-helper.h"
>> diff --git a/target-arm/translate.c b/target-arm/translate.c
>> index 9da4ea1..c147491 100644
>> --- a/target-arm/translate.c
>> +++ b/target-arm/translate.c
>> @@ -4346,6 +4346,8 @@ static const uint8_t neon_3r_sizes[] = {
>>  #define NEON_2RM_VREV16 2
>>  #define NEON_2RM_VPADDL 4
>>  #define NEON_2RM_VPADDL_U 5
>> +#define NEON_2RM_AESE 6 /* Includes AESD */
>> +#define NEON_2RM_AESMC 7 /* Includes AESIMC */
>>  #define NEON_2RM_VCLS 8
>>  #define NEON_2RM_VCLZ 9
>>  #define NEON_2RM_VCNT 10
>> @@ -4403,6 +4405,8 @@ static const uint8_t neon_2rm_sizes[] = {
>>      [NEON_2RM_VREV16] = 0x1,
>>      [NEON_2RM_VPADDL] = 0x7,
>>      [NEON_2RM_VPADDL_U] = 0x7,
>> +    [NEON_2RM_AESE] = 0x1,
>> +    [NEON_2RM_AESMC] = 0x1,
>>      [NEON_2RM_VCLS] = 0x7,
>>      [NEON_2RM_VCLZ] = 0x7,
>>      [NEON_2RM_VCNT] = 0x1,
>> @@ -5925,6 +5929,20 @@ static int disas_neon_data_insn(CPUARMState * env, 
>> DisasContext *s, uint32_t ins
>>                      tcg_temp_free_i32(tmp2);
>>                      tcg_temp_free_i32(tmp3);
>>                      break;
>> +                case NEON_2RM_AESE: case NEON_2RM_AESMC:
>> +                    if (!arm_feature(env, ARM_FEATURE_V8_AES)) {
>
> This needs an "|| ((rm | rd) & 1)"  clause adding, I think
> (pseudocode says that UNDEFs).
>
>> +                        return 1;
>> +                    }
>> +                    tmp = tcg_const_i32(rd);
>> +                    tmp2 = tcg_const_i32(rm);
>> +                    tmp3 = tcg_const_i32((insn >> 6) & 1);
>
> tmp3 = tcg_const_i32(extract32(insn, 6, 1));
> (extract32() is fairly new so a lot of existing code does by-hand
> bit manipulation, but I'm trying to encourage use of it in new code.)
>
> We could also use a comment to save people wading through four
> different parts of the ARM ARM:
>  /* Bit 6 is the lowest opcode bit; it distinguishes AESE from AESD
>   * and AESIMC from AESMC
>   */
>

OK.

>> +
>> +                    if (op == NEON_2RM_AESE) {
>> +                        gen_helper_crypto_aese(cpu_env, tmp, tmp2, tmp3);
>> +                    } else {
>> +                        gen_helper_crypto_aesmc(cpu_env, tmp, tmp2, tmp3);
>> +                    }
>
> You need
>    tcg_free_temp_i32(tmp1);
>    tcg_free_temp_i32(tmp2);
>    tcg_free_temp_i32(tmp3);
>
> here, or we will leak TCG temps.
>

OK

>> +                    break;
>>                  default:
>>                  elementwise:
>>                      for (pass = 0; pass < (q ? 4 : 2); pass++) {
>> --
>> 1.8.3.2
>>
>

I will post an updated patch by the end of the week.

Regards,
Ard.