[Qemu-devel] [PATCH v2 0/6] HWCAP_CPUID registers for aarch64

[Qemu-devel] [PATCH v2 0/6] HWCAP_CPUID registers for aarch64

[Alex Bennée]

Hi,

I've re-spun the cpuid patches with the changes suggested by Peter's
review. The biggest change is the squashing of bits is now all data
driven with ARMCPRegUserSpaceInfo being used to control how bits are
altered for userspace presentation. This includes using glob matching
to set whole bunches to RAZ.

The testcase isn't as comprehensive as it could be because you need a
fairly new compiler (binutils) to emit all the various system register
id's to test. I did look into upgrading debian-arm64-cross with Buster
but I managed to find a bug in Debian's dependencies which rules out
upgrading for now.

checkpatch is complaining about the _m macro I used to group together
words in the masks I defined. I'm not sure adding the spaces makes it
as readable though.

The following patches need review:
 patch 0001/target arm relax permission checks for HWCAP_CPUI.patch
 patch 0002/target arm expose CPUID registers to userspace.patch
 patch 0003/target arm expose MPIDR_EL1 to userspace.patch
 patch 0004/target arm expose remaining CPUID registers as RA.patch
 patch 0006/tests tcg aarch64 userspace system register test.patch


Alex Bennée (6):
  target/arm: relax permission checks for HWCAP_CPUID registers
  target/arm: expose CPUID registers to userspace
  target/arm: expose MPIDR_EL1 to userspace
  target/arm: expose remaining CPUID registers as RAZ
  linux-user/elfload: enable HWCAP_CPUID for AArch64
  tests/tcg/aarch64: userspace system register test

 linux-user/elfload.c              |   1 +
 target/arm/cpu.h                  |  36 +++++++
 target/arm/helper.c               | 106 ++++++++++++++++--
 tests/tcg/aarch64/Makefile.target |   4 +-
 tests/tcg/aarch64/sysregs.c       | 172 ++++++++++++++++++++++++++++++
 5 files changed, 310 insertions(+), 9 deletions(-)
 create mode 100644 tests/tcg/aarch64/sysregs.c

-- 
2.20.1

[Qemu-devel] [PATCH v2 1/6] target/arm: relax permission checks for HWCAP_CPUID registers

[Alex Bennée]

Although technically not visible to userspace the kernel does make
them visible via a trap and emulate ABI. We provide a new permission
mask (PL0U_R) which maps to PL0_R for CONFIG_USER builds and adjust
the minimum permission check accordingly.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/arm/cpu.h    | 12 ++++++++++++
 target/arm/helper.c |  6 +++++-
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index a68bcc9fed..1616632dcb 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -2211,6 +2211,18 @@ static inline bool cptype_valid(int cptype)
 #define PL0_R (0x02 | PL1_R)
 #define PL0_W (0x01 | PL1_W)
 
+/*
+ * For user-mode some registers are accessible to EL0 via a kernel
+ * trap-and-emulate ABI. In this case we define the read permissions
+ * as actually being PL0_R. However some bits of any given register
+ * may still be masked.
+ */
+#ifdef CONFIG_USER_ONLY
+#define PL0U_R PL0_R
+#else
+#define PL0U_R PL1_R
+#endif
+
 #define PL3_RW (PL3_R | PL3_W)
 #define PL2_RW (PL2_R | PL2_W)
 #define PL1_RW (PL1_R | PL1_W)
