On 25 March 2014 16:27, Ard Biesheuvel <ard.biesheu...@linaro.org> wrote:
> This adds support for the SHA1 and SHA256 instructions that are available
> on some v8 implementations of Aarch32.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
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 <peter.mayd...@linaro.org>
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
