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

[Michal Orzel <michal.orzel@xxxxxxx>]

On 21/06/2023 12:02, Julien Grall wrote:
> 
> 
> 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?
Whatever you prefer. If you care about my tag and want to have more than two, 
feel free to add it to
all the patches.

> 
>>
>> 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
> 
Although having isb after dsb would be clearer to the user and consistent with 
other TBL invalidations,
the one after msr can do the job for now, I agree.

~Michal