[Qemu-devel] [PATCH v3 0/5] support reading some CPUID/CNT registers from user-space

[Qemu-devel] [PATCH v3 0/5] support reading some CPUID/CNT registers from user-space

[Alex Bennée]

Hi,

This expands on the previous version by adding the additional CPUID
registers as exposed by the HWCAP_CPUID kernel feature. Most of the
work involves judicious use of macros. There is a easily extensible
test case that uses the shiny new check-tcg infrastructure ;-)

Alex Bennée (5):
  target/arm: support reading of CNT[VCT|FRQ]_EL0 from user-space
  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                  |  7 +++
 target/arm/helper.c               | 80 +++++++++++++++++++------
 tests/tcg/aarch64/Makefile.target |  2 +-
 tests/tcg/aarch64/sysregs.c       | 99 +++++++++++++++++++++++++++++++
 5 files changed, 170 insertions(+), 19 deletions(-)
 create mode 100644 tests/tcg/aarch64/sysregs.c

-- 
2.17.1

[Qemu-devel] [PATCH v3 1/5] target/arm: support reading of CNT[VCT|FRQ]_EL0 from user-space

[Alex Bennée]

Since kernel commit a86bd139f2 (arm64: arch_timer: Enable CNTVCT_EL0
trap..) user-space has been able to read these system registers. As we
can't use QEMUTimer's in linux-user mode we just directly call
cpu_get_clock().

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

---
v2
  - include CNTFRQ_EL0 for PL0_R only
v3
  - use NANOSECONDS_PER_SECOND / GTIMER_SCALE
---
 target/arm/helper.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 1248d84e6f..6e6b1762e8 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -2166,11 +2166,32 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[] = {
 };
 
 #else
-/* In user-mode none of the generic timer registers are accessible,
- * and their implementation depends on QEMU_CLOCK_VIRTUAL and qdev gpio outputs,
- * so instead just don't register any of them.
+
+/* In user-mode most of the generic timer registers are inaccessible
+ * however modern kernels (4.12+) allow access to cntvct_el0
  */
+
+static uint64_t gt_virt_cnt_read(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+    /* Currently we have no support for QEMUTimer in linux-user so we
+     * can't call gt_get_countervalue(env), instead we directly
+     * call the lower level functions.
+     */
+    return cpu_get_clock() / GTIMER_SCALE;
+}
+
 static const ARMCPRegInfo generic_timer_cp_reginfo[] = {
+    { .name = "CNTFRQ_EL0", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 3, .crn = 14, .crm = 0, .opc2 = 0,
+      .type = ARM_CP_CONST, .access = PL0_R /* no PL1_RW in linux-user */,
+      .fieldoffset = offsetof(CPUARMState, cp15.c14_cntfrq),
+      .resetvalue = NANOSECONDS_PER_SECOND / GTIMER_SCALE,
+    },
+    { .name = "CNTVCT_EL0", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 3, .crn = 14, .crm = 0, .opc2 = 2,
+      .access = PL0_R, .type = ARM_CP_NO_RAW | ARM_CP_IO,
+      .readfn = gt_virt_cnt_read,
+    },
     REGINFO_SENTINEL
 };
 
-- 
2.17.1

Re: [Qemu-devel] [PATCH v3 1/5] target/arm: support reading of CNT[VCT|FRQ]_EL0 from user-space

[Richard Henderson]

On 06/25/2018 09:00 AM, Alex Bennée wrote:
> Since kernel commit a86bd139f2 (arm64: arch_timer: Enable CNTVCT_EL0
> trap..) user-space has been able to read these system registers. As we
> can't use QEMUTimer's in linux-user mode we just directly call
> cpu_get_clock().
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> ---
> v2
>   - include CNTFRQ_EL0 for PL0_R only
> v3
>   - use NANOSECONDS_PER_SECOND / GTIMER_SCALE
> ---
>  target/arm/helper.c | 27 ++++++++++++++++++++++++---
>  1 file changed, 24 insertions(+), 3 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH v3 1/5] target/arm: support reading of CNT[VCT|FRQ]_EL0 from user-space

