Re: [PATCH early-RFC 4/5] xen/arm: mm: Rework switch_ttbr()

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Julien,

> On 7 Apr 2022, at 16:38, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi,
> 
> On 25/03/2022 14:48, Bertrand Marquis wrote:
>>> On 25 Mar 2022, at 15:42, Julien Grall <julien@xxxxxxx> wrote:
>>> 
>>> Hi Bertrand,
>>> 
>>> On 25/03/2022 14:35, Bertrand Marquis wrote:
>>>>> On 25 Mar 2022, at 15:24, Julien Grall <julien@xxxxxxx> wrote:
>>>>> On 25/03/2022 13:47, Bertrand Marquis wrote:
>>>>>> Hi Julien,
>>>>> 
>>>>> Hi Bertrand,
>>>>> 
>>>>>>> On 9 Mar 2022, at 12:20, Julien Grall <julien@xxxxxxx> wrote:
>>>>>>> 
>>>>>>> From: Julien Grall <jgrall@xxxxxxxxxx>
>>>>>>> 
>>>>>>> At the moment, switch_ttbr() is switching the TTBR whilst the MMU is
>>>>>>> still on.
>>>>>>> 
>>>>>>> Switching TTBR is like replacing existing mappings with new ones. So
>>>>>>> we need to follow the break-before-make sequence.
>>>>>>> 
>>>>>>> In this case, it means the MMU needs to be switched off while the
>>>>>>> TTBR is updated. In order to disable the MMU, we need to first
>>>>>>> jump to an identity mapping.
>>>>>>> 
>>>>>>> Rename switch_ttbr() to switch_ttbr_id() and create an helper on
>>>>>>> top to temporary map the identity mapping and call switch_ttbr()
>>>>>>> via the identity address.
>>>>>>> 
>>>>>>> switch_ttbr_id() is now reworked to temporarily turn off the MMU
>>>>>>> before updating the TTBR.
>>>>>>> 
>>>>>>> We also need to make sure the helper switch_ttbr() is part of the
>>>>>>> identity mapping. So move _end_boot past it.
>>>>>>> 
>>>>>>> Take the opportunity to instruction cache flush as the operation is
>>>>>>> only necessary when the memory is updated.
>>>>>> Your code is actually remove the instruction cache invalidation so
>>>>>> this sentence is a bit misleading.
>>>>> 
>>>>> I forgot to add the word "remove" in the sentence.
>>>> Ok (my sentence was also wrong by the way)
>>>>> 
>>>>>> Also an open question: shouldn’t we flush the data cache ?
>>>>> Do you mean clean/invalidate to PoC/PoU? Something else?
>>>> Yes, probably to PoU.
>>>>> 
>>>>>> As we switch from one TTBR to an other, there might be some data
>>>>>> in the cache dependent that could be flushed while the MMU is off
>>>>> 
>>>>> I am a bit confused. Those flush could also happen with the MMU on. So 
>>>>> how turning off the MMU would result to a problem? Note that the data 
>>>>> cache is still enabled during the switch.
>>>> If the first level of cache is VIPT and we turn off the MMU, I am 
>>>> wondering if this could not create troubles and could require the cache to 
>>>> be flushed before turning the MMU off.
>>> My reading of the Arm Arm (D5.11.1 "Data and unified caches" ARM DDI 
>>> 0487F.c) suggests the data cache is always PIPT.
>> You are right, only the instruction cache is VIPT.
>> So the problem most probably does not exist.
> 
> As discussed yesterda, I tweaked a bit switch_ttbr(). Below the version I 
> plan to use:
> 
>        /* 1) Ensure any previous read/write have completed */
>        dsb   sy /* XXX: Can this be a ish? */

I think here “ish” is enough as we only need the domain impacted by the MMU off 
operation to have his operations done.

For reference this is an extract from the code of trusted firmware before 
enabling MMU:

        /*
         * Ensure all translation table writes have drained into memory, the TLB
         * invalidation is complete, and translation register writes are
         * committed before enabling the MMU
         */
        dsb ish
        isb


>        isb
> 
>        /* 2) Turn off MMU */
>        mrs    x1, SCTLR_EL2
>        bic    x1, x1, #SCTLR_Axx_ELx_M
>        msr    SCTLR_EL2, x1
>        isb
> 
>        /*
>         * 3) Flush the TLBs.
>         * See asm/arm64/flushtlb.h for the explanation of the sequence.
>         */
>        dsb   nshst
>        tlbi  alle2
>        dsb   nsh
>        isb
> 
>        /* 4) Update the TTBR */
>        msr   TTBR0_EL2, x0
>        isb
> 
>        /* 5) Turn on the MMU */
>        mrs   x1, SCTLR_EL2
>        orr   x1, x1, #SCTLR_Axx_ELx_M  /* Enable MMU */
>        msr   SCTLR_EL2, x1
>        isb

Rest here sounds ok to me.

Cheers
Bertrand

> 
>        ret
> 
> Cheers,
> 
> -- 
> Julien Grall