Re: [PATCH v3 2/7] target/arm: Honor the HCR_EL2.{TVM,TRVM} bits

[Peter Maydell]

On Tue, 18 Feb 2020 at 19:10, Richard Henderson
<address@hidden> wrote:
>
> These bits trap EL1 access to various virtual memory controls.
>
> Buglink: https://bugs.launchpad.net/bugs/1855072
> Signed-off-by: Richard Henderson <address@hidden>
> ---
> v2: Include TTBCR.
> ---
>  target/arm/helper.c | 77 ++++++++++++++++++++++++++++++---------------
>  1 file changed, 52 insertions(+), 25 deletions(-)

> +/* Check for traps from EL1 due to HCR_EL2.TVM and HCR_EL2.TRVM.  */
> +static CPAccessResult access_tvm_trvm(CPUARMState *env, const ARMCPRegInfo 
> *ri,
> +                                      bool isread)
> +{
> +    if (arm_current_el(env) == 1) {
> +        uint64_t trap = isread ? HCR_TRVM : HCR_TVM;
> +        if (arm_hcr_el2_eff(env) & trap) {
> +            return CP_ACCESS_TRAP_EL2;
> +        }
> +    }
> +    return CP_ACCESS_OK;
> +}

v7 doesn't have HCR_TRVM -- we should stop the guest being
able to write to that bit if we don't want to do a version
check on the CPU here to see whether to honour it.

> +
>  static void dacr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t 
> value)
>  {
>      ARMCPU *cpu = env_archcpu(env);
> @@ -785,7 +798,8 @@ static const ARMCPRegInfo cp_reginfo[] = {
>       */
>      { .name = "CONTEXTIDR_EL1", .state = ARM_CP_STATE_BOTH,
>        .opc0 = 3, .opc1 = 0, .crn = 13, .crm = 0, .opc2 = 1,
> -      .access = PL1_RW, .secure = ARM_CP_SECSTATE_NS,
> +      .access = PL1_RW, .accessfn = access_tvm_trvm,
> +      .secure = ARM_CP_SECSTATE_NS,
>        .fieldoffset = offsetof(CPUARMState, cp15.contextidr_el[1]),
>        .resetvalue = 0, .writefn = contextidr_write, .raw_writefn = 
> raw_write, },
>      { .name = "CONTEXTIDR_S", .state = ARM_CP_STATE_AA32,

We could I guess add the accessfn to CONTEXTIDR_S, which will
do nothing now but would save us forgetting it if we ever
implement emulation of secure EL2... (For the other regs
touched by this patch this happens automatically because they
don't specify a secure-state and so one regdef does both.)

> @@ -877,9 +891,11 @@ static const ARMCPRegInfo not_v7_cp_reginfo[] = {
>        .opc1 = CP_ANY, .opc2 = 3, .access = PL1_W, .writefn = tlbimvaa_write,
>        .type = ARM_CP_NO_RAW },
>      { .name = "PRRR", .cp = 15, .crn = 10, .crm = 2,
> -      .opc1 = 0, .opc2 = 0, .access = PL1_RW, .type = ARM_CP_NOP },
> +      .opc1 = 0, .opc2 = 0, .access = PL1_RW, .accessfn = access_tvm_trvm,
> +      .type = ARM_CP_NOP },
>      { .name = "NMRR", .cp = 15, .crn = 10, .crm = 2,
> -      .opc1 = 0, .opc2 = 1, .access = PL1_RW, .type = ARM_CP_NOP },
> +      .opc1 = 0, .opc2 = 1, .access = PL1_RW, .accessfn = access_tvm_trvm,
> +      .type = ARM_CP_NOP },

Why are we adding an accessfn that checks bits in a v7-and-later
register to regdefs in the "not_v7_cp_reginfo" array? These only
get used for v6 and earlier CPUs...

> @@ -4158,12 +4185,12 @@ static const ARMCPRegInfo lpae_cp_reginfo[] = {
>      /* NOP AMAIR0/1 */
>      { .name = "AMAIR0", .state = ARM_CP_STATE_BOTH,
>        .opc0 = 3, .crn = 10, .crm = 3, .opc1 = 0, .opc2 = 0,
> -      .access = PL1_RW, .type = ARM_CP_CONST,
> -      .resetvalue = 0 },
> +      .access = PL1_RW, .accessfn = access_tvm_trvm,
> +      .type = ARM_CP_CONST, .resetvalue = 0 },
>      /* AMAIR1 is mapped to AMAIR_EL1[63:32] */
>      { .name = "AMAIR1", .cp = 15, .crn = 10, .crm = 3, .opc1 = 0, .opc2 = 1,
> -      .access = PL1_RW, .type = ARM_CP_CONST,
> -      .resetvalue = 0 },
> +      .access = PL1_RW, .accessfn = access_tvm_trvm,
> +      .type = ARM_CP_CONST, .resetvalue = 0 },
>      { .name = "PAR", .cp = 15, .crm = 7, .opc1 = 0,
>        .access = PL1_RW, .type = ARM_CP_64BIT, .resetvalue = 0,
>        .bank_fieldoffsets = { offsetof(CPUARMState, cp15.par_s),

I think you have missed adding the accessfn to the 64-bit
TTBR0 and TTBR1 regdefs in lpae_cp_reginfo[].

> @@ -4889,7 +4916,7 @@ static const ARMCPRegInfo v8_cp_reginfo[] = {
>        .type = ARM_CP_NOP, .access = PL1_W },
>      /* MMU Domain access control / MPU write buffer control */
>      { .name = "DACR", .cp = 15, .opc1 = 0, .crn = 3, .crm = 0, .opc2 = 0,
> -      .access = PL1_RW, .resetvalue = 0,
> +      .access = PL1_RW, .accessfn = access_tvm_trvm, .resetvalue = 0,
>        .writefn = dacr_write, .raw_writefn = raw_write,
>        .bank_fieldoffsets = { offsetoflow32(CPUARMState, cp15.dacr_s),
>                               offsetoflow32(CPUARMState, cp15.dacr_ns) } },

There is also a DACR definition in not_v8_cp_reginfo[] which
I think needs the accessfn as well.

> @@ -7716,7 +7743,7 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>          ARMCPRegInfo sctlr = {
>              .name = "SCTLR", .state = ARM_CP_STATE_BOTH,
>              .opc0 = 3, .opc1 = 0, .crn = 1, .crm = 0, .opc2 = 0,
> -            .access = PL1_RW,
> +            .access = PL1_RW, .accessfn = access_tvm_trvm,
>              .bank_fieldoffsets = { offsetof(CPUARMState, cp15.sctlr_s),
>                                     offsetof(CPUARMState, cp15.sctlr_ns) },
>              .writefn = sctlr_write, .resetvalue = cpu->reset_sctlr,

thanks
-- PMM