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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 2/16/26 12:50 PM, Jan Beulich wrote:
> 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?
According to the RISC-V spec:
  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

Based on that my understand is that the following code could provide an issue:
(1) csr_write(CSR_SEPC, guest_b->sepc);   ...   (2) csr_write(CSR_VSATP, 
0);   csr_write(CSR_HATP, guest_b->hgatp);   csr_write(CSR_VSATP, 
guest_b->vsatp); As IIUC speculation could happen between (1) and (2) 
and we could have some VS-stage translations connected to SEPC'c of 
guest B but with address from guest A page tables. So just to be sure 
that such isuse won't happen I wrote a comment that first VSATP, then 
others CSRs then setting hgatp and vsatp for new guest.

> > --- 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?
Please, look at the reply above.

> > +{
> > +    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?
Sure, I will do that. Also, then I have update in some other patches the name 
of similar
variable.

> > +    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?
It could be just 'unsigned int' or 'unsigned short'.

> > +    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?
I misread a spec for the case when implementation uses two TLBs: one that
maps guest virtual addresses to guest physical addresses, and another that
maps guest physical addresses to supervisor physical addresses, that it should
be HFENCE.VVMA, but it is written that HFENCE.GVMA.

So flush_tlb_guest_local() should be dropped here.

~ Oleksii