diff --git a/target/arm/helper.c b/target/arm/helper.c
index d070879894..5857c0ba96 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6851,7 +6851,11 @@ void define_one_arm_cp_reg_with_opaque(ARMCPU *cpu,
     if (r->state != ARM_CP_STATE_AA32) {
         int mask = 0;
         switch (r->opc1) {
-        case 0: case 1: case 2:
+        case 0:
+            /* min_EL EL1, but some accessible to EL0 via kernel ABI */
+            mask = PL0U_R | PL1_RW;
+            break;
+        case 1: case 2:
             /* min_EL EL1 */
             mask = PL1_RW;
             break;
-- 
2.20.1

[Qemu-devel] [PATCH v2 2/6] target/arm: expose CPUID registers to userspace

[Alex Bennée]

A number of CPUID registers are exposed to userspace by modern Linux
kernels thanks to the "ARM64 CPU Feature Registers" ABI. For QEMU's
user-mode emulation we don't need to emulate the kernels trap but just
return the value the trap would have done. To avoid too much #ifdef
hackery we process ARMCPRegInfo with a new helper (modify_arm_cp_regs)
before defining the registers. The modify routine is driven by a
simple data structure which describes which bits are exported and
which are fixed.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v4
  - tweak commit message
  - use PL0U_R instead of PL1U_R to be less confusing
  - more CONFIG_USER logic for special cases
  - mask a bunch of bits for some registers
v5
  - use data driven modify_arm_cp_regs
---
 target/arm/cpu.h    | 21 ++++++++++++++++
 target/arm/helper.c | 59 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 80 insertions(+)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 1616632dcb..354df22102 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -2449,6 +2449,27 @@ static inline void define_one_arm_cp_reg(ARMCPU *cpu, const ARMCPRegInfo *regs)
 }
 const ARMCPRegInfo *get_arm_cp_reginfo(GHashTable *cpregs, uint32_t encoded_cp);
 
+/*
+ * Definition of an ARM co-processor register as viewed from
+ * userspace. This is used for presenting sanitised versions of
+ * registers to userspace when emulating the Linux AArch64 CPU
+ * ID/feature ABI (advertised as HWCAP_CPUID).
+ */
+typedef struct ARMCPRegUserSpaceInfo {
+    /* Name of register */
+    const char *name;
+
+    /* Only some bits are exported to user space */
+    uint64_t exported_bits;
+
+    /* Fixed bits are applied after the mask */
+    uint64_t fixed_bits;
+} ARMCPRegUserSpaceInfo;
+
+#define REGUSERINFO_SENTINEL { .name = NULL }
+
+void modify_arm_cp_regs(ARMCPRegInfo *regs, const ARMCPRegUserSpaceInfo *mods);
+
 /* CPWriteFn that can be used to implement writes-ignored behaviour */
 void arm_cp_write_ignore(CPUARMState *env, const ARMCPRegInfo *ri,
                          uint64_t value);
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 5857c0ba96..f90754cc11 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6103,6 +6103,30 @@ void register_cp_regs_for_features(ARMCPU *cpu)
               .resetvalue = cpu->pmceid1 },
             REGINFO_SENTINEL
         };
+#ifdef CONFIG_USER_ONLY
+        ARMCPRegUserSpaceInfo v8_user_idregs[] = {
+            { .name = "ID_AA64PFR0_EL1",
+              .exported_bits = 0x000f000f00ff0000,
+              .fixed_bits    = 0x0000000000000011 },
+            { .name = "ID_AA64PFR1_EL1",
+              .exported_bits = 0x00000000000000f0 },
+            { .name = "ID_AA64ZFR0_EL1"           },
+            { .name = "ID_AA64MMFR0_EL1",
+              .fixed_bits    = 0x00000000ff000000 },
+            { .name = "ID_AA64MMFR1_EL1"          },
+            { .name = "ID_AA64DFR0_EL1",
+              .fixed_bits    = 0x0000000000000006 },
+            { .name = "ID_AA64DFR1_EL1"           },
+            { .name = "ID_AA64AFR0_EL1"           },
+            { .name = "ID_AA64AFR1_EL1"           },
+            { .name = "ID_AA64ISAR0_EL1",
+              .exported_bits = 0x00fffffff0fffff0 },
+            { .name = "ID_AA64ISAR1_EL1",
+              .exported_bits = 0x000000f0ffffffff },
+            REGUSERINFO_SENTINEL
+        };
+        modify_arm_cp_regs(v8_idregs, v8_user_idregs);
+#endif
         /* RVBAR_EL1 is only implemented if EL1 is the highest EL */
         if (!arm_feature(env, ARM_FEATURE_EL3) &&
             !arm_feature(env, ARM_FEATURE_EL2)) {
@@ -6379,6 +6403,15 @@ void register_cp_regs_for_features(ARMCPU *cpu)
             .opc1 = CP_ANY, .opc2 = CP_ANY, .access = PL1_W,
             .type = ARM_CP_NOP | ARM_CP_OVERRIDE
         };
