Re: [Qemu-devel] [PATCH v2 3/3] target/arm: Flush only the TLBs affected by TTBR*_EL1

[Peter Maydell]

On 19 October 2018 at 16:21, Richard Henderson
<address@hidden> wrote:
> On 10/19/18 7:28 AM, Peter Maydell wrote:
>> On 19 October 2018 at 02:56, Richard Henderson
>> <address@hidden> wrote:
>>> Only the EL0 and EL1 TLBs are affected by the EL1 register,
>>> so flush only 2 of the 8 TLBs.
>>>
>>> In testing a boot of the Ubuntu installer to the first menu, this
>>> accounts for nearly all of the full tlb flushes: all but 11k of
>>> the 1.2M instances without the patch.
>>>
>>> Signed-off-by: Richard Henderson <address@hidden>
>>> ---
>>>  target/arm/helper.c | 16 +++++++++-------
>>>  1 file changed, 9 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>>> index ed70ac645e..3ba8e66487 100644
>>> --- a/target/arm/helper.c
>>> +++ b/target/arm/helper.c
>>> @@ -2706,14 +2706,16 @@ static void vmsa_tcr_el1_write(CPUARMState *env, 
>>> const ARMCPRegInfo *ri,
>>>      tcr->raw_tcr = value;
>>>  }
>>>
>>> -static void vmsa_ttbr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>>> -                            uint64_t value)
>>> +static void vmsa_ttbr_el1_write(CPUARMState *env, const ARMCPRegInfo *ri,
>>> +                                uint64_t value)
>>>  {
>>>      /* If the ASID changes (with a 64-bit write), we must flush the TLB.  
>>> */
>>>      if (cpreg_field_is_64bit(ri) &&
>>>          extract64(raw_read(env, ri) ^ value, 48, 16) != 0) {
>>>          ARMCPU *cpu = arm_env_get_cpu(env);
>>> -        tlb_flush(CPU(cpu));
>>> +        tlb_flush_by_mmuidx(CPU(cpu),
>>> +                            ARMMMUIdxBit_S12NSE1 |
>>> +                            ARMMMUIdxBit_S12NSE0);
>>
>> This isn't taking account of the possibility of secure mode.
>> ARMMMUIdxBit_S1SE0 and ARMMMUIdxBit_S1SE1 might also be affected.
>
> Ah.  Is there an easy way to tell if secure mode is present/enabled?  It'd be
> nice to not flush tlbs that aren't in use...

If ARM_FEATURE_EL3 is set, we have an EL3. It is probably possible
to look at the current state of the CPU and determine whether we
need to flush the NS TLBs or the S TLBs, but I would need to
think about that. For AArch32 there are banked registers here, so
we could in theory distinguish writes to the S banked and NS banked
regs. For AArch64 there's only one EL1 register, so we'd
need to check the Arm ARM for how interleaved writes to the TTBR_EL1
and flipping between S and NS work (and when the guest is supposed
to do the TLB maintenance anyway).

A conservative check is probably:
  if arm_current_el() < 3
     // TTBR definitely can only be affecting the EL0/1
     // translation regime for the current security state
     if arm_is_secure_below_el3()
        if EL3 is AArch32
            flush S1SE0, S1E3
        else
            flush S1SE0, S1SE1
     else
        flush S12NSE1, S12NSE0
  else
     // err on the side of flushing more than maybe we need to
     flush S1SE0, S12NSE1, S12NSE0
     if EL3 is AArch32
          flush S1E3
     else
          flush S1SE1

(but you should check my logic ;-))

>> And for AArch32, this writefn is used for the secure-banked versions
>> of TTBR0/TTBR1, which means ARMMMUIdxBit_S1E3 may also need flushing.
>
> For aarch32, we don't have an asid, and so do not flush at all.

We do for AArch32 with TTBCR.EAE == 1 (ie LPAE, when you want to
use the 64-bit form of the register, accessed via MRRC/MCRR).
cpreg_field_is_64bit() is true for both "AArch64 sysreg" and
"AArch32 64-bit cp reg".

thanks
-- PMM