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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.02.2026 17:36, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/p2m.c
> +++ b/xen/arch/riscv/p2m.c
> @@ -1434,3 +1434,126 @@ struct page_info *p2m_get_page_from_gfn(struct 
> p2m_domain *p2m, gfn_t gfn,
>  
>      return get_page(page, p2m->domain) ? page : NULL;
>  }
> +
> +void p2m_ctxt_switch_from(struct vcpu *p)
> +{
> +    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 is constructed each time when
> +     * p2m_handle_vmenter() is called.
> +     */
> +}
> +
> +void p2m_ctxt_switch_to(struct vcpu *n)
> +{
> +    if ( is_idle_vcpu(n) )
> +        return;
> +
> +    n->domain->arch.p2m.is_ctxt_switch_finished = false;

How can the context switch of a vCPU affect domain-wide state?

> +    /*
> +     * Nothing to do with HGATP or VSATP, they will be set in
> +     * p2_handle_vmenter()
> +     */

Why can this not be done here?

> +}
> +
> +void p2m_handle_vmenter(void)
> +{
> +    struct p2m_domain *p2m = &current->domain->arch.p2m;

To save yourself (or others) future work, please never open-code 
p2m_get_hostp2m()
(applies further up as well, as I notice only now).

> +    struct vcpu_vmid *p_vmid = &current->arch.vmid;
> +    uint16_t old_vmid, new_vmid;
> +    bool need_flush;
> +    register_t vsatp_old = 0;
> +
> +    BUG_ON(is_idle_vcpu(current));

This is the 3rd use of current - latch into a local variable?

> +    /*
> +     * 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
> +     *
> +     * CSR_VSATP is already set to 0 in p2m_ctxt_switch_from() in the
> +     * case when n->arch.is_p2m_switch_finished = false. Also, there is
> +     * BUG_ON() below to verify that.
> +     */
> +    if ( p2m->is_ctxt_switch_finished )
> +        vsatp_old = csr_swap(CSR_VSATP, 0);

This shouldn't be needed when ...

> +    old_vmid = p_vmid->vmid;
> +    need_flush = vmid_handle_vmenter(p_vmid);
> +    new_vmid = p_vmid->vmid;

... the VMID doesn't change. Imo you want to drop is_ctxt_switch_finished
again, handle things normally in p2m_ctxt_switch_to(), and deal with merely
a changing VMID here.

> +#ifdef P2M_DEBUG
> +    printk("%pv: oldvmid(%d) new_vmid(%d), need_flush(%d)\n",
> +           current, old_vmid, new_vmid, need_flush);
> +#endif
> +
> +    csr_write(CSR_HGATP, construct_hgatp(p2m_get_hostp2m(current->domain),
> +              new_vmid));

Bad indentation - new_vmid isn't an argument to csr_write().

Jan