+#ifdef CONFIG_USER_ONLY
+        ARMCPRegUserSpaceInfo id_v8_user_midr_cp_reginfo[] = {
+            { .name = "MIDR_EL1",
+              .exported_bits = 0x00000000ffffffff },
+            { .name = "REVIDR_EL1"                },
+            REGUSERINFO_SENTINEL
+        };
+        modify_arm_cp_regs(id_v8_midr_cp_reginfo, id_v8_user_midr_cp_reginfo);
+#endif
         if (arm_feature(env, ARM_FEATURE_OMAPCP) ||
             arm_feature(env, ARM_FEATURE_STRONGARM)) {
             ARMCPRegInfo *r;
@@ -6960,6 +6993,32 @@ void define_arm_cp_regs_with_opaque(ARMCPU *cpu,
     }
 }
 
+/*
+ * Modify ARMCPRegInfo for access from userspace.
+ *
+ * This is a data driven modification directed by
+ * ARMCPRegUserSpaceInfo. All registers become ARM_CP_CONST as
+ * user-space cannot alter any values and dynamic values pertaining to
+ * execution state are hidden from user space view anyway.
+ */
+void modify_arm_cp_regs(ARMCPRegInfo *regs, const ARMCPRegUserSpaceInfo *mods)
+{
+    const ARMCPRegUserSpaceInfo *m;
+    ARMCPRegInfo *r;
+
+    for (m = mods; m->name; m++) {
+        for (r = regs; r->type != ARM_CP_SENTINEL; r++) {
+            if (strcmp(r->name, m->name) == 0) {
+                r->type = ARM_CP_CONST;
+                r->access = PL0U_R;
+                r->resetvalue &= m->exported_bits;
+                r->resetvalue |= m->fixed_bits;
+                break;
+            }
+        }
+    }
+}
+
 const ARMCPRegInfo *get_arm_cp_reginfo(GHashTable *cpregs, uint32_t encoded_cp)
 {
     return g_hash_table_lookup(cpregs, &encoded_cp);
-- 
2.20.1

[Qemu-devel] [PATCH v2 3/6] target/arm: expose MPIDR_EL1 to userspace

[Alex Bennée]

As this is a single register we could expose it with a simple ifdef
but we use the existing modify_arm_cp_regs mechanism for consistency.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/arm/helper.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index f90754cc11..f2f868ff92 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -3657,13 +3657,6 @@ static uint64_t mpidr_read(CPUARMState *env, const ARMCPRegInfo *ri)
     return mpidr_read_val(env);
 }
 
