[Qemu-devel] [PATCH v1 0/4] HWCAP_CPUID registers for aarch64

[Qemu-devel] [PATCH v1 0/4] HWCAP_CPUID registers for aarch64

[Alex Bennée]

Hi,

I posted a series to expose some of the CPU registers to user-space.
The counter registers got merged at the time but the HW_CPUID had some
comments that needed addressing. I've re-spun the series and cleaned
it up, hopefully addressing the comments at the same time. The test
case has been expanded.

The ABI is described in the kernel document:

  https://www.kernel.org/doc/Documentation/arm64/cpu-feature-registers.txt

but from QEMU's point of view we just need to ensure the correct
values are reported for CONFIG_USER_ONLY.

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

 linux-user/elfload.c              |   1 +
 target/arm/cpu.h                  |  12 +++
 target/arm/helper.c               |  57 ++++++++++----
 tests/tcg/aarch64/Makefile.target |   2 +-
 tests/tcg/aarch64/sysregs.c       | 120 ++++++++++++++++++++++++++++++
 5 files changed, 175 insertions(+), 17 deletions(-)
 create mode 100644 tests/tcg/aarch64/sysregs.c

-- 
2.17.1

[Qemu-devel] [PATCH v1 1/4] 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 ff81db420d..3b3c359cca 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -2202,6 +2202,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 92666e5208..42c1c0b144 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6731,7 +6731,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.17.1

[Qemu-devel] [PATCH v1 2/4] 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. For this we use the PL0U_R
permission mask which allows this access in CONFIG_USER mode.

Some registers only return a subset of their contents so we need
specific CONFIG_USER_ONLY logic to do this.

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
---
 target/arm/helper.c | 51 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 36 insertions(+), 15 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 42c1c0b144..68808e7293 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -3543,7 +3543,7 @@ static uint64_t mpidr_read(CPUARMState *env, const ARMCPRegInfo *ri)
 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 },
+      .access = PL0U_R, .readfn = mpidr_read, .type = ARM_CP_NO_RAW },
     REGINFO_SENTINEL
 };
 
@@ -5488,6 +5488,7 @@ static uint64_t id_pfr1_read(CPUARMState *env, const ARMCPRegInfo *ri)
     return pfr1;
 }
 
+#ifndef CONFIG_USER_ONLY
 static uint64_t id_aa64pfr0_read(CPUARMState *env, const ARMCPRegInfo *ri)
 {
     ARMCPU *cpu = arm_env_get_cpu(env);
@@ -5498,6 +5499,7 @@ static uint64_t id_aa64pfr0_read(CPUARMState *env, const ARMCPRegInfo *ri)
     }
     return pfr0;
 }
