Re: [PATCH v3] xen/riscv: add p2m context switch handling for VSATP and HGATP

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.02.2026 17:29, Oleksii Kurochko wrote:
> Introduce helpers to manage VS-stage and G-stage translation state during
> vCPU context switches.
> 
> As VSATP and HGATP cannot be updated atomically, clear VSATP on context
> switch-out to prevent speculative VS-stage translations from being associated
> with an incorrect VMID. On context switch-in, restore HGATP and VSATP in the
> required order.
> 
> Add p2m_handle_vmenter() to perform VMID management and issue TLB flushes
> only when required (e.g. on VMID reuse or generation change).
> 
> This provides the necessary infrastructure for correct p2m context switching
> on RISC-V.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> ---
> Changes in v3:
>  - Add comment above p2m_ctxt_switch_{to, from}().

I find these and other speculation related comments problematic: You can't
prevent every kind of speculation that way, yet all these comments are
written as if that was the case. What I think you mean in all cases is
speculation using the wrong set of page tables?

> --- a/xen/arch/riscv/p2m.c
> +++ b/xen/arch/riscv/p2m.c
> @@ -1434,3 +1434,82 @@ struct page_info *p2m_get_page_from_gfn(struct 
> p2m_domain *p2m, gfn_t gfn,
>  
>      return get_page(page, p2m->domain) ? page : NULL;
>  }
> +
> +/* Should be called before other CSRs are stored to avoid speculation */
> +void p2m_ctxt_switch_from(struct vcpu *p)

What interaction with the storing of other CSRs would be problematic?

> +{
> +    if ( is_idle_vcpu(p) )
> +        return;
> +
> +    /*
> +     * 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 what will be done in
> +     * p2m_handle_vmenter().
> +     */
> +    p->arch.vsatp = csr_swap(CSR_VSATP, 0);
> +
> +    /*
> +     * Nothing to do with HGATP as it will be update in p2m_ctxt_switch_to()
> +     * or/and in p2m_handle_vmenter().
> +     */
> +}
> +
> +/* Should be called after other CSRs are restored to avoid speculation */
> +void p2m_ctxt_switch_to(struct vcpu *n)

Same question here.

> +{
> +    struct p2m_domain *p2m = p2m_get_hostp2m(n->domain);
> +
> +    if ( is_idle_vcpu(n) )
> +        return;
> +
> +    csr_write(CSR_HGATP, construct_hgatp(p2m, n->arch.vmid.vmid));
> +    /*
> +     * As VMID is unique per vCPU and just re-used here thereby there is no
> +     * need for G-stage TLB flush here.
> +     */
> +
> +    csr_write(CSR_VSATP, n->arch.vsatp);
> +    /*
> +     * As at the start of context switch VSATP were set to 0, so no 
> speculation
> +     * could happen thereby there is no need for VS TLB flush here.
> +     */
> +}
> +
> +void p2m_handle_vmenter(void)
> +{
> +    struct vcpu *c = current;

Can you please stick to conventional names, i.e. "curr" here?

> +    struct p2m_domain *p2m = p2m_get_hostp2m(c->domain);
> +    struct vcpu_vmid *p_vmid = &c->arch.vmid;
> +    uint16_t old_vmid, new_vmid;

Nit: No real need for a fixed-width type here?

> +    bool need_flush;
> +
> +    BUG_ON(is_idle_vcpu(current));
> +
> +    old_vmid = p_vmid->vmid;
> +    need_flush = vmid_handle_vmenter(p_vmid);
> +    new_vmid = p_vmid->vmid;
> +
> +#ifdef P2M_DEBUG
> +    printk("%pv: oldvmid(%d) new_vmid(%d), need_flush(%d)\n",
> +           c, old_vmid, new_vmid, need_flush);
> +#endif
> +
> +    if ( old_vmid != new_vmid )
> +        csr_write(CSR_HGATP, construct_hgatp(p2m, p_vmid->vmid));
> +
> +    if ( unlikely(need_flush) )
> +    {
> +        local_hfence_gvma_all();
> +        flush_tlb_guest_local();
> +    }

Why would the latter be needed here at all? And if it was needed, why
would it depend on whether a VMID roll-over occurred?

Jan