-static const ARMCPRegInfo mpidr_cp_reginfo[] = {
-    { .name = "MPIDR", .state = ARM_CP_STATE_BOTH,
-      .opc0 = 3, .crn = 0, .crm = 0, .opc1 = 0, .opc2 = 5,
-      .access = PL1_R, .readfn = mpidr_read, .type = ARM_CP_NO_RAW },
-    REGINFO_SENTINEL
-};
-
 static const ARMCPRegInfo lpae_cp_reginfo[] = {
     /* NOP AMAIR0/1 */
     { .name = "AMAIR0", .state = ARM_CP_STATE_BOTH,
@@ -6445,6 +6438,20 @@ void register_cp_regs_for_features(ARMCPU *cpu)
     }
 
     if (arm_feature(env, ARM_FEATURE_MPIDR)) {
+        ARMCPRegInfo mpidr_cp_reginfo[] = {
+            { .name = "MPIDR_EL1", .state = ARM_CP_STATE_BOTH,
+              .opc0 = 3, .crn = 0, .crm = 0, .opc1 = 0, .opc2 = 5,
+              .access = PL1_R, .readfn = mpidr_read, .type = ARM_CP_NO_RAW },
+            REGINFO_SENTINEL
+        };
+#ifdef CONFIG_USER_ONLY
+        ARMCPRegUserSpaceInfo mpidr_user_cp_reginfo[] = {
+            { .name = "MPIDR_EL1",
+              .fixed_bits = 0x0000000080000000 },
+            REGUSERINFO_SENTINEL
+        };
+        modify_arm_cp_regs(mpidr_cp_reginfo, mpidr_user_cp_reginfo);
+#endif
         define_arm_cp_regs(cpu, mpidr_cp_reginfo);
     }
 
-- 
2.20.1

[Qemu-devel] [PATCH v2 4/6] target/arm: expose remaining CPUID registers as RAZ

[Alex Bennée]

There are a whole bunch more registers in the CPUID space which are
currently not used but are exposed as RAZ. To avoid too much
duplication we expand ARMCPRegUserSpaceInfo to understand glob
patterns so we only need one entry to tweak whole ranges of registers.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/arm/cpu.h    |  3 +++
 target/arm/helper.c | 26 +++++++++++++++++++++++---
 2 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 354df22102..ae8ccc7dec 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -2459,6 +2459,9 @@ typedef struct ARMCPRegUserSpaceInfo {
     /* Name of register */
     const char *name;
 
+    /* Is the name actually a glob pattern */
+    bool is_glob;
+
     /* Only some bits are exported to user space */
     uint64_t exported_bits;
 
diff --git a/target/arm/helper.c b/target/arm/helper.c
index f2f868ff92..e999da165b 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6103,19 +6103,27 @@ void register_cp_regs_for_features(ARMCPU *cpu)
               .fixed_bits    = 0x0000000000000011 },
             { .name = "ID_AA64PFR1_EL1",
               .exported_bits = 0x00000000000000f0 },
+            { .name = "ID_AA64PFR*_EL1_RESERVED",
+              .is_glob = true                     },
             { .name = "ID_AA64ZFR0_EL1"           },
             { .name = "ID_AA64MMFR0_EL1",
               .fixed_bits    = 0x00000000ff000000 },
             { .name = "ID_AA64MMFR1_EL1"          },
+            { .name = "ID_AA64MMFR*_EL1_RESERVED",
+              .is_glob = true                     },
             { .name = "ID_AA64DFR0_EL1",
               .fixed_bits    = 0x0000000000000006 },
             { .name = "ID_AA64DFR1_EL1"           },
-            { .name = "ID_AA64AFR0_EL1"           },
-            { .name = "ID_AA64AFR1_EL1"           },
+            { .name = "ID_AA64DFR*_EL1_RESERVED",
+              .is_glob = true                     },
+            { .name = "ID_AA64AFR*",
+              .is_glob = true                     },
             { .name = "ID_AA64ISAR0_EL1",
               .exported_bits = 0x00fffffff0fffff0 },
             { .name = "ID_AA64ISAR1_EL1",
               .exported_bits = 0x000000f0ffffffff },
+            { .name = "ID_AA64ISAR*_EL1_RESERVED",
+              .is_glob = true                     },
             REGUSERINFO_SENTINEL
         };
         modify_arm_cp_regs(v8_idregs, v8_user_idregs);
@@ -7014,8 +7022,17 @@ void modify_arm_cp_regs(ARMCPRegInfo *regs, const ARMCPRegUserSpaceInfo *mods)
     ARMCPRegInfo *r;
 
     for (m = mods; m->name; m++) {
+        GPatternSpec *pat = NULL;
+        if (m->is_glob) {
+            pat = g_pattern_spec_new(m->name);
+        }
         for (r = regs; r->type != ARM_CP_SENTINEL; r++) {
-            if (strcmp(r->name, m->name) == 0) {
+            if (pat && g_pattern_match_string(pat, r->name)) {
+                r->type = ARM_CP_CONST;
+                r->access = PL0U_R;
+                r->resetvalue = 0;
+                /* continue */
+            } else if (strcmp(r->name, m->name) == 0) {
                 r->type = ARM_CP_CONST;
                 r->access = PL0U_R;
                 r->resetvalue &= m->exported_bits;
@@ -7023,6 +7040,9 @@ void modify_arm_cp_regs(ARMCPRegInfo *regs, const ARMCPRegUserSpaceInfo *mods)
                 break;
             }
         }
+        if (pat) {
+            g_pattern_spec_free(pat);
+        }
     }
 }
 
-- 
2.20.1

[Qemu-devel] [PATCH v2 5/6] linux-user/elfload: enable HWCAP_CPUID for AArch64