+#endif
 
 /* Shared logic between LORID and the rest of the LOR* registers.
  * Secure state has already been delt with.
@@ -5799,18 +5801,26 @@ void register_cp_regs_for_features(ARMCPU *cpu)
          * define new registers here.
          */
         ARMCPRegInfo v8_idregs[] = {
-            /* ID_AA64PFR0_EL1 is not a plain ARM_CP_CONST because we don't
-             * know the right value for the GIC field until after we
-             * define these regs.
+            /* ID_AA64PFR0_EL1 is not a plain ARM_CP_CONST for system
+             * emulation because we don't know the right value for the
+             * GIC field until after we define these regs. For
+             * user-mode HWCAP_CPUID emulation the GIC bits are masked
+             * anyway.
              */
             { .name = "ID_AA64PFR0_EL1", .state = ARM_CP_STATE_AA64,
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 0,
+#ifndef CONFIG_USER_ONLY
               .access = PL1_R, .type = ARM_CP_NO_RAW,
               .readfn = id_aa64pfr0_read,
-              .writefn = arm_cp_write_ignore },
+              .writefn = arm_cp_write_ignore
+#else
+              .access = PL0U_R, .type = ARM_CP_CONST,
+              .resetvalue = cpu->isar.id_aa64pfr0 & 0x000f000f0ff0000ULL
+#endif
+            },
             { .name = "ID_AA64PFR1_EL1", .state = ARM_CP_STATE_AA64,
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 1,
-              .access = PL1_R, .type = ARM_CP_CONST,
+              .access = PL0U_R, .type = ARM_CP_CONST,
               .resetvalue = cpu->isar.id_aa64pfr1},
             { .name = "ID_AA64PFR2_EL1_RESERVED", .state = ARM_CP_STATE_AA64,
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 2,
@@ -5839,11 +5849,11 @@ void register_cp_regs_for_features(ARMCPU *cpu)
               .resetvalue = 0 },
             { .name = "ID_AA64DFR0_EL1", .state = ARM_CP_STATE_AA64,
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 0,
-              .access = PL1_R, .type = ARM_CP_CONST,
+              .access = PL0U_R, .type = ARM_CP_CONST,
               .resetvalue = cpu->id_aa64dfr0 },
             { .name = "ID_AA64DFR1_EL1", .state = ARM_CP_STATE_AA64,
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 1,
-              .access = PL1_R, .type = ARM_CP_CONST,
+              .access = PL0U_R, .type = ARM_CP_CONST,
               .resetvalue = cpu->id_aa64dfr1 },
             { .name = "ID_AA64DFR2_EL1_RESERVED", .state = ARM_CP_STATE_AA64,
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 2,
@@ -5871,11 +5881,16 @@ void register_cp_regs_for_features(ARMCPU *cpu)
               .resetvalue = 0 },
             { .name = "ID_AA64ISAR0_EL1", .state = ARM_CP_STATE_AA64,
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 6, .opc2 = 0,
-              .access = PL1_R, .type = ARM_CP_CONST,
-              .resetvalue = cpu->isar.id_aa64isar0 },
+              .access = PL0U_R, .type = ARM_CP_CONST,
+#ifdef CONFIG_USER_ONLY
+              .resetvalue = cpu->isar.id_aa64isar0 & 0x000fffffff0ffff0ULL
+#else
+              .resetvalue = cpu->isar.id_aa64isar0
+#endif
+            },
             { .name = "ID_AA64ISAR1_EL1", .state = ARM_CP_STATE_AA64,
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 6, .opc2 = 1,
-              .access = PL1_R, .type = ARM_CP_CONST,
+              .access = PL0U_R, .type = ARM_CP_CONST,
               .resetvalue = cpu->isar.id_aa64isar1 },
             { .name = "ID_AA64ISAR2_EL1_RESERVED", .state = ARM_CP_STATE_AA64,
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 6, .opc2 = 2,
@@ -5903,11 +5918,11 @@ void register_cp_regs_for_features(ARMCPU *cpu)
               .resetvalue = 0 },
             { .name = "ID_AA64MMFR0_EL1", .state = ARM_CP_STATE_AA64,
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 7, .opc2 = 0,
-              .access = PL1_R, .type = ARM_CP_CONST,
+              .access = PL0U_R, .type = ARM_CP_CONST,
               .resetvalue = cpu->isar.id_aa64mmfr0 },
             { .name = "ID_AA64MMFR1_EL1", .state = ARM_CP_STATE_AA64,
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 7, .opc2 = 1,
-              .access = PL1_R, .type = ARM_CP_CONST,
+              .access = PL0U_R, .type = ARM_CP_CONST,
               .resetvalue = cpu->isar.id_aa64mmfr1 },
             { .name = "ID_AA64MMFR2_EL1_RESERVED", .state = ARM_CP_STATE_AA64,
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 7, .opc2 = 2,
@@ -6211,7 +6226,7 @@ void register_cp_regs_for_features(ARMCPU *cpu)
         ARMCPRegInfo id_v8_midr_cp_reginfo[] = {
             { .name = "MIDR_EL1", .state = ARM_CP_STATE_BOTH,
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 0, .opc2 = 0,
-              .access = PL1_R, .type = ARM_CP_NO_RAW, .resetvalue = cpu->midr,
+              .access = PL0U_R, .type = ARM_CP_NO_RAW, .resetvalue = cpu->midr,
               .fieldoffset = offsetof(CPUARMState, cp15.c0_cpuid),
               .readfn = midr_read },
             /* crn = 0 op1 = 0 crm = 0 op2 = 4,7 : AArch32 aliases of MIDR */
@@ -6223,7 +6238,13 @@ void register_cp_regs_for_features(ARMCPU *cpu)
               .access = PL1_R, .resetvalue = cpu->midr },
             { .name = "REVIDR_EL1", .state = ARM_CP_STATE_BOTH,
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 0, .opc2 = 6,
-              .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = cpu->revidr },
+#ifdef CONFIG_USER_ONLY
+              .access = PL0U_R, .type = ARM_CP_CONST,
+              .resetvalue = 0 /* HW_CPUID IMPDEF fields are 0 */ },
+#else
+              .access = PL1_R, .type = ARM_CP_CONST,
+              .resetvalue = cpu->revidr },
+#endif
             REGINFO_SENTINEL
         };
         ARMCPRegInfo id_cp_reginfo[] = {
-- 
2.17.1

[Qemu-devel] [PATCH v1 3/4] 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 4cff9e1a31..e95c162097 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -571,6 +571,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.17.1

[Qemu-devel] [PATCH v1 4/4] 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
---
 tests/tcg/aarch64/Makefile.target |   2 +-
 tests/tcg/aarch64/sysregs.c       | 120 ++++++++++++++++++++++++++++++
 2 files changed, 121 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..cc1a7eb486 100644
--- a/tests/tcg/aarch64/Makefile.target
+++ b/tests/tcg/aarch64/Makefile.target
@@ -7,7 +7,7 @@ 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
diff --git a/tests/tcg/aarch64/sysregs.c b/tests/tcg/aarch64/sysregs.c
new file mode 100644
index 0000000000..8e11288ee3
--- /dev/null
+++ b/tests/tcg/aarch64/sysregs.c
@@ -0,0 +1,120 @@
+/*
+ * Check emulated system register access for linux-user mode.
+ *
+ * See: https://www.kernel.org/doc/Documentation/arm64/cpu-feature-registers.txt
+ */
+
+#include <asm/hwcap.h>
+#include <stdio.h>
+#include <sys/auxv.h>
+#include <signal.h>
+#include <string.h>
+#include <stdbool.h>
+
+int failed_mask_count;
+
+#define get_cpu_reg(id) ({                                      \
+            unsigned long __val = 0xdeadbeef;                   \
+            asm("mrs %0, "#id : "=r" (__val));                  \
+            printf("%-20s: 0x%016lx\n", #id, __val);            \
+            __val;                                               \
+        })
+
+#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_mask_count++;                            \
+            }                                                   \
+})
+
+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;
+    }
+
+    /* since 4.12 */
+    printf("Checking CNT registers\n");
+
+    get_cpu_reg(ctr_el0);
+    get_cpu_reg(cntvct_el0);
+    get_cpu_reg(cntfrq_el0);
+
+    /* when (getauxval(AT_HWCAP) & HWCAP_CPUID), since 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 IMDEF is exported as 0 to user-space.
+     */
+    get_cpu_reg_check_mask(id_aa64isar0_el1, 0x000fffffff0ffff0ULL);
+    get_cpu_reg_check_mask(id_aa64isar1_el1, 0x00000000ffffffffULL);
+    get_cpu_reg(id_aa64mmfr0_el1);
+    get_cpu_reg(id_aa64mmfr1_el1);
+    get_cpu_reg_check_mask(id_aa64pfr0_el1, 0x000f000f0ff0000ULL);
+    get_cpu_reg(id_aa64pfr1_el1);
+    get_cpu_reg(id_aa64dfr0_el1);
+    get_cpu_reg(id_aa64dfr1_el1);
+
+    get_cpu_reg_check_mask(midr_el1, 0x00000000ffffffffULL);
+    get_cpu_reg(mpidr_el1);
+    /* REVIDR is all IMPDEF so should be all zeros to user-space */
+    get_cpu_reg_check_mask(revidr_el1, 0x0);
+
+    printf("Remaining registers should fail\n");
+    should_fail = true;
+
+    /* Unexposed register access causes SIGILL */
+    get_cpu_reg(id_mmfr0_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_mask_count > 0) {
+        printf("Extra information leaked to user-space!\n");
+        return 1;
+    }
+
+    return should_fail_count == 1 ? 0 : 1;
+}
-- 
2.17.1

