Re: [PATCH 3/7] xen/arm64: head: Add missing isb in setup_fixmap()

[Julien Grall <julien@xxxxxxx>]

Hi,

On 21/06/2023 10:33, Michal Orzel wrote:
> On 19/06/2023 19:01, Julien Grall wrote:
> > From: Julien Grall <jgrall@xxxxxxxxxx>
> >
> > On older version of the Arm Arm (ARM DDI 0487E.a, B2-125) there were
> > the following paragraph:
> >
> > "DMB and DSB instructions affect reads and writes to the memory system
> > generated by Load/Store instructions and data or unified cache
> > maintenance instructions being executed by the PE. Instruction fetches
> > or accesses caused by a hardware translation table access are not
> > explicit accesses."
> >
> > Newer revision (e.g. ARM DDI 0487J.a) doesn't have the second sentence
> > (it might be somewhere else in the Arm Arm). But the interpretation is
> > not much different.
> >
> > In setup_fixmap(), we write the fixmap area and may be used soon after,
> > for instance, to write to the UART. IOW, there could be hardware
> > translation table access. So we need to ensure the 'dsb' has completed
> > before continuing. Therefore add an 'isb'.
> >
> > Fixes: 2b11c3646105 ("xen/arm64: head: Remove 1:1 mapping as soon as it is not 
> > used")
> > Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>
>
> I'm happy with the whole series but I do not see a point in flooding each patch 
> with my tag
> since you already got two (from Henry and Luca).
Thanks. To clarify, shall I add it in each patch or only this one?

> When it comes to essential isb() after dsb() in arm64 head.S, I can see that we 
> are missing one in enable_mmu()
> after TLB invalidation. On HW without FEAT_ETS the TLB is "guaranteed to be 
> complete after the execution of
> DSB by that PE, followed by a Context synchronization event", so I view isb as 
> necessary there.
While there is no ISB directly after DSB NSH, there are one right after 
MSR. I don't think we need one before because nothing will use the TLBs 
between before the ISB.
        /*
         * The state of the TLBs is unknown before turning on the MMU.
         * Flush them to avoid stale one.
         */
        tlbi  alle2                  /* Flush hypervisor TLBs */
        dsb   nsh

        /* Write Xen's PT's paddr into TTBR0_EL2 */
        load_paddr x0, boot_pgtable
        msr   TTBR0_EL2, x0
        isb


> We could also
> introduce (just like for arm32) flush_xen_tlb_local macro and use it there + 
> remove opencoding it.
That would be good. But I don't think this is necessary here (see above).

Cheers,

--
Julien Grall