Re: [PATCH v2 12/39] xen/riscv: implement vCPU context switching

[Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>]

As I understand it, a generation wrap doesn't retire a single VMID, it
resets next_vmid to 1, which makes every VMID in 1..max_vmid reusable
again in the new generation. We do a full (local) flush at that point to
avoid two different vCPUs ending up with the same VMID valid at once,
across generations.

If this is correct, doing a full flush there also throws away entries
for the current vCPU that a local HFENCE.GVMA(vmid) per retired VMID
could have preserved. A local-flush-per-VMID approach could also
reduce how often we need a full flush at all.

Is there a reason we don't do local flushing instead? I see x86 and KVM
use the same flush-all design on wrap, so I assume there's a reason I'm
missing, I'd like to understand it.

Thanks in advance.

> H/VS CSRs, virtual timer and P2M context, and __context_switch() in assembly,
> which switches Xen's own callee-saved state (and thereby the stack) from
> prev to next. Virtual interrupt controller context switch will be
> introduced later.
> 
> Add offsets of struct arch_vcpu's xen_saved_context to asm-offsets.c for
> use by __context_switch().
> 
> henvcfg and htimedelta are 64-bit on both RV32 and RV64, so store them as
> uint64_t and use csr_{read,write}64() instead of open-coding accesses to
> the high halves.
> 
> A hart which drops out of a domain's dirty_cpumask stops being a target
> of p2m_tlb_flush() while its TLB may still hold G-stage translations of
> that domain, and neither the vCPU which just ran nor any other vCPU of
> that domain which ran there earlier has had its VMID invalidated. Move
> the hart to a new VMID generation at that point: a VMID number is never
> re-used until a full local flush has happened, hence none of those
> translations can be reached again.
> 
> Claim the VMID in p2m_ctxt_switch_to() rather than at the next guest
> entry. VMIDs are a per-hart resource, so the (generation, vmid) pair a
> migrating vCPU brings from another hart is meaningless here and may even
> match this hart's current generation, leaving the vCPU under a VMID owned
> by another domain. ctxt_switch_to() invalidates that pair, but claiming a
> replacement only on guest entry is too late: p2m_ctxt_switch_to() has by
> then already made HGATP live, and speculation can populate G-stage entries
> of the incoming domain under the stale VMID. The local flush for a wrapped
> generation moves along with the claim.
> 
> That leaves p2m_handle_vmenter() with nothing to do, so drop it together
> with its call from check_for_pcpu_work(). A VMID can only be invalidated
> while its vCPU isn't running: vmid_flush_vcpu() is called for the vCPU
> being switched in, and vmid_flush_hart() runs either from schedule_tail(),
> ahead of ctxt_switch_to(), or from the wrap path of vmid_handle_vmenter()
> itself. A P2M change on another hart doesn't invalidate it either, as
> p2m_tlb_flush() drops the stale entries directly with
> sbi_remote_hfence_gvma() instead of retiring the VMIDs which tag them. A
> guest therefore always runs under the VMID claimed on its way in, and
> there is nothing left for a guest entry hook to notice.
> 
> p2m_handle_vmenter() also skipped the HGATP write when the VMID it claimed
> was unchanged. That isn't carried over: HGATP holds the G-stage root as
> well, and skipping the write is only correct where that root is already
> the incoming domain's. On the guest entry path it is, on the context
> switch path it is not.
> 
> While at it, fix the inclusion order of headers in asm-offsets.c: Xen's
> headers go first, then arch specific ones.
This could have a dedicated patch no?
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
>
> diff --git a/xen/arch/riscv/domain.c b/xen/arch/riscv/domain.c
> index ec327a5e8a..91a46d630f 100644
> --- a/xen/arch/riscv/domain.c
> +++ b/xen/arch/riscv/domain.c
> @@ -11,9 +11,11 @@
>  #include <asm/bitops.h>
>  #include <asm/cpufeature.h>
>  #include <asm/csr.h>
> +#include <asm/current.h>
>  #include <asm/intc.h>
>  #include <asm/mmio.h>
>  #include <asm/riscv_encoding.h>
> +#include <asm/vmid.h>
>  #include <asm/vtimer.h>
>  
>  struct csr_masks {
> @@ -158,6 +160,8 @@ int arch_vcpu_create(struct vcpu *v)
>      if ( is_idle_vcpu(v) )
>          return 0;
>  
> +    v->arch.last_cpu = NR_CPUS;
> +
>      vcpu_csr_init(v);
>  
>      if ( (rc = vcpu_vtimer_init(v)) )
> @@ -329,6 +333,169 @@ int arch_domain_create(struct domain *d,
>      return rc;
>  }
>  
> +static void save_csr_regs(struct vcpu *vcpu)
> +{
> +    /*
> +     * There is no need to save these CSRs as only hypervisor writes them in
> +     * restore_csr_regs() and guest can't access them so they shouldn't be
> +     * stored here. Keep them commented here just for symmetry with the
> +     * restore CSRs register part.
> +     *
> +     * vcpu->arch.hedeleg = csr_read(CSR_HEDELEG);
> +     * vcpu->arch.hideleg = csr_read(CSR_HIDELEG);
> +     * vcpu->arch.henvcfg = csr_read64(CSR_HENVCFG);
> +     * vcpu->arch.hcounteren = csr_read(CSR_HCOUNTEREN);
> +     * vcpu->arch.htimedelta = csr_read64(CSR_HTIMEDELTA);
> +     *
> +     * if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_smstateen) )
> +     *     vcpu->arch.hstateen0 = csr_read(CSR_HSTATEEN0);
> +     */
> +
> +    vcpu->arch.hvip = csr_read(CSR_HVIP);
> +
> +    vcpu->arch.vsstatus = csr_read(CSR_VSSTATUS);
> +    vcpu->arch.vsie = csr_read(CSR_VSIE);


> +    vcpu->arch.vstvec = csr_read(CSR_VSTVEC);
> +    vcpu->arch.vsscratch = csr_read(CSR_VSSCRATCH);
> +    vcpu->arch.vscause = csr_read(CSR_VSCAUSE);
> +    vcpu->arch.vstval = csr_read(CSR_VSTVAL);
> +    vcpu->arch.vsepc = csr_read(CSR_VSEPC);
> +}
> +
> +static void restore_csr_regs(struct vcpu *vcpu)
> +{
> +    csr_write(CSR_HEDELEG, vcpu->arch.hedeleg);
> +    csr_write(CSR_HIDELEG, vcpu->arch.hideleg);
> +    csr_write(CSR_HVIP, vcpu->arch.hvip);
> +    csr_write64(CSR_HENVCFG, vcpu->arch.henvcfg);
> +    csr_write(CSR_HCOUNTEREN, vcpu->arch.hcounteren);
> +    csr_write64(CSR_HTIMEDELTA, vcpu->arch.htimedelta);
> +
> +    if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_smstateen) )
> +        csr_write(CSR_HSTATEEN0, vcpu->arch.hstateen0);
> +
> +    csr_write(CSR_VSSTATUS, vcpu->arch.vsstatus);
> +    csr_write(CSR_VSIE, vcpu->arch.vsie);


> +    csr_write(CSR_VSTVEC, vcpu->arch.vstvec);
> +    csr_write(CSR_VSSCRATCH, vcpu->arch.vsscratch);
> +    csr_write(CSR_VSCAUSE, vcpu->arch.vscause);
> +    csr_write(CSR_VSTVAL, vcpu->arch.vstval);
> +    csr_write(CSR_VSEPC, vcpu->arch.vsepc);
> +}
> +
> +static void ctxt_switch_from(struct vcpu *p)
Is it expected to have diverse names for the vcpu arg? Above it's vcpu,
here it's p (I assume it's for `previous` but I think the _from alone is
enough to understand) and below it's n. Shouldn't be better to keep the same 
name?

-- 
Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>