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

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

Tested on arm64; cross-build tested for arm32.


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   | 63 +++++++++++++++++++++++++++++++++++--
 target/arm/machine.c |  3 +-
 9 files changed, 114 insertions(+), 43 deletions(-)

-- 
2.17.2

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

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

[Richard Henderson]

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

diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index 5de8ff0ac5..6ed80eadc2 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -443,17 +443,41 @@ 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;
+    }
+    assert(ret <= UINT32_MAX);
+    *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 +498,43 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
     ahcf->target = init.target;
     ahcf->dtb_compatible = "arm,arm-v8";
 
+    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));
+
+    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));
+
     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 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.

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..da08f7aab8 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 i, 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 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 da08f7aab8..f23cc77d9e 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 i, 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 5/5] target/arm: Convert t32ee from feature bit to isar3 test

[Richard Henderson]

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 055f6a95ab..45d6836bb9 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 f23cc77d9e..bf39937a08 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 3/5] target/arm: Introduce read_sys_reg32 for kvm32

[Richard Henderson]

On 10/24/18 12:37 PM, Richard Henderson wrote:
> -    int i, ret, fdarray[3];
> +    int i, err = 0, fdarray[3];

Bah.  The bit about having compile-tested for arm32 was fake news -- "i" is now
unused.  Ho hum.

> @@ -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);


r~

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

[Peter Maydell]

On 24 October 2018 at 12:37, Richard Henderson
<richard.henderson@linaro.org> wrote:
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/kvm64.c | 63 ++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 61 insertions(+), 2 deletions(-)
>
> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> index 5de8ff0ac5..6ed80eadc2 100644
> --- a/target/arm/kvm64.c
> +++ b/target/arm/kvm64.c
> @@ -443,17 +443,41 @@ 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;
> +    }
> +    assert(ret <= UINT32_MAX);

In AArch64, there isn't really any such thing as a 32 bit sysreg.
Where the Arm ARM refers to 32 bit system registers this is just
a shorthand for "64-bit register where the top 32 bits are currently
RES0". This assert() would result in QEMU crashing if we ever ran
it on a host for some hypothetical future architecture which
assigned meanings to some of the high bits. (Granted, this is
unlikely for the specific case of the ID registers which track
the AArch32 ID register values.)

I think I would prefer it if we expanded the id_isar* fields
in the ARMISARegisters struct to uint64_t. If you dislike
that, I think we should make this code fail a bit more gracefully
in the presence of an unexpected extension into the high bits
of these registers. Or just ignore the high bits, since we're
effectively trusting that future architecture versions use
a compatible meaning for these registers anyway.

> +    *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);
> +}
>

thanks
-- PMM

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

[Peter Maydell]

On 24 October 2018 at 12:37, 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>

> +    /*
> +     * FIXME: There is not yet a way to read MVFR2.
> +     * Fortunately there is not yet anything in there that affects migration.
> +     */

