diff mbox series

[v6,30/41] target/arm: Flush tlb for ASID changes in EL2&0 translation regime

Message ID 20200201192916.31796-31-richard.henderson@linaro.org
State New
Headers show
Series target/arm: Implement ARMv8.1-VHE | expand

Commit Message

Richard Henderson Feb. 1, 2020, 7:29 p.m. UTC 
Since we only support a single ASID, flush the tlb when it changes.

Note that TCR_EL2, like TCR_EL1, has the A1 bit that chooses between
the two TTBR* registers for the location of the ASID.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/helper.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

Comments

Peter Maydell Feb. 3, 2020, 11:36 a.m. UTC | #1 
On Sat, 1 Feb 2020 at 19:29, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Since we only support a single ASID, flush the tlb when it changes.
>
> Note that TCR_EL2, like TCR_EL1, has the A1 bit that chooses between
> the two TTBR* registers for the location of the ASID.
>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/helper.c | 22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index cfa6ce59dc..f9be6b052f 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -3763,7 +3763,7 @@ static void vmsa_ttbcr_reset(CPUARMState *env, const ARMCPRegInfo *ri)
>      tcr->base_mask = 0xffffc000u;
>  }
>
> -static void vmsa_tcr_el1_write(CPUARMState *env, const ARMCPRegInfo *ri,
> +static void vmsa_tcr_el12_write(CPUARMState *env, const ARMCPRegInfo *ri,
>                                 uint64_t value)
>  {
>      ARMCPU *cpu = env_archcpu(env);
> @@ -3789,7 +3789,17 @@ static void vmsa_ttbr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>  static void vmsa_tcr_ttbr_el2_write(CPUARMState *env, const ARMCPRegInfo *ri,
>                                      uint64_t value)
>  {
> -    /* TODO: There are ASID fields in here with HCR_EL2.E2H */
> +    /*
> +     * If we are running with E2&0 regime, then an ASID is active.
> +     * Flush if that might be changing.  Note we're not checking
> +     * TCR_EL2.A1 to know if this is really the TTBRx_EL2 that
> +     * holds the active ASID, only checking the field that might.
> +     */
> +    if (extract64(raw_read(env, ri) ^ value, 48, 16) &&
> +        (arm_hcr_el2_eff(env) & HCR_E2H)) {
> +        tlb_flush_by_mmuidx(env_cpu(env),
> +                            ARMMMUIdxBit_E20_2 | ARMMMUIdxBit_E20_0);
> +    }
>      raw_write(env, ri, value);

Since we don't flush TLBs when HCR_EL2.E2H changes, I'm wondering
about this sequence:

 * initially HCR_EL2.E2H == 1 and the E2&0 TLBs are populated
 * HCR_EL2.E2H is set to 0
 * TTBR1_EL2 is written with a different ASID from step 1,
   but we don't flush the TLBs because HCR_EL2.E2H is 0
 * HCR_EL2.E2H is set to 1
 * guest will pick up wrong-ASID TLB entries from step 1

Does the architecture require that the guest did some TLB
maintenance ops somewhere along the line to avoid this?
I haven't tried to look for them, but given the different
ASIDs I'm not sure it does...

thanks
-- PMM
Peter Maydell Feb. 3, 2020, 11:49 a.m. UTC | #2 
On Mon, 3 Feb 2020 at 11:36, Peter Maydell <peter.maydell@linaro.org> wrote:
> Since we don't flush TLBs when HCR_EL2.E2H changes, I'm wondering
> about this sequence:
>
>  * initially HCR_EL2.E2H == 1 and the E2&0 TLBs are populated
>  * HCR_EL2.E2H is set to 0
>  * TTBR1_EL2 is written with a different ASID from step 1,
>    but we don't flush the TLBs because HCR_EL2.E2H is 0
>  * HCR_EL2.E2H is set to 1
>  * guest will pick up wrong-ASID TLB entries from step 1
>
> Does the architecture require that the guest did some TLB
> maintenance ops somewhere along the line to avoid this?
> I haven't tried to look for them, but given the different
> ASIDs I'm not sure it does...

...HCR_EL2.E2H documents that it "is permitted to be cached
in a TLB", which means that the guest has to do *some*
TLB maintenance ops if it changes it; unclear exactly which,
though...

thanks
-- PMM
Richard Henderson Feb. 4, 2020, 1:58 p.m. UTC | #3 
On 2/3/20 11:49 AM, Peter Maydell wrote:
> On Mon, 3 Feb 2020 at 11:36, Peter Maydell <peter.maydell@linaro.org> wrote:
>> Since we don't flush TLBs when HCR_EL2.E2H changes, I'm wondering
>> about this sequence:
>>
>>  * initially HCR_EL2.E2H == 1 and the E2&0 TLBs are populated
>>  * HCR_EL2.E2H is set to 0
>>  * TTBR1_EL2 is written with a different ASID from step 1,
>>    but we don't flush the TLBs because HCR_EL2.E2H is 0
>>  * HCR_EL2.E2H is set to 1
>>  * guest will pick up wrong-ASID TLB entries from step 1
>>
>> Does the architecture require that the guest did some TLB
>> maintenance ops somewhere along the line to avoid this?
>> I haven't tried to look for them, but given the different
>> ASIDs I'm not sure it does...
> 
> ...HCR_EL2.E2H documents that it "is permitted to be cached
> in a TLB", which means that the guest has to do *some*
> TLB maintenance ops if it changes it; unclear exactly which,
> though...

TLBI ALLE2 would seem to fit the bill after E2H change.


r~
diff mbox series

Patch

diff --git a/target/arm/helper.c b/target/arm/helper.c
index cfa6ce59dc..f9be6b052f 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -3763,7 +3763,7 @@  static void vmsa_ttbcr_reset(CPUARMState *env, const ARMCPRegInfo *ri)
     tcr->base_mask = 0xffffc000u;
 }
 
-static void vmsa_tcr_el1_write(CPUARMState *env, const ARMCPRegInfo *ri,
+static void vmsa_tcr_el12_write(CPUARMState *env, const ARMCPRegInfo *ri,
                                uint64_t value)
 {
     ARMCPU *cpu = env_archcpu(env);
@@ -3789,7 +3789,17 @@  static void vmsa_ttbr_write(CPUARMState *env, const ARMCPRegInfo *ri,
 static void vmsa_tcr_ttbr_el2_write(CPUARMState *env, const ARMCPRegInfo *ri,
                                     uint64_t value)
 {
-    /* TODO: There are ASID fields in here with HCR_EL2.E2H */
+    /*
+     * If we are running with E2&0 regime, then an ASID is active.
+     * Flush if that might be changing.  Note we're not checking
+     * TCR_EL2.A1 to know if this is really the TTBRx_EL2 that
+     * holds the active ASID, only checking the field that might.
+     */
+    if (extract64(raw_read(env, ri) ^ value, 48, 16) &&
+        (arm_hcr_el2_eff(env) & HCR_E2H)) {
+        tlb_flush_by_mmuidx(env_cpu(env),
+                            ARMMMUIdxBit_E20_2 | ARMMMUIdxBit_E20_0);
+    }
     raw_write(env, ri, value);
 }
 
@@ -3849,7 +3859,7 @@  static const ARMCPRegInfo vmsa_cp_reginfo[] = {
                              offsetof(CPUARMState, cp15.ttbr1_ns) } },
     { .name = "TCR_EL1", .state = ARM_CP_STATE_AA64,
       .opc0 = 3, .crn = 2, .crm = 0, .opc1 = 0, .opc2 = 2,
-      .access = PL1_RW, .writefn = vmsa_tcr_el1_write,
+      .access = PL1_RW, .writefn = vmsa_tcr_el12_write,
       .resetfn = vmsa_ttbcr_reset, .raw_writefn = raw_write,
       .fieldoffset = offsetof(CPUARMState, cp15.tcr_el[1]) },
     { .name = "TTBCR", .cp = 15, .crn = 2, .crm = 0, .opc1 = 0, .opc2 = 2,
@@ -5175,10 +5185,8 @@  static const ARMCPRegInfo el2_cp_reginfo[] = {
       .resetvalue = 0 },
     { .name = "TCR_EL2", .state = ARM_CP_STATE_BOTH,
       .opc0 = 3, .opc1 = 4, .crn = 2, .crm = 0, .opc2 = 2,
-      .access = PL2_RW,
-      /* no .writefn needed as this can't cause an ASID change;
-       * no .raw_writefn or .resetfn needed as we never use mask/base_mask
-       */
+      .access = PL2_RW, .writefn = vmsa_tcr_el12_write,
+      /* no .raw_writefn or .resetfn needed as we never use mask/base_mask */
       .fieldoffset = offsetof(CPUARMState, cp15.tcr_el[2]) },
     { .name = "VTCR", .state = ARM_CP_STATE_AA32,
       .cp = 15, .opc1 = 4, .crn = 2, .crm = 1, .opc2 = 2,