Re: [Qemu-devel] [PATCH] target-arm: Add missed AArch32 TLBI sytem registers

[Peter Maydell]

On 23 June 2016 at 16:11, Sergey Sorokin <address@hidden> wrote:
> Some PL2 related TLBI system registers are missed in AArch32
> implementation. The patch fixes it.
>
> Signed-off-by: Sergey Sorokin <address@hidden>
> ---
>  target-arm/helper.c | 148 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 148 insertions(+)

Apologies for the delayed review, I think I missed this patch and
failed to put it in my to-review queue.

A couple of minor points below but most of this is good.

> +static void tlbimva_hyp_write(CPUARMState *env, const ARMCPRegInfo *ri,
> +                              uint64_t value)
> +{
> +    CPUState *cs = ENV_GET_CPU(env);
> +
> +    tlb_flush_page_by_mmuidx(cs, value & TARGET_PAGE_MASK, ARMMMUIdx_S1E2, 
> -1);

TARGET_PAGE_MASK isn't what you think it is (TARGET_PAGE_BITS is only 10).
You need to do something that's definitely a 12 bit mask (use extract32,
or MAKE_64BIT_MASK, or a literal, I don't care much).

> +}
> +
> +static void tlbimva_hyp_is_write(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                 uint64_t value)
> +{
> +    CPUState *other_cs;
> +    uint64_t pageaddr = value & TARGET_PAGE_MASK;

Ditto.

> +
> +    CPU_FOREACH(other_cs) {
> +        tlb_flush_page_by_mmuidx(other_cs, pageaddr, ARMMMUIdx_S1E2, -1);
> +    }
> +}
> +
>  static const ARMCPRegInfo cp_reginfo[] = {
>      /* Define the secure and non-secure FCSE identifier CP registers
>       * separately because there is no secure bank in V8 (no _EL3).  This 
> allows
> @@ -1238,6 +1343,14 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
>        .type = ARM_CP_NO_RAW, .access = PL1_W, .writefn = tlbiasid_write },
>      { .name = "TLBIMVAA", .cp = 15, .opc1 = 0, .crn = 8, .crm = 7, .opc2 = 3,
>        .type = ARM_CP_NO_RAW, .access = PL1_W, .writefn = tlbimvaa_write },
> +    { .name = "TLBIALLNSNH",
> +      .cp = 15, .opc1 = 4, .crn = 8, .crm = 7, .opc2 = 4,
> +      .type = ARM_CP_NO_RAW, .access = PL2_W,
> +      .writefn = tlbiall_nsnh_write },
> +    { .name = "TLBIALLNSNHIS",
> +      .cp = 15, .opc1 = 4, .crn = 8, .crm = 3, .opc2 = 4,
> +      .type = ARM_CP_NO_RAW, .access = PL2_W,
> +      .writefn = tlbiall_nsnh_is_write },

These don't exist on v7 unless the virtualization extensions are present
(though they do exist on v8 without EL3).

>      REGINFO_SENTINEL
>  };
>
> @@ -3273,6 +3386,22 @@ static const ARMCPRegInfo v8_cp_reginfo[] = {
>        .type = ARM_CP_NO_RAW, .access = PL1_W, .writefn = tlbimva_write },
>      { .name = "TLBIMVAAL", .cp = 15, .opc1 = 0, .crn = 8, .crm = 7, .opc2 = 
> 7,
>        .type = ARM_CP_NO_RAW, .access = PL1_W, .writefn = tlbimvaa_write },
> +    { .name = "TLBIIPAS2",
> +      .cp = 15, .opc1 = 4, .crn = 8, .crm = 4, .opc2 = 1,
> +      .type = ARM_CP_NO_RAW, .access = PL2_W,
> +      .writefn = tlbiipas2_write },
> +    { .name = "TLBIIPAS2IS",
> +      .cp = 15, .opc1 = 4, .crn = 8, .crm = 0, .opc2 = 1,
> +      .type = ARM_CP_NO_RAW, .access = PL2_W,
> +      .writefn = tlbiipas2_is_write },
> +    { .name = "TLBIIPAS2L",
> +      .cp = 15, .opc1 = 4, .crn = 8, .crm = 4, .opc2 = 5,
> +      .type = ARM_CP_NO_RAW, .access = PL2_W,
> +      .writefn = tlbiipas2_write },
> +    { .name = "TLBIIPAS2LIS",
> +      .cp = 15, .opc1 = 4, .crn = 8, .crm = 0, .opc2 = 5,
> +      .type = ARM_CP_NO_RAW, .access = PL2_W,
> +      .writefn = tlbiipas2_is_write },
>      /* 32 bit cache operations */
>      { .name = "ICIALLUIS", .cp = 15, .opc1 = 0, .crn = 7, .crm = 1, .opc2 = 
> 0,
>        .type = ARM_CP_NOP, .access = PL1_W },
> @@ -3605,6 +3734,25 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
>      { .name = "HTTBR", .cp = 15, .opc1 = 4, .crm = 2,
>        .access = PL2_RW, .type = ARM_CP_64BIT | ARM_CP_ALIAS,
>        .fieldoffset = offsetof(CPUARMState, cp15.ttbr0_el[2]) },
> +    { .name = "TLBIALLH", .cp = 15, .opc1 = 4, .crn = 8, .crm = 7, .opc2 = 0,
> +      .type = ARM_CP_NO_RAW, .access = PL2_W,
> +      .writefn = tlbiall_hyp_write },
> +    { .name = "TLBIALLHIS", .cp = 15, .opc1 = 4, .crn = 8, .crm = 3, .opc2 = 
> 0,
> +      .type = ARM_CP_NO_RAW, .access = PL2_W,
> +      .writefn = tlbiall_hyp_is_write },
> +    { .name = "TLBIMVAH", .cp = 15, .opc1 = 4, .crn = 8, .crm = 7, .opc2 = 1,
> +      .type = ARM_CP_NO_RAW, .access = PL2_W,
> +      .writefn = tlbimva_hyp_write },
> +    { .name = "TLBIMVAHIS", .cp = 15, .opc1 = 4, .crn = 8, .crm = 3, .opc2 = 
> 1,
> +      .type = ARM_CP_NO_RAW, .access = PL2_W,
> +      .writefn = tlbimva_hyp_is_write },
> +    { .name = "TLBIMVALH", .cp = 15, .opc1 = 4, .crn = 8, .crm = 7, .opc2 = 
> 5,
> +      .type = ARM_CP_NO_RAW, .access = PL2_W,
> +      .writefn = tlbimva_hyp_write },

This doesn't exist on v7.

> +    { .name = "TLBIMVALHIS",
> +      .cp = 15, .opc1 = 4, .crn = 8, .crm = 3, .opc2 = 5,
> +      .type = ARM_CP_NO_RAW, .access = PL2_W,
> +      .writefn = tlbimva_hyp_is_write },

Nor does this.

>      { .name = "TLBI_ALLE2", .state = ARM_CP_STATE_AA64,
>        .opc0 = 1, .opc1 = 4, .crn = 8, .crm = 7, .opc2 = 0,
>        .type = ARM_CP_NO_RAW, .access = PL2_W,
> --
> 1.9.3

thanks
-- PMM