We should bring that up with the KVM folks (cc'd)...

Presumably KVM_REG_ARM_VFP_MVFR2 should be 0x1005 and the code
in the kernel for handling this bit of the get/set-one-reg
ioctls needs to have code for it (including returning 0
for v7 CPUs, where this VMRS register encoding was UNPREDICTABLE).

thanks
-- PMM

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

[Peter Maydell]

On 29 October 2018 at 15:13, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 24 October 2018 at 12:37, 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>
>
>> +    /*
>> +     * FIXME: There is not yet a way to read MVFR2.
>> +     * Fortunately there is not yet anything in there that affects migration.
>> +     */
>
> We should bring that up with the KVM folks (cc'd)...
>
> Presumably KVM_REG_ARM_VFP_MVFR2 should be 0x1005 and the code
> in the kernel for handling this bit of the get/set-one-reg
> ioctls needs to have code for it (including returning 0
> for v7 CPUs, where this VMRS register encoding was UNPREDICTABLE).

There's an argument for "fail the ioctl on v7 CPUs" rather than
"return 0"; I don't think I have a strong opinion, since in
practice userspace needs to handle the "doesn't exist" case
when it's running on an older kernel anyway.

thanks
-- PMM

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

[Marc Zyngier]

On 29/10/18 15:21, Peter Maydell wrote:
> On 29 October 2018 at 15:13, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 24 October 2018 at 12:37, 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>
>>
>>> +    /*
>>> +     * FIXME: There is not yet a way to read MVFR2.
>>> +     * Fortunately there is not yet anything in there that affects migration.
>>> +     */
>>
>> We should bring that up with the KVM folks (cc'd)...
>>
>> Presumably KVM_REG_ARM_VFP_MVFR2 should be 0x1005 and the code
>> in the kernel for handling this bit of the get/set-one-reg
>> ioctls needs to have code for it (including returning 0
>> for v7 CPUs, where this VMRS register encoding was UNPREDICTABLE).
> 
> There's an argument for "fail the ioctl on v7 CPUs" rather than
> "return 0"; I don't think I have a strong opinion, since in
> practice userspace needs to handle the "doesn't exist" case
> when it's running on an older kernel anyway.

My temptation would be not to expose it at all when running on a v7
core, and return an error rather than zero.

The other issue is that we currently don't support running 32bit KVM on
any ARMv8 platform, as we strictly check the CPUs we want to run on (A7
and A15). I remember seeing patches that would allow any host core to be
used (similar to what we have on the 64bit side), but that never made it
in the tree.

If there is interest, I can revive this effort.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

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

[Peter Maydell]

On 29 October 2018 at 15:40, Marc Zyngier <marc.zyngier@arm.com> wrote:
> My temptation would be not to expose it at all when running on a v7
> core, and return an error rather than zero.
>
> The other issue is that we currently don't support running 32bit KVM on
> any ARMv8 platform, as we strictly check the CPUs we want to run on (A7
> and A15). I remember seeing patches that would allow any host core to be
> used (similar to what we have on the 64bit side), but that never made it
> in the tree.

Ah, that's convenient in some ways. It means we can define the
API to be "for v7, no register available via the ONE_REG API;
for v8, always present", provided we add the constant and the
support before we turn on any actual v8 CPUs for 32-bit KVM
(avoiding the awkward case of "v8 but kernel doesn't expose
MVFR2").

I don't think we particularly care about the 32-bit-kvm-on-v8
part, but it would be good to nail down this wrinkle so we don't
forget about it, maybe ?

thanks
-- PMM

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

[Marc Zyngier]

On 29/10/18 15:48, Peter Maydell wrote:
> On 29 October 2018 at 15:40, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> My temptation would be not to expose it at all when running on a v7
>> core, and return an error rather than zero.
>>
>> The other issue is that we currently don't support running 32bit KVM on
>> any ARMv8 platform, as we strictly check the CPUs we want to run on (A7
>> and A15). I remember seeing patches that would allow any host core to be
>> used (similar to what we have on the 64bit side), but that never made it
>> in the tree.
> 
> Ah, that's convenient in some ways. It means we can define the
> API to be "for v7, no register available via the ONE_REG API;
> for v8, always present", provided we add the constant and the
> support before we turn on any actual v8 CPUs for 32-bit KVM
> (avoiding the awkward case of "v8 but kernel doesn't expose
> MVFR2").
> 
> I don't think we particularly care about the 32-bit-kvm-on-v8
> part, but it would be good to nail down this wrinkle so we don't
> forget about it, maybe ?

Absolutely. I'll write something up.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

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

[Richard Henderson]

On 10/29/18 2:58 PM, Peter Maydell wrote:
> I think I would prefer it if we expanded the id_isar* fields
> in the ARMISARegisters struct to uint64_t. If you dislike
> that, I think we should make this code fail a bit more gracefully
> in the presence of an unexpected extension into the high bits
> of these registers. Or just ignore the high bits, since we're
> effectively trusting that future architecture versions use
> a compatible meaning for these registers anyway.

Given these options, I'd prefer to just ignore the high bits.
I'm fairly comfortable trusting that the architecture gods
won't mess up and assign values in there.  Otherwise they
would have already expanded instead of adding ID_ISAR6.


r~

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

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> 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.
>
> Tested on arm64; cross-build tested for arm32.

I'm still seeing the following assert on qemu-test:

  qemu-system-aarch64: /home/alex/lsrc/qemu.git/target/arm/cpu.c:832: arm_cpu_realizefn: Assertion `cpu_isar_feature(arm_div, cpu)' failed.

Which is a regression caused by:

  7e0cf8b: target/arm: Convert division from feature bits to isar0 tests

I think the problem is the we trip over the assert because:

    /* Some features automatically imply others: */
    if (arm_feature(env, ARM_FEATURE_V8)) {
        if (arm_feature(env, ARM_FEATURE_M)) {
            set_feature(env, ARM_FEATURE_V7);
        } else {
            set_feature(env, ARM_FEATURE_V7VE);
        }
    }

Allows:

    if (arm_feature(env, ARM_FEATURE_V7VE)) {
        assert(cpu_isar_feature(arm_div, cpu));

Which isn't strictly true on kvm guests.


>
>
> 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   | 63 +++++++++++++++++++++++++++++++++++--
>  target/arm/machine.c |  3 +-
>  9 files changed, 114 insertions(+), 43 deletions(-)


--
Alex Bennée

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

[Peter Maydell]

On 1 November 2018 at 17:26, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Richard Henderson <richard.henderson@linaro.org> writes:
>
>> 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.
>>
>> Tested on arm64; cross-build tested for arm32.
>
> I'm still seeing the following assert on qemu-test:
>
>   qemu-system-aarch64: /home/alex/lsrc/qemu.git/target/arm/cpu.c:832: arm_cpu_realizefn: Assertion `cpu_isar_feature(arm_div, cpu)' failed.
>
> Which is a regression caused by:
>
>   7e0cf8b: target/arm: Convert division from feature bits to isar0 tests
>
> I think the problem is the we trip over the assert because:
>
>     /* Some features automatically imply others: */
>     if (arm_feature(env, ARM_FEATURE_V8)) {
>         if (arm_feature(env, ARM_FEATURE_M)) {
>             set_feature(env, ARM_FEATURE_V7);
>         } else {
>             set_feature(env, ARM_FEATURE_V7VE);
>         }
>     }
>
> Allows:
>
>     if (arm_feature(env, ARM_FEATURE_V7VE)) {
>         assert(cpu_isar_feature(arm_div, cpu));
>
> Which isn't strictly true on kvm guests.

KVM guests should definitely all be v7VE and all have
the arm divide instruction, if they implement AArch32 at all.
I think what we're hitting here is the case where the host
CPU has no AArch32 support. In that case the ID_ISAR0_EL1
sysreg (which we read from KVM and use to populate the
cpu->isar struct) has an UNKNOWN value.

thanks
-- PMM

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

[Peter Maydell]

On 1 November 2018 at 17:30, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 1 November 2018 at 17:26, Alex Bennée <alex.bennee@linaro.org> wrote:
>> I think the problem is the we trip over the assert because:
>>
>>     /* Some features automatically imply others: */
>>     if (arm_feature(env, ARM_FEATURE_V8)) {
>>         if (arm_feature(env, ARM_FEATURE_M)) {
>>             set_feature(env, ARM_FEATURE_V7);
>>         } else {
>>             set_feature(env, ARM_FEATURE_V7VE);
>>         }
>>     }
>>
>> Allows:
>>
>>     if (arm_feature(env, ARM_FEATURE_V7VE)) {
>>         assert(cpu_isar_feature(arm_div, cpu));
>>
>> Which isn't strictly true on kvm guests.
>
> KVM guests should definitely all be v7VE and all have
> the arm divide instruction, if they implement AArch32 at all.
> I think what we're hitting here is the case where the host
> CPU has no AArch32 support. In that case the ID_ISAR0_EL1
> sysreg (which we read from KVM and use to populate the
> cpu->isar struct) has an UNKNOWN value.

So I think the assert should probably be
"assert that if this cpu has any EL that can execute at
AArch32 then it has the arm_div ISAR feature". You can
determine the former by looking at ID_AA64PFR0_EL1 field
EL0 (bits [3:0]), which will be >= 0b0010 if AArch32 is
supported at any EL.

thanks
-- PMM

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

[Peter Maydell]

On 24 October 2018 at 12:37, Richard Henderson
<richard.henderson@linaro.org> wrote:
> 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.
>
> 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;
>  }

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

thanks
-- PMM

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

[Peter Maydell]

On 24 October 2018 at 12:37, Richard Henderson
<richard.henderson@linaro.org> wrote:
> Assert that the value to be written is the correct size.
> No change in functionality here, just mirroring the same
> function from kvm64.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/kvm32.c | 41 ++++++++++++++++-------------------------
>  1 file changed, 16 insertions(+), 25 deletions(-)

> -    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));


Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
with the declaration of 'i' removed.

(Confusingly, ENCODE_CP_REG and ARM_CP15_REG32 take the
op1/crn/crm/op2 arguments in different orders.)

thanks
-- PMM

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

[Peter Maydell]

On 24 October 2018 at 12:37, Richard Henderson
<richard.henderson@linaro.org> wrote:
> 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(-)

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

thanks
-- PMM

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

[Richard Henderson]

On 11/2/18 2:30 PM, Peter Maydell wrote:
> (Confusingly, ENCODE_CP_REG and ARM_CP15_REG32 take the
> op1/crn/crm/op2 arguments in different orders.)

As Shaggy said, "It wasn't me".  ;-)


r~

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

[Peter Maydell]

On 29 October 2018 at 16:03, Richard Henderson
<richard.henderson@linaro.org> wrote:
> On 10/29/18 2:58 PM, Peter Maydell wrote:
>> I think I would prefer it if we expanded the id_isar* fields
>> in the ARMISARegisters struct to uint64_t. If you dislike
>> that, I think we should make this code fail a bit more gracefully
>> in the presence of an unexpected extension into the high bits
>> of these registers. Or just ignore the high bits, since we're
>> effectively trusting that future architecture versions use
>> a compatible meaning for these registers anyway.
>
> Given these options, I'd prefer to just ignore the high bits.
> I'm fairly comfortable trusting that the architecture gods
> won't mess up and assign values in there.  Otherwise they
> would have already expanded instead of adding ID_ISAR6.

OK with me.

I think we could also add a comment noting that if the host
CPU doesn't have AArch32 then the AArch32 sysregs still exist
to be read, but they return UNKNOWN values.

thanks
-- PMM