Re: [PATCH] KVM: arm/arm64: Close VMID generation race

[Marc Zyngier]

On 16/04/18 11:05, Shannon Zhao wrote:
> 
> 
> On 2018/4/11 9:30, Shannon Zhao wrote:
>>
>> On 2018/4/10 23:37, Marc Zyngier wrote:

[...]

 I don't mind either way. If you can be bothered to write a proper commit
 log for this, I'll take it. What I'd really want is Shannon to indicate
 whether or not this solves the issue he was seeing.

>> I'll test Marc's patch. This will take about 3 days since it's not 100%
>> reproducible.
> Hi Marc,
> 
> I've run the test for about 4 days. The issue doesn't appear.
> So Tested-by: Shannon Zhao 

Thanks Shannon, much appreciated. I'll send the fix upstream towards the
end of the week.

Cheers,

M.
-- 
Jazz is not dead. It just smells funny...
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH] KVM: arm/arm64: Close VMID generation race

[Shannon Zhao]

On 2018/4/11 9:30, Shannon Zhao wrote:
> 
> On 2018/4/10 23:37, Marc Zyngier wrote:
>> > On 10/04/18 16:24, Mark Rutland wrote:
>>> >> On Tue, Apr 10, 2018 at 05:05:40PM +0200, Christoffer Dall wrote:
 >>> On Tue, Apr 10, 2018 at 11:51:19AM +0100, Mark Rutland wrote:
>  I think we also need to update kvm->arch.vttbr before updating
>  kvm->arch.vmid_gen, otherwise another CPU can come in, see that the
>  vmid_gen is up-to-date, jump to hyp, and program a stale VTTBR (with 
>  the
>  old VMID).
> 
>  With the smp_wmb() and update of kvm->arch.vmid_gen moved to the end 
>  of
>  the critical section, I think that works, modulo using READ_ONCE() 
>  and
>  WRITE_ONCE() to ensure single-copy-atomicity of the fields we access
>  locklessly.
 >>>
 >>> Indeed, you're right.  I would look something like this, then:
 >>>
 >>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
 >>> index 2e43f9d42bd5..6cb08995e7ff 100644
 >>> --- a/virt/kvm/arm/arm.c
 >>> +++ b/virt/kvm/arm/arm.c
 >>> @@ -450,7 +450,9 @@ void force_vm_exit(const cpumask_t *mask)
 >>>   */
 >>>  static bool need_new_vmid_gen(struct kvm *kvm)
 >>>  {
 >>> -  return unlikely(kvm->arch.vmid_gen != 
 >>> atomic64_read(_vmid_gen));
 >>> +  u64 current_vmid_gen = atomic64_read(_vmid_gen);
 >>> +  smp_rmb(); /* Orders read of kvm_vmid_gen and kvm->arch.vmid */
 >>> +  return unlikely(READ_ONCE(kvm->arch.vmid_gen) != 
 >>> current_vmid_gen);
 >>>  }
 >>>  
 >>>  /**
 >>> @@ -500,7 +502,6 @@ static void update_vttbr(struct kvm *kvm)
 >>>kvm_call_hyp(__kvm_flush_vm_context);
 >>>}
 >>>  
 >>> -  kvm->arch.vmid_gen = atomic64_read(_vmid_gen);
 >>>kvm->arch.vmid = kvm_next_vmid;
 >>>kvm_next_vmid++;
 >>>kvm_next_vmid &= (1 << kvm_vmid_bits) - 1;
 >>> @@ -509,7 +510,10 @@ static void update_vttbr(struct kvm *kvm)
 >>>pgd_phys = virt_to_phys(kvm->arch.pgd);
 >>>BUG_ON(pgd_phys & ~VTTBR_BADDR_MASK);
 >>>vmid = ((u64)(kvm->arch.vmid) << VTTBR_VMID_SHIFT) & 
 >>> VTTBR_VMID_MASK(kvm_vmid_bits);
 >>> -  kvm->arch.vttbr = pgd_phys | vmid;
 >>> +  WRITE_ONCE(kvm->arch.vttbr, pgd_phys | vmid);
 >>> +
 >>> +  smp_wmb(); /* Ensure vttbr update is observed before vmid_gen 
 >>> update */
 >>> +  kvm->arch.vmid_gen = atomic64_read(_vmid_gen);
 >>>  
 >>>spin_unlock(_vmid_lock);
 >>>  }
>>> >>
>>> >> I think that's right, yes.
>>> >>
>>> >> We could replace the smp_{r,w}mb() barriers with an acquire of the
>>> >> kvm_vmid_gen and a release of kvm->arch.vmid_gen, but if we're really
>>> >> trying to optimize things there are larger algorithmic changes necessary
>>> >> anyhow.
>>> >>
 >>> It's probably easier to convince ourselves about the correctness of
 >>> Marc's code using a rwlock instead, though.  Thoughts?
>>> >>
>>> >> I believe that Marc's preference was the rwlock; I have no preference
>>> >> either way.
>> > 
>> > I don't mind either way. If you can be bothered to write a proper commit
>> > log for this, I'll take it. What I'd really want is Shannon to indicate
>> > whether or not this solves the issue he was seeing.
>> > 
> I'll test Marc's patch. This will take about 3 days since it's not 100%
> reproducible.
Hi Marc,

I've run the test for about 4 days. The issue doesn't appear.
So Tested-by: Shannon Zhao 

Thanks,
-- 
Shannon

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH] KVM: arm/arm64: Close VMID generation race

[Shannon Zhao]

