Re: [PATCH 5/5] xen/riscv: add SFENCE.VMA after enabling paging

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

On 8/27/26 6:53 PM, Oleksii Kurochko wrote:
> 
> 
> On 8/27/26 5:33 PM, Baptiste Le Duc wrote:
>> turn_on_mmu() writes satp to switch on Sv39 paging but never fences
>> afterwards.
>>
>> Xen never allocates a non-zero ASID, so per the Privileged spec, sec.
>> 12.2.1 "Supervisor Memory-Management Fence Instruction":
>>
>>    "If the implementation does not provide ASIDs, or software chooses
>>    to always use ASID 0, then after every satp write, software should
>>    execute SFENCE.VMA with rs1=x0."
>>
>> The spec text around this rule hedges with "may be necessary", but
>> RISC-V spec co-author Andrew Waterman confirmed on the ISA manual
>> issue tracker that the fence after a satp write is not optional in
>> this case: "The SFENCE after the SATP write is definitely necessary
>> ... In general, you need to SFENCE after you've recycled an ASID.
>> Since we don't use ASIDs in the Linux kernel yet, every context
>> switch is effectively an ASID reuse, hence the full TLB flush." [1]
>> The same reasoning applies to Xen: with ASID always 0, this satp
>> write is indistinguishable from an ASID reuse to the hart, so the
>> fence is required for correctness.
> 
> But at the moment of execution of turn_on_mmu() we don't use any ASID, 
> do we? It was used in check_pgtbl_mode_support() but at the end it is done:
> 
>      csr_write(CSR_SATP, 0);
> 
>      sfence_vma();
> 
> So basically Bare mode + flush all TLBs presented before and then up to
> 
> ...
> 
>>
>> Add the missing SFENCE.VMA to order those page-table stores before
>> the hart's first translation under the new mapping.
>>
>> [1] https://github.com/riscv/riscv-isa-manual/issues/226
>>
>> Fixes: f5035d480f7a ("xen: add files needed for minimal riscv build")
>> Assisted-by: Claude:claude-opus-5
>> Signed-off-by: Baptiste Le Duc <baptiste.le-duc@vates.tech>
>> ---
>>   xen/arch/riscv/riscv64/head.S | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/xen/arch/riscv/riscv64/head.S b/xen/arch/riscv/riscv64/ 
>> head.S
>> index 9c40512e61..7f6edc972f 100644
>> --- a/xen/arch/riscv/riscv64/head.S
>> +++ b/xen/arch/riscv/riscv64/head.S
>> @@ -98,6 +98,7 @@ FUNC(turn_on_mmu)
>>           srli    t1, t1, PAGE_SHIFT
>>           or      t1, t1, t0
>>           csrw    CSR_SATP, t1
> 
> ... ASID isn't used as we are in Bare mode.
> 
> What am I missing?
> 
>> +        sfence.vma
> 
> The one thing which possibly matters here, and could explain why 
> sfence.vma is needed, is:
> ```
> Implementations with virtual memory are permitted to perform address 
> translations speculatively and earlier than required by an explicit 
> memory access, and are permitted to cache them in address translation 
> cache structures—including possibly caching the identity mappings from 
> effective address to physical address used in Bare translation modes and 
> M-mode.
> ```
> 
> So the TLB could potentially be populated with identity mappings, and I 
> agree that it would be better to flush those.
> 
> I’m not entirely convinced, though, that the reason here is the ASID 
> itself. Rather, it seems that we want to flush because of potentially 
> cached speculative identity mappings.
> 
> If this reasoning looks correct to you, could we update the commit 
> message to reflect this rationale for why sfence.vma is needed here?

My suggestion is:

xen/riscv: add SFENCE.VMA after writing satp in turn_on_mmu()

The existing SFENCE.VMA before the satp write only orders the page
table stores from setup_initial_pagetables() against subsequent
implicit reads. It cannot invalidate translations cached after it
retires, and the Privileged spec permits an implementation to
translate speculatively and to cache the identity mappings used in
Bare mode. Such an entry would shadow the Sv39 translation once
paging is on, which matters because turn_on_mmu() jumps to a linker
address that is not identity mapped.

Does it make sense?

~ Oleksii