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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Jun 21, 2012 at 4:13 AM, Avi Kivity <avi@xxxxxxxxxx> wrote:
> 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.
>

you are absolutely right.

>>
>>  /**
>> + * 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.
>

that's not functionally incorrect though is it? It may simply increase
the latency for the signal delivery as far as I can see, but I
definitely don't mind changing this path in any case.

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

nice ;)
--
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


[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux