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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
