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 1:56 PM, Jan Beulich wrote:
> On 12.02.2026 12:57, Oleksii Kurochko wrote:
> > 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.
> And I didn't suggest to calculate a new one here.
>
> > 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()).
> Is this a concern resulting from particular logic you expect to appear
> in the window between context switch and entering the guest, or is this
> merely an abstract concern?
If we will have VS TLB flush unconditionally in VM entry then it is merely an
abstract concern. Otherwise, considering that speculation could happen between
context switch and VM entry what could lead to that some entries were added to
VS TLB flush with old VMID in the case if then in VM entry vCPU might receive 
new
VMID.

> > 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.
> I think the simpler, straightforward approach should be used initially,
> with improvements made once a performance issue was actually determined, or
> once a less ugly (sorry) approach was found. For example, assuming CSR
> reads aren't overly expensive, it looks to me as if during VM entry
> - vsatp only needs writing when vsatp.MODE is zero,
> - hgatp only needs writing when vsatp.MODE is zero or when the VMID needs
>    updating.
Do you mean to do that only in p2m_handle_vmenter() and having
p2m_ctxt_switch_to() does nothing (or just dropped)?
If yes then it could be an option.

I thought about such approach but decided to go with an introduction
of is_ctxt_switch_finished as it looked to me that it is better for
potential TLB flushing optimization.
But I am okay to start with suggested way and re-work it once a
performance issue will be actually determined.

~ Oleksii