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 <address@hidden> wrote:
>
> This tests a bunch of registers that the kernel allows userspace to
> read including the CPUID registers.
>
> Signed-off-by: Alex Bennée <address@hidden>
>
> ---
> 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