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

[Julien Grall <julien@xxxxxxx>]

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? */
        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

        ret

Cheers,

--
Julien Grall