On 2018/4/10 23:37, Marc Zyngier wrote:
> On 10/04/18 16:24, Mark Rutland wrote:
>> On Tue, Apr 10, 2018 at 05:05:40PM +0200, Christoffer Dall wrote:
>>> On Tue, Apr 10, 2018 at 11:51:19AM +0100, Mark Rutland wrote:
 I think we also need to update kvm->arch.vttbr before updating
 kvm->arch.vmid_gen, otherwise another CPU can come in, see that the
 vmid_gen is up-to-date, jump to hyp, and program a stale VTTBR (with the
 old VMID).

 With the smp_wmb() and update of kvm->arch.vmid_gen moved to the end of
 the critical section, I think that works, modulo using READ_ONCE() and
 WRITE_ONCE() to ensure single-copy-atomicity of the fields we access
 locklessly.
>>>
>>> Indeed, you're right.  I would look something like this, then:
>>>
>>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
>>> index 2e43f9d42bd5..6cb08995e7ff 100644
>>> --- a/virt/kvm/arm/arm.c
>>> +++ b/virt/kvm/arm/arm.c
>>> @@ -450,7 +450,9 @@ void force_vm_exit(const cpumask_t *mask)
>>>   */
>>>  static bool need_new_vmid_gen(struct kvm *kvm)
>>>  {
>>> -   return unlikely(kvm->arch.vmid_gen != atomic64_read(_vmid_gen));
>>> +   u64 current_vmid_gen = atomic64_read(_vmid_gen);
>>> +   smp_rmb(); /* Orders read of kvm_vmid_gen and kvm->arch.vmid */
>>> +   return unlikely(READ_ONCE(kvm->arch.vmid_gen) != current_vmid_gen);
>>>  }
>>>  
>>>  /**
>>> @@ -500,7 +502,6 @@ static void update_vttbr(struct kvm *kvm)
>>> kvm_call_hyp(__kvm_flush_vm_context);
>>> }
>>>  
>>> -   kvm->arch.vmid_gen = atomic64_read(_vmid_gen);
>>> kvm->arch.vmid = kvm_next_vmid;
>>> kvm_next_vmid++;
>>> kvm_next_vmid &= (1 << kvm_vmid_bits) - 1;
>>> @@ -509,7 +510,10 @@ static void update_vttbr(struct kvm *kvm)
>>> pgd_phys = virt_to_phys(kvm->arch.pgd);
>>> BUG_ON(pgd_phys & ~VTTBR_BADDR_MASK);
>>> vmid = ((u64)(kvm->arch.vmid) << VTTBR_VMID_SHIFT) & 
>>> VTTBR_VMID_MASK(kvm_vmid_bits);
>>> -   kvm->arch.vttbr = pgd_phys | vmid;
>>> +   WRITE_ONCE(kvm->arch.vttbr, pgd_phys | vmid);
>>> +
>>> +   smp_wmb(); /* Ensure vttbr update is observed before vmid_gen update */
>>> +   kvm->arch.vmid_gen = atomic64_read(_vmid_gen);
>>>  
>>> spin_unlock(_vmid_lock);
>>>  }
>>
>> I think that's right, yes.
>>
>> We could replace the smp_{r,w}mb() barriers with an acquire of the
>> kvm_vmid_gen and a release of kvm->arch.vmid_gen, but if we're really
>> trying to optimize things there are larger algorithmic changes necessary
>> anyhow.
>>
>>> It's probably easier to convince ourselves about the correctness of
>>> Marc's code using a rwlock instead, though.  Thoughts?
>>
>> I believe that Marc's preference was the rwlock; I have no preference
>> either way.
> 
> I don't mind either way. If you can be bothered to write a proper commit
> log for this, I'll take it. What I'd really want is Shannon to indicate
> whether or not this solves the issue he was seeing.
> 
I'll test Marc's patch. This will take about 3 days since it's not 100%
reproducible.

Thanks,
-- 
Shannon

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH] KVM: arm/arm64: Close VMID generation race

[Christoffer Dall]

