Re: [PATCH 0/3] KVM: arm64: Load the stage-2 MMU from vcpu_load() for VHE

[Marc Zyngier <maz@kernel.org>]

On Fri, 06 Oct 2023 13:34:34 +0100,
Marc Zyngier <maz@kernel.org> wrote:
> 
> On Fri, 06 Oct 2023 12:13:00 +0100,
> Marc Zyngier <maz@kernel.org> wrote:
> > 
> > On Fri, 06 Oct 2023 10:35:57 +0100,
> > Oliver Upton <oliver.upton@linux.dev> wrote:
> > > 
> > > Unlike nVHE, there is no need to switch the stage-2 MMU around on guest
> > > entry/exit in VHE mode as the host is running at EL2. Despite this KVM
> > > reloads the stage-2 on every guest entry, which is needless.
> > > 
> > > This series moves the setup of the stage-2 MMU context to vcpu_load()
> > > when running in VHE mode. This is likely to be a win across the board,
> > > but also allows us to remove an ISB on the guest entry path for systems
> > > with one of the speculative AT errata.
> > > 
> > > None of my machines affected by the AT errata are VHE-capable, so it'd
> > > be appreciated if someone could give this series a go and make sure I
> > > haven't wrecked anything.
> > 
> > It totally breaks on my A55 board. Running a single guest seems OK,
> > but running a number of the concurrently makes them explode early on
> > (faults in EFI...)
> > 
> > I guess we end-up running with the wrong VTTBR at times, which would
> > be interesting...
> 
> Fun fact:
> 
> diff --git a/arch/arm64/kvm/hyp/vhe/switch.c b/arch/arm64/kvm/hyp/vhe/switch.c
> index b0cafd7c5f8f..40c84db5884a 100644
> --- a/arch/arm64/kvm/hyp/vhe/switch.c
> +++ b/arch/arm64/kvm/hyp/vhe/switch.c
> @@ -195,6 +195,11 @@ static int __kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
>  	sysreg_restore_guest_state_vhe(guest_ctxt);
>  	__debug_switch_to_guest(vcpu);
>  
> +	WARN_ONCE(kvm_get_vttbr(vcpu->arch.hw_mmu) != read_sysreg(vttbr_el2),
> +		  "Oh crap %llx vs %llx\n",
> +		  kvm_get_vttbr(vcpu->arch.hw_mmu),
> +		  read_sysreg(vttbr_el2));
> +
>  	if (is_hyp_ctxt(vcpu))
>  		vcpu_set_flag(vcpu, VCPU_HYP_CONTEXT);
>  	else
> 
> [   36.190355] Oh crap 10000057a6001 vs 57a6001
> 
> My bet is that the VMID isn't allocated on first load, and everything
> goes downhill from there.

So I was correct that the VMID isn't allocated on the first run, and
the following patch should address that particular problem:

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index e2d38c7d6555..759adee42018 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -1025,7 +1025,7 @@ int kvm_arm_pvtime_has_attr(struct kvm_vcpu *vcpu,
 extern unsigned int __ro_after_init kvm_arm_vmid_bits;
 int __init kvm_arm_vmid_alloc_init(void);
 void __init kvm_arm_vmid_alloc_free(void);
-void kvm_arm_vmid_update(struct kvm_vmid *kvm_vmid);
+bool kvm_arm_vmid_update(struct kvm_vmid *kvm_vmid);
 void kvm_arm_vmid_clear_active(void);
 
 static inline void kvm_arm_pvtime_vcpu_init(struct kvm_vcpu_arch *vcpu_arch)
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 39c969c05990..584be562b1d4 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -950,7 +950,10 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
 		 * making a thread's VMID inactive. So we need to call
 		 * kvm_arm_vmid_update() in non-premptible context.
 		 */
-		kvm_arm_vmid_update(&vcpu->arch.hw_mmu->vmid);
+		if (kvm_arm_vmid_update(&vcpu->arch.hw_mmu->vmid) &&
+		    has_vhe())
+			__load_stage2(vcpu->arch.hw_mmu,
+				      vcpu->arch.hw_mmu->arch);
 
 		kvm_pmu_flush_hwstate(vcpu);
 
diff --git a/arch/arm64/kvm/vmid.c b/arch/arm64/kvm/vmid.c
index 7fe8ba1a2851..281e4f86d9a2 100644
--- a/arch/arm64/kvm/vmid.c
+++ b/arch/arm64/kvm/vmid.c
@@ -135,10 +135,11 @@ void kvm_arm_vmid_clear_active(void)
 	atomic64_set(this_cpu_ptr(&active_vmids), VMID_ACTIVE_INVALID);
 }
 
-void kvm_arm_vmid_update(struct kvm_vmid *kvm_vmid)
+bool kvm_arm_vmid_update(struct kvm_vmid *kvm_vmid)
 {
 	unsigned long flags;
 	u64 vmid, old_active_vmid;
+	bool updated = false;
 
 	vmid = atomic64_read(&kvm_vmid->id);
 
@@ -156,17 +157,21 @@ void kvm_arm_vmid_update(struct kvm_vmid *kvm_vmid)
 	if (old_active_vmid != 0 && vmid_gen_match(vmid) &&
 	    0 != atomic64_cmpxchg_relaxed(this_cpu_ptr(&active_vmids),
 					  old_active_vmid, vmid))
-		return;
+		return false;
 
 	raw_spin_lock_irqsave(&cpu_vmid_lock, flags);
 
 	/* Check that our VMID belongs to the current generation. */
 	vmid = atomic64_read(&kvm_vmid->id);
-	if (!vmid_gen_match(vmid))
+	if (!vmid_gen_match(vmid)) {
 		vmid = new_vmid(kvm_vmid);
+		updated = true;
+	}
 
 	atomic64_set(this_cpu_ptr(&active_vmids), vmid);
 	raw_spin_unlock_irqrestore(&cpu_vmid_lock, flags);
+
+	return updated;
 }
 
 /*

However, this isn't enough.

[   63.450113] Oh crap 400000435c001 vs 3000004430001

So there are situations where we end-up with the wrong VTTBR, rather
than the wrong VMID, which is even worse. Haven't worked out the
scenario yet, but it apparently involves being preempted by a vcpu
from a different VM and not doing the right thing.

	M.

-- 
Without deviation from the norm, progress is not possible.