[Emilio G. Cota]

On Mon, Jun 25, 2018 at 17:00:05 +0100, Alex Bennée wrote:
> Since kernel commit a86bd139f2 (arm64: arch_timer: Enable CNTVCT_EL0
> trap..)

I'd also mention here that this feature became available in v4.12.

Thanks,

		Emilio

[Qemu-devel] [PATCH v3 2/5] 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 trap and emulate. For user mode we can provide the
value directly but we need to relax our permission checks to do this.

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

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 6e6b1762e8..9d81feb124 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -5813,7 +5813,19 @@ 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:
+#ifdef CONFIG_USER_ONLY
+            /* Some AArch64 CPU ID/feature are exported to userspace
+             * by the kernel (see HWCAP_CPUID) */
+            if (r->opc0 == 3 && r->crn == 0 &&
+                (r->crm == 0 ||
+                 (r->crm >= 4 && r->crm <= 7))) {
+                mask = PL0_R;
+                break;
+            }
+#endif
+            /* fall-through */
+        case 1: case 2:
             /* min_EL EL1 */
             mask = PL1_RW;
             break;
-- 
2.17.1

Re: [Qemu-devel] [PATCH v3 2/5] target/arm: relax permission checks for HWCAP_CPUID registers

[Richard Henderson]

On 06/25/2018 09:00 AM, Alex Bennée wrote:
> +#ifdef CONFIG_USER_ONLY
> +            /* Some AArch64 CPU ID/feature are exported to userspace
> +             * by the kernel (see HWCAP_CPUID) */
> +            if (r->opc0 == 3 && r->crn == 0 &&
> +                (r->crm == 0 ||
> +                 (r->crm >= 4 && r->crm <= 7))) {
> +                mask = PL0_R;
> +                break;
> +            }
> +#endif

Why not just set mask to PL0U_R | PL1_RW?
This mask doesn't affect the actual permissions, just the check.

Then of course merge with the next patch.


r~

Re: [Qemu-devel] [PATCH v3 2/5] target/arm: relax permission checks for HWCAP_CPUID registers

[Peter Maydell]

On 25 June 2018 at 17:00, Alex Bennée <alex.bennee@linaro.org> wrote:
> Although technically not visible to userspace the kernel does make
> them visible via trap and emulate. For user mode we can provide the
> value directly but we need to relax our permission checks to do this.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  target/arm/helper.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 6e6b1762e8..9d81feb124 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -5813,7 +5813,19 @@ 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:
> +#ifdef CONFIG_USER_ONLY
> +            /* Some AArch64 CPU ID/feature are exported to userspace
> +             * by the kernel (see HWCAP_CPUID) */
> +            if (r->opc0 == 3 && r->crn == 0 &&
> +                (r->crm == 0 ||
> +                 (r->crm >= 4 && r->crm <= 7))) {
> +                mask = PL0_R;
> +                break;
> +            }
> +#endif
> +            /* fall-through */
> +        case 1: case 2:
>              /* min_EL EL1 */
>              mask = PL1_RW;
>              break;

This looks like a rather inelegant place to shove a CONFIG_USER_ONLY
special case. Isn't there a cleaner way to do whatever this is trying
to achieve?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 2/5] target/arm: relax permission checks for HWCAP_CPUID registers

[Alex Bennée]

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