On Tue, Apr 10, 2018 at 04:37:12PM +0100, Marc Zyngier wrote:
> On 10/04/18 16:24, Mark Rutland wrote:
> > On Tue, Apr 10, 2018 at 05:05:40PM +0200, Christoffer Dall wrote:
> >> On Tue, Apr 10, 2018 at 11:51:19AM +0100, Mark Rutland wrote:
> >>> I think we also need to update kvm->arch.vttbr before updating
> >>> kvm->arch.vmid_gen, otherwise another CPU can come in, see that the
> >>> vmid_gen is up-to-date, jump to hyp, and program a stale VTTBR (with the
> >>> old VMID).
> >>>
> >>> With the smp_wmb() and update of kvm->arch.vmid_gen moved to the end of
> >>> the critical section, I think that works, modulo using READ_ONCE() and
> >>> WRITE_ONCE() to ensure single-copy-atomicity of the fields we access
> >>> locklessly.
> >>
> >> Indeed, you're right.  I would look something like this, then:
> >>
> >> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> >> index 2e43f9d42bd5..6cb08995e7ff 100644
> >> --- a/virt/kvm/arm/arm.c
> >> +++ b/virt/kvm/arm/arm.c
> >> @@ -450,7 +450,9 @@ void force_vm_exit(const cpumask_t *mask)
> >>   */
> >>  static bool need_new_vmid_gen(struct kvm *kvm)
> >>  {
> >> -  return unlikely(kvm->arch.vmid_gen != atomic64_read(_vmid_gen));
> >> +  u64 current_vmid_gen = atomic64_read(_vmid_gen);
> >> +  smp_rmb(); /* Orders read of kvm_vmid_gen and kvm->arch.vmid */
> >> +  return unlikely(READ_ONCE(kvm->arch.vmid_gen) != current_vmid_gen);
> >>  }
> >>  
> >>  /**
> >> @@ -500,7 +502,6 @@ static void update_vttbr(struct kvm *kvm)
> >>kvm_call_hyp(__kvm_flush_vm_context);
> >>}
> >>  
> >> -  kvm->arch.vmid_gen = atomic64_read(_vmid_gen);
> >>kvm->arch.vmid = kvm_next_vmid;
> >>kvm_next_vmid++;
> >>kvm_next_vmid &= (1 << kvm_vmid_bits) - 1;
> >> @@ -509,7 +510,10 @@ static void update_vttbr(struct kvm *kvm)
> >>pgd_phys = virt_to_phys(kvm->arch.pgd);
> >>BUG_ON(pgd_phys & ~VTTBR_BADDR_MASK);
> >>vmid = ((u64)(kvm->arch.vmid) << VTTBR_VMID_SHIFT) & 
> >> VTTBR_VMID_MASK(kvm_vmid_bits);
> >> -  kvm->arch.vttbr = pgd_phys | vmid;
> >> +  WRITE_ONCE(kvm->arch.vttbr, pgd_phys | vmid);
> >> +
> >> +  smp_wmb(); /* Ensure vttbr update is observed before vmid_gen update */
> >> +  kvm->arch.vmid_gen = atomic64_read(_vmid_gen);
> >>  
> >>spin_unlock(_vmid_lock);
> >>  }
> > 
> > I think that's right, yes.
> > 
> > We could replace the smp_{r,w}mb() barriers with an acquire of the
> > kvm_vmid_gen and a release of kvm->arch.vmid_gen, but if we're really
> > trying to optimize things there are larger algorithmic changes necessary
> > anyhow.
> > 
> >> It's probably easier to convince ourselves about the correctness of
> >> Marc's code using a rwlock instead, though.  Thoughts?
> > 
> > I believe that Marc's preference was the rwlock; I have no preference
> > either way.
> 
> I don't mind either way. If you can be bothered to write a proper commit
> log for this, I'll take it. 

You've already done the work, and your patch is easier to read, so let's
just go ahead with that.

I was just curious to which degree my original implementation was
broken; was I trying to achieve something impossible or was I just
writing buggy code.  Seems the latter.  Oh well.

> What I'd really want is Shannon to indicate
> whether or not this solves the issue he was seeing.
> 

Agreed, would like to see that too.

Thanks (and sorry for being noisy),
-Christoffer
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH] KVM: arm/arm64: Close VMID generation race

[Marc Zyngier]

On 10/04/18 16:24, Mark Rutland wrote:
> On Tue, Apr 10, 2018 at 05:05:40PM +0200, Christoffer Dall wrote:
>> On Tue, Apr 10, 2018 at 11:51:19AM +0100, Mark Rutland wrote:
>>> I think we also need to update kvm->arch.vttbr before updating
>>> kvm->arch.vmid_gen, otherwise another CPU can come in, see that the
>>> vmid_gen is up-to-date, jump to hyp, and program a stale VTTBR (with the
>>> old VMID).
>>>
>>> With the smp_wmb() and update of kvm->arch.vmid_gen moved to the end of
>>> the critical section, I think that works, modulo using READ_ONCE() and
>>> WRITE_ONCE() to ensure single-copy-atomicity of the fields we access
>>> locklessly.
>>
>> Indeed, you're right.  I would look something like this, then:
>>
>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
>> index 2e43f9d42bd5..6cb08995e7ff 100644
>> --- a/virt/kvm/arm/arm.c
>> +++ b/virt/kvm/arm/arm.c
>> @@ -450,7 +450,9 @@ void force_vm_exit(const cpumask_t *mask)
>>   */
>>  static bool need_new_vmid_gen(struct kvm *kvm)
>>  {
>> -return unlikely(kvm->arch.vmid_gen != atomic64_read(_vmid_gen));
>> +u64 current_vmid_gen = atomic64_read(_vmid_gen);
>> +smp_rmb(); /* Orders read of kvm_vmid_gen and kvm->arch.vmid */
>> +return unlikely(READ_ONCE(kvm->arch.vmid_gen) != current_vmid_gen);
>>  }
>>  
>>  /**
>> @@ -500,7 +502,6 @@ static void update_vttbr(struct kvm *kvm)
>>  kvm_call_hyp(__kvm_flush_vm_context);
>>  }
>>  
>> -kvm->arch.vmid_gen = atomic64_read(_vmid_gen);
>>  kvm->arch.vmid = kvm_next_vmid;
>>  kvm_next_vmid++;
>>  kvm_next_vmid &= (1 << kvm_vmid_bits) - 1;
>> @@ -509,7 +510,10 @@ static void update_vttbr(struct kvm *kvm)
>>  pgd_phys = virt_to_phys(kvm->arch.pgd);
>>  BUG_ON(pgd_phys & ~VTTBR_BADDR_MASK);
>>  vmid = ((u64)(kvm->arch.vmid) << VTTBR_VMID_SHIFT) & 
>> VTTBR_VMID_MASK(kvm_vmid_bits);
>> -kvm->arch.vttbr = pgd_phys | vmid;
>> +WRITE_ONCE(kvm->arch.vttbr, pgd_phys | vmid);
>> +
>> +smp_wmb(); /* Ensure vttbr update is observed before vmid_gen update */
>> +kvm->arch.vmid_gen = atomic64_read(_vmid_gen);
>>  
>>  spin_unlock(_vmid_lock);
>>  }
> 
> I think that's right, yes.
> 
> We could replace the smp_{r,w}mb() barriers with an acquire of the
> kvm_vmid_gen and a release of kvm->arch.vmid_gen, but if we're really
> trying to optimize things there are larger algorithmic changes necessary
> anyhow.
> 
>> It's probably easier to convince ourselves about the correctness of
>> Marc's code using a rwlock instead, though.  Thoughts?
> 
> I believe that Marc's preference was the rwlock; I have no preference
> either way.

I don't mind either way. If you can be bothered to write a proper commit
log for this, I'll take it. What I'd really want is Shannon to indicate
whether or not this solves the issue he was seeing.

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH] KVM: arm/arm64: Close VMID generation race

[Christoffer Dall]

On Tue, Apr 10, 2018 at 04:24:20PM +0100, Mark Rutland wrote:
> On Tue, Apr 10, 2018 at 05:05:40PM +0200, Christoffer Dall wrote:
> > On Tue, Apr 10, 2018 at 11:51:19AM +0100, Mark Rutland wrote:
> > > I think we also need to update kvm->arch.vttbr before updating
> > > kvm->arch.vmid_gen, otherwise another CPU can come in, see that the
> > > vmid_gen is up-to-date, jump to hyp, and program a stale VTTBR (with the
> > > old VMID).
> > > 
> > > With the smp_wmb() and update of kvm->arch.vmid_gen moved to the end of
> > > the critical section, I think that works, modulo using READ_ONCE() and
> > > WRITE_ONCE() to ensure single-copy-atomicity of the fields we access
> > > locklessly.
> > 
> > Indeed, you're right.  I would look something like this, then:
> > 
> > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> > index 2e43f9d42bd5..6cb08995e7ff 100644
> > --- a/virt/kvm/arm/arm.c
> > +++ b/virt/kvm/arm/arm.c
> > @@ -450,7 +450,9 @@ void force_vm_exit(const cpumask_t *mask)
> >   */
> >  static bool need_new_vmid_gen(struct kvm *kvm)
> >  {
> > -   return unlikely(kvm->arch.vmid_gen != atomic64_read(_vmid_gen));
> > +   u64 current_vmid_gen = atomic64_read(_vmid_gen);
> > +   smp_rmb(); /* Orders read of kvm_vmid_gen and kvm->arch.vmid */
> > +   return unlikely(READ_ONCE(kvm->arch.vmid_gen) != current_vmid_gen);
> >  }
> >  
> >  /**
> > @@ -500,7 +502,6 @@ static void update_vttbr(struct kvm *kvm)
> > kvm_call_hyp(__kvm_flush_vm_context);
> > }
> >  
> > -   kvm->arch.vmid_gen = atomic64_read(_vmid_gen);
> > kvm->arch.vmid = kvm_next_vmid;
> > kvm_next_vmid++;
> > kvm_next_vmid &= (1 << kvm_vmid_bits) - 1;
> > @@ -509,7 +510,10 @@ static void update_vttbr(struct kvm *kvm)
> > pgd_phys = virt_to_phys(kvm->arch.pgd);
> > BUG_ON(pgd_phys & ~VTTBR_BADDR_MASK);
> > vmid = ((u64)(kvm->arch.vmid) << VTTBR_VMID_SHIFT) & 
> > VTTBR_VMID_MASK(kvm_vmid_bits);
> > -   kvm->arch.vttbr = pgd_phys | vmid;
> > +   WRITE_ONCE(kvm->arch.vttbr, pgd_phys | vmid);
> > +
> > +   smp_wmb(); /* Ensure vttbr update is observed before vmid_gen update */
> > +   kvm->arch.vmid_gen = atomic64_read(_vmid_gen);
> >  
> > spin_unlock(_vmid_lock);
> >  }
> 
> I think that's right, yes.
> 
> We could replace the smp_{r,w}mb() barriers with an acquire of the
> kvm_vmid_gen and a release of kvm->arch.vmid_gen, but if we're really
> trying to optimize things there are larger algorithmic changes necessary
> anyhow.
> 
> > It's probably easier to convince ourselves about the correctness of
> > Marc's code using a rwlock instead, though.  Thoughts?
> 
> I believe that Marc's preference was the rwlock; I have no preference
> either way.
> 

I'm fine with both approaches as well, but it was educational for me to
see if this could be done in the lockless way as well.  Thanks for
having a look at that!

-Christoffer
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH] KVM: arm/arm64: Close VMID generation race

[Mark Rutland]