[Alex Bennée]

Userspace programs should (in theory) query the ELF HWCAP before
probing these registers. Now we have implemented them all make it
public.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/elfload.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 775a36ccdd..3a50d587ff 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -580,6 +580,7 @@ static uint32_t get_elf_hwcap(void)
 
     hwcaps |= ARM_HWCAP_A64_FP;
     hwcaps |= ARM_HWCAP_A64_ASIMD;
+    hwcaps |= ARM_HWCAP_A64_CPUID;
 
     /* probe for the extra features */
 #define GET_FEATURE_ID(feat, hwcap) \
-- 
2.20.1

[Qemu-devel] [PATCH v2 6/6] tests/tcg/aarch64: userspace system register test

[Alex Bennée]

This tests a bunch of registers that the kernel allows userspace to
read including the CPUID registers.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v4
  - also test for extra bits that shouldn't be exposed
v5
  - work around missing HWCAP_CPUID on older compilers
  - add more registers to test and some aarch32 regs
  - add more details commentary
  - fix up the masks (add a helper to help keep track)
  - add copyright header
---
 tests/tcg/aarch64/Makefile.target |   4 +-
 tests/tcg/aarch64/sysregs.c       | 172 ++++++++++++++++++++++++++++++
 2 files changed, 175 insertions(+), 1 deletion(-)
 create mode 100644 tests/tcg/aarch64/sysregs.c

diff --git a/tests/tcg/aarch64/Makefile.target b/tests/tcg/aarch64/Makefile.target
index 08c45b8470..fb40896e7b 100644
--- a/tests/tcg/aarch64/Makefile.target
+++ b/tests/tcg/aarch64/Makefile.target
@@ -7,11 +7,13 @@ VPATH 		+= $(AARCH64_SRC)
 
 # we don't build any of the ARM tests
 AARCH64_TESTS=$(filter-out $(ARM_TESTS), $(TESTS))
-AARCH64_TESTS+=fcvt
+AARCH64_TESTS+=fcvt sysregs
 TESTS:=$(AARCH64_TESTS)
 
 fcvt: LDFLAGS+=-lm
 
+sysregs: CFLAGS+=-march=armv8.1-a+sve
+
 run-fcvt: fcvt
 	$(call run-test,$<,$(QEMU) $<, "$< on $(TARGET_NAME)")
 	$(call diff-out,$<,$(AARCH64_SRC)/fcvt.ref)
