Re: [Qemu-devel] [PATCH v4 3/4] target/arm: implement SM3 instructions

[Peter Maydell]

On 19 January 2018 at 18:22, Ard Biesheuvel <address@hidden> wrote:
> This implements emulation of the new SM3 instructions that have
> been added as an optional extension to the ARMv8 Crypto Extensions
> in ARM v8.2.
>
> Signed-off-by: Ard Biesheuvel <address@hidden>


> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index 787b94047286..1e3ff9a6152f 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -11148,28 +11148,39 @@ static void 
> disas_crypto_three_reg_sha512(DisasContext *s, uint32_t insn)
>      CryptoThreeOpFn *genfn;
>      int feature;
>
> -    if (o != 0) {
> -        unallocated_encoding(s);
> -        return;
> -    }

If you wrote this code in the first patch in the
     if (o == 0) {
         stuff;
     } else {
         unallocated_encoding(s);
         return;
     }

form, it would make this patch easier to read as it wouldn't
need to change the lines for the SHA512 cases.

> -
> -    switch (opcode) {
> -    case 0: /* SHA512H */
> -        feature = ARM_FEATURE_V8_SHA512;
> -        genfn = gen_helper_crypto_sha512h;
> -        break;
> -    case 1: /* SHA512H2 */
> -        feature = ARM_FEATURE_V8_SHA512;
> -        genfn = gen_helper_crypto_sha512h2;
> -        break;
> -    case 2: /* SHA512SU1 */
> -        feature = ARM_FEATURE_V8_SHA512;
> -        genfn = gen_helper_crypto_sha512su1;
> -        break;
> -    case 3: /* RAX1 */
> -        feature = ARM_FEATURE_V8_SHA3;
> -        genfn = NULL;
> -        break;
> +    if (o == 0) {
> +        switch (opcode) {
> +        case 0: /* SHA512H */
> +            feature = ARM_FEATURE_V8_SHA512;
> +            genfn = gen_helper_crypto_sha512h;
> +            break;
> +        case 1: /* SHA512H2 */
> +            feature = ARM_FEATURE_V8_SHA512;
> +            genfn = gen_helper_crypto_sha512h2;
> +            break;
> +        case 2: /* SHA512SU1 */
> +            feature = ARM_FEATURE_V8_SHA512;
> +            genfn = gen_helper_crypto_sha512su1;
> +            break;
> +        case 3: /* RAX1 */
> +            feature = ARM_FEATURE_V8_SHA3;
> +            genfn = NULL;
> +            break;
> +        }
> +    } else {
> +        switch (opcode) {
> +        case 0: /* SM3PARTW1 */
> +            feature = ARM_FEATURE_V8_SM3;
> +            genfn = gen_helper_crypto_sm3partw1;
> +            break;
> +        case 1: /* SM3PARTW2 */
> +            feature = ARM_FEATURE_V8_SM3;
> +            genfn = gen_helper_crypto_sm3partw2;
> +            break;
> +        default:
> +            unallocated_encoding(s);
> +            return;
> +        }

This seems to be missing support for SM4EKEY (which is O==1
opcode == 0b10 and also part of the v8.2 SM feature) ?


>      }
>
>      if (!arm_dc_feature(s, feature)) {
> @@ -11273,10 +11284,22 @@ static void disas_crypto_four_reg(DisasContext *s, 
> uint32_t insn)
>      int ra = extract32(insn, 10, 5);
>      int rn = extract32(insn, 5, 5);
>      int rd = extract32(insn, 0, 5);
> -    TCGv_i64 tcg_op1, tcg_op2, tcg_op3, tcg_res[2];
> -    int pass;
> +    int feature;
>
> -    if (op0 > 1 || !arm_dc_feature(s, ARM_FEATURE_V8_SHA3)) {
> +    switch (op0) {
> +    case 0: /* EOR3 */
> +    case 1: /* BCAX */
> +        feature = ARM_FEATURE_V8_SHA3;
> +        break;
> +    case 2: /* SM3SS1 */
> +        feature = ARM_FEATURE_V8_SM3;
> +        break;
> +    default:
> +        unallocated_encoding(s);
> +        return;
> +    }
> +
> +    if (!arm_dc_feature(s, feature)) {
>          unallocated_encoding(s);
>          return;
>      }
> @@ -11285,34 +11308,54 @@ static void disas_crypto_four_reg(DisasContext *s, 
> uint32_t insn)
>          return;
>      }
>
> -    tcg_op1 = tcg_temp_new_i64();
> -    tcg_op2 = tcg_temp_new_i64();
> -    tcg_op3 = tcg_temp_new_i64();
> -    tcg_res[0] = tcg_temp_new_i64();
> -    tcg_res[1] = tcg_temp_new_i64();
> +    if (op0 == 2) {
> +        TCGv_ptr tcg_rd_ptr, tcg_rn_ptr, tcg_ra_ptr, tcg_rm_ptr;
>
> -    for (pass = 0; pass < 2; pass++) {
> -        read_vec_element(s, tcg_op1, rn, pass, MO_64);
> -        read_vec_element(s, tcg_op2, rm, pass, MO_64);
> -        read_vec_element(s, tcg_op3, ra, pass, MO_64);
> +        tcg_rd_ptr = vec_full_reg_ptr(s, rd);
> +        tcg_rn_ptr = vec_full_reg_ptr(s, rn);
> +        tcg_ra_ptr = vec_full_reg_ptr(s, ra);
> +        tcg_rm_ptr = vec_full_reg_ptr(s, rm);

Similarly this part of this patch is a pain to read, and you
could avoid that by making the patch that introduces the function
structure things so this patch doesn't need do then reformat them.


thanks
-- PMM