On Tue, Apr 10, 2018 at 05:05:40PM +0200, Christoffer Dall wrote:
> On Tue, Apr 10, 2018 at 11:51:19AM +0100, Mark Rutland wrote:
> > I think we also need to update kvm->arch.vttbr before updating
> > kvm->arch.vmid_gen, otherwise another CPU can come in, see that the
> > vmid_gen is up-to-date, jump to hyp, and program a stale VTTBR (with the
> > old VMID).
> > 
> > With the smp_wmb() and update of kvm->arch.vmid_gen moved to the end of
> > the critical section, I think that works, modulo using READ_ONCE() and
> > WRITE_ONCE() to ensure single-copy-atomicity of the fields we access
> > locklessly.
> 
> Indeed, you're right.  I would look something like this, then:
> 
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index 2e43f9d42bd5..6cb08995e7ff 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -450,7 +450,9 @@ void force_vm_exit(const cpumask_t *mask)
>   */
>  static bool need_new_vmid_gen(struct kvm *kvm)
>  {
> - return unlikely(kvm->arch.vmid_gen != atomic64_read(_vmid_gen));
> + u64 current_vmid_gen = atomic64_read(_vmid_gen);
> + smp_rmb(); /* Orders read of kvm_vmid_gen and kvm->arch.vmid */
> + return unlikely(READ_ONCE(kvm->arch.vmid_gen) != current_vmid_gen);
>  }
>  
>  /**
> @@ -500,7 +502,6 @@ static void update_vttbr(struct kvm *kvm)
>   kvm_call_hyp(__kvm_flush_vm_context);
>   }
>  
> - kvm->arch.vmid_gen = atomic64_read(_vmid_gen);
>   kvm->arch.vmid = kvm_next_vmid;
>   kvm_next_vmid++;
>   kvm_next_vmid &= (1 << kvm_vmid_bits) - 1;
> @@ -509,7 +510,10 @@ static void update_vttbr(struct kvm *kvm)
>   pgd_phys = virt_to_phys(kvm->arch.pgd);
>   BUG_ON(pgd_phys & ~VTTBR_BADDR_MASK);
>   vmid = ((u64)(kvm->arch.vmid) << VTTBR_VMID_SHIFT) & 
> VTTBR_VMID_MASK(kvm_vmid_bits);
> - kvm->arch.vttbr = pgd_phys | vmid;
> + WRITE_ONCE(kvm->arch.vttbr, pgd_phys | vmid);
> +
> + smp_wmb(); /* Ensure vttbr update is observed before vmid_gen update */
> + kvm->arch.vmid_gen = atomic64_read(_vmid_gen);
>  
>   spin_unlock(_vmid_lock);
>  }

I think that's right, yes.

We could replace the smp_{r,w}mb() barriers with an acquire of the
kvm_vmid_gen and a release of kvm->arch.vmid_gen, but if we're really
trying to optimize things there are larger algorithmic changes necessary
anyhow.

> It's probably easier to convince ourselves about the correctness of
> Marc's code using a rwlock instead, though.  Thoughts?

I believe that Marc's preference was the rwlock; I have no preference
either way.

Thanks,
Mark.
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH] KVM: arm/arm64: Close VMID generation race

[Christoffer Dall]

On Tue, Apr 10, 2018 at 11:51:19AM +0100, Mark Rutland wrote:
> On Mon, Apr 09, 2018 at 10:51:39PM +0200, Christoffer Dall wrote:
> > On Mon, Apr 09, 2018 at 06:07:06PM +0100, Marc Zyngier wrote:
> > > Before entering the guest, we check whether our VMID is still
> > > part of the current generation. In order to avoid taking a lock,
> > > we start with checking that the generation is still current, and
> > > only if not current do we take the lock, recheck, and update the
> > > generation and VMID.
> > > 
> > > This leaves open a small race: A vcpu can bump up the global
> > > generation number as well as the VM's, but has not updated
> > > the VMID itself yet.
> > > 
> > > At that point another vcpu from the same VM comes in, checks
> > > the generation (and finds it not needing anything), and jumps
> > > into the guest. At this point, we end-up with two vcpus belonging
> > > to the same VM running with two different VMIDs. Eventually, the
> > > VMID used by the second vcpu will get reassigned, and things will
> > > really go wrong...
> > > 
> > > A simple solution would be to drop this initial check, and always take
> > > the lock. This is likely to cause performance issues. A middle ground
> > > is to convert the spinlock to a rwlock, and only take the read lock
> > > on the fast path. If the check fails at that point, drop it and
> > > acquire the write lock, rechecking the condition.
> > > 
> > > This ensures that the above scenario doesn't occur.
> > > 
> > > Reported-by: Mark Rutland 
> > > Signed-off-by: Marc Zyngier 
> > > ---
> > > I haven't seen any reply from Shannon, so reposting this to
> > > a slightly wider audience for feedback.
> > > 
> > >  virt/kvm/arm/arm.c | 15 ++-
> > >  1 file changed, 10 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> > > index dba629c5f8ac..a4c1b76240df 100644
> > > --- a/virt/kvm/arm/arm.c
> > > +++ b/virt/kvm/arm/arm.c
> > > @@ -63,7 +63,7 @@ static DEFINE_PER_CPU(struct kvm_vcpu *, 
> > > kvm_arm_running_vcpu);
> > >  static atomic64_t kvm_vmid_gen = ATOMIC64_INIT(1);
> > >  static u32 kvm_next_vmid;
> > >  static unsigned int kvm_vmid_bits __read_mostly;
> > > -static DEFINE_SPINLOCK(kvm_vmid_lock);
> > > +static DEFINE_RWLOCK(kvm_vmid_lock);
> > >  
> > >  static bool vgic_present;
> > >  
> > > @@ -473,11 +473,16 @@ static void update_vttbr(struct kvm *kvm)
> > >  {
> > >   phys_addr_t pgd_phys;
> > >   u64 vmid;
> > > + bool new_gen;
> > >  
> > > - if (!need_new_vmid_gen(kvm))
> > > + read_lock(_vmid_lock);
> > > + new_gen = need_new_vmid_gen(kvm);
> > > + read_unlock(_vmid_lock);
> > > +
> > > + if (!new_gen)
> > >   return;
> > >  
> > > - spin_lock(_vmid_lock);
> > > + write_lock(_vmid_lock);
> > >  
> > >   /*
> > >* We need to re-check the vmid_gen here to ensure that if another vcpu
> > > @@ -485,7 +490,7 @@ static void update_vttbr(struct kvm *kvm)
> > >* use the same vmid.
> > >*/
> > >   if (!need_new_vmid_gen(kvm)) {
> > > - spin_unlock(_vmid_lock);
> > > + write_unlock(_vmid_lock);
> > >   return;
> > >   }
> > >  
> > > @@ -519,7 +524,7 @@ static void update_vttbr(struct kvm *kvm)
> > >   vmid = ((u64)(kvm->arch.vmid) << VTTBR_VMID_SHIFT) & 
> > > VTTBR_VMID_MASK(kvm_vmid_bits);
> > >   kvm->arch.vttbr = kvm_phys_to_vttbr(pgd_phys) | vmid;
> > >  
> > > - spin_unlock(_vmid_lock);
> > > + write_unlock(_vmid_lock);
> > >  }
> > >  
> > >  static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
> > > -- 
> > > 2.14.2
> > > 
> > 
> > The above looks correct to me.  I am wondering if something like the
> > following would also work, which may be slightly more efficient,
> > although I doubt the difference can be measured:
> > 