> On 25 June 2018 at 17:00, Alex Bennée <alex.bennee@linaro.org> wrote:
>> Although technically not visible to userspace the kernel does make
>> them visible via trap and emulate. For user mode we can provide the
>> value directly but we need to relax our permission checks to do this.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  target/arm/helper.c | 14 +++++++++++++-
>>  1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>> index 6e6b1762e8..9d81feb124 100644
>> --- a/target/arm/helper.c
>> +++ b/target/arm/helper.c
>> @@ -5813,7 +5813,19 @@ 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:
>> +#ifdef CONFIG_USER_ONLY
>> +            /* Some AArch64 CPU ID/feature are exported to userspace
>> +             * by the kernel (see HWCAP_CPUID) */
>> +            if (r->opc0 == 3 && r->crn == 0 &&
>> +                (r->crm == 0 ||
>> +                 (r->crm >= 4 && r->crm <= 7))) {
>> +                mask = PL0_R;
>> +                break;
>> +            }
>> +#endif
>> +            /* fall-through */
>> +        case 1: case 2:
>>              /* min_EL EL1 */
>>              mask = PL1_RW;
>>              break;
>
> This looks like a rather inelegant place to shove a CONFIG_USER_ONLY
> special case. Isn't there a cleaner way to do whatever this is trying
> to achieve?

Well technically those registers aren't accessible to user space and
this is a sanity check to ensure we don't accidentally make them
accessible. But it does get in the way of emulating the traps for
USER_ONLY.


>
> thanks
> -- PMM


--
Alex Bennée

[Qemu-devel] [PATCH v3 3/5] 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
CONFIG_USER_ONLY we don't emulate the kernels trap and emulate but
instead just lower the read permission to PL0_R (hidden behind the
PL1U_R macro).

The ID_AA64PFR0_EL1 is a little special as the GIC version is hidden
from userspace so we can define a ARM_CP_CONST version of the register
for usermode.

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

squash! target/arm: expose CPUID registers to userspace
---
 target/arm/cpu.h    |  7 +++++++
 target/arm/helper.c | 39 +++++++++++++++++++++++++--------------
 2 files changed, 32 insertions(+), 14 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index a4507a2d6f..156c811654 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1924,6 +1924,13 @@ static inline bool cptype_valid(int cptype)
 #define PL0_R (0x02 | PL1_R)
 #define PL0_W (0x01 | PL1_W)
 
+/* for AArch64 HWCAP_CPUID to userspace */
+#ifdef CONFIG_USER_ONLY
+#define PL1U_R PL0_R
+#else
+#define PL1U_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 9d81feb124..1ea0dc4593 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -2987,7 +2987,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 = PL1U_R, .readfn = mpidr_read, .type = ARM_CP_NO_RAW },
     REGINFO_SENTINEL
 };
 
@@ -4776,6 +4776,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);
@@ -4786,6 +4787,7 @@ static uint64_t id_aa64pfr0_read(CPUARMState *env, const ARMCPRegInfo *ri)
     }
     return pfr0;
 }