diff --git a/tests/tcg/aarch64/sysregs.c b/tests/tcg/aarch64/sysregs.c
new file mode 100644
index 0000000000..40cf8d2877
--- /dev/null
+++ b/tests/tcg/aarch64/sysregs.c
@@ -0,0 +1,172 @@
+/*
+ * Check emulated system register access for linux-user mode.
+ *
+ * See: https://www.kernel.org/doc/Documentation/arm64/cpu-feature-registers.txt
+ *
+ * Copyright (c) 2019 Linaro
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include <asm/hwcap.h>
+#include <stdio.h>
+#include <sys/auxv.h>
+#include <signal.h>
+#include <string.h>
+#include <stdbool.h>
+
+#ifndef HWCAP_CPUID
+#define HWCAP_CPUID (1 << 11)
+#endif
+
+int failed_bit_count;
+
+/* Read and print system register `id' value */
+#define get_cpu_reg(id) ({                                      \
+            unsigned long __val = 0xdeadbeef;                   \
+            asm("mrs %0, "#id : "=r" (__val));                  \
+            printf("%-20s: 0x%016lx\n", #id, __val);            \
+            __val;                                               \
+        })
+
+/* As above but also check no bits outside of `mask' are set*/
+#define get_cpu_reg_check_mask(id, mask) ({                     \
+            unsigned long __cval = get_cpu_reg(id);             \
+            unsigned long __extra = __cval & ~mask;             \
+            if (__extra) {                                      \
+                printf("%-20s: 0x%016lx\n", "  !!extra bits!!", __extra);   \
+                failed_bit_count++;                            \
+            }                                                   \
+})
+
+/* As above but check RAZ */
+#define get_cpu_reg_check_zero(id) ({                           \
+            unsigned long __val = 0xdeadbeef;                   \
+            asm("mrs %0, "#id : "=r" (__val));                  \
+            if (__val) {                                        \
+                printf("%-20s: 0x%016lx (not RAZ!)\n", #id, __val);        \
+                failed_bit_count++;                            \
+            }                                                   \
+})
+
+/* Chunk up mask into 63:48, 47:32, 31:16, 15:0 to ease counting */
+#define _m(a, b, c, d) (0x ## a ## b ## c ## d ##ULL)
+
+bool should_fail;
+int should_fail_count;
+int should_not_fail_count;
+uintptr_t failed_pc[10];
+
+void sigill_handler(int signo, siginfo_t *si, void *data)
+{
+    ucontext_t *uc = (ucontext_t *)data;
+
+    if (should_fail) {
+        should_fail_count++;
+    } else {
+        uintptr_t pc = (uintptr_t) uc->uc_mcontext.pc;
+        failed_pc[should_not_fail_count++] =  pc;
+    }
+    uc->uc_mcontext.pc += 4;
+}
+
+int main(void)
+{
+    struct sigaction sa;
+
+    /* Hook in a SIGILL handler */
+    memset(&sa, 0, sizeof(struct sigaction));
+    sa.sa_flags = SA_SIGINFO;
+    sa.sa_sigaction = &sigill_handler;
+    sigemptyset(&sa.sa_mask);
+
+    if (sigaction(SIGILL, &sa, 0) != 0) {
+        perror("sigaction");
+        return 1;
+    }
+
+    /* Counter values have been exposed since Linux 4.12 */
+    printf("Checking Counter registers\n");
+
+    get_cpu_reg(ctr_el0);
+    get_cpu_reg(cntvct_el0);
+    get_cpu_reg(cntfrq_el0);
+
+    /* HWCAP_CPUID indicates we can read feature registers, since Linux 4.11 */
+    if (!(getauxval(AT_HWCAP) & HWCAP_CPUID)) {
+        printf("CPUID registers unavailable\n");
+        return 1;
+    } else {
+        printf("Checking CPUID registers\n");
+    }
+
+    /*
+     * Some registers only expose some bits to user-space. Anything
+     * that is IMPDEF is exported as 0 to user-space. The _mask checks
+     * assert no extra bits are set.
+     *
+     * This check is *not* comprehensive as some fields are set to
+     * minimum valid fields - for the purposes of this check allowed
+     * to have non-zero values.
+     */
+    get_cpu_reg_check_mask(id_aa64isar0_el1, _m(00ff,ffff,f0ff,fff0));
+    get_cpu_reg_check_mask(id_aa64isar1_el1, _m(0000,00f0,ffff,ffff));
+    /* TGran4 & TGran64 as pegged to -1 */
+    get_cpu_reg_check_mask(id_aa64mmfr0_el1, _m(0000,0000,ff00,0000));
+    get_cpu_reg_check_zero(id_aa64mmfr1_el1);
+    /* EL1/EL0 reported as AA64 only */
+    get_cpu_reg_check_mask(id_aa64pfr0_el1,  _m(000f,000f,00ff,0011));
+    get_cpu_reg_check_mask(id_aa64pfr1_el1,  _m(0000,0000,0000,00f0));
+    /* all hidden, DebugVer fixed to 0x6 (ARMv8 debug architecture) */
+    get_cpu_reg_check_mask(id_aa64dfr0_el1,  _m(0000,0000,0000,0006));
+    get_cpu_reg_check_zero(id_aa64dfr1_el1);
+    get_cpu_reg_check_zero(id_aa64zfr0_el1);
+
+    get_cpu_reg_check_zero(id_aa64afr0_el1);
+    get_cpu_reg_check_zero(id_aa64afr1_el1);
+
+    get_cpu_reg_check_mask(midr_el1,         _m(0000,0000,ffff,ffff));
+    /* mpidr sets bit 31, everything else hidden */
+    get_cpu_reg_check_mask(mpidr_el1,        _m(0000,0000,8000,0000));
+    /* REVIDR is all IMPDEF so should be all zeros to user-space */
+    get_cpu_reg_check_zero(revidr_el1);
+
+    /*
+     * There are a block of more registers that are RAZ in the rest of
+     * the Op0=3, Op1=0, CRn=0, CRm=0,4,5,6,7 space. However for
+     * brevity we don't check stuff that is currently un-allocated
+     * here. Feel free to add them ;-)
+     */
+
+    printf("Remaining registers should fail\n");
+    should_fail = true;
+
+    /* Unexposed register access causes SIGILL */
+    get_cpu_reg(id_mmfr0_el1);
+    get_cpu_reg(id_mmfr1_el1);
+    get_cpu_reg(id_mmfr2_el1);
+    get_cpu_reg(id_mmfr3_el1);
+
+    get_cpu_reg(mvfr0_el1);
+    get_cpu_reg(mvfr1_el1);
+
+    if (should_not_fail_count > 0) {
+        int i;
+        for (i = 0; i < should_not_fail_count; i++) {
+            uintptr_t pc = failed_pc[i];
+            uint32_t insn = *(uint32_t *) pc;
+            printf("insn %#x @ %#lx unexpected FAIL\n", insn, pc);
+        }
+        return 1;
+    }
+
+    if (failed_bit_count > 0) {
+        printf("Extra information leaked to user-space!\n");
+        return 1;
+    }
+
+    return should_fail_count == 6 ? 0 : 1;
+}
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 0/6] HWCAP_CPUID registers for aarch64

