[PATCH] arm/arm64: KVM: Perform local TLB invalidation when multiplexing vcpus on a single CPU

[Marc Zyngier]

Hi Mark,

On 24/10/16 17:16, Mark Rutland wrote:
> Hi Marc,
> 
> On Mon, Oct 24, 2016 at 04:31:28PM +0100, Marc Zyngier wrote:
>> Architecturally, TLBs are private to the (physical) CPU they're
>> associated with. But when multiple vcpus from the same VM are
>> being multiplexed on the same CPU, the TLBs are not private
>> to the vcpus (and are actually shared across the VMID).
>>
>> Let's consider the following scenario:
>>
>> - vcpu-0 maps PA to VA
>> - vcpu-1 maps PA' to VA
>>
>> If run on the same physical CPU, vcpu-1 can hit TLB entries generated
>> by vcpu-0 accesses, and access the wrong physical page.
> 
> It might be worth noting that this could also lead to TLB conflicts and
> other such fun usually associated with missing TLB maintenance,
> depending on the two mappings (or the intermediate stages thereof).
> 
>> The solution to this is to keep a per-VM map of which vcpu ran last
>> on each given physical CPU, and invalidate local TLBs when switching
>> to a different vcpu from the same VM.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier at arm.com>
> 
> Modulo my comment on preemptiblity of kvm_arch_sched_in, everything
> below is a nit. Assuming that's not preemptible, this looks right to me.
> 
> FWIW, with or without the other comments considered:
> 
> Reviewed-by: Mark Rutland <mark.rutland at arm.com>
> 
>> ---
>>  arch/arm/include/asm/kvm_host.h   |  5 +++++
>>  arch/arm/include/asm/kvm_hyp.h    |  1 +
>>  arch/arm/kvm/arm.c                | 35 ++++++++++++++++++++++++++++++++++-
>>  arch/arm/kvm/hyp/switch.c         |  9 +++++++++
>>  arch/arm64/include/asm/kvm_host.h |  6 +++++-
>>  arch/arm64/kvm/hyp/switch.c       |  8 ++++++++
>>  6 files changed, 62 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
>> index 2d19e02..035e744 100644
>> --- a/arch/arm/include/asm/kvm_host.h
>> +++ b/arch/arm/include/asm/kvm_host.h
>> @@ -57,6 +57,8 @@ struct kvm_arch {
>>  	/* VTTBR value associated with below pgd and vmid */
>>  	u64    vttbr;
>>  
>> +	int __percpu *last_vcpu_ran;
>> +
>>  	/* Timer */
>>  	struct arch_timer_kvm	timer;
>>  
>> @@ -174,6 +176,9 @@ struct kvm_vcpu_arch {
>>  	/* vcpu power-off state */
>>  	bool power_off;
>>  
>> +	/* TLBI required */
>> +	bool requires_tlbi;
> 
> A bit of a nit, but it's not clear which class of TLBI is required, or
> why. It's probably worth a comment (and perhaps a bikeshed), like:
> 
> 	/*
> 	 * Local TLBs potentially contain conflicting entries from
> 	 * another vCPU within this VMID. All entries for this VMID must
> 	 * be invalidated from (local) TLBs before we run this vCPU.
> 	 */
> 	bool tlb_vmid_stale;

Yup, looks good.

> 
> [...]
> 
>> +void kvm_arch_sched_in(struct kvm_vcpu *vcpu, int cpu)
>> +{
>> +	int *last_ran;
>> +
>> +	last_ran = per_cpu_ptr(vcpu->kvm->arch.last_vcpu_ran, cpu);
>> +
>> +	/*
>> +	 * If we're very unlucky and get preempted before having ran
>> +	 * this vcpu for real, we'll end-up in a situation where any
>> +	 * vcpu that gets scheduled will perform an invalidation (this
>> +	 * vcpu explicitely requires it, and all the others will have
>> +	 * a different vcpu_id).
>> +	 */
> 
> Nit: s/explicitely/explicitly/
> 
> To bikeshed a little further, perhaps:
> 
> 	/*
> 	 * We might get preempted before the vCPU actually runs, but
> 	 * this is fine. Our TLBI stays pending until we actually make
> 	 * it to __activate_vm, so we won't miss a TLBI. If another vCPU
> 	 * gets scheduled, it will see our vcpu_id in last_ran, and pend
> 	 * a TLBI for itself.
> 	 */

Looks good too. I'll incorporate this into v2.

> 
>> +	if (*last_ran != vcpu->vcpu_id) {
>> +		if (*last_ran != -1)
>> +			vcpu->arch.requires_tlbi = true;
>> +
>> +		*last_ran = vcpu->vcpu_id;
>> +	}
>> +}
> 
> I take it this function is run in some non-preemptible context; if so,
> this looks correct to me.
> 
> If this is preemptible, then this looks racy.

This function is called from a preempt notifier, which itself isn't
preemptible.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...