Re: [PATCH v7 1/5] xen/arm32: head: Widen the use of the temporary mapping

[Julien Grall <julien@xxxxxxx>]

Hi all,

On 16/04/2023 15:32, Julien Grall wrote:
> From: Julien Grall <jgrall@xxxxxxxxxx>
>
> At the moment, the temporary mapping is only used when the virtual
> runtime region of Xen is clashing with the physical region.
>
> In follow-up patches, we will rework how secondary CPU bring-up works
> and it will be convenient to use the fixmap area for accessing
> the root page-table (it is per-cpu).
>
> Rework the code to use temporary mapping when the Xen physical address
> is not overlapping with the temporary mapping.
>
> This also has the advantage to simplify the logic to identity map
> Xen.
I had to revert this patch a couple of months ago because Xen was not 
booting anymore on the Arndale. I finally managed to figure out what was 
the issue. It was an interesting read through the Arm Arm.
In switch_to_runtime_mapping we have the following code:

        flush_xen_tlb_local r0

        /* Map boot_second into boot_pgtable */
        mov_w r0, XEN_VIRT_START
        create_table_entry boot_pgtable, boot_second, r0, 1

        /* Ensure any page table updates are visible before continuing */
        dsb   nsh

ready_to_switch:
        mov   pc, lr

Per Arm Arm (Armv7 ARM DDI 0406C.d, section A3-148):

"The DMB and DSB memory barriers affect reads and writes to the memory 
system generated by load/store
instructions and data or unified cache maintenance operations being 
executed by the processor. Instruction fetches
or accesses caused by a hardware translation table access are not 
explicit accesses."
In the example above, 'lr' points to a region covered by the mapping we 
created just above. As the 'dsb' doesn't affect instruction fetch, it 
means it could happen before the page-table entry was observed and 
therefore result to a translation fault.
There is another situation where this could happen (taken from Linux 
commit d0b7a302d58a "Revert "arm64: Remove unnecessary ISBs from 
set_{pte,pmd,pud}"):
            MOV     X0, <valid pte>
            STR     X0, [Xptep]     // Store new PTE to page table
            DSB     ISHST
            LDR     X1, [X2]        // Translates using the new PTE

The dsb needs to be followed by an isb otherwise, a translation fault 
could occur.
There are a few places in where where the isb is missing. I think we 
didn't notice it before because we don't often create a new PTE and then 
directly access it.
Note that this issue is not in this patch but in fbd9b5fb4c26 
("xen/arm32: head: Remove restriction where to load Xen"). We didn't 
notice it because the temporary mapping wasn't much used before.
I will prepare a patch series to add the missing isb.

Cheers,

--
Julien Grall