[...]

> 
> I think we also need to update kvm->arch.vttbr before updating
> kvm->arch.vmid_gen, otherwise another CPU can come in, see that the
> vmid_gen is up-to-date, jump to hyp, and program a stale VTTBR (with the
> old VMID).
> 
> With the smp_wmb() and update of kvm->arch.vmid_gen moved to the end of
> the critical section, I think that works, modulo using READ_ONCE() and
> WRITE_ONCE() to ensure single-copy-atomicity of the fields we access
> locklessly.

Indeed, you're right.  I would look something like this, then:

diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index 2e43f9d42bd5..6cb08995e7ff 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -450,7 +450,9 @@ void force_vm_exit(const cpumask_t *mask)
  */
 static bool need_new_vmid_gen(struct kvm *kvm)
 {
-   return unlikely(kvm->arch.vmid_gen != atomic64_read(_vmid_gen));
+   u64 current_vmid_gen = atomic64_read(_vmid_gen);
+   smp_rmb(); /* Orders read of kvm_vmid_gen and kvm->arch.vmid */
+   return unlikely(READ_ONCE(kvm->arch.vmid_gen) != current_vmid_gen);
 }
 
 /**
@@ -500,7 +502,6 @@ static void update_vttbr(struct kvm *kvm)

Re: [PATCH] KVM: arm/arm64: Close VMID generation race

[Mark Rutland]

On Mon, Apr 09, 2018 at 10:51:39PM +0200, Christoffer Dall wrote:
> On Mon, Apr 09, 2018 at 06:07:06PM +0100, Marc Zyngier wrote:
> > Before entering the guest, we check whether our VMID is still
> > part of the current generation. In order to avoid taking a lock,
> > we start with checking that the generation is still current, and
> > only if not current do we take the lock, recheck, and update the
> > generation and VMID.
> > 
> > This leaves open a small race: A vcpu can bump up the global
> > generation number as well as the VM's, but has not updated
> > the VMID itself yet.
> > 
> > At that point another vcpu from the same VM comes in, checks
> > the generation (and finds it not needing anything), and jumps
> > into the guest. At this point, we end-up with two vcpus belonging
> > to the same VM running with two different VMIDs. Eventually, the
> > VMID used by the second vcpu will get reassigned, and things will
> > really go wrong...
> > 
> > A simple solution would be to drop this initial check, and always take
> > the lock. This is likely to cause performance issues. A middle ground
> > is to convert the spinlock to a rwlock, and only take the read lock
> > on the fast path. If the check fails at that point, drop it and
> > acquire the write lock, rechecking the condition.
> > 
> > This ensures that the above scenario doesn't occur.
> > 
> > Reported-by: Mark Rutland 
> > Signed-off-by: Marc Zyngier 
> > ---
> > I haven't seen any reply from Shannon, so reposting this to
> > a slightly wider audience for feedback.
> > 
> >  virt/kvm/arm/arm.c | 15 ++-
> >  1 file changed, 10 insertions(+), 5 deletions(-)
> > 
> > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> > index dba629c5f8ac..a4c1b76240df 100644
> > --- a/virt/kvm/arm/arm.c
> > +++ b/virt/kvm/arm/arm.c
> > @@ -63,7 +63,7 @@ static DEFINE_PER_CPU(struct kvm_vcpu *, 
> > kvm_arm_running_vcpu);
> >  static atomic64_t kvm_vmid_gen = ATOMIC64_INIT(1);
> >  static u32 kvm_next_vmid;
> >  static unsigned int kvm_vmid_bits __read_mostly;
> > -static DEFINE_SPINLOCK(kvm_vmid_lock);
> > +static DEFINE_RWLOCK(kvm_vmid_lock);
> >  
> >  static bool vgic_present;
> >  
> > @@ -473,11 +473,16 @@ static void update_vttbr(struct kvm *kvm)
> >  {
> > phys_addr_t pgd_phys;
> > u64 vmid;
> > +   bool new_gen;
> >  
> > -   if (!need_new_vmid_gen(kvm))
> > +   read_lock(_vmid_lock);
> > +   new_gen = need_new_vmid_gen(kvm);
> > +   read_unlock(_vmid_lock);
> > +
> > +   if (!new_gen)
> > return;
> >  
> > -   spin_lock(_vmid_lock);
> > +   write_lock(_vmid_lock);
> >  
> > /*
> >  * We need to re-check the vmid_gen here to ensure that if another vcpu
> > @@ -485,7 +490,7 @@ static void update_vttbr(struct kvm *kvm)
> >  * use the same vmid.
> >  */
> > if (!need_new_vmid_gen(kvm)) {
> > -   spin_unlock(_vmid_lock);
> > +   write_unlock(_vmid_lock);
> > return;
> > }
> >  
> > @@ -519,7 +524,7 @@ static void update_vttbr(struct kvm *kvm)
> > vmid = ((u64)(kvm->arch.vmid) << VTTBR_VMID_SHIFT) & 
> > VTTBR_VMID_MASK(kvm_vmid_bits);
> > kvm->arch.vttbr = kvm_phys_to_vttbr(pgd_phys) | vmid;
> >  
> > -   spin_unlock(_vmid_lock);
> > +   write_unlock(_vmid_lock);
> >  }
> >  
> >  static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
> > -- 
> > 2.14.2
> > 
> 
> The above looks correct to me.  I am wondering if something like the
> following would also work, which may be slightly more efficient,
> although I doubt the difference can be measured:
> 
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index dba629c5f8ac..7ac869bcad21 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -458,7 +458,9 @@ void force_vm_exit(const cpumask_t *mask)
>   */
>  static bool need_new_vmid_gen(struct kvm *kvm)
>  {
> - return unlikely(kvm->arch.vmid_gen != atomic64_read(_vmid_gen));
> + u64 current_vmid_gen = atomic64_read(_vmid_gen);
> + smp_rmb(); /* Orders read of kvm_vmid_gen and kvm->arch.vmid */
> + return unlikely(kvm->arch.vmid_gen != current_vmid_gen);
>  }
>  
>  /**
> @@ -508,10 +510,11 @@ static void update_vttbr(struct kvm *kvm)
>   kvm_call_hyp(__kvm_flush_vm_context);
>   }
>  
> - kvm->arch.vmid_gen = atomic64_read(_vmid_gen);
>   kvm->arch.vmid = kvm_next_vmid;
>   kvm_next_vmid++;
>   kvm_next_vmid &= (1 << kvm_vmid_bits) - 1;
> + smp_wmb();
> + kvm->arch.vmid_gen = atomic64_read(_vmid_gen);
>  
>   /* update vttbr to be used with the new vmid */
>   pgd_phys = virt_to_phys(kvm->arch.pgd);
> 

