Re: [Qemu-devel] [PATCH] target-arm: add support for v8 SHA1 and SHA256 instructions

[Ard Biesheuvel]

On 29 May 2014 18:46, Peter Maydell <address@hidden> wrote:
> On 25 March 2014 16:27, Ard Biesheuvel <address@hidden> wrote:
>> This adds support for the SHA1 and SHA256 instructions that are available
>> on some v8 implementations of Aarch32.
>>
>> Signed-off-by: Ard Biesheuvel <address@hidden>
>
> Apologies for the incredibly late review; I was hugely busy
> back in March and then this slipped through the cracks and
> I forgot I needed to review it.
>

No worries, my work is not blocked by it.

> I have a few nits below, but nothing major. I've tested this
> patch (comparison vs hardware) and apart from the missing
> UNDEF for Q!=1 check I mention below it is good.
>
> At the bottom of my review comments I've put the diff
> which I'm planning to squash into this patch; I'll send
> out that as a v2 of this patch, to save you doing the
> rebase and minor fixes yourself.
>

Sounds good. My primary contribution is the core SHA code, and how it
should integrate into QEMU is entirely up to you, so please apply
whatever change is required to make it suitable for inclusion.

Thanks,
Ard.





>> +    } else {
>> +        int i;
>> +
>> +        for (i = 0; i < 4; i++) {
>> +                uint32_t t;
>
> Bad indent.
>
>> +
>> +                switch (op) {
>> +                default:
>> +                        /* not reached */
>
> g_assert_not_reached();
>
>> @@ -4955,6 +4961,46 @@ static int disas_neon_data_insn(CPUARMState * env, 
>> DisasContext *s, uint32_t ins
>>          if (q && ((rd | rn | rm) & 1)) {
>>              return 1;
>>          }
>> +        /*
>> +         * The SHA-1/SHA-256 3-register instructions require special 
>> treatment
>> +         * here, as their size field is overloaded as an op type selector, 
>> and
>> +         * they all consume their input in a single pass.
>> +         */
>> +        if (op == NEON_3R_SHA) {
>
> Missing UNDEF case:
>     if (!q) {
>         return 1;
>     }
>
>> +                case NEON_2RM_SHA1SU1:
>> +                    if ((rm | rd) & 1) {
>> +                            return 1;
>> +                    }
>> +                    /* bit 6: set -> SHA256SU0, cleared -> SHA1SU1 */
>> +                    if (extract32(insn, 6, 1)) {
>
> This is 'q', you don't need to re-extract it.
>
> Otherwise
> Reviewed-by: Peter Maydell <address@hidden>
>
> Below is the diff I'm going to squash into this patch:
>
> diff --git a/target-arm/crypto_helper.c b/target-arm/crypto_helper.c
> index 4619dde..3e4b5f7 100644
> --- a/target-arm/crypto_helper.c
> +++ b/target-arm/crypto_helper.c
> @@ -322,28 +322,28 @@ void HELPER(crypto_sha1_3reg)(CPUARMState *env,
> uint32_t rd, uint32_t rn,
>          int i;
>
>          for (i = 0; i < 4; i++) {
> -                uint32_t t;
> -
> -                switch (op) {
> -                default:
> -                        /* not reached */
> -                case 0: /* sha1c */
> -                    t = cho(d.words[1], d.words[2], d.words[3]);
> -                    break;
> -                case 1: /* sha1p */
> -                    t = par(d.words[1], d.words[2], d.words[3]);
> -                    break;
> -                case 2: /* sha1m */
> -                    t = maj(d.words[1], d.words[2], d.words[3]);
> -                    break;
> -                }
> -                t += rol32(d.words[0], 5) + n.words[0] + m.words[i];
> -
> -                n.words[0] = d.words[3];
> -                d.words[3] = d.words[2];
> -                d.words[2] = ror32(d.words[1], 2);
> -                d.words[1] = d.words[0];
> -                d.words[0] = t;
> +            uint32_t t;
> +
> +            switch (op) {
> +            case 0: /* sha1c */
> +                t = cho(d.words[1], d.words[2], d.words[3]);
> +                break;
> +            case 1: /* sha1p */
> +                t = par(d.words[1], d.words[2], d.words[3]);
> +                break;
> +            case 2: /* sha1m */
> +                t = maj(d.words[1], d.words[2], d.words[3]);
> +                break;
> +            default:
> +                g_assert_not_reached();
> +            }
> +            t += rol32(d.words[0], 5) + n.words[0] + m.words[i];
> +
> +            n.words[0] = d.words[3];
> +            d.words[3] = d.words[2];
> +            d.words[2] = ror32(d.words[1], 2);
> +            d.words[1] = d.words[0];
> +            d.words[0] = t;
>          }
>      }
>      env->vfp.regs[rd] = make_float64(d.l[0]);
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index d5ee7a1..ab3713d 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -5022,6 +5022,9 @@ static int disas_neon_data_insn(CPUARMState *
> env, DisasContext *s, uint32_t ins
>           * they all consume their input in a single pass.
>           */
>          if (op == NEON_3R_SHA) {
> +            if (!q) {
> +                return 1;
> +            }
>              if (!u) { /* SHA-1 */
>                  if (!arm_feature(env, ARM_FEATURE_V8_SHA1)) {
>                      return 1;
> @@ -6548,8 +6551,8 @@ static int disas_neon_data_insn(CPUARMState *
> env, DisasContext *s, uint32_t ins
>                      if ((rm | rd) & 1) {
>                              return 1;
>                      }
> -                    /* bit 6: set -> SHA256SU0, cleared -> SHA1SU1 */
> -                    if (extract32(insn, 6, 1)) {
> +                    /* bit 6 (q): set -> SHA256SU0, cleared -> SHA1SU1 */
> +                    if (q) {
>                          if (!arm_feature(env, ARM_FEATURE_V8_SHA256)) {
>                              return 1;
>                          }
> @@ -6558,7 +6561,7 @@ static int disas_neon_data_insn(CPUARMState *
> env, DisasContext *s, uint32_t ins
>                      }
>                      tmp = tcg_const_i32(rd);
>                      tmp2 = tcg_const_i32(rm);
> -                    if (extract32(insn, 6, 1)) {
> +                    if (q) {
>                          gen_helper_crypto_sha256su0(cpu_env, tmp, tmp2);
>                      } else {
>                          gen_helper_crypto_sha1su1(cpu_env, tmp, tmp2);
>
> thanks
> -- PMM