Re: [PATCH v1 3/3] xen/riscv: implement p2m_ctx_switch_{to,from}_state()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.02.2026 11:32, Oleksii Kurochko wrote:
> On 2/4/26 4:02 PM, Jan Beulich wrote:
>> On 02.02.2026 13:57, Oleksii Kurochko wrote:
>>> +void p2m_ctx_switch_to(struct vcpu *n)
>>> +{
>>> +    struct vcpu_vmid *p_vmid = &n->arch.vmid;
>>> +    uint16_t old_vmid, new_vmid;
>>> +    bool need_flush;
>>> +
>>> +    if ( is_idle_vcpu(n) )
>>> +        return;
>> Shouldn't the other function have such a check, too?
> 
> Yes, it should. I will add the check.
> 
>>> +    old_vmid = p_vmid->vmid;
>>> +    need_flush = vmid_handle_vmenter(p_vmid);
>> As you're re-using x86'es model, I'm not quite sure why this would be needed.
>> I don't think we have such in x86; aiui this should be handled while actually
>> doing the VM entry.
> 
> I can move this to the end of do_trap().
> 
> However, isn’t this effectively the same?
> If a context switch to vCPU n happens, it means that once the trap is fully 
> handled,
> we will enter vCPU n.
> The same will be if to put the changes at the end of do_trap() before control
> will be given to VM.

Thing is - you will need it at the end of do_trap() (or wherever VM entry is
going to be dealt with) anyway, to cover situation other than context switch
as well.

>>> +    new_vmid = p_vmid->vmid;
>>> +
>>> +#ifdef P2M_DEBUG
>>> +    printk(XENLOG_INFO, "%pv: oldvmid(%d) new_vmid(%d), need_flush(%d)\n",
>>> +           n, old_vmid, new_vmid, need_flush);
>>> +#endif
>>> +
>>> +    csr_write(CSR_HGATP, construct_hgatp(p2m_get_hostp2m(current->domain),
>>> +              new_vmid));
>>> +
>>> +    if ( unlikely(need_flush) )
>>> +        local_hfence_gvma_all();
>>> +
>>> +    /*
>>> +     * According to the RISC-V specification, speculation can happen
>>> +     * during an update of hgatp and vsatp:
>>> +     *   No mechanism is provided to atomically change vsatp and hgatp
>>> +     *   together. Hence, to prevent speculative execution causing one
>>> +     *   guest’s VS-stage translations to be cached under another guest’s
>>> +     *   VMID, world-switch code should zero vsatp, then swap hgatp, then
>>> +     *   finally write the new vsatp value. Similarly, if henvcfg.PBMTE
>>> +     *   need be world-switched, it should be switched after zeroing vsatp
>>> +     *   but before writing the new vsatp value, obviating the need to
>>> +     *   execute an HFENCE.VVMA instruction.
>>> +     * So just flush TLBs for VS-Stage and G-stage after both of regs are
>>> +     * touched.
>>> +     */
>>> +    flush_tlb_guest_local();
>> Which "both regs" does the comment talk about? Doesn't the flush want to come
>> ...
>>
>>> +    /*
>>> +     * The vsatp register is a VSXLEN-bit read/write register that is
>>> +     * VS-mode’s version of supervisor register satp, so the following is
>>> +     * true for VSATP registers:
>>> +     *  Changing satp.MODE from Bare to other modes and vice versa also 
>>> takes
>>> +     *  effect immediately, without the need to execute an SFENCE.VMA
>>> +     *  instruction. Likewise, changes to satp.ASID take effect 
>>> immediately.
>>> +     * Considering the mentioned above and that VS-stage TLB flush has been
>>> +     * already done there is no need to flush VS-stage TLB after an update
>>> +     * of VSATP from Bare mode to what is written in `n->arch.vsatp`.
>>> +     */
>>> +    csr_write(CSR_VSATP, n->arch.vsatp);
>> ... after this? Then some of the commentary also doesn't look to be 
>> necessary.
> 
> I think there is no need for a guest TLB flush here or above
> (i.e. flush_tlb_guest_local()), because:
> - If CSR_VSATP is set to 0, or changed from 0 to a non-zero value, no 
> speculation
>    can occur. Therefore, the guest TLB cannot be polluted with unwanted 
> entries,
>    and no flush is required.
> - If need_flush is false, it means that no VMID wraparound has occurred.
>    In that case, the VMID is still unique, so we are safe and no guest TLB 
> flush is
>    needed.
> 
> Additionally, it seems that I do not need to introduce 
> flush_tlb_guest_local() at
> all, since local_hfence_gvma_all() already provides the same functionality.
> Instead, it probably makes sense to introduce an HFENCE_VVMA() helper as a 
> generic
> wrapper around the hfence.vvma instruction, and then reuse it inside
> local_hfence_gvma_all().

Hmm, I fear I'm confused: hfence.gvma != hfence.vvma ?

Jan