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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 2/12/26 11:16 AM, Jan Beulich wrote:
> 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?
It is wrong to have is_ctxt_switch_finished per domain, it should be
vCPU field.

> > +    /*
> > +     * Nothing to do with HGATP or VSATP, they will be set in
> > +     * p2_handle_vmenter()
> > +     */
> Why can this not be done here?
As VMID should be calculated on VM enter.

We can update HGATP and VSATP here with VMID stored before in 
p2m_ctxt_switch_from(),
but then it is possible when vmid_handle_vmenter() will be called before VM 
entry
VMID could be changed and it will be needed again to update HGATP and VSATP what
will lead to flushing of VS TLB twice (one in p2m_ctxt_switch_to() and another 
one
in p2m_handle_vmenter()).

This is also an answer to ...

> > +}
> > +
> > +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.
... (check the answer above)

If it is okay to have potential double VS TLB flush and double update of
HGATP and VSATP when old_vmid != new_vmid then we can do in this way.

> > +#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().
Oh, sure, I will update that.

Thanks.

~ Oleksii