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

On 6 November 2013 11:22, Ard Biesheuvel ard.biesheuvel@linaro.org wrote:

This adds support for the AESE/AESD/AESMC/AESIMC instructions that are available on some v8 implementations of Aarch32.

Thanks for this patch. Please could you send QEMU patches to qemu-devel@nongnu.org? (we do all our work on upstream qemu and in the upstream mailing list). You might also like to read http://wiki.qemu.org/Contribute/SubmitAPatch which discusses some minor issues of formatting and so on (for instance it tells you about the coding style checking script, which would tell you about some minor errors in your patch).

I'll do a first pass review here but you should definitely send subsequent versions to the qemu list.

How much testing have you done on this patch?

• • This program is free software; you can redistribute it and/or modify
• • it under the terms of the GNU General Public License version 2 as
• • published by the Free Software Foundation.

Upstream prefers 2-or-later.

+static void mix_columns(union AES_STATE *out, union AES_STATE *in) +{

• int i;
• for (i = 0; i < 16; i++)

•    out->bytes[i] =
  

•        gf8_mul_x(in->bytes[i]) ^
  

•        gf8_mul_x(in->bytes[((i + 1) % 4) | (i & ~3)]) ^
  

•            in->bytes[((i + 1) % 4) | (i & ~3)] ^
  

•            in->bytes[((i + 2) % 4) | (i & ~3)] ^
  

•            in->bytes[((i + 3) % 4) | (i & ~3)];
  

Coding style demands braces on for() and if() statements even if the body is a single statement (checkpatch.pl will tell you this).

--- 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,24 @@ 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:
  

You need a check here: if (!ENABLE_ARCH_8) { return 1; }

so we don't decode these instructions on cores where they don't exist.

•                tmp = tcg_const_i32(rd);
  

•                tmp2 = tcg_const_i32(rm);
  

•                if (insn & (1 << 6))
  

•                    gen_helper_crypto_aesd(cpu_env, tmp, tmp2);
  

•                else
  

•                    gen_helper_crypto_aese(cpu_env, tmp, tmp2);
  

•                break;
  

•            case NEON_2RM_AESMC:
  

•                tmp = tcg_const_i32(rd);
  

•                tmp2 = tcg_const_i32(rm);
  

•                if (insn & (1 << 6))
  

•                    gen_helper_crypto_aesimc(cpu_env, tmp, tmp2);
  

•                else
  

•                    gen_helper_crypto_aesmc(cpu_env, tmp, tmp2);
  

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

thanks -- PMM