+#endif
 
 void register_cp_regs_for_features(ARMCPU *cpu)
 {
@@ -4934,18 +4936,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 = PL0_R, .type = ARM_CP_CONST,
+              .resetvalue = cpu->id_aa64pfr0
+#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 = PL1U_R, .type = ARM_CP_CONST,
               .resetvalue = cpu->id_aa64pfr1},
             { .name = "ID_AA64PFR2_EL1_RESERVED", .state = ARM_CP_STATE_AA64,
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 2,
@@ -4973,11 +4983,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 = PL1U_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 = PL1U_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,
@@ -5005,11 +5015,11 @@ 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,
+              .access = PL1U_R, .type = ARM_CP_CONST,
               .resetvalue = cpu->id_aa64isar0 },
             { .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 = PL1U_R, .type = ARM_CP_CONST,
               .resetvalue = cpu->id_aa64isar1 },
             { .name = "ID_AA64ISAR2_EL1_RESERVED", .state = ARM_CP_STATE_AA64,
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 6, .opc2 = 2,
@@ -5037,11 +5047,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 = PL1U_R, .type = ARM_CP_CONST,
               .resetvalue = cpu->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 = PL1U_R, .type = ARM_CP_CONST,
               .resetvalue = cpu->id_aa64mmfr1 },
             { .name = "ID_AA64MMFR2_EL1_RESERVED", .state = ARM_CP_STATE_AA64,
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 7, .opc2 = 2,
@@ -5335,7 +5345,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 = PL1U_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 */
@@ -5347,7 +5357,8 @@ 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 },
+              .access = PL1U_R, .type = ARM_CP_CONST,
+              .resetvalue = cpu->revidr },
             REGINFO_SENTINEL
         };
         ARMCPRegInfo id_cp_reginfo[] = {
-- 
2.17.1

Re: [Qemu-devel] [PATCH v3 3/5] target/arm: expose CPUID registers to userspace

[Peter Maydell]

On 25 June 2018 at 17:00, 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
> CONFIG_USER_ONLY we don't emulate the kernels trap and emulate but
> instead just lower the read permission to PL0_R (hidden behind the
> PL1U_R macro).
>
> The ID_AA64PFR0_EL1 is a little special as the GIC version is hidden
> from userspace so we can define a ARM_CP_CONST version of the register
> for usermode.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> squash! target/arm: expose CPUID registers to userspace
> ---
>  target/arm/cpu.h    |  7 +++++++
>  target/arm/helper.c | 39 +++++++++++++++++++++++++--------------
>  2 files changed, 32 insertions(+), 14 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index a4507a2d6f..156c811654 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -1924,6 +1924,13 @@ static inline bool cptype_valid(int cptype)
>  #define PL0_R (0x02 | PL1_R)
>  #define PL0_W (0x01 | PL1_W)
>
> +/* for AArch64 HWCAP_CPUID to userspace */

The comment here should define what the semantics of this new #define are.

> +#ifdef CONFIG_USER_ONLY
> +#define PL1U_R PL0_R
> +#else
> +#define PL1U_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)

>  void register_cp_regs_for_features(ARMCPU *cpu)
>  {
> @@ -4934,18 +4936,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 = PL0_R, .type = ARM_CP_CONST,
> +              .resetvalue = cpu->id_aa64pfr0
> +#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 = PL1U_R, .type = ARM_CP_CONST,
>                .resetvalue = cpu->id_aa64pfr1},
>              { .name = "ID_AA64PFR2_EL1_RESERVED", .state = ARM_CP_STATE_AA64,
>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 2,


The spec says that the values revealed to userspace are masked,
so only certain fields are shown to userspace and others (reserved,
impdef, invisible fields) are masked out and replaced with fixed
values. I don't see where in this patchset we're doing that.

Also, in order to correctly reveal the ID regs to linux-user code,
we need to set them correctly for what we're emulating, which at
the moment we do not if the cpu is 'max' or 'any'. (See the comment
in arm_max_initfn().)

> @@ -4973,11 +4983,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 = PL1U_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 = PL1U_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,
> @@ -5005,11 +5015,11 @@ 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,
> +              .access = PL1U_R, .type = ARM_CP_CONST,
>                .resetvalue = cpu->id_aa64isar0 },
>              { .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 = PL1U_R, .type = ARM_CP_CONST,
>                .resetvalue = cpu->id_aa64isar1 },
>              { .name = "ID_AA64ISAR2_EL1_RESERVED", .state = ARM_CP_STATE_AA64,
>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 6, .opc2 = 2,
> @@ -5037,11 +5047,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 = PL1U_R, .type = ARM_CP_CONST,
>                .resetvalue = cpu->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 = PL1U_R, .type = ARM_CP_CONST,
>                .resetvalue = cpu->id_aa64mmfr1 },
>              { .name = "ID_AA64MMFR2_EL1_RESERVED", .state = ARM_CP_STATE_AA64,
>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 7, .opc2 = 2,
> @@ -5335,7 +5345,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 = PL1U_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 */
> @@ -5347,7 +5357,8 @@ 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 },
> +              .access = PL1U_R, .type = ARM_CP_CONST,
> +              .resetvalue = cpu->revidr },

This is a STATE_BOTH register, so just changing the .access value
will reveal the aarch32 cpreg to userspace, which is wrong.

