Re: [PATCH v8 11/15] ARM: KVM: World-switch implementation

[Avi Kivity <avi@redhat.com>]

On 06/20/2012 07:40 AM, Christoffer Dall wrote:
> 
>>>
>>> cpu0                       cpu1
>>> (vmid=0, gen=1)            (gen=0)
>>> -------------------------- ----------------------
>>> gen == global_gen, return
>>>
>>>                           gen != global_gen
>>>                           increment global_gen
>>>                           smp_call_function
>>> reset_vm_context
>>>                           vmid=0
>>>
>>> enter with vmid=0          enter with vmid=0
>>
>> I can't see how the scenario above can happen. First, no-one can run
>> with vmid 0 - it is reserved for the host. If we bump global_gen on
>> cpuN, then since we do smp_call_function(x, x, wait=1) we are now sure
>> that after this call, all cpus(N-1) potentially being inside a VM will
>> have exited, called reset_vm_context, but before they can re-enter
>> into the guest, they will call update_vttbr, and if their generation
>> counter differs from global_gen, they will try to grab that spinlock
>> and everything should happen in order.
>>
> 
> the whole vmid=0 confused me a bit. The point is since we moved the
> generation check outside the spin_lock we have to re-check, I see:

In fact I think the problem occured with the original code as well.  The
problem is that the check is not atomic wrt guest entry, so

  spin_lock
  check/update
  spin_unlock

  enter guest

has a hole between spin_unlock and guest entry.

> 
>  /**
> + * check_new_vmid_gen - check that the VMID is still valid
> + * @kvm: The VM's VMID to checkt
> + *
> + * return true if there is a new generation of VMIDs being used
> + *
> + * The hardware supports only 256 values with the value zero reserved for the
> + * host, so we check if an assigned value belongs to a previous generation,
> + * which which requires us to assign a new value. If we're the first to use a
> + * VMID for the new generation, we must flush necessary caches and TLBs on all
> + * CPUs.
> + */
> +static bool check_new_vmid_gen(struct kvm *kvm)
> +{
> +	if (likely(kvm->arch.vmid_gen == atomic64_read(&kvm_vmid_gen)))
> +		return;
> +}
> +
> +/**
>   * update_vttbr - Update the VTTBR with a valid VMID before the guest runs
>   * @kvm	The guest that we are about to run
>   *
> @@ -324,15 +342,7 @@ static void update_vttbr(struct kvm *kvm)
>  {
>  	phys_addr_t pgd_phys;
> 
> -	/*
> -	 *  Check that the VMID is still valid.
> -	 *  (The hardware supports only 256 values with the value zero
> -	 *   reserved for the host, so we check if an assigned value belongs to
> -	 *   a previous generation, which which requires us to assign a new
> -	 *   value. If we're the first to use a VMID for the new generation,
> -	 *   we must flush necessary caches and TLBs on all CPUs.)
> -	 */
> -	if (likely(kvm->arch.vmid_gen == atomic64_read(&kvm_vmid_gen)))
> +	if (!check_new_vmid_gen(kvm))
>  		return;
> 
>  	spin_lock(&kvm_vmid_lock);
> @@ -504,6 +514,13 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu
> *vcpu, struct kvm_run *run)
>  		 */
>  		preempt_disable();
>  		local_irq_disable();
> +
> +		if (check_new_vmid_gen(kvm)) {
> +			local_irq_enable();
> +			preempt_enable();
> +			continue;
> +		}
> +

I see the same race with signals.  Your signal_pending() check needs to
be after the local_irq_disable(), otherwise we can enter a guest with a
pending signal.

Better place the signal check before the vmid_gen check, to avoid the
possibility of a a signal being held up by other guests.

-- 
error compiling committee.c: too many arguments to function