Re: [Qemu-devel] [PATCH v1 3/4] linux-user/elfload: enable HWCAP_CPUID for AArch64

[Laurent Vivier]

On 28/01/2019 18:39, Alex Bennée wrote:
> 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 4cff9e1a31..e95c162097 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -571,6 +571,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) \
> 

Acked-by: Laurent Vivier <laurent@vivier.eu>

Re: [Qemu-devel] [PATCH v1 4/4] tests/tcg/aarch64: userspace system register test

[Alex Bennée]

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

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

I'll also merge the following fix:

modified   tests/tcg/aarch64/sysregs.c
@@ -11,6 +11,10 @@
 #include <string.h>
 #include <stdbool.h>

+#ifndef HWCAP_CPUID
+#define HWCAP_CPUID (1 << 11)
+#endif
+
 int failed_mask_count;

 #define get_cpu_reg(id) ({                                      \


>
> ---
> v4
>   - also test for extra bits that shouldn't be exposed
> ---
>  tests/tcg/aarch64/Makefile.target |   2 +-
>  tests/tcg/aarch64/sysregs.c       | 120 ++++++++++++++++++++++++++++++
>  2 files changed, 121 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..cc1a7eb486 100644
> --- a/tests/tcg/aarch64/Makefile.target
> +++ b/tests/tcg/aarch64/Makefile.target
> @@ -7,7 +7,7 @@ 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
> diff --git a/tests/tcg/aarch64/sysregs.c b/tests/tcg/aarch64/sysregs.c
> new file mode 100644
> index 0000000000..8e11288ee3
> --- /dev/null
> +++ b/tests/tcg/aarch64/sysregs.c
> @@ -0,0 +1,120 @@
> +/*
> + * Check emulated system register access for linux-user mode.
> + *
> + * See: https://www.kernel.org/doc/Documentation/arm64/cpu-feature-registers.txt
> + */
> +
> +#include <asm/hwcap.h>
> +#include <stdio.h>
> +#include <sys/auxv.h>
> +#include <signal.h>
> +#include <string.h>
> +#include <stdbool.h>
> +
> +int failed_mask_count;
> +
> +#define get_cpu_reg(id) ({                                      \
> +            unsigned long __val = 0xdeadbeef;                   \
> +            asm("mrs %0, "#id : "=r" (__val));                  \
> +            printf("%-20s: 0x%016lx\n", #id, __val);            \
> +            __val;                                               \
> +        })
> +
> +#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_mask_count++;                            \
> +            }                                                   \
> +})
> +
> +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;
> +    }
> +
> +    /* since 4.12 */
> +    printf("Checking CNT registers\n");
> +
> +    get_cpu_reg(ctr_el0);
> +    get_cpu_reg(cntvct_el0);
> +    get_cpu_reg(cntfrq_el0);
> +
> +    /* when (getauxval(AT_HWCAP) & HWCAP_CPUID), since 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 IMDEF is exported as 0 to user-space.
> +     */
> +    get_cpu_reg_check_mask(id_aa64isar0_el1, 0x000fffffff0ffff0ULL);
> +    get_cpu_reg_check_mask(id_aa64isar1_el1, 0x00000000ffffffffULL);
> +    get_cpu_reg(id_aa64mmfr0_el1);
> +    get_cpu_reg(id_aa64mmfr1_el1);
> +    get_cpu_reg_check_mask(id_aa64pfr0_el1, 0x000f000f0ff0000ULL);
> +    get_cpu_reg(id_aa64pfr1_el1);
> +    get_cpu_reg(id_aa64dfr0_el1);
> +    get_cpu_reg(id_aa64dfr1_el1);
> +
> +    get_cpu_reg_check_mask(midr_el1, 0x00000000ffffffffULL);
> +    get_cpu_reg(mpidr_el1);
> +    /* REVIDR is all IMPDEF so should be all zeros to user-space */
> +    get_cpu_reg_check_mask(revidr_el1, 0x0);
> +
> +    printf("Remaining registers should fail\n");
> +    should_fail = true;
> +
> +    /* Unexposed register access causes SIGILL */
> +    get_cpu_reg(id_mmfr0_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_mask_count > 0) {
> +        printf("Extra information leaked to user-space!\n");
> +        return 1;
> +    }
> +
> +    return should_fail_count == 1 ? 0 : 1;
> +}