>              REGINFO_SENTINEL
>          };
>          ARMCPRegInfo id_cp_reginfo[] = {
> --
> 2.17.1

thanks
-- PMM

[Qemu-devel] [PATCH v3 4/5] 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>
---
 linux-user/elfload.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 13bc78d0c8..76d7674649 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -566,6 +566,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(feat, hwcap) \
-- 
2.17.1

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

[Richard Henderson]

On 06/25/2018 09:00 AM, 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>
> ---
>  linux-user/elfload.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[Qemu-devel] [PATCH v3 5/5] 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>
---
 tests/tcg/aarch64/Makefile.target |  2 +-
 tests/tcg/aarch64/sysregs.c       | 99 +++++++++++++++++++++++++++++++
 2 files changed, 100 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..177d1fe33b
--- /dev/null
+++ b/tests/tcg/aarch64/sysregs.c
@@ -0,0 +1,99 @@
+/*
+ * 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>
+
+#define get_cpu_reg(id) ({                                      \
+            unsigned long __val = 0xdeadbeef;                   \
+            asm("mrs %0, "#id : "=r" (__val));                  \
+            printf("%-20s: 0x%016lx\n", #id, __val);            \
+        })
+
+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");
+    }
+
+    get_cpu_reg(id_aa64isar0_el1);
+    get_cpu_reg(id_aa64isar1_el1);
+    get_cpu_reg(id_aa64mmfr0_el1);
+    get_cpu_reg(id_aa64mmfr1_el1);
+    get_cpu_reg(id_aa64pfr0_el1);
+    get_cpu_reg(id_aa64pfr1_el1);
+    get_cpu_reg(id_aa64dfr0_el1);
+    get_cpu_reg(id_aa64dfr1_el1);
+
+    get_cpu_reg(midr_el1);
+    get_cpu_reg(mpidr_el1);
+    get_cpu_reg(revidr_el1);
+
+    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;
+    }
+
+    return should_fail_count == 1 ? 0 : 1;
+}
-- 
2.17.1

Re: [Qemu-devel] [PATCH v3 5/5] 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>
> ---
>  tests/tcg/aarch64/Makefile.target |  2 +-
>  tests/tcg/aarch64/sysregs.c       | 99 +++++++++++++++++++++++++++++++
>  2 files changed, 100 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..177d1fe33b
> --- /dev/null
> +++ b/tests/tcg/aarch64/sysregs.c
> @@ -0,0 +1,99 @@
> +/*
> + * 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>
> +
<snip>
> +
> +    /* 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");
> +    }

Annoyingly this fails on qemu:debian-arm64-cross as it uses an older set
of headers than my desktop cross environment:

  aarch64-linux-gnu-gcc (Ubuntu/Linaro 7.3.0-16ubuntu3) 7.3.0

with the libc:

  Source: cross-toolchain-base (25ubuntu6)
  Version: 2.27-3ubuntu1cross1
  Provides: libc6-arm64-dcv1

So I'm thinking an #ifndef HWCAP_CPUID and define it would be acceptable
for a test case.

--
Alex Bennée

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

[Richard Henderson]

On 06/25/2018 09:00 AM, Alex Bennée 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>
> ---
>  tests/tcg/aarch64/Makefile.target |  2 +-
>  tests/tcg/aarch64/sysregs.c       | 99 +++++++++++++++++++++++++++++++
>  2 files changed, 100 insertions(+), 1 deletion(-)
>  create mode 100644 tests/tcg/aarch64/sysregs.c

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH v3 0/5] support reading some CPUID/CNT registers from user-space

[Peter Maydell]

On 25 June 2018 at 17:00, Alex Bennée <alex.bennee@linaro.org> wrote:
> Hi,
>
> This expands on the previous version by adding the additional CPUID
> registers as exposed by the HWCAP_CPUID kernel feature. Most of the
> work involves judicious use of macros. There is a easily extensible
> test case that uses the shiny new check-tcg infrastructure ;-)
>
> Alex Bennée (5):
>   target/arm: support reading of CNT[VCT|FRQ]_EL0 from user-space
>   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

Patch 1 is a bugfix not very related to the rest, so I've applied
it to target-arm.next, and left review comments on 2/3.

thanks
-- PMM