[Alex Bennée]

Alex Bennée <alex.bennee@linaro.org> writes:

> Hi,
>
> I've re-spun the cpuid patches with the changes suggested by Peter's
> review. The biggest change is the squashing of bits is now all data
> driven with ARMCPRegUserSpaceInfo being used to control how bits are
> altered for userspace presentation. This includes using glob matching
> to set whole bunches to RAZ.

Ping (+ CC'd which I missed on the submission)

>
> The testcase isn't as comprehensive as it could be because you need a
> fairly new compiler (binutils) to emit all the various system register
> id's to test. I did look into upgrading debian-arm64-cross with Buster
> but I managed to find a bug in Debian's dependencies which rules out
> upgrading for now.

We have a newer compiler so I can add more on the next re-spin, reviews
pending.

>
> checkpatch is complaining about the _m macro I used to group together
> words in the masks I defined. I'm not sure adding the spaces makes it
> as readable though.
>
> The following patches need review:
>  patch 0001/target arm relax permission checks for HWCAP_CPUI.patch
>  patch 0002/target arm expose CPUID registers to userspace.patch
>  patch 0003/target arm expose MPIDR_EL1 to userspace.patch
>  patch 0004/target arm expose remaining CPUID registers as RA.patch
>  patch 0006/tests tcg aarch64 userspace system register test.patch
>
>
> Alex Bennée (6):
>   target/arm: relax permission checks for HWCAP_CPUID registers
>   target/arm: expose CPUID registers to userspace
>   target/arm: expose MPIDR_EL1 to userspace
>   target/arm: expose remaining CPUID registers as RAZ
>   linux-user/elfload: enable HWCAP_CPUID for AArch64
>   tests/tcg/aarch64: userspace system register test
>
>  linux-user/elfload.c              |   1 +
>  target/arm/cpu.h                  |  36 +++++++
>  target/arm/helper.c               | 106 ++++++++++++++++--
>  tests/tcg/aarch64/Makefile.target |   4 +-
>  tests/tcg/aarch64/sysregs.c       | 172 ++++++++++++++++++++++++++++++
>  5 files changed, 310 insertions(+), 9 deletions(-)
>  create mode 100644 tests/tcg/aarch64/sysregs.c


--
Alex Bennée

Re: [Qemu-devel] [PATCH v2 0/6] HWCAP_CPUID registers for aarch64

[Peter Maydell]

On Tue, 5 Feb 2019 at 20:21, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Hi,
>
> I've re-spun the cpuid patches with the changes suggested by Peter's
> review. The biggest change is the squashing of bits is now all data
> driven with ARMCPRegUserSpaceInfo being used to control how bits are
> altered for userspace presentation. This includes using glob matching
> to set whole bunches to RAZ.
>
> The testcase isn't as comprehensive as it could be because you need a
> fairly new compiler (binutils) to emit all the various system register
> id's to test. I did look into upgrading debian-arm64-cross with Buster
> but I managed to find a bug in Debian's dependencies which rules out
> upgrading for now.
>
> checkpatch is complaining about the _m macro I used to group together
> words in the masks I defined. I'm not sure adding the spaces makes it
> as readable though.
>
> The following patches need review:
>  patch 0001/target arm relax permission checks for HWCAP_CPUI.patch
>  patch 0002/target arm expose CPUID registers to userspace.patch
>  patch 0003/target arm expose MPIDR_EL1 to userspace.patch
>  patch 0004/target arm expose remaining CPUID registers as RA.patch
>  patch 0006/tests tcg aarch64 userspace system register test.patch

I'll take 1-5 into target-arm.next, but 6 breaks 'make check-tcg'
for me:

  BUILD   aarch64 guest-tests with aarch64-linux-gnu-gcc
cc1: error: invalid feature modifier in ‘-march=armv8.1-a+sve’

I imagine you're assuming a newer binutils than Ubuntu ships.
You should probably go for just encoding the insns rather than using
their names -- binutils should accept "S<op0>_<op1>_<Cn>_<Cm>_<op2>"
as a register name for mrs and msr insn.

It would also be nice to test some of the sysreg cases which
go through your "glob the register name" code path -- I think
at the moment you are only testing the ones which use the
specific-match case ? (Ideally we'd loop through and test that
we could read everything in the defined-to-be-exposed encoding
range, though that would require some slightly tedious preprocessor
macro effort.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 0/6] HWCAP_CPUID registers for aarch64

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Tue, 5 Feb 2019 at 20:21, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> Hi,
>>
>> I've re-spun the cpuid patches with the changes suggested by Peter's
>> review. The biggest change is the squashing of bits is now all data
>> driven with ARMCPRegUserSpaceInfo being used to control how bits are
>> altered for userspace presentation. This includes using glob matching
>> to set whole bunches to RAZ.
>>
>> The testcase isn't as comprehensive as it could be because you need a
>> fairly new compiler (binutils) to emit all the various system register
>> id's to test. I did look into upgrading debian-arm64-cross with Buster
>> but I managed to find a bug in Debian's dependencies which rules out
>> upgrading for now.
>>
>> checkpatch is complaining about the _m macro I used to group together
>> words in the masks I defined. I'm not sure adding the spaces makes it
>> as readable though.
>>
>> The following patches need review:
>>  patch 0001/target arm relax permission checks for HWCAP_CPUI.patch
>>  patch 0002/target arm expose CPUID registers to userspace.patch
>>  patch 0003/target arm expose MPIDR_EL1 to userspace.patch
>>  patch 0004/target arm expose remaining CPUID registers as RA.patch
>>  patch 0006/tests tcg aarch64 userspace system register test.patch
>
> I'll take 1-5 into target-arm.next, but 6 breaks 'make check-tcg'
> for me:
>
>   BUILD   aarch64 guest-tests with aarch64-linux-gnu-gcc
> cc1: error: invalid feature modifier in ‘-march=armv8.1-a+sve’
>
> I imagine you're assuming a newer binutils than Ubuntu ships.
> You should probably go for just encoding the insns rather than using
> their names -- binutils should accept "S<op0>_<op1>_<Cn>_<Cm>_<op2>"
> as a register name for mrs and msr insn.


Yeah I'm now on buster so have a newer gcc:

  aarch64-linux-gnu-gcc (Debian 8.2.0-11) 8.2.0

But I'm surprised that the pauth test cases worked on your system.
However they should all work with the recent docker update which you
merged earlier this week.

> It would also be nice to test some of the sysreg cases which
> go through your "glob the register name" code path -- I think
> at the moment you are only testing the ones which use the
> specific-match case ? (Ideally we'd loop through and test that
> we could read everything in the defined-to-be-exposed encoding
> range, though that would require some slightly tedious preprocessor
> macro effort.)

Yeah the main reason I didn't push to hard to do it but I guess we
should for completeness sake. It's times like this I wish C's macros
weren't the pale imitations of what you can do with lisp (or rust ;-).

--
Alex Bennée