--
Alex Bennée

Re: [Qemu-devel] [PATCH v1 4/4] tests/tcg/aarch64: userspace system register test

[Peter Maydell]

On Mon, 28 Jan 2019 at 17:39, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> 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
> ---
>  tests/tcg/aarch64/Makefile.target |   2 +-
>  tests/tcg/aarch64/sysregs.c       | 120 ++++++++++++++++++++++++++++++
>  2 files changed, 121 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..cc1a7eb486 100644
> --- a/tests/tcg/aarch64/Makefile.target
> +++ b/tests/tcg/aarch64/Makefile.target
> @@ -7,7 +7,7 @@ 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
> diff --git a/tests/tcg/aarch64/sysregs.c b/tests/tcg/aarch64/sysregs.c
> new file mode 100644
> index 0000000000..8e11288ee3
> --- /dev/null
> +++ b/tests/tcg/aarch64/sysregs.c
> @@ -0,0 +1,120 @@
> +/*
> + * Check emulated system register access for linux-user mode.
> + *
> + * See: https://www.kernel.org/doc/Documentation/arm64/cpu-feature-registers.txt
> + */

This needs the usual copyright line and license statement.

> +
> +#include <asm/hwcap.h>
> +#include <stdio.h>
> +#include <sys/auxv.h>
> +#include <signal.h>
> +#include <string.h>
> +#include <stdbool.h>
> +
> +int failed_mask_count;
> +
> +#define get_cpu_reg(id) ({                                      \
> +            unsigned long __val = 0xdeadbeef;                   \
> +            asm("mrs %0, "#id : "=r" (__val));                  \
> +            printf("%-20s: 0x%016lx\n", #id, __val);            \
> +            __val;                                               \
> +        })
> +
> +#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_mask_count++;                            \
> +            }                                                   \
> +})

A brief comment describing what the arguments to these macros
do would be helpful if we ever have to add more registers in future.

> +
> +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;
> +    }
> +
> +    /* since 4.12 */

This is a rather cryptic comment.

> +    printf("Checking CNT registers\n");
> +
> +    get_cpu_reg(ctr_el0);
> +    get_cpu_reg(cntvct_el0);
> +    get_cpu_reg(cntfrq_el0);
> +
> +    /* when (getauxval(AT_HWCAP) & HWCAP_CPUID), since 4.11*/

Missing space before "*/".

> +    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 IMDEF is exported as 0 to user-space.

IMPDEF

> +     */
> +    get_cpu_reg_check_mask(id_aa64isar0_el1, 0x000fffffff0ffff0ULL);

I can't make sense of this mask value. Presumably it's supposed to be
1s where the value is visible, but the docs say that the visible
bits for ID_AA64ISAR0_EL1 are [23..4] and [55..28], whereas the
mask value here has 1s in [51..24] and [20..4], unless I've miscounted.

> +    get_cpu_reg_check_mask(id_aa64isar1_el1, 0x00000000ffffffffULL);
> +    get_cpu_reg(id_aa64mmfr0_el1);
> +    get_cpu_reg(id_aa64mmfr1_el1);
> +    get_cpu_reg_check_mask(id_aa64pfr0_el1, 0x000f000f0ff0000ULL);

This seems to have [31..28] set but not [35..32]. The bits that
should be [51..48] are also in the wrong place, and the whole
number has one hex digit too few to be 64 bits.

> +    get_cpu_reg(id_aa64pfr1_el1);
> +    get_cpu_reg(id_aa64dfr0_el1);
> +    get_cpu_reg(id_aa64dfr1_el1);
> +
> +    get_cpu_reg_check_mask(midr_el1, 0x00000000ffffffffULL);
> +    get_cpu_reg(mpidr_el1);
> +    /* REVIDR is all IMPDEF so should be all zeros to user-space */
> +    get_cpu_reg_check_mask(revidr_el1, 0x0);

Missing check of the mask value for ID_AA64MMFR2_EL1 ? Should have
bits [35..32] visible.

Missing checks for:
 ID_AA64ZFR0_EL1
 ID_AA64AFR0_EL1
 ID_AA64AFR1_EL1

Missing checks for all the parts of the ID space that are currently
unallocated (and which should be exposed as RAZ/WI).

Missing checks for registers which are exposed but with all
features hidden, eg ID_PFR0_EL1 and the other aarch32 ID regs.

The kernel returns some value, ie does not fault, all the
architecturally defined registers in Op0=3, Op1=0, CRn=0,
CRm=0,4,5,6,7. For CRm = 0 only op2 = {0,5,6} [MIDR_EL1,
MPIDR_EL1, REVIDR_EL1] are valid, but for CRm = {4,5,6,7}
the whole set of op2 = {0..7} are defined (and RAZ if not
yet given some meaning).

thanks
-- PMM

Re: [Qemu-devel] [PATCH v1 2/4] target/arm: expose CPUID registers to userspace

