[Qemu-devel] [PATCH v3 0/5] target/arm: KVM vs ARMISARegisters

[Qemu-devel] [PATCH v3 0/5] target/arm: KVM vs ARMISARegisters

[Richard Henderson]

My previous patch set for replacing feature bits with id registers
failed to consider that these id registers are beginning to control
migration, and thus we must fill them in for KVM as well.

Thus, we want to initialize these values within CPU from the host.

Finally, re-send the T32EE conversion patch, fixing the build
failure on an arm32 host in kvm32.c.

Changes, v2->v3:
  * Work around sysreg read failures from old host kernels.

Changes, v1->v2:
  * Remove assert that AArch32 sysreg <= UINT32_MAX.
  * Remove unused local variable.
  * Add commentary for AArch32 sysregs vs missing AArch32 support.


r~


Richard Henderson (5):
  target/arm: Install ARMISARegisters from kvm host
  target/arm: Fill in ARMISARegisters for kvm64
  target/arm: Introduce read_sys_reg32 for kvm32
  target/arm: Fill in ARMISARegisters for kvm32
  target/arm: Convert t32ee from feature bit to isar3 test

 target/arm/cpu.h     |  6 ++-
 target/arm/kvm_arm.h |  1 +
 linux-user/elfload.c |  2 +-
 target/arm/cpu.c     |  4 --
 target/arm/helper.c  |  2 +-
 target/arm/kvm.c     |  1 +
 target/arm/kvm32.c   | 75 ++++++++++++++++++++----------------
 target/arm/kvm64.c   | 90 +++++++++++++++++++++++++++++++++++++++++++-
 target/arm/machine.c |  3 +-
 9 files changed, 141 insertions(+), 43 deletions(-)

-- 
2.17.2

[Qemu-devel] [PATCH v3 1/5] target/arm: Install ARMISARegisters from kvm host

[Richard Henderson]

The ID registers are replacing (some of) the feature bits.
We need (some of) these values to determine the set of data
to be handled during migration.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/kvm_arm.h | 1 +
 target/arm/kvm.c     | 1 +
 2 files changed, 2 insertions(+)

diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
index 21c0129da2..6393455b1d 100644
--- a/target/arm/kvm_arm.h
+++ b/target/arm/kvm_arm.h
@@ -183,6 +183,7 @@ void kvm_arm_destroy_scratch_host_vcpu(int *fdarray);
  * by asking the host kernel)
  */
 typedef struct ARMHostCPUFeatures {
+    ARMISARegisters isar;
     uint64_t features;
     uint32_t target;
     const char *dtb_compatible;
diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index 09a86e2820..44dd0ce6ce 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -158,6 +158,7 @@ void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu)
 
     cpu->kvm_target = arm_host_cpu_features.target;
     cpu->dtb_compatible = arm_host_cpu_features.dtb_compatible;
+    cpu->isar = arm_host_cpu_features.isar;
     env->features = arm_host_cpu_features.features;
 }
 
-- 
2.17.2

[Qemu-devel] [PATCH v3 2/5] target/arm: Fill in ARMISARegisters for kvm64

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/kvm64.c | 90 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 88 insertions(+), 2 deletions(-)

diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index 5de8ff0ac5..e1128b94b2 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -443,17 +443,40 @@ static inline void unset_feature(uint64_t *features, int feature)
     *features &= ~(1ULL << feature);
 }
 