I think we also need to update kvm->arch.vttbr before updating
kvm->arch.vmid_gen, otherwise another CPU can come in, see that the
vmid_gen is up-to-date, jump to hyp, and program a stale VTTBR (with the
old VMID).

With the smp_wmb() and update of kvm->arch.vmid_gen moved to the end of
the critical section, I think that works, modulo using 

Re: [PATCH] KVM: arm/arm64: Close VMID generation race

[Christoffer Dall]

On Mon, Apr 09, 2018 at 06:07:06PM +0100, Marc Zyngier wrote:
> Before entering the guest, we check whether our VMID is still
> part of the current generation. In order to avoid taking a lock,
> we start with checking that the generation is still current, and
> only if not current do we take the lock, recheck, and update the
> generation and VMID.
> 
> This leaves open a small race: A vcpu can bump up the global
> generation number as well as the VM's, but has not updated
> the VMID itself yet.
> 
> At that point another vcpu from the same VM comes in, checks
> the generation (and finds it not needing anything), and jumps
> into the guest. At this point, we end-up with two vcpus belonging
> to the same VM running with two different VMIDs. Eventually, the
> VMID used by the second vcpu will get reassigned, and things will
> really go wrong...
> 
> A simple solution would be to drop this initial check, and always take
> the lock. This is likely to cause performance issues. A middle ground
> is to convert the spinlock to a rwlock, and only take the read lock
> on the fast path. If the check fails at that point, drop it and
> acquire the write lock, rechecking the condition.
> 
> This ensures that the above scenario doesn't occur.
> 
> Reported-by: Mark Rutland 
> Signed-off-by: Marc Zyngier 
> ---
> I haven't seen any reply from Shannon, so reposting this to
> a slightly wider audience for feedback.
> 
>  virt/kvm/arm/arm.c | 15 ++-
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index dba629c5f8ac..a4c1b76240df 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -63,7 +63,7 @@ static DEFINE_PER_CPU(struct kvm_vcpu *, 
> kvm_arm_running_vcpu);
>  static atomic64_t kvm_vmid_gen = ATOMIC64_INIT(1);
>  static u32 kvm_next_vmid;
>  static unsigned int kvm_vmid_bits __read_mostly;
> -static DEFINE_SPINLOCK(kvm_vmid_lock);
> +static DEFINE_RWLOCK(kvm_vmid_lock);
>  
>  static bool vgic_present;
>  
> @@ -473,11 +473,16 @@ static void update_vttbr(struct kvm *kvm)
>  {
>   phys_addr_t pgd_phys;
>   u64 vmid;
> + bool new_gen;
>  
> - if (!need_new_vmid_gen(kvm))
> + read_lock(_vmid_lock);
> + new_gen = need_new_vmid_gen(kvm);
> + read_unlock(_vmid_lock);
> +
> + if (!new_gen)
>   return;
>  
> - spin_lock(_vmid_lock);
> + write_lock(_vmid_lock);
>  
>   /*
>* We need to re-check the vmid_gen here to ensure that if another vcpu
> @@ -485,7 +490,7 @@ static void update_vttbr(struct kvm *kvm)
>* use the same vmid.
>*/
>   if (!need_new_vmid_gen(kvm)) {
> - spin_unlock(_vmid_lock);
> + write_unlock(_vmid_lock);
>   return;
>   }
>  
> @@ -519,7 +524,7 @@ static void update_vttbr(struct kvm *kvm)
>   vmid = ((u64)(kvm->arch.vmid) << VTTBR_VMID_SHIFT) & 
> VTTBR_VMID_MASK(kvm_vmid_bits);
>   kvm->arch.vttbr = kvm_phys_to_vttbr(pgd_phys) | vmid;
>  
> - spin_unlock(_vmid_lock);
> + write_unlock(_vmid_lock);
>  }
>  
>  static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
> -- 
> 2.14.2
> 