[Peter Maydell]

On Mon, 28 Jan 2019 at 17:39, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> 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. For this we use the PL0U_R
> permission mask which allows this access in CONFIG_USER mode.
>
> Some registers only return a subset of their contents so we need
> specific CONFIG_USER_ONLY logic to do this.
>
> 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
> ---
>  target/arm/helper.c | 51 ++++++++++++++++++++++++++++++++-------------
>  1 file changed, 36 insertions(+), 15 deletions(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 42c1c0b144..68808e7293 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -3543,7 +3543,7 @@ static uint64_t mpidr_read(CPUARMState *env, const ARMCPRegInfo *ri)
>  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 },
> +      .access = PL0U_R, .readfn = mpidr_read, .type = ARM_CP_NO_RAW },
>      REGINFO_SENTINEL
>  };
>
> @@ -5488,6 +5488,7 @@ static uint64_t id_pfr1_read(CPUARMState *env, const ARMCPRegInfo *ri)
>      return pfr1;
>  }
>
> +#ifndef CONFIG_USER_ONLY
>  static uint64_t id_aa64pfr0_read(CPUARMState *env, const ARMCPRegInfo *ri)
>  {
>      ARMCPU *cpu = arm_env_get_cpu(env);
> @@ -5498,6 +5499,7 @@ static uint64_t id_aa64pfr0_read(CPUARMState *env, const ARMCPRegInfo *ri)
>      }
>      return pfr0;
>  }
> +#endif
>
>  /* Shared logic between LORID and the rest of the LOR* registers.
>   * Secure state has already been delt with.
> @@ -5799,18 +5801,26 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>           * define new registers here.
>           */
>          ARMCPRegInfo v8_idregs[] = {
> -            /* ID_AA64PFR0_EL1 is not a plain ARM_CP_CONST because we don't
> -             * know the right value for the GIC field until after we
> -             * define these regs.
> +            /* ID_AA64PFR0_EL1 is not a plain ARM_CP_CONST for system
> +             * emulation because we don't know the right value for the
> +             * GIC field until after we define these regs. For
> +             * user-mode HWCAP_CPUID emulation the GIC bits are masked
> +             * anyway.
>               */
>              { .name = "ID_AA64PFR0_EL1", .state = ARM_CP_STATE_AA64,
>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 0,
> +#ifndef CONFIG_USER_ONLY
>                .access = PL1_R, .type = ARM_CP_NO_RAW,
>                .readfn = id_aa64pfr0_read,
> -              .writefn = arm_cp_write_ignore },
> +              .writefn = arm_cp_write_ignore
> +#else
> +              .access = PL0U_R, .type = ARM_CP_CONST,
> +              .resetvalue = cpu->isar.id_aa64pfr0 & 0x000f000f0ff0000ULL
> +#endif
> +            },
>              { .name = "ID_AA64PFR1_EL1", .state = ARM_CP_STATE_AA64,
>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 1,
> -              .access = PL1_R, .type = ARM_CP_CONST,
> +              .access = PL0U_R, .type = ARM_CP_CONST,
>                .resetvalue = cpu->isar.id_aa64pfr1},
>              { .name = "ID_AA64PFR2_EL1_RESERVED", .state = ARM_CP_STATE_AA64,
>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 2,
> @@ -5839,11 +5849,11 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>                .resetvalue = 0 },
>              { .name = "ID_AA64DFR0_EL1", .state = ARM_CP_STATE_AA64,
>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 0,
> -              .access = PL1_R, .type = ARM_CP_CONST,
> +              .access = PL0U_R, .type = ARM_CP_CONST,
>                .resetvalue = cpu->id_aa64dfr0 },

This doesn't seem right -- I think ID_AA64DFR0_EL1 should be exposed
as zero, not whatever the underlying register value is.

More generally, this ifdeffing of "maybe mask the values" doesn't
seem very future-proof. If we add something to one of the cpu-id_*
values, that should be masked out to zero by default for linux-user,
not defaulting to passed through.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v1 2/4] target/arm: expose CPUID registers to userspace

[Alex Bennée]

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

> On Mon, 28 Jan 2019 at 17:39, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> 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. For this we use the PL0U_R
>> permission mask which allows this access in CONFIG_USER mode.
>>
>> Some registers only return a subset of their contents so we need
>> specific CONFIG_USER_ONLY logic to do this.
>>
>> 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
>> ---
>>  target/arm/helper.c | 51 ++++++++++++++++++++++++++++++++-------------
>>  1 file changed, 36 insertions(+), 15 deletions(-)
>>
>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>> index 42c1c0b144..68808e7293 100644
>> --- a/target/arm/helper.c
>> +++ b/target/arm/helper.c
>> @@ -3543,7 +3543,7 @@ static uint64_t mpidr_read(CPUARMState *env, const ARMCPRegInfo *ri)
>>  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 },
>> +      .access = PL0U_R, .readfn = mpidr_read, .type = ARM_CP_NO_RAW },
>>      REGINFO_SENTINEL
>>  };
>>
>> @@ -5488,6 +5488,7 @@ static uint64_t id_pfr1_read(CPUARMState *env, const ARMCPRegInfo *ri)
>>      return pfr1;
>>  }
>>
>> +#ifndef CONFIG_USER_ONLY
>>  static uint64_t id_aa64pfr0_read(CPUARMState *env, const ARMCPRegInfo *ri)
>>  {
>>      ARMCPU *cpu = arm_env_get_cpu(env);
>> @@ -5498,6 +5499,7 @@ static uint64_t id_aa64pfr0_read(CPUARMState *env, const ARMCPRegInfo *ri)
>>      }
>>      return pfr0;
>>  }
>> +#endif
>>
>>  /* Shared logic between LORID and the rest of the LOR* registers.
>>   * Secure state has already been delt with.
>> @@ -5799,18 +5801,26 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>>           * define new registers here.
>>           */
>>          ARMCPRegInfo v8_idregs[] = {
>> -            /* ID_AA64PFR0_EL1 is not a plain ARM_CP_CONST because we don't
>> -             * know the right value for the GIC field until after we
>> -             * define these regs.
>> +            /* ID_AA64PFR0_EL1 is not a plain ARM_CP_CONST for system
>> +             * emulation because we don't know the right value for the
>> +             * GIC field until after we define these regs. For
>> +             * user-mode HWCAP_CPUID emulation the GIC bits are masked
>> +             * anyway.
>>               */
>>              { .name = "ID_AA64PFR0_EL1", .state = ARM_CP_STATE_AA64,
>>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 0,
>> +#ifndef CONFIG_USER_ONLY
>>                .access = PL1_R, .type = ARM_CP_NO_RAW,
>>                .readfn = id_aa64pfr0_read,
>> -              .writefn = arm_cp_write_ignore },
>> +              .writefn = arm_cp_write_ignore
>> +#else
>> +              .access = PL0U_R, .type = ARM_CP_CONST,
>> +              .resetvalue = cpu->isar.id_aa64pfr0 & 0x000f000f0ff0000ULL
>> +#endif
>> +            },
>>              { .name = "ID_AA64PFR1_EL1", .state = ARM_CP_STATE_AA64,
>>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 1,
>> -              .access = PL1_R, .type = ARM_CP_CONST,
>> +              .access = PL0U_R, .type = ARM_CP_CONST,
>>                .resetvalue = cpu->isar.id_aa64pfr1},
>>              { .name = "ID_AA64PFR2_EL1_RESERVED", .state = ARM_CP_STATE_AA64,
>>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 2,
>> @@ -5839,11 +5849,11 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>>                .resetvalue = 0 },
>>              { .name = "ID_AA64DFR0_EL1", .state = ARM_CP_STATE_AA64,
>>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 0,
>> -              .access = PL1_R, .type = ARM_CP_CONST,
>> +              .access = PL0U_R, .type = ARM_CP_CONST,
>>                .resetvalue = cpu->id_aa64dfr0 },
>
> This doesn't seem right -- I think ID_AA64DFR0_EL1 should be exposed
> as zero, not whatever the underlying register value is.

Under a kernel I'm seeing numbers reported so I don't think it's totally
squashed:

  id_aa64dfr0_el1     : 0x0000000000000006
  id_aa64dfr1_el1     : 0x0000000000000000

Confusingly the kernel does:

static const struct arm64_ftr_bits ftr_id_aa64dfr0[] = {
	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_EXACT, 36, 28, 0),
	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR0_PMSVER_SHIFT, 4, 0),
	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_CTX_CMPS_SHIFT, 4, 0),
	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_WRPS_SHIFT, 4, 0),
	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_BRPS_SHIFT, 4, 0),
	/*
	 * We can instantiate multiple PMU instances with different levels
	 * of support.
	 */
	S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_EXACT, ID_AA64DFR0_PMUVER_SHIFT, 4, 0),
	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_EXACT, ID_AA64DFR0_TRACEVER_SHIFT, 4, 0),
	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_EXACT, ID_AA64DFR0_DEBUGVER_SHIFT, 4, 0x6),
	ARM64_FTR_END,
};

So I'm not sure if that is intentional as the line:

  ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_EXACT, ID_AA64DFR0_DEBUGVER_SHIFT, 4, 0x6)

implies the DEBUGVER is hidden but actually 0x6?

> More generally, this ifdeffing of "maybe mask the values" doesn't
> seem very future-proof. If we add something to one of the cpu-id_*
> values, that should be masked out to zero by default for linux-user,
> not defaulting to passed through.

Do you mean setting cpu->id_foo during cpu_init? I wasn't sure if that
might interact badly with the recent changes to base our features off
what's actually set in them.

>
> thanks
> -- PMM


--
Alex Bennée

Re: [Qemu-devel] [PATCH v1 2/4] target/arm: expose CPUID registers to userspace

[Peter Maydell]

On Mon, 4 Feb 2019 at 15:33, Alex Bennée <alex.bennee@linaro.org> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
> > This doesn't seem right -- I think ID_AA64DFR0_EL1 should be exposed
> > as zero, not whatever the underlying register value is.
>
> Under a kernel I'm seeing numbers reported so I don't think it's totally
> squashed:
>
>   id_aa64dfr0_el1     : 0x0000000000000006
>   id_aa64dfr1_el1     : 0x0000000000000000
>
> Confusingly the kernel does:
>
> static const struct arm64_ftr_bits ftr_id_aa64dfr0[] = {
>         ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_EXACT, 36, 28, 0),
>         ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR0_PMSVER_SHIFT, 4, 0),
>         ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_CTX_CMPS_SHIFT, 4, 0),
>         ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_WRPS_SHIFT, 4, 0),
>         ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_BRPS_SHIFT, 4, 0),
>         /*
>          * We can instantiate multiple PMU instances with different levels
>          * of support.
>          */
>         S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_EXACT, ID_AA64DFR0_PMUVER_SHIFT, 4, 0),
>         ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_EXACT, ID_AA64DFR0_TRACEVER_SHIFT, 4, 0),
>         ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_EXACT, ID_AA64DFR0_DEBUGVER_SHIFT, 4, 0x6),
>         ARM64_FTR_END,
> };
>
> So I'm not sure if that is intentional as the line:
>
>   ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_EXACT, ID_AA64DFR0_DEBUGVER_SHIFT, 4, 0x6)
>
> implies the DEBUGVER is hidden but actually 0x6?

I think this is because the field in the register is architecturally
defined to have a restricted set of permitted values, of which 0 is
not one. (6 means "ARMv8 debug architecture" so is the effective minimum.)

> > More generally, this ifdeffing of "maybe mask the values" doesn't
> > seem very future-proof. If we add something to one of the cpu-id_*
> > values, that should be masked out to zero by default for linux-user,
> > not defaulting to passed through.
>
> Do you mean setting cpu->id_foo during cpu_init? I wasn't sure if that
> might interact badly with the recent changes to base our features off
> what's actually set in them.

No, I meant having something where we either:
 (a) have a separate set of cpreg structs for the userspace visible versions,
or (b) have some code which modifies the cpreg structs in v8_idregs[] (it's
not const) before handing it to define_arm_cp_regs().
(b) sounds better to me -- the ideal here I think would be something where we:
 * mark all the regs in the space the kernel covers as PL0U_R with a bit
   of code rather than by changing all the cpreg structs
 * default them to "no bits of the real value are exposed, all bits 0"
 * have a hopefully short list of overrides, probably defined in a special
   purpose array with entries like
 { /* ID_AA64DFR0_EL1 */, .crm = 5, .opc2 = 0, .passbits = 0x0,
.fixedbits = 0x6 },
Or we could match the override entries with cpreg values by register name,
which would let you say
 { "ID_AA64DFR0_EL1", .passbits = 0x0, .fixedbits = 0x6 },
and avoid having to repeat the encoding values, which might be neater.

thanks
-- PMM