qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Peter Maydell
Subject: Re: [Qemu-devel] [PATCH] target-arm: add support for v8 SHA1 and SHA256 instructions
Date: Thu, 29 May 2014 17:46:59 +0100

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.

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.

> +    } 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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]