Re: [Qemu-devel] [PATCH v3 07/10] target/arm: Introduce ARM_FEATURE_V8_ATOMICS and initial decode

[Peter Maydell]

On 8 May 2018 at 16:14, Richard Henderson <address@hidden> wrote:
> The insns in the ARMv8.1-Atomics are added to the existing
> load/store exclusive and load/store reg opcode spaces.
> Rearrange the top-level decoders for these to accomodate.
> The Atomics insns themselves still generate Unallocated.
>
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  target/arm/cpu.h           |   2 +
>  linux-user/elfload.c       |   1 +
>  target/arm/translate-a64.c | 182 +++++++++++++++++++++++++++----------
>  3 files changed, 139 insertions(+), 46 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 44e6b77151..f71a78d908 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -1449,6 +1449,8 @@ enum arm_features {
>      ARM_FEATURE_V8_SHA3, /* implements SHA3 part of v8 Crypto Extensions */
>      ARM_FEATURE_V8_SM3, /* implements SM3 part of v8 Crypto Extensions */
>      ARM_FEATURE_V8_SM4, /* implements SM4 part of v8 Crypto Extensions */
> +    ARM_FEATURE_V8_1,
> +    ARM_FEATURE_V8_ATOMICS = ARM_FEATURE_V8_1, /* mandatory extension */

This isn't the way we handle "this bit of the spec is mandatory
for architecture version X" for existing features. We have
a feature bit for the arch version X, and a feature bit for
the portion of the spec, and arm_cpu_realizefn() sets the
spec-portion feature bit if the version-X bit is set.
Doing it the way you have here means we can't model a
v8.0 CPU that also has the atomics extension but not the rest
of v8.1. (A v8.x implementation can include any arbitrary
subset of the v8.(x+1) features, so this is a legal combination.)

In fact we already have a mandatory-in-v8.1 feature:
ARM_FEATURE_V8_RDM, which we've just given its own feature bit.

I can apply this to target-arm.next with this change squashed in:

--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1449,8 +1449,7 @@ enum arm_features {
     ARM_FEATURE_V8_SHA3, /* implements SHA3 part of v8 Crypto Extensions */
     ARM_FEATURE_V8_SM3, /* implements SM3 part of v8 Crypto Extensions */
     ARM_FEATURE_V8_SM4, /* implements SM4 part of v8 Crypto Extensions */
-    ARM_FEATURE_V8_1,
-    ARM_FEATURE_V8_ATOMICS = ARM_FEATURE_V8_1, /* mandatory extension */
+    ARM_FEATURE_V8_ATOMICS, /* ARMv8.1-Atomics feature */
     ARM_FEATURE_V8_RDM, /* implements v8.1 simd round multiply */
     ARM_FEATURE_V8_FP16, /* implements v8.2 half-precision float */
     ARM_FEATURE_V8_FCMA, /* has complex number part of v8.3 extensions.  */

Is that OK?


We can have a separate patch which adds ARM_FEATURE_V8_1 and the
code in realizefn to do
    if (arm_feature(env, ARM_FEATURE_V8_1)) {
        set_feature(env, ARM_FEATURE_V8);
        set_feature(env, ARM_FEATURE_V8_ATOMICS);
        set_feature(env, ARM_FEATURE_V8_RDM);
    }

thanks
-- PMM