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

[Qemu-devel] [PATCH v2 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, 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   | 69 ++++++++++++++++++++++++++++++++++++++--
 target/arm/machine.c |  3 +-
 9 files changed, 120 insertions(+), 43 deletions(-)

-- 
2.17.2

[Qemu-devel] [PATCH v2 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 v2 2/5] target/arm: Fill in ARMISARegisters for kvm64

[Richard Henderson]

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

diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index 5de8ff0ac5..d4d4e63140 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 = 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. Fortunately these old kernels
@@ -474,8 +497,50 @@ 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_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));
+    err |= read_sys_reg64(fdarray[2], &ahcf->isar.id_aa64pfr0,
+                          ARM64_SYS_REG(3, 0, 0, 4, 0));
+    err |= read_sys_reg64(fdarray[2], &ahcf->isar.id_aa64pfr1,
+                          ARM64_SYS_REG(3, 0, 0, 4, 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

[Qemu-devel] [PATCH v2 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 v2 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

[Qemu-devel] [PATCH v2 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 8e6779936e..895f9909d8 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 8f16e96b6c..e08a2d2d79 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1440,7 +1440,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;
@@ -1509,7 +1508,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
@@ -1572,7 +1570,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);
@@ -1618,7 +1615,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 0ea95b0815..bea4d5350d 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -5455,7 +5455,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] [PATCH v2 0/5] target/arm: KVM vs ARMISARegisters

[Peter Maydell]

On 2 November 2018 at 14:54, Richard Henderson
<richard.henderson@linaro.org> wrote:
> 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, v1->v2:
>   * Remove assert that AArch32 sysreg <= UINT32_MAX.
>   * Remove unused local variable.
>   * Add commentary for AArch32 sysregs vs missing AArch32 support.

As noted on IRC, on my admittedly pretty ancient 4.8.0 kernel some
of these ID register reads via KVM_GET_ONE_REG fail ENOENT.
strace says:

openat(AT_FDCWD, "/dev/kvm", O_RDWR|O_CLOEXEC) = 18
ioctl(18, KVM_CREATE_VM or LOGGER_GET_LOG_BUF_SIZE, 0) = 19
ioctl(19, KVM_CREATE_VCPU, 0)           = 20
ioctl(19, KVM_ARM_PREFERRED_TARGET, 0xffffcfeb4e88) = 0
ioctl(20, KVM_ARM_VCPU_INIT, 0xffffcfeb4e88) = 0
ioctl(20, KVM_ARM_SET_DEVICE_ADDR or KVM_GET_ONE_REG, 0xffffcfeb4e28)
= -1 ENOENT (No such file or directory)
ioctl(20, KVM_ARM_SET_DEVICE_ADDR or KVM_GET_ONE_REG, 0xffffcfeb4e28)
= -1 ENOENT (No such file or directory)
ioctl(20, KVM_ARM_SET_DEVICE_ADDR or KVM_GET_ONE_REG, 0xffffcfeb4e28)
= -1 ENOENT (No such file or directory)
ioctl(20, KVM_ARM_SET_DEVICE_ADDR or KVM_GET_ONE_REG, 0xffffcfeb4e28)
= -1 ENOENT (No such file or directory)
ioctl(20, KVM_ARM_SET_DEVICE_ADDR or KVM_GET_ONE_REG, 0xffffcfeb4e28) = 0
ioctl(20, KVM_ARM_SET_DEVICE_ADDR or KVM_GET_ONE_REG, 0xffffcfeb4e28) = 0
ioctl(20, KVM_ARM_SET_DEVICE_ADDR or KVM_GET_ONE_REG, 0xffffcfeb4e28) = 0
ioctl(20, KVM_ARM_SET_DEVICE_ADDR or KVM_GET_ONE_REG, 0xffffcfeb4e28) = 0
ioctl(20, KVM_ARM_SET_DEVICE_ADDR or KVM_GET_ONE_REG, 0xffffcfeb4e28) = 0
ioctl(20, KVM_ARM_SET_DEVICE_ADDR or KVM_GET_ONE_REG, 0xffffcfeb4e28) = 0
ioctl(20, KVM_ARM_SET_DEVICE_ADDR or KVM_GET_ONE_REG, 0xffffcfeb4e28)
= -1 ENOENT (No such file or directory)
ioctl(20, KVM_ARM_SET_DEVICE_ADDR or KVM_GET_ONE_REG, 0xffffcfeb4e28)
= -1 ENOENT (No such file or directory)
ioctl(20, KVM_ARM_SET_DEVICE_ADDR or KVM_GET_ONE_REG, 0xffffcfeb4e28)
= -1 ENOENT (No such file or directory)
ioctl(20, KVM_ARM_SET_DEVICE_ADDR or KVM_GET_ONE_REG, 0xffffcfeb4e28)
= -1 ENOENT (No such file or directory)


I added a bit of extra tracing, since strace doesn't
print the ID field for the ioctl:

peter.maydell@mustang-maydell:~/qemu$
~/test-images/virtv8-for-nesting/runme-kvm
./build/for-kvm/aarch64-softmmu/qemu-system-aarch64 -enable-kvm -cpu
max -machine gic-version=max
read_sys_reg64: reading ID 0x603000000013c030...-1
read_sys_reg64: reading ID 0x603000000013c031...-1
read_sys_reg64: reading ID 0x603000000013c020...-1
read_sys_reg64: reading ID 0x603000000013c021...-1
read_sys_reg32: reading ID 0x603000000013c010...0
read_sys_reg32: reading ID 0x603000000013c011...0
read_sys_reg32: reading ID 0x603000000013c012...0
read_sys_reg32: reading ID 0x603000000013c013...0
read_sys_reg32: reading ID 0x603000000013c014...0
read_sys_reg32: reading ID 0x603000000013c015...0
read_sys_reg32: reading ID 0x603000000013c017...-1
read_sys_reg32: reading ID 0x603000000013c018...-1
read_sys_reg32: reading ID 0x603000000013c019...-1
read_sys_reg32: reading ID 0x603000000013c01a...-1
qemu-system-aarch64: Failed to retrieve host CPU features

It looks like the kernel can handle reads of ID_ISAR0_EL1
through ID_ISAR5_EL1, but not ID_ISAR6_EL1, any of the
MVFR*_EL1 or ID_AA64_ISAR* or ID_AA64PFR*.

This is probably because the kernel is way too old to be
interestingly supportable for KVM, but we did previously
manage to boot on this setup.

We should probably at least figure out which version of
the kernel fixed this bug and made the ID registers available
to userspace... if it's sufficiently ancient we could
likely say "not supported", but if it's more recent we
need a workaround somehow. I have cc'd a couple of kernel
folks who might be able to help with the "which version"
question.

thanks
-- PMM

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

[Christoffer Dall]

On Fri, Nov 02, 2018 at 04:36:35PM +0000, Peter Maydell wrote:
> On 2 November 2018 at 14:54, Richard Henderson
> <richard.henderson@linaro.org> wrote:
> > 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, v1->v2:
> >   * Remove assert that AArch32 sysreg <= UINT32_MAX.
> >   * Remove unused local variable.
> >   * Add commentary for AArch32 sysregs vs missing AArch32 support.
> 
> As noted on IRC, on my admittedly pretty ancient 4.8.0 kernel some
> of these ID register reads via KVM_GET_ONE_REG fail ENOENT.
> strace says:
> 
> openat(AT_FDCWD, "/dev/kvm", O_RDWR|O_CLOEXEC) = 18
> ioctl(18, KVM_CREATE_VM or LOGGER_GET_LOG_BUF_SIZE, 0) = 19
> ioctl(19, KVM_CREATE_VCPU, 0)           = 20
> ioctl(19, KVM_ARM_PREFERRED_TARGET, 0xffffcfeb4e88) = 0
> ioctl(20, KVM_ARM_VCPU_INIT, 0xffffcfeb4e88) = 0
> ioctl(20, KVM_ARM_SET_DEVICE_ADDR or KVM_GET_ONE_REG, 0xffffcfeb4e28)
> = -1 ENOENT (No such file or directory)
> ioctl(20, KVM_ARM_SET_DEVICE_ADDR or KVM_GET_ONE_REG, 0xffffcfeb4e28)
> = -1 ENOENT (No such file or directory)
> ioctl(20, KVM_ARM_SET_DEVICE_ADDR or KVM_GET_ONE_REG, 0xffffcfeb4e28)
> = -1 ENOENT (No such file or directory)
> ioctl(20, KVM_ARM_SET_DEVICE_ADDR or KVM_GET_ONE_REG, 0xffffcfeb4e28)
> = -1 ENOENT (No such file or directory)
> ioctl(20, KVM_ARM_SET_DEVICE_ADDR or KVM_GET_ONE_REG, 0xffffcfeb4e28) = 0
> ioctl(20, KVM_ARM_SET_DEVICE_ADDR or KVM_GET_ONE_REG, 0xffffcfeb4e28) = 0
> ioctl(20, KVM_ARM_SET_DEVICE_ADDR or KVM_GET_ONE_REG, 0xffffcfeb4e28) = 0
> ioctl(20, KVM_ARM_SET_DEVICE_ADDR or KVM_GET_ONE_REG, 0xffffcfeb4e28) = 0
> ioctl(20, KVM_ARM_SET_DEVICE_ADDR or KVM_GET_ONE_REG, 0xffffcfeb4e28) = 0
> ioctl(20, KVM_ARM_SET_DEVICE_ADDR or KVM_GET_ONE_REG, 0xffffcfeb4e28) = 0
> ioctl(20, KVM_ARM_SET_DEVICE_ADDR or KVM_GET_ONE_REG, 0xffffcfeb4e28)
> = -1 ENOENT (No such file or directory)
> ioctl(20, KVM_ARM_SET_DEVICE_ADDR or KVM_GET_ONE_REG, 0xffffcfeb4e28)
> = -1 ENOENT (No such file or directory)
> ioctl(20, KVM_ARM_SET_DEVICE_ADDR or KVM_GET_ONE_REG, 0xffffcfeb4e28)
> = -1 ENOENT (No such file or directory)
> ioctl(20, KVM_ARM_SET_DEVICE_ADDR or KVM_GET_ONE_REG, 0xffffcfeb4e28)
> = -1 ENOENT (No such file or directory)
> 
> 
> I added a bit of extra tracing, since strace doesn't
> print the ID field for the ioctl:
> 
> peter.maydell@mustang-maydell:~/qemu$
> ~/test-images/virtv8-for-nesting/runme-kvm
> ./build/for-kvm/aarch64-softmmu/qemu-system-aarch64 -enable-kvm -cpu
> max -machine gic-version=max
> read_sys_reg64: reading ID 0x603000000013c030...-1
> read_sys_reg64: reading ID 0x603000000013c031...-1
> read_sys_reg64: reading ID 0x603000000013c020...-1
> read_sys_reg64: reading ID 0x603000000013c021...-1
> read_sys_reg32: reading ID 0x603000000013c010...0
> read_sys_reg32: reading ID 0x603000000013c011...0
> read_sys_reg32: reading ID 0x603000000013c012...0
> read_sys_reg32: reading ID 0x603000000013c013...0
> read_sys_reg32: reading ID 0x603000000013c014...0
> read_sys_reg32: reading ID 0x603000000013c015...0
> read_sys_reg32: reading ID 0x603000000013c017...-1
> read_sys_reg32: reading ID 0x603000000013c018...-1
> read_sys_reg32: reading ID 0x603000000013c019...-1
> read_sys_reg32: reading ID 0x603000000013c01a...-1
> qemu-system-aarch64: Failed to retrieve host CPU features
> 
> It looks like the kernel can handle reads of ID_ISAR0_EL1
> through ID_ISAR5_EL1, but not ID_ISAR6_EL1, any of the
> MVFR*_EL1 or ID_AA64_ISAR* or ID_AA64PFR*.
> 
> This is probably because the kernel is way too old to be
> interestingly supportable for KVM, but we did previously
> manage to boot on this setup.

I'm a little confused. v4.8 used to work (although it was perhaps not
the most stable at that time).  What changed?  Is this attempting to
restore a VM from a newer kernel, or has QEMU been updated to detect
this?

> 
> We should probably at least figure out which version of
> the kernel fixed this bug and made the ID registers available
> to userspace... if it's sufficiently ancient we could
> likely say "not supported", but if it's more recent we
> need a workaround somehow. I have cc'd a couple of kernel
> folks who might be able to help with the "which version"
> question.
> 

It appears the support for exposing a bunch of ID registers was
introduced with:

93390c0a1b20 (arm64: KVM: Hide unsupported AArch64 CPU features from guests, 2017-10-31)

Which Dave (cc'ed) wrote and which was introduced in v4.15.

As per my question above, I'm not exactly sure what (if anything) we
need to fix on the kernel side?


Thanks,

    Christoffer

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

[Richard Henderson]

On 11/2/18 7:35 PM, Christoffer Dall wrote:
>> It looks like the kernel can handle reads of ID_ISAR0_EL1
>> through ID_ISAR5_EL1, but not ID_ISAR6_EL1, any of the
>> MVFR*_EL1 or ID_AA64_ISAR* or ID_AA64PFR*.
>>
>> This is probably because the kernel is way too old to be
>> interestingly supportable for KVM, but we did previously
>> manage to boot on this setup.
> 
> I'm a little confused. v4.8 used to work (although it was perhaps not
> the most stable at that time).  What changed?

We are changing QEMU to depend on ID registers to define the set of features
that are supported, instead of re-defining those same features with the
ARM_FEATURE_* enumeration.

For KVM, there are only a few of those that are important in the end: those
that affect migration.  However, it seemed to me to be most future-proof if I
just read in all of the relevant ID registers so that should a code path shared
between TCG and KVM test a field, we get the right value.

Which appeared to work, since I was using a 4.15 kernel.  But I hadn't imagined
that such support had only just appeared.

> 93390c0a1b20 (arm64: KVM: Hide unsupported AArch64 CPU features from guests, 2017-10-31)
> 
> Which Dave (cc'ed) wrote and which was introduced in v4.15.
> 
> As per my question above, I'm not exactly sure what (if anything) we
> need to fix on the kernel side?

It doesn't sound like there's anything in the kernel that needs fixing.  But it
does sound like we need a workaround in userspace to deal with old kernels.

Peter, what about filling in some defaults via MIDR and friends, using the
values we have from the TCG side.  Then overwriting that default with the
registers that we can read from the kernel.  Ignore ENOENT failures and just
leave the default.

There are currently only two features that affect migration, and so must not be
approximated: T2EE and SVE.  The former is in ID_ISAR3, which is in the set
that is supported by 4.8.  The latter you were never going to run on a kernel
that old anyway.

Although, while I have you, Christopher, is there a way to extract the Limited
Ordering Region registers without modifying guest state?  Those are multiplexed
with LORN_EL1.


r~

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

[Marc Zyngier]

On Sat, 3 Nov 2018 09:53:44 +0000
Richard Henderson <richard.henderson@linaro.org> wrote:

Hi Richard,

> Although, while I have you, Christopher, is there a way to extract the Limited
> Ordering Region registers without modifying guest state?  Those are multiplexed
> with LORN_EL1.

We actively hide the LORegion feature from the guest since
cc33c4e20185a391766ed5e78e2acc97e17ba511 (in the 4.17 time frame), so
you shouldn't be able to obtain these on a recent host.

I'm not sure this answers your question though...

Thanks,

	M.
-- 
Without deviation from the norm, progress is not possible.

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

[Richard Henderson]

On 11/3/18 12:32 PM, Marc Zyngier wrote:
> We actively hide the LORegion feature from the guest since
> cc33c4e20185a391766ed5e78e2acc97e17ba511 (in the 4.17 time frame), so
> you shouldn't be able to obtain these on a recent host.

I don't think that patch is ideal.

In particular, LOR is a mandatory requirement of ARMv8.1.
The OS can rightly presume it is present in context.

Better, I think, is to trap the LOR registers, as you are doing,
and have them act as RAZ/WI.  This indicates, via LORID_EL1, that
there are zero supported LO regions, which is a valid ARMv8.1
configuration.


r~

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

[Marc Zyngier]

Hi Richard,

On Sun, 04 Nov 2018 09:50:29 +0000,
Richard Henderson <richard.henderson@linaro.org> wrote:
> 
> On 11/3/18 12:32 PM, Marc Zyngier wrote:
> > We actively hide the LORegion feature from the guest since
> > cc33c4e20185a391766ed5e78e2acc97e17ba511 (in the 4.17 time frame), so
> > you shouldn't be able to obtain these on a recent host.
> 
> I don't think that patch is ideal.
> 
> In particular, LOR is a mandatory requirement of ARMv8.1.
> The OS can rightly presume it is present in context.
> 
> Better, I think, is to trap the LOR registers, as you are doing,
> and have them act as RAZ/WI.  This indicates, via LORID_EL1, that
> there are zero supported LO regions, which is a valid ARMv8.1
> configuration.

I agree this looks like a much better solution. I seem to remember
Mark (now cc'd) battling some half baked implementation that didn't
expose the LOR feature, and yet allowed the various LOR registers to
be freely used, with unknown consequences.

Since we unfortunately need to handle that sorry excuse for a CPU (and
assuming I remember the case correctly), I'd suggest the following
(untested) change:

- We leave the LOR feature visible
- We handle the LOR registers as RAZ/WI
- We still delivering an UNDEF when we're in the aforementioned case.

It would also solve the pathological cases that would result from
mixing 8.0 and 8.1+ CPUs in the same system (yeah, we all foolishly
thought it would never happen...).

Thoughts?

	M.

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 22fbbdbece3c..d50f912d3f4a 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -314,10 +314,15 @@ static bool trap_raz_wi(struct kvm_vcpu *vcpu,
 		return read_zero(vcpu, p);
 }
 
-static bool trap_undef(struct kvm_vcpu *vcpu,
-		       struct sys_reg_params *p,
-		       const struct sys_reg_desc *r)
+static bool trap_loregion(struct kvm_vcpu *vcpu,
+			  struct sys_reg_params *p,
+			  const struct sys_reg_desc *r)
 {
+	u64 val = read_sanitised_ftr_reg(SYS_ID_AA64MMFR1_EL1);
+
+	if (val & (0xfUL << ID_AA64MMFR1_LOR_SHIFT))
+		return trap_raz_wi(vcpu, p, r);
+
 	kvm_inject_undefined(vcpu);
 	return false;
 }
@@ -1040,11 +1045,6 @@ static u64 read_id_reg(struct sys_reg_desc const *r, bool raz)
 			kvm_debug("SVE unsupported for guests, suppressing\n");
 
 		val &= ~(0xfUL << ID_AA64PFR0_SVE_SHIFT);
-	} else if (id == SYS_ID_AA64MMFR1_EL1) {
-		if (val & (0xfUL << ID_AA64MMFR1_LOR_SHIFT))
-			kvm_debug("LORegions unsupported for guests, suppressing\n");
-
-		val &= ~(0xfUL << ID_AA64MMFR1_LOR_SHIFT);
 	}
 
 	return val;
@@ -1330,11 +1330,11 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	{ SYS_DESC(SYS_MAIR_EL1), access_vm_reg, reset_unknown, MAIR_EL1 },
 	{ SYS_DESC(SYS_AMAIR_EL1), access_vm_reg, reset_amair_el1, AMAIR_EL1 },
 
-	{ SYS_DESC(SYS_LORSA_EL1), trap_undef },
-	{ SYS_DESC(SYS_LOREA_EL1), trap_undef },
-	{ SYS_DESC(SYS_LORN_EL1), trap_undef },
-	{ SYS_DESC(SYS_LORC_EL1), trap_undef },
-	{ SYS_DESC(SYS_LORID_EL1), trap_undef },
+	{ SYS_DESC(SYS_LORSA_EL1), trap_loregion },
+	{ SYS_DESC(SYS_LOREA_EL1), trap_loregion },
+	{ SYS_DESC(SYS_LORN_EL1), trap_loregion },
+	{ SYS_DESC(SYS_LORC_EL1), trap_loregion },
+	{ SYS_DESC(SYS_LORID_EL1), trap_loregion },
 
 	{ SYS_DESC(SYS_VBAR_EL1), NULL, reset_val, VBAR_EL1, 0 },
 	{ SYS_DESC(SYS_DISR_EL1), NULL, reset_val, DISR_EL1, 0 },


-- 
Jazz is not dead, it just smell funny.

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

[Mark Rutland]

On Sun, Nov 04, 2018 at 11:25:00AM +0000, Marc Zyngier wrote:
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 22fbbdbece3c..d50f912d3f4a 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -314,10 +314,15 @@ static bool trap_raz_wi(struct kvm_vcpu *vcpu,
>  		return read_zero(vcpu, p);
>  }
>  
> -static bool trap_undef(struct kvm_vcpu *vcpu,
> -		       struct sys_reg_params *p,
> -		       const struct sys_reg_desc *r)

We should probably have a comment here, like:

/*
 * ARMv8.1 mandates at least a trivial LORegion implementation, where
 * all the RW registers are RES0 (which we can implement as RAZ/WI). On
 * an ARMv8.0 system, these registers should UNDEF.
 */

> +static bool trap_loregion(struct kvm_vcpu *vcpu,
> +			  struct sys_reg_params *p,
> +			  const struct sys_reg_desc *r)
>  {
> +	u64 val = read_sanitised_ftr_reg(SYS_ID_AA64MMFR1_EL1);
> +
> +	if (val & (0xfUL << ID_AA64MMFR1_LOR_SHIFT))
> +		return trap_raz_wi(vcpu, p, r);
> +
>  	kvm_inject_undefined(vcpu);
>  	return false;
>  }
> @@ -1040,11 +1045,6 @@ static u64 read_id_reg(struct sys_reg_desc const *r, bool raz)
>  			kvm_debug("SVE unsupported for guests, suppressing\n");
>  
>  		val &= ~(0xfUL << ID_AA64PFR0_SVE_SHIFT);
> -	} else if (id == SYS_ID_AA64MMFR1_EL1) {
> -		if (val & (0xfUL << ID_AA64MMFR1_LOR_SHIFT))
> -			kvm_debug("LORegions unsupported for guests, suppressing\n");
> -
> -		val &= ~(0xfUL << ID_AA64MMFR1_LOR_SHIFT);
>  	}
>  
>  	return val;
> @@ -1330,11 +1330,11 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>  	{ SYS_DESC(SYS_MAIR_EL1), access_vm_reg, reset_unknown, MAIR_EL1 },
>  	{ SYS_DESC(SYS_AMAIR_EL1), access_vm_reg, reset_amair_el1, AMAIR_EL1 },
>  
> -	{ SYS_DESC(SYS_LORSA_EL1), trap_undef },
> -	{ SYS_DESC(SYS_LOREA_EL1), trap_undef },
> -	{ SYS_DESC(SYS_LORN_EL1), trap_undef },
> -	{ SYS_DESC(SYS_LORC_EL1), trap_undef },
> -	{ SYS_DESC(SYS_LORID_EL1), trap_undef },
> +	{ SYS_DESC(SYS_LORSA_EL1), trap_loregion },
> +	{ SYS_DESC(SYS_LOREA_EL1), trap_loregion },
> +	{ SYS_DESC(SYS_LORN_EL1), trap_loregion },
> +	{ SYS_DESC(SYS_LORC_EL1), trap_loregion },
> +	{ SYS_DESC(SYS_LORID_EL1), trap_loregion },

LORID_EL1 is always RO, so we need to UNDEF on writes. Either we add a
separate helper, or special-case LORID_EL1 in trap_loregion().

Otherwise, this looks good to me.

Thanks,
Mark.