Re: [PATCH 2/5] KVM: arm/arm64: replace vcpu->arch.pause with a vcpu-request

[Christoffer Dall <cdall@linaro.org>]

On Mon, Mar 13, 2017 at 06:27:55PM +0100, Andrew Jones wrote:
> On Mon, Mar 13, 2017 at 11:30:05AM +0100, Christoffer Dall wrote:
> > On Wed, Mar 08, 2017 at 03:44:11PM +0100, Andrew Jones wrote:
> > > On Wed, Mar 08, 2017 at 06:33:12AM -0800, Christoffer Dall wrote:
> > > > >  static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
> > > > > @@ -621,7 +617,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
> > > > >  
> > > > >  		update_vttbr(vcpu->kvm);
> > > > >  
> > > > > -		if (vcpu->arch.power_off || vcpu->arch.pause)
> > > > > +		if (vcpu->arch.power_off || __kvm_request_test(KVM_REQ_VCPU_PAUSE, vcpu))
> > > > >  			vcpu_sleep(vcpu);
> > > > >  
> > > > >  		/*
> > > > > @@ -644,8 +640,13 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
> > > > >  			run->exit_reason = KVM_EXIT_INTR;
> > > > >  		}
> > > > >  
> > > > > +		vcpu->mode = IN_GUEST_MODE;
> > > > > +		smp_mb(); /* ensure vcpu->mode is visible to kvm_vcpu_kick */
> > > > 
> > > > I think this comment is misleading, because this smp_mb() is really
> > > > about ensuring that with respect to other CPUs, the write to vcpu->mode
> > > > is observable before the read of kvm_request_pending below, and the
> > > > corresponding other barrier is the barrier implied in cmpxchg used by
> > > > kvm_vcpu_exiting_guest_mode, which gets called by kvm_vcpu_kick(), which
> > > > is called after __kvm_set_request.
> > > 
> > > Agreed
> > > 
> > 
> > Just an adjustment to our conclusion from last week:
> > 
> > Will Deacon clarified that the cmpxchg doesn't have barrier semantics if
> > the cmpxchg operation fails.  My brain hurts trying to work out if we're
> > still safe in that case.  Thoughts?
> >
> 
> OK, I just ran my brain through a blender, and then I grabbed Radim to
> bounce my thoughts off him first. I think he and I are in agreement that
> kvm_arch_vcpu_should_kick() should get a read barrier before the cmpxchg.
> That smp_rmb() would pair with the general barrier that I have poorly
> commented in the code above.
> 
> I'll refresh this series in the next couple days, adding that barrier,
> and some better comments.
> 

Comments and an explicit barrier sounds like a welcome change.

Thanks,
-Christoffer