+static int read_sys_reg32(int fd, uint32_t *pret, uint64_t id)
+{
+    uint64_t ret;
+    struct kvm_one_reg idreg = { .id = id, .addr = (uintptr_t)&ret };
+    int err;
+
+    assert((id & KVM_REG_SIZE_MASK) == KVM_REG_SIZE_U64);
+    err = ioctl(fd, KVM_GET_ONE_REG, &idreg);
+    if (err < 0) {
+        return -1;
+    }
+    *pret = ret;
+    return 0;
+}
+
+static int read_sys_reg64(int fd, uint64_t *pret, uint64_t id)
+{
+    struct kvm_one_reg idreg = { .id = id, .addr = (uintptr_t)pret };
+
+    assert((id & KVM_REG_SIZE_MASK) == KVM_REG_SIZE_U64);
+    return ioctl(fd, KVM_GET_ONE_REG, &idreg);
+}
+
 bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
 {
     /* Identify the feature bits corresponding to the host CPU, and
      * fill out the ARMHostCPUClass fields accordingly. To do this
      * we have to create a scratch VM, create a single CPU inside it,
      * and then query that CPU for the relevant ID registers.
-     * For AArch64 we currently don't care about ID registers at
-     * all; we just want to know the CPU type.
      */
     int fdarray[3];
     uint64_t features = 0;
+    int err;
+
     /* Old kernels may not know about the PREFERRED_TARGET ioctl: however
      * we know these will only support creating one kind of guest CPU,
      * which is its preferred CPU type. Fortunately these old kernels
@@ -474,8 +497,71 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
     ahcf->target = init.target;
     ahcf->dtb_compatible = "arm,arm-v8";
 
+    err = read_sys_reg64(fdarray[2], &ahcf->isar.id_aa64pfr0,
+                         ARM64_SYS_REG(3, 0, 0, 4, 0));
+    if (unlikely(err < 0)) {
+        /*
+         * Before v4.15, the kernel only exposed a limited number of system
+         * registers, not including any of the interesting AArch64 ID regs.
+         * For the most part we could leave these fields as zero with minimal
+         * effect, since this does not affect the values seen by the guest.
+         *
+         * However, it could cause problems down the line for QEMU,
+         * so provide a minimal v8.0 default.
+         *
+         * ??? Could read MIDR and use knowledge from cpu64.c.
+         * ??? Could map a page of memory into our temp guest and
+         *     run the tiniest of hand-crafted kernels to extract
+         *     the values seen by the guest.
+         * ??? Either of these sounds like too much effort just
+         *     to work around running a modern host kernel.
+         */
+        ahcf->isar.id_aa64pfr0 = 0x00000011; /* EL1&0, AArch64 only */
+        err = 0;
+    } else {
+        err |= read_sys_reg64(fdarray[2], &ahcf->isar.id_aa64pfr1,
+                              ARM64_SYS_REG(3, 0, 0, 4, 1));
+        err |= read_sys_reg64(fdarray[2], &ahcf->isar.id_aa64isar0,
+                              ARM64_SYS_REG(3, 0, 0, 6, 0));
+        err |= read_sys_reg64(fdarray[2], &ahcf->isar.id_aa64isar1,
+                              ARM64_SYS_REG(3, 0, 0, 6, 1));
+
+        /*
+         * Note that if AArch32 support is not present in the host,
+         * the AArch32 sysregs are present to be read, but will
+         * return UNKNOWN values.  This is neither better nor worse
+         * than skipping the reads and leaving 0, as we must avoid
+         * considering the values in every case.
+         */
+        err |= read_sys_reg32(fdarray[2], &ahcf->isar.id_isar0,
+                              ARM64_SYS_REG(3, 0, 0, 2, 0));
+        err |= read_sys_reg32(fdarray[2], &ahcf->isar.id_isar1,
+                              ARM64_SYS_REG(3, 0, 0, 2, 1));
+        err |= read_sys_reg32(fdarray[2], &ahcf->isar.id_isar2,
+                              ARM64_SYS_REG(3, 0, 0, 2, 2));
+        err |= read_sys_reg32(fdarray[2], &ahcf->isar.id_isar3,
+                              ARM64_SYS_REG(3, 0, 0, 2, 3));
+        err |= read_sys_reg32(fdarray[2], &ahcf->isar.id_isar4,
+                              ARM64_SYS_REG(3, 0, 0, 2, 4));
+        err |= read_sys_reg32(fdarray[2], &ahcf->isar.id_isar5,
+                              ARM64_SYS_REG(3, 0, 0, 2, 5));
+        err |= read_sys_reg32(fdarray[2], &ahcf->isar.id_isar6,
+                              ARM64_SYS_REG(3, 0, 0, 2, 7));
+
+        err |= read_sys_reg32(fdarray[2], &ahcf->isar.mvfr0,
+                              ARM64_SYS_REG(3, 0, 0, 3, 0));
+        err |= read_sys_reg32(fdarray[2], &ahcf->isar.mvfr1,
+                              ARM64_SYS_REG(3, 0, 0, 3, 1));
+        err |= read_sys_reg32(fdarray[2], &ahcf->isar.mvfr2,
+                              ARM64_SYS_REG(3, 0, 0, 3, 2));
+    }
+
     kvm_arm_destroy_scratch_host_vcpu(fdarray);
 
+    if (err < 0) {
+        return false;
+    }
+
    /* We can assume any KVM supporting CPU is at least a v8
      * with VFPv4+Neon; this in turn implies most of the other
      * feature bits.
-- 
2.17.2

Re: [Qemu-devel] [PATCH v3 2/5] target/arm: Fill in ARMISARegisters for kvm64

[Peter Maydell]

On 8 November 2018 at 17:52, Richard Henderson
<richard.henderson@linaro.org> wrote:
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---

>      /* Old kernels may not know about the PREFERRED_TARGET ioctl: however
>       * we know these will only support creating one kind of guest CPU,
>       * which is its preferred CPU type. Fortunately these old kernels
> @@ -474,8 +497,71 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
>      ahcf->target = init.target;
>      ahcf->dtb_compatible = "arm,arm-v8";
>
> +    err = read_sys_reg64(fdarray[2], &ahcf->isar.id_aa64pfr0,
> +                         ARM64_SYS_REG(3, 0, 0, 4, 0));
> +    if (unlikely(err < 0)) {
> +        /*
> +         * Before v4.15, the kernel only exposed a limited number of system
> +         * registers, not including any of the interesting AArch64 ID regs.
> +         * For the most part we could leave these fields as zero with minimal
> +         * effect, since this does not affect the values seen by the guest.

These older kernels do implement reading of id_isar0 through
id_isar5, though -- we could read and use those values rather than
leaving them zero.

> +         *
> +         * However, it could cause problems down the line for QEMU,
> +         * so provide a minimal v8.0 default.
> +         *
> +         * ??? Could read MIDR and use knowledge from cpu64.c.
> +         * ??? Could map a page of memory into our temp guest and
> +         *     run the tiniest of hand-crafted kernels to extract
> +         *     the values seen by the guest.
> +         * ??? Either of these sounds like too much effort just
> +         *     to work around running a modern host kernel.
> +         */
> +        ahcf->isar.id_aa64pfr0 = 0x00000011; /* EL1&0, AArch64 only */
> +        err = 0;

Doesn't this code path leave everything except id_aa64pfr0 as
zero, thus leaving us with the "could cause problems down the
line" situation ?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 2/5] target/arm: Fill in ARMISARegisters for kvm64

[Richard Henderson]

On 11/12/18 12:37 PM, Peter Maydell wrote:
> On 8 November 2018 at 17:52, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
> 
>>      /* Old kernels may not know about the PREFERRED_TARGET ioctl: however
>>       * we know these will only support creating one kind of guest CPU,
>>       * which is its preferred CPU type. Fortunately these old kernels
>> @@ -474,8 +497,71 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
>>      ahcf->target = init.target;
>>      ahcf->dtb_compatible = "arm,arm-v8";
>>
>> +    err = read_sys_reg64(fdarray[2], &ahcf->isar.id_aa64pfr0,
>> +                         ARM64_SYS_REG(3, 0, 0, 4, 0));
>> +    if (unlikely(err < 0)) {
>> +        /*
>> +         * Before v4.15, the kernel only exposed a limited number of system
>> +         * registers, not including any of the interesting AArch64 ID regs.
>> +         * For the most part we could leave these fields as zero with minimal
>> +         * effect, since this does not affect the values seen by the guest.
> 
> These older kernels do implement reading of id_isar0 through
> id_isar5, though -- we could read and use those values rather than
> leaving them zero.

But without id_aa64pfr0, we don't know if id_isar0 et al have UNKNOWN values
due to aa32 mode not being present.  E.g. run a 4.8 kernel on a thunderx1 cpu.

>> +        ahcf->isar.id_aa64pfr0 = 0x00000011; /* EL1&0, AArch64 only */
>> +        err = 0;
> 
> Doesn't this code path leave everything except id_aa64pfr0 as
> zero, thus leaving us with the "could cause problems down the
> line" situation ?

The other id_aa64* register fields are all extensions to v8.0, so they should
be zero.  I am of course assuming that AdvSIMD is present, otherwise qemu
itself probably have failed before now.


r~

Re: [Qemu-devel] [PATCH v3 2/5] target/arm: Fill in ARMISARegisters for kvm64

[Peter Maydell]

On 12 November 2018 at 14:09, Richard Henderson
<richard.henderson@linaro.org> wrote:
> On 11/12/18 12:37 PM, Peter Maydell wrote:
>> On 8 November 2018 at 17:52, Richard Henderson
>> <richard.henderson@linaro.org> wrote:
>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>> ---
>>
>>>      /* Old kernels may not know about the PREFERRED_TARGET ioctl: however
>>>       * we know these will only support creating one kind of guest CPU,
>>>       * which is its preferred CPU type. Fortunately these old kernels
>>> @@ -474,8 +497,71 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
>>>      ahcf->target = init.target;
>>>      ahcf->dtb_compatible = "arm,arm-v8";
>>>
>>> +    err = read_sys_reg64(fdarray[2], &ahcf->isar.id_aa64pfr0,
>>> +                         ARM64_SYS_REG(3, 0, 0, 4, 0));
>>> +    if (unlikely(err < 0)) {
>>> +        /*
>>> +         * Before v4.15, the kernel only exposed a limited number of system
>>> +         * registers, not including any of the interesting AArch64 ID regs.
>>> +         * For the most part we could leave these fields as zero with minimal
>>> +         * effect, since this does not affect the values seen by the guest.
>>
>> These older kernels do implement reading of id_isar0 through
>> id_isar5, though -- we could read and use those values rather than
>> leaving them zero.
>
> But without id_aa64pfr0, we don't know if id_isar0 et al have UNKNOWN values
> due to aa32 mode not being present.  E.g. run a 4.8 kernel on a thunderx1 cpu.

Good point, which makes things kind of awkward.

>>> +        ahcf->isar.id_aa64pfr0 = 0x00000011; /* EL1&0, AArch64 only */
>>> +        err = 0;
>>
>> Doesn't this code path leave everything except id_aa64pfr0 as
>> zero, thus leaving us with the "could cause problems down the
>> line" situation ?
>
> The other id_aa64* register fields are all extensions to v8.0, so they should
> be zero.  I am of course assuming that AdvSIMD is present, otherwise qemu
> itself probably have failed before now.

You don't set the id_isar* to say "AdvSIMD present" (or "VFP present"), though.
So if we in a later commit change target/arm/machine.c:vfp_needed() to
check an id_isar* field instead of ARM_FEATURE_VFP then we'll break
migration for these old kernel versions. That was the kind of thing I was
hoping we could avoid having to remember for the future...

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 2/5] target/arm: Fill in ARMISARegisters for kvm64

[Richard Henderson]

On 11/12/18 3:34 PM, Peter Maydell wrote:
>> The other id_aa64* register fields are all extensions to v8.0, so they should
>> be zero.  I am of course assuming that AdvSIMD is present, otherwise qemu
>> itself probably have failed before now.
> 
> You don't set the id_isar* to say "AdvSIMD present" (or "VFP present"), though.

Those two are almost unique in that they are signed fields.  So 0 indicates
present and -1 indicates not present (and 1 indicates fp16 support).


r~

[Qemu-devel] [PATCH v3 3/5] target/arm: Introduce read_sys_reg32 for kvm32

[Richard Henderson]

Assert that the value to be written is the correct size.
No change in functionality here, just mirroring the same
function from kvm64.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/kvm32.c | 41 ++++++++++++++++-------------------------
 1 file changed, 16 insertions(+), 25 deletions(-)

diff --git a/target/arm/kvm32.c b/target/arm/kvm32.c
index 0f1e94c7b5..de573f9aa8 100644
--- a/target/arm/kvm32.c
+++ b/target/arm/kvm32.c
@@ -28,6 +28,14 @@ static inline void set_feature(uint64_t *features, int feature)
     *features |= 1ULL << feature;
 }
 
+static int read_sys_reg32(int fd, uint32_t *pret, uint64_t id)
+{
+    struct kvm_one_reg idreg = { .id = id, .addr = (uintptr_t)pret };
+
+    assert((id & KVM_REG_SIZE_MASK) == KVM_REG_SIZE_U32);
+    return ioctl(fd, KVM_GET_ONE_REG, &idreg);
+}
+
 bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
 {
     /* Identify the feature bits corresponding to the host CPU, and
@@ -35,9 +43,10 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
      * we have to create a scratch VM, create a single CPU inside it,
      * and then query that CPU for the relevant ID registers.
      */
-    int i, ret, fdarray[3];
+    int err = 0, fdarray[3];
     uint32_t midr, id_pfr0, mvfr1;
     uint64_t features = 0;
+
     /* Old kernels may not know about the PREFERRED_TARGET ioctl: however
      * we know these will only support creating one kind of guest CPU,
      * which is its preferred CPU type.
@@ -47,23 +56,6 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
         QEMU_KVM_ARM_TARGET_NONE
     };
     struct kvm_vcpu_init init;
-    struct kvm_one_reg idregs[] = {
-        {
-            .id = KVM_REG_ARM | KVM_REG_SIZE_U32
-            | ENCODE_CP_REG(15, 0, 0, 0, 0, 0, 0),
-            .addr = (uintptr_t)&midr,
-        },
-        {
-            .id = KVM_REG_ARM | KVM_REG_SIZE_U32
-            | ENCODE_CP_REG(15, 0, 0, 0, 1, 0, 0),
-            .addr = (uintptr_t)&id_pfr0,
-        },
-        {
-            .id = KVM_REG_ARM | KVM_REG_SIZE_U32
-            | KVM_REG_ARM_VFP | KVM_REG_ARM_VFP_MVFR1,
-            .addr = (uintptr_t)&mvfr1,
-        },
-    };
 
     if (!kvm_arm_create_scratch_host_vcpu(cpus_to_try, fdarray, &init)) {
         return false;
@@ -77,16 +69,15 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
      */
     ahcf->dtb_compatible = "arm,arm-v7";
 
-    for (i = 0; i < ARRAY_SIZE(idregs); i++) {
-        ret = ioctl(fdarray[2], KVM_GET_ONE_REG, &idregs[i]);
-        if (ret) {
-            break;
-        }
-    }
+    err |= read_sys_reg32(fdarray[2], &midr, ARM_CP15_REG32(0, 0, 0, 0));
+    err |= read_sys_reg32(fdarray[2], &id_pfr0, ARM_CP15_REG32(0, 0, 1, 0));
+    err |= read_sys_reg32(fdarray[2], &mvfr1,
+                          KVM_REG_ARM | KVM_REG_SIZE_U32 |
+                          KVM_REG_ARM_VFP | KVM_REG_ARM_VFP_MVFR1);
 
     kvm_arm_destroy_scratch_host_vcpu(fdarray);
 
-    if (ret) {
+    if (err < 0) {
         return false;
     }
 
-- 
2.17.2

[Qemu-devel] [PATCH v3 4/5] target/arm: Fill in ARMISARegisters for kvm32

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/kvm32.c | 33 ++++++++++++++++++++++++++++-----
 1 file changed, 28 insertions(+), 5 deletions(-)

diff --git a/target/arm/kvm32.c b/target/arm/kvm32.c
index de573f9aa8..9ededa3c73 100644
--- a/target/arm/kvm32.c
+++ b/target/arm/kvm32.c
@@ -44,7 +44,7 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
      * and then query that CPU for the relevant ID registers.
      */
     int err = 0, fdarray[3];
-    uint32_t midr, id_pfr0, mvfr1;
+    uint32_t midr, id_pfr0;
     uint64_t features = 0;
 
     /* Old kernels may not know about the PREFERRED_TARGET ioctl: however
@@ -71,9 +71,32 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
 
     err |= read_sys_reg32(fdarray[2], &midr, ARM_CP15_REG32(0, 0, 0, 0));
     err |= read_sys_reg32(fdarray[2], &id_pfr0, ARM_CP15_REG32(0, 0, 1, 0));
-    err |= read_sys_reg32(fdarray[2], &mvfr1,
+
+    err |= read_sys_reg32(fdarray[2], &ahcf->isar.id_isar0,
+                          ARM_CP15_REG32(0, 0, 2, 0));
+    err |= read_sys_reg32(fdarray[2], &ahcf->isar.id_isar1,
+                          ARM_CP15_REG32(0, 0, 2, 1));
+    err |= read_sys_reg32(fdarray[2], &ahcf->isar.id_isar2,
+                          ARM_CP15_REG32(0, 0, 2, 2));
+    err |= read_sys_reg32(fdarray[2], &ahcf->isar.id_isar3,
+                          ARM_CP15_REG32(0, 0, 2, 3));
+    err |= read_sys_reg32(fdarray[2], &ahcf->isar.id_isar4,
+                          ARM_CP15_REG32(0, 0, 2, 4));
+    err |= read_sys_reg32(fdarray[2], &ahcf->isar.id_isar5,
+                          ARM_CP15_REG32(0, 0, 2, 5));
+    err |= read_sys_reg32(fdarray[2], &ahcf->isar.id_isar6,
+                          ARM_CP15_REG32(0, 0, 2, 7));
+
+    err |= read_sys_reg32(fdarray[2], &ahcf->isar.mvfr0,
+                          KVM_REG_ARM | KVM_REG_SIZE_U32 |
+                          KVM_REG_ARM_VFP | KVM_REG_ARM_VFP_MVFR0);
+    err |= read_sys_reg32(fdarray[2], &ahcf->isar.mvfr1,
                           KVM_REG_ARM | KVM_REG_SIZE_U32 |
                           KVM_REG_ARM_VFP | KVM_REG_ARM_VFP_MVFR1);
+    /*
+     * FIXME: There is not yet a way to read MVFR2.
+     * Fortunately there is not yet anything in there that affects migration.
+     */
 
     kvm_arm_destroy_scratch_host_vcpu(fdarray);
 
@@ -95,13 +118,13 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
     if (extract32(id_pfr0, 12, 4) == 1) {
         set_feature(&features, ARM_FEATURE_THUMB2EE);
     }
-    if (extract32(mvfr1, 20, 4) == 1) {
+    if (extract32(ahcf->isar.mvfr1, 20, 4) == 1) {
         set_feature(&features, ARM_FEATURE_VFP_FP16);
     }
-    if (extract32(mvfr1, 12, 4) == 1) {
+    if (extract32(ahcf->isar.mvfr1, 12, 4) == 1) {
         set_feature(&features, ARM_FEATURE_NEON);
     }
-    if (extract32(mvfr1, 28, 4) == 1) {
+    if (extract32(ahcf->isar.mvfr1, 28, 4) == 1) {
         /* FMAC support implies VFPv4 */
         set_feature(&features, ARM_FEATURE_VFP4);
     }
-- 
2.17.2

Re: [Qemu-devel] [PATCH v3 4/5] target/arm: Fill in ARMISARegisters for kvm32

[Peter Maydell]

On 8 November 2018 at 17:52, Richard Henderson
<richard.henderson@linaro.org> wrote:
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/kvm32.c | 33 ++++++++++++++++++++++++++++-----
>  1 file changed, 28 insertions(+), 5 deletions(-)
>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 4/5] target/arm: Fill in ARMISARegisters for kvm32

[Peter Maydell]

On 8 November 2018 at 17:52, Richard Henderson
<richard.henderson@linaro.org> wrote:
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/kvm32.c | 33 ++++++++++++++++++++++++++++-----
>  1 file changed, 28 insertions(+), 5 deletions(-)
>
> diff --git a/target/arm/kvm32.c b/target/arm/kvm32.c
> index de573f9aa8..9ededa3c73 100644
> --- a/target/arm/kvm32.c
> +++ b/target/arm/kvm32.c
> @@ -44,7 +44,7 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
>       * and then query that CPU for the relevant ID registers.
>       */
>      int err = 0, fdarray[3];
> -    uint32_t midr, id_pfr0, mvfr1;
> +    uint32_t midr, id_pfr0;
>      uint64_t features = 0;
>
>      /* Old kernels may not know about the PREFERRED_TARGET ioctl: however
> @@ -71,9 +71,32 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
>
>      err |= read_sys_reg32(fdarray[2], &midr, ARM_CP15_REG32(0, 0, 0, 0));
>      err |= read_sys_reg32(fdarray[2], &id_pfr0, ARM_CP15_REG32(0, 0, 1, 0));
> -    err |= read_sys_reg32(fdarray[2], &mvfr1,
> +
> +    err |= read_sys_reg32(fdarray[2], &ahcf->isar.id_isar0,
> +                          ARM_CP15_REG32(0, 0, 2, 0));
> +    err |= read_sys_reg32(fdarray[2], &ahcf->isar.id_isar1,
> +                          ARM_CP15_REG32(0, 0, 2, 1));
> +    err |= read_sys_reg32(fdarray[2], &ahcf->isar.id_isar2,
> +                          ARM_CP15_REG32(0, 0, 2, 2));
> +    err |= read_sys_reg32(fdarray[2], &ahcf->isar.id_isar3,
> +                          ARM_CP15_REG32(0, 0, 2, 3));
> +    err |= read_sys_reg32(fdarray[2], &ahcf->isar.id_isar4,
> +                          ARM_CP15_REG32(0, 0, 2, 4));
> +    err |= read_sys_reg32(fdarray[2], &ahcf->isar.id_isar5,
> +                          ARM_CP15_REG32(0, 0, 2, 5));
> +    err |= read_sys_reg32(fdarray[2], &ahcf->isar.id_isar6,
> +                          ARM_CP15_REG32(0, 0, 2, 7));
> +
> +    err |= read_sys_reg32(fdarray[2], &ahcf->isar.mvfr0,
> +                          KVM_REG_ARM | KVM_REG_SIZE_U32 |
> +                          KVM_REG_ARM_VFP | KVM_REG_ARM_VFP_MVFR0);
> +    err |= read_sys_reg32(fdarray[2], &ahcf->isar.mvfr1,
>                            KVM_REG_ARM | KVM_REG_SIZE_U32 |
>                            KVM_REG_ARM_VFP | KVM_REG_ARM_VFP_MVFR1);

Testing this on my aarch32 system (a cubieboard running 4.0.0-rc4)
this fails with
qemu-system-arm: Failed to retrieve host CPU features

strace says:
ioctl(14, KVM_CREATE_VM, 0)             = 15
ioctl(15, KVM_CREATE_VCPU, 0)           = 16
ioctl(15, 0x8020aeaf, 0xbea1bc74)       = 0
ioctl(16, KVM_ARM_VCPU_INIT, 0xbea1bc74) = 0
ioctl(16, KVM_ARM_SET_DEVICE_ADDR or KVM_GET_ONE_REG, 0xbea1bc30) = 0
ioctl(16, KVM_ARM_SET_DEVICE_ADDR or KVM_GET_ONE_REG, 0xbea1bc30) = 0
ioctl(16, KVM_ARM_SET_DEVICE_ADDR or KVM_GET_ONE_REG, 0xbea1bc30) = 0
ioctl(16, KVM_ARM_SET_DEVICE_ADDR or KVM_GET_ONE_REG, 0xbea1bc30) = 0
ioctl(16, KVM_ARM_SET_DEVICE_ADDR or KVM_GET_ONE_REG, 0xbea1bc30) = 0
ioctl(16, KVM_ARM_SET_DEVICE_ADDR or KVM_GET_ONE_REG, 0xbea1bc30) = 0
ioctl(16, KVM_ARM_SET_DEVICE_ADDR or KVM_GET_ONE_REG, 0xbea1bc30) = 0
ioctl(16, KVM_ARM_SET_DEVICE_ADDR or KVM_GET_ONE_REG, 0xbea1bc30) = 0
ioctl(16, KVM_ARM_SET_DEVICE_ADDR or KVM_GET_ONE_REG, 0xbea1bc30) = -1
ENOENT (No such file or directory)
ioctl(16, KVM_ARM_SET_DEVICE_ADDR or KVM_GET_ONE_REG, 0xbea1bc30) = 0
ioctl(16, KVM_ARM_SET_DEVICE_ADDR or KVM_GET_ONE_REG, 0xbea1bc30) = 0

so I deduce that this is because the kernel doesn't support reading
ID_ISAR6.

Adding the following change allowed me to boot:

diff --git a/target/arm/kvm32.c b/target/arm/kvm32.c
index 8b2c9b3..2f7df81 100644
--- a/target/arm/kvm32.c
+++ b/target/arm/kvm32.c
@@ -84,9 +84,15 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
                           ARM_CP15_REG32(0, 0, 2, 4));
     err |= read_sys_reg32(fdarray[2], &ahcf->isar.id_isar5,
                           ARM_CP15_REG32(0, 0, 2, 5));
-    err |= read_sys_reg32(fdarray[2], &ahcf->isar.id_isar6,
-                          ARM_CP15_REG32(0, 0, 2, 7));
-
+    if (read_sys_reg32(fdarray[2], &ahcf->isar.id_isar6,
+                          ARM_CP15_REG32(0, 0, 2, 7))) {
+        /*
+         * Older kernels don't support reading ID_ISAR6. This register was
+         * only introduced in ARMv8, so we can assume that it is zero on a
+         * CPU that a kernel this old is running on.
+         */
+        ahcf->isar.id_isar6 = 0;
+    }
     err |= read_sys_reg32(fdarray[2], &ahcf->isar.mvfr0,
                           KVM_REG_ARM | KVM_REG_SIZE_U32 |
                           KVM_REG_ARM_VFP | KVM_REG_ARM_VFP_MVFR0);

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 4/5] target/arm: Fill in ARMISARegisters for kvm32

[Richard Henderson]

On 11/12/18 1:42 PM, Peter Maydell wrote:
> Adding the following change allowed me to boot:
> 
> diff --git a/target/arm/kvm32.c b/target/arm/kvm32.c
> index 8b2c9b3..2f7df81 100644
> --- a/target/arm/kvm32.c
> +++ b/target/arm/kvm32.c
> @@ -84,9 +84,15 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
>                            ARM_CP15_REG32(0, 0, 2, 4));
>      err |= read_sys_reg32(fdarray[2], &ahcf->isar.id_isar5,
>                            ARM_CP15_REG32(0, 0, 2, 5));
> -    err |= read_sys_reg32(fdarray[2], &ahcf->isar.id_isar6,
> -                          ARM_CP15_REG32(0, 0, 2, 7));
> -
> +    if (read_sys_reg32(fdarray[2], &ahcf->isar.id_isar6,
> +                          ARM_CP15_REG32(0, 0, 2, 7))) {
> +        /*
> +         * Older kernels don't support reading ID_ISAR6. This register was
> +         * only introduced in ARMv8, so we can assume that it is zero on a
> +         * CPU that a kernel this old is running on.
> +         */
> +        ahcf->isar.id_isar6 = 0;
> +    }

Ah, right, thanks.


r~

[Qemu-devel] [PATCH v3 5/5] target/arm: Convert t32ee from feature bit to isar3 test

[Richard Henderson]

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/cpu.h     | 6 +++++-
 linux-user/elfload.c | 2 +-
 target/arm/cpu.c     | 4 ----
 target/arm/helper.c  | 2 +-
 target/arm/kvm32.c   | 3 ---
 target/arm/machine.c | 3 +--
 6 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index b5eff79f73..5c2c77c31d 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1575,7 +1575,6 @@ enum arm_features {
     ARM_FEATURE_NEON,
     ARM_FEATURE_M, /* Microcontroller profile.  */
     ARM_FEATURE_OMAPCP, /* OMAP specific CP15 ops handling.  */
-    ARM_FEATURE_THUMB2EE,
     ARM_FEATURE_V7MP,    /* v7 Multiprocessing Extensions */
     ARM_FEATURE_V7VE, /* v7 Virtualization Extensions (non-EL2 parts) */
     ARM_FEATURE_V4T,
@@ -3172,6 +3171,11 @@ static inline bool isar_feature_jazelle(const ARMISARegisters *id)
     return FIELD_EX32(id->id_isar1, ID_ISAR1, JAZELLE) != 0;
 }
 
+static inline bool isar_feature_t32ee(const ARMISARegisters *id)
+{
+    return FIELD_EX32(id->id_isar3, ID_ISAR3, T32EE) != 0;
+}
+
 static inline bool isar_feature_aa32_aes(const ARMISARegisters *id)
 {
     return FIELD_EX32(id->id_isar5, ID_ISAR5, AES) != 0;
diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 5bccd2e243..a3503c83c9 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -466,7 +466,7 @@ static uint32_t get_elf_hwcap(void)
     GET_FEATURE(ARM_FEATURE_V5, ARM_HWCAP_ARM_EDSP);
     GET_FEATURE(ARM_FEATURE_VFP, ARM_HWCAP_ARM_VFP);
     GET_FEATURE(ARM_FEATURE_IWMMXT, ARM_HWCAP_ARM_IWMMXT);
-    GET_FEATURE(ARM_FEATURE_THUMB2EE, ARM_HWCAP_ARM_THUMBEE);
+    GET_FEATURE_ID(t32ee, ARM_HWCAP_ARM_THUMBEE);
     GET_FEATURE(ARM_FEATURE_NEON, ARM_HWCAP_ARM_NEON);
     GET_FEATURE(ARM_FEATURE_VFP3, ARM_HWCAP_ARM_VFPv3);
     GET_FEATURE(ARM_FEATURE_V6K, ARM_HWCAP_ARM_TLS);
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 784a4c2dfc..d4dc0bc225 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1451,7 +1451,6 @@ static void cortex_a8_initfn(Object *obj)
     set_feature(&cpu->env, ARM_FEATURE_V7);
     set_feature(&cpu->env, ARM_FEATURE_VFP3);
     set_feature(&cpu->env, ARM_FEATURE_NEON);
-    set_feature(&cpu->env, ARM_FEATURE_THUMB2EE);
     set_feature(&cpu->env, ARM_FEATURE_DUMMY_C15_REGS);
     set_feature(&cpu->env, ARM_FEATURE_EL3);
     cpu->midr = 0x410fc080;
@@ -1520,7 +1519,6 @@ static void cortex_a9_initfn(Object *obj)
     set_feature(&cpu->env, ARM_FEATURE_VFP3);
     set_feature(&cpu->env, ARM_FEATURE_VFP_FP16);
     set_feature(&cpu->env, ARM_FEATURE_NEON);
-    set_feature(&cpu->env, ARM_FEATURE_THUMB2EE);
     set_feature(&cpu->env, ARM_FEATURE_EL3);
     /* Note that A9 supports the MP extensions even for
      * A9UP and single-core A9MP (which are both different
@@ -1583,7 +1581,6 @@ static void cortex_a7_initfn(Object *obj)
     set_feature(&cpu->env, ARM_FEATURE_V7VE);
     set_feature(&cpu->env, ARM_FEATURE_VFP4);
     set_feature(&cpu->env, ARM_FEATURE_NEON);
-    set_feature(&cpu->env, ARM_FEATURE_THUMB2EE);
     set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
     set_feature(&cpu->env, ARM_FEATURE_DUMMY_C15_REGS);
     set_feature(&cpu->env, ARM_FEATURE_CBAR_RO);
@@ -1629,7 +1626,6 @@ static void cortex_a15_initfn(Object *obj)
     set_feature(&cpu->env, ARM_FEATURE_V7VE);
     set_feature(&cpu->env, ARM_FEATURE_VFP4);
     set_feature(&cpu->env, ARM_FEATURE_NEON);
-    set_feature(&cpu->env, ARM_FEATURE_THUMB2EE);
     set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
     set_feature(&cpu->env, ARM_FEATURE_DUMMY_C15_REGS);
     set_feature(&cpu->env, ARM_FEATURE_CBAR_RO);
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 96301930cc..e28770833a 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -5457,7 +5457,7 @@ void register_cp_regs_for_features(ARMCPU *cpu)
         define_arm_cp_regs(cpu, vmsa_pmsa_cp_reginfo);
         define_arm_cp_regs(cpu, vmsa_cp_reginfo);
     }
-    if (arm_feature(env, ARM_FEATURE_THUMB2EE)) {
+    if (cpu_isar_feature(t32ee, cpu)) {
         define_arm_cp_regs(cpu, t2ee_cp_reginfo);
     }
     if (arm_feature(env, ARM_FEATURE_GENERIC_TIMER)) {
diff --git a/target/arm/kvm32.c b/target/arm/kvm32.c
index 9ededa3c73..8b2c9b3075 100644
--- a/target/arm/kvm32.c
+++ b/target/arm/kvm32.c
@@ -115,9 +115,6 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
     set_feature(&features, ARM_FEATURE_VFP3);
     set_feature(&features, ARM_FEATURE_GENERIC_TIMER);
 
-    if (extract32(id_pfr0, 12, 4) == 1) {
-        set_feature(&features, ARM_FEATURE_THUMB2EE);
-    }
     if (extract32(ahcf->isar.mvfr1, 20, 4) == 1) {
         set_feature(&features, ARM_FEATURE_VFP_FP16);
     }
diff --git a/target/arm/machine.c b/target/arm/machine.c
index 239fe4e84d..07f904709a 100644
--- a/target/arm/machine.c
+++ b/target/arm/machine.c
@@ -321,9 +321,8 @@ static const VMStateDescription vmstate_m = {
 static bool thumb2ee_needed(void *opaque)
 {
     ARMCPU *cpu = opaque;
-    CPUARMState *env = &cpu->env;
 
-    return arm_feature(env, ARM_FEATURE_THUMB2EE);
+    return cpu_isar_feature(t32ee, cpu);
 }
 
 static const VMStateDescription vmstate_thumb2ee = {
-- 
2.17.2

Re: [Qemu-devel] [Qemu-arm] [PATCH v3 5/5] target/arm: Convert t32ee from feature bit to isar3 test

[Philippe Mathieu-Daudé]

On Thu, Nov 8, 2018 at 7:02 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  target/arm/cpu.h     | 6 +++++-
>  linux-user/elfload.c | 2 +-
>  target/arm/cpu.c     | 4 ----
>  target/arm/helper.c  | 2 +-
>  target/arm/kvm32.c   | 3 ---
>  target/arm/machine.c | 3 +--
>  6 files changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index b5eff79f73..5c2c77c31d 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -1575,7 +1575,6 @@ enum arm_features {
>      ARM_FEATURE_NEON,
>      ARM_FEATURE_M, /* Microcontroller profile.  */
>      ARM_FEATURE_OMAPCP, /* OMAP specific CP15 ops handling.  */
> -    ARM_FEATURE_THUMB2EE,
>      ARM_FEATURE_V7MP,    /* v7 Multiprocessing Extensions */
>      ARM_FEATURE_V7VE, /* v7 Virtualization Extensions (non-EL2 parts) */
>      ARM_FEATURE_V4T,
> @@ -3172,6 +3171,11 @@ static inline bool isar_feature_jazelle(const ARMISARegisters *id)
>      return FIELD_EX32(id->id_isar1, ID_ISAR1, JAZELLE) != 0;
>  }
>
> +static inline bool isar_feature_t32ee(const ARMISARegisters *id)
> +{
> +    return FIELD_EX32(id->id_isar3, ID_ISAR3, T32EE) != 0;
> +}
> +
>  static inline bool isar_feature_aa32_aes(const ARMISARegisters *id)
>  {
>      return FIELD_EX32(id->id_isar5, ID_ISAR5, AES) != 0;
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index 5bccd2e243..a3503c83c9 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -466,7 +466,7 @@ static uint32_t get_elf_hwcap(void)
>      GET_FEATURE(ARM_FEATURE_V5, ARM_HWCAP_ARM_EDSP);
>      GET_FEATURE(ARM_FEATURE_VFP, ARM_HWCAP_ARM_VFP);
>      GET_FEATURE(ARM_FEATURE_IWMMXT, ARM_HWCAP_ARM_IWMMXT);
> -    GET_FEATURE(ARM_FEATURE_THUMB2EE, ARM_HWCAP_ARM_THUMBEE);
> +    GET_FEATURE_ID(t32ee, ARM_HWCAP_ARM_THUMBEE);
>      GET_FEATURE(ARM_FEATURE_NEON, ARM_HWCAP_ARM_NEON);
>      GET_FEATURE(ARM_FEATURE_VFP3, ARM_HWCAP_ARM_VFPv3);
>      GET_FEATURE(ARM_FEATURE_V6K, ARM_HWCAP_ARM_TLS);
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 784a4c2dfc..d4dc0bc225 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1451,7 +1451,6 @@ static void cortex_a8_initfn(Object *obj)
>      set_feature(&cpu->env, ARM_FEATURE_V7);
>      set_feature(&cpu->env, ARM_FEATURE_VFP3);
>      set_feature(&cpu->env, ARM_FEATURE_NEON);
> -    set_feature(&cpu->env, ARM_FEATURE_THUMB2EE);
>      set_feature(&cpu->env, ARM_FEATURE_DUMMY_C15_REGS);
>      set_feature(&cpu->env, ARM_FEATURE_EL3);
>      cpu->midr = 0x410fc080;
> @@ -1520,7 +1519,6 @@ static void cortex_a9_initfn(Object *obj)
>      set_feature(&cpu->env, ARM_FEATURE_VFP3);
>      set_feature(&cpu->env, ARM_FEATURE_VFP_FP16);
>      set_feature(&cpu->env, ARM_FEATURE_NEON);
> -    set_feature(&cpu->env, ARM_FEATURE_THUMB2EE);
>      set_feature(&cpu->env, ARM_FEATURE_EL3);
>      /* Note that A9 supports the MP extensions even for
>       * A9UP and single-core A9MP (which are both different
> @@ -1583,7 +1581,6 @@ static void cortex_a7_initfn(Object *obj)
>      set_feature(&cpu->env, ARM_FEATURE_V7VE);
>      set_feature(&cpu->env, ARM_FEATURE_VFP4);
>      set_feature(&cpu->env, ARM_FEATURE_NEON);
> -    set_feature(&cpu->env, ARM_FEATURE_THUMB2EE);
>      set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
>      set_feature(&cpu->env, ARM_FEATURE_DUMMY_C15_REGS);
>      set_feature(&cpu->env, ARM_FEATURE_CBAR_RO);
> @@ -1629,7 +1626,6 @@ static void cortex_a15_initfn(Object *obj)
>      set_feature(&cpu->env, ARM_FEATURE_V7VE);
>      set_feature(&cpu->env, ARM_FEATURE_VFP4);
>      set_feature(&cpu->env, ARM_FEATURE_NEON);
> -    set_feature(&cpu->env, ARM_FEATURE_THUMB2EE);
>      set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
>      set_feature(&cpu->env, ARM_FEATURE_DUMMY_C15_REGS);
>      set_feature(&cpu->env, ARM_FEATURE_CBAR_RO);
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 96301930cc..e28770833a 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -5457,7 +5457,7 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>          define_arm_cp_regs(cpu, vmsa_pmsa_cp_reginfo);
>          define_arm_cp_regs(cpu, vmsa_cp_reginfo);
>      }
> -    if (arm_feature(env, ARM_FEATURE_THUMB2EE)) {
> +    if (cpu_isar_feature(t32ee, cpu)) {
>          define_arm_cp_regs(cpu, t2ee_cp_reginfo);
>      }
>      if (arm_feature(env, ARM_FEATURE_GENERIC_TIMER)) {
> diff --git a/target/arm/kvm32.c b/target/arm/kvm32.c
> index 9ededa3c73..8b2c9b3075 100644
> --- a/target/arm/kvm32.c
> +++ b/target/arm/kvm32.c
> @@ -115,9 +115,6 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
>      set_feature(&features, ARM_FEATURE_VFP3);
>      set_feature(&features, ARM_FEATURE_GENERIC_TIMER);
>
> -    if (extract32(id_pfr0, 12, 4) == 1) {
> -        set_feature(&features, ARM_FEATURE_THUMB2EE);
> -    }
>      if (extract32(ahcf->isar.mvfr1, 20, 4) == 1) {
>          set_feature(&features, ARM_FEATURE_VFP_FP16);
>      }
> diff --git a/target/arm/machine.c b/target/arm/machine.c
> index 239fe4e84d..07f904709a 100644
> --- a/target/arm/machine.c
> +++ b/target/arm/machine.c
> @@ -321,9 +321,8 @@ static const VMStateDescription vmstate_m = {
>  static bool thumb2ee_needed(void *opaque)
>  {
>      ARMCPU *cpu = opaque;
> -    CPUARMState *env = &cpu->env;
>
> -    return arm_feature(env, ARM_FEATURE_THUMB2EE);
> +    return cpu_isar_feature(t32ee, cpu);
>  }
>
>  static const VMStateDescription vmstate_thumb2ee = {
> --
> 2.17.2
>
>