The above looks correct to me.  I am wondering if something like the
following would also work, which may be slightly more efficient,
although I doubt the difference can be measured:

diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index dba629c5f8ac..7ac869bcad21 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -458,7 +458,9 @@ void force_vm_exit(const cpumask_t *mask)
  */
 static bool need_new_vmid_gen(struct kvm *kvm)
 {
-   return unlikely(kvm->arch.vmid_gen != atomic64_read(_vmid_gen));
+   u64 current_vmid_gen = atomic64_read(_vmid_gen);
+   smp_rmb(); /* Orders read of kvm_vmid_gen and kvm->arch.vmid */
+   return unlikely(kvm->arch.vmid_gen != current_vmid_gen);
 }
 
 /**
@@ -508,10 +510,11 @@ static void update_vttbr(struct kvm *kvm)
kvm_call_hyp(__kvm_flush_vm_context);
}
 
-   kvm->arch.vmid_gen = atomic64_read(_vmid_gen);
kvm->arch.vmid = kvm_next_vmid;
kvm_next_vmid++;
kvm_next_vmid &= (1 << kvm_vmid_bits) - 1;
+   smp_wmb();
+   kvm->arch.vmid_gen = atomic64_read(_vmid_gen);
 
/* update vttbr to be used with the new vmid */
pgd_phys = virt_to_phys(kvm->arch.pgd);


Thanks,
-Christoffer
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH] KVM: arm/arm64: Close VMID generation race

[Marc Zyngier]

Before entering the guest, we check whether our VMID is still
part of the current generation. In order to avoid taking a lock,
we start with checking that the generation is still current, and
only if not current do we take the lock, recheck, and update the
generation and VMID.

This leaves open a small race: A vcpu can bump up the global
generation number as well as the VM's, but has not updated
the VMID itself yet.

At that point another vcpu from the same VM comes in, checks
the generation (and finds it not needing anything), and jumps
into the guest. At this point, we end-up with two vcpus belonging
to the same VM running with two different VMIDs. Eventually, the
VMID used by the second vcpu will get reassigned, and things will
really go wrong...

A simple solution would be to drop this initial check, and always take
the lock. This is likely to cause performance issues. A middle ground
is to convert the spinlock to a rwlock, and only take the read lock
on the fast path. If the check fails at that point, drop it and
acquire the write lock, rechecking the condition.

This ensures that the above scenario doesn't occur.

Reported-by: Mark Rutland 
Signed-off-by: Marc Zyngier 
---
I haven't seen any reply from Shannon, so reposting this to
a slightly wider audience for feedback.

 virt/kvm/arm/arm.c | 15 ++-
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index dba629c5f8ac..a4c1b76240df 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -63,7 +63,7 @@ static DEFINE_PER_CPU(struct kvm_vcpu *, 
kvm_arm_running_vcpu);
 static atomic64_t kvm_vmid_gen = ATOMIC64_INIT(1);
 static u32 kvm_next_vmid;
 static unsigned int kvm_vmid_bits __read_mostly;
-static DEFINE_SPINLOCK(kvm_vmid_lock);
+static DEFINE_RWLOCK(kvm_vmid_lock);
 
 static bool vgic_present;
 
@@ -473,11 +473,16 @@ static void update_vttbr(struct kvm *kvm)
 {
phys_addr_t pgd_phys;
u64 vmid;
+   bool new_gen;
 
-   if (!need_new_vmid_gen(kvm))
+   read_lock(_vmid_lock);
+   new_gen = need_new_vmid_gen(kvm);
+   read_unlock(_vmid_lock);
+
+   if (!new_gen)
return;
 
-   spin_lock(_vmid_lock);
+   write_lock(_vmid_lock);
 
/*
 * We need to re-check the vmid_gen here to ensure that if another vcpu
@@ -485,7 +490,7 @@ static void update_vttbr(struct kvm *kvm)
 * use the same vmid.
 */
if (!need_new_vmid_gen(kvm)) {
-   spin_unlock(_vmid_lock);
+   write_unlock(_vmid_lock);
return;
}
 
@@ -519,7 +524,7 @@ static void update_vttbr(struct kvm *kvm)
vmid = ((u64)(kvm->arch.vmid) << VTTBR_VMID_SHIFT) & 
VTTBR_VMID_MASK(kvm_vmid_bits);
kvm->arch.vttbr = kvm_phys_to_vttbr(pgd_phys) | vmid;
 
-   spin_unlock(_vmid_lock);
+   write_unlock(_vmid_lock);
 }
 
 static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
-- 
2.14.2

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm