Re: [PATCH v4 09/11] KVM: arm/arm64: use vcpu requests for irq injection

[Christoffer Dall <cdall@linaro.org>]

On Thu, Jun 01, 2017 at 03:38:43PM +0200, Andrew Jones wrote:
> On Thu, Jun 01, 2017 at 03:27:16PM +0200, Christoffer Dall wrote:
> > On Thu, Jun 01, 2017 at 12:59:21PM +0200, Andrew Jones wrote:
> > > On Thu, Jun 01, 2017 at 12:35:33PM +0200, Christoffer Dall wrote:
> > > > On Tue, May 16, 2017 at 04:20:33AM +0200, Andrew Jones wrote:
> > > > > Don't use request-less VCPU kicks when injecting IRQs, as a VCPU
> > > > > kick meant to trigger the interrupt injection could be sent while
> > > > > the VCPU is outside guest mode, which means no IPI is sent, and
> > > > > after it has called kvm_vgic_flush_hwstate(), meaning it won't see
> > > > > the updated GIC state until its next exit some time later for some
> > > > > other reason.  The receiving VCPU only needs to check this request
> > > > > in VCPU RUN to handle it.  By checking it, if it's pending, a
> > > > > memory barrier will be issued that ensures all state is visible.
> > > > > We still create a vcpu_req_irq_pending() function (which is a nop),
> > > > > though, in order to allow us to use the standard request checking
> > > > > pattern.
> > > > > 
> > > > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > > > ---
> > > > >  arch/arm/include/asm/kvm_host.h   |  1 +
> > > > >  arch/arm64/include/asm/kvm_host.h |  1 +
> > > > >  virt/kvm/arm/arm.c                | 12 ++++++++++++
> > > > >  virt/kvm/arm/vgic/vgic.c          |  9 +++++++--
> > > > >  4 files changed, 21 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> > > > > index fdd644c01c89..00ad56ee6455 100644
> > > > > --- a/arch/arm/include/asm/kvm_host.h
> > > > > +++ b/arch/arm/include/asm/kvm_host.h
> > > > > @@ -46,6 +46,7 @@
> > > > >  
> > > > >  #define KVM_REQ_SLEEP \
> > > > >  	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> > > > > +#define KVM_REQ_IRQ_PENDING	KVM_ARCH_REQ(1)
> > > > >  
> > > > >  u32 *kvm_vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num, u32 mode);
> > > > >  int __attribute_const__ kvm_target_cpu(void);
> > > > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > > > > index 9bd0d1040de9..0c4fd1f46e10 100644
> > > > > --- a/arch/arm64/include/asm/kvm_host.h
> > > > > +++ b/arch/arm64/include/asm/kvm_host.h
> > > > > @@ -43,6 +43,7 @@
> > > > >  
> > > > >  #define KVM_REQ_SLEEP \
> > > > >  	KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> > > > > +#define KVM_REQ_IRQ_PENDING	KVM_ARCH_REQ(1)
> > > > >  
> > > > >  int __attribute_const__ kvm_target_cpu(void);
> > > > >  int kvm_reset_vcpu(struct kvm_vcpu *vcpu);
> > > > > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> > > > > index ddc833987dfb..73a75ca91e41 100644
> > > > > --- a/virt/kvm/arm/arm.c
> > > > > +++ b/virt/kvm/arm/arm.c
> > > > > @@ -570,6 +570,15 @@ static void vcpu_req_sleep(struct kvm_vcpu *vcpu)
> > > > >  	}
> > > > >  }
> > > > >  
> > > > > +static void vcpu_req_irq_pending(struct kvm_vcpu *vcpu)
> > > > > +{
> > > > > +	/*
> > > > > +	 * Nothing to do here. kvm_check_request() already issued a memory
> > > > > +	 * barrier that pairs with kvm_make_request(), so all hardware state
> > > > > +	 * we need to flush should now be visible.
> > > > > +	 */
> > > > 
> > > > I don't understand this comment :(
> > > 
> > > We need a kvm_check_request() to pair with a requesting VCPU's setting
> > > of virtual irq state and call of kvm_make_request(). The requester's
> > > kvm_make_request() ensures the target VCPU executes VCPU RUN, and the
> > > barriers wrapped in kvm_check/make_request() ensure that when VCPU RUN
> > > calls kvm_vgic_flush_hwstate() that the virtual irq state set by the
> > > requester is visible to the target VCPU.
> > > 
> > > But you knew all that already :-) So, maybe I just need to replace
> > > 'all hardware state we need to flush should now be visible.' with
> > > 'the virtual irq state is now visible.'?
> > > 
> > 
> > Yes, the hardware state is definitely vague here, but I also feel like
> > you're trying to repeat parts of the documentation here, and it's better
> > to just refer to the complete documentation for the how the barreirs
> > etc. work, and just explain why you don't need to do anything, yet still
> > have a vcpu request.
> > 
> > Which is why I suggested the alternative comment below instead ;)
> > 
> > > > 
> > > > And I don't much like this empty function either.
> > > > 
> > > > 
> > > > > +}
> > > > > +
> > > > >  static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
> > > > >  {
> > > > >  	return vcpu->arch.target >= 0;
> > > > > @@ -580,6 +589,8 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
> > > > >  	if (kvm_request_pending(vcpu)) {
> > > > >  		if (kvm_check_request(KVM_REQ_SLEEP, vcpu))
> > > > >  			vcpu_req_sleep(vcpu);
> > > > > +		if (kvm_check_request(KVM_REQ_IRQ_PENDING, vcpu))
> > > > > +			vcpu_req_irq_pending(vcpu);
> > > > 
> > > > 
> > > > Can we just do:
> > > > 		/* 
> > > > 		 * Clear IRQ_PENDING requests that were made to
> > > > 		 * guarantee that a VCPU sees new virtual interrupts.
> > > > 		 */
> > > > 		kvm_check_request(KVM_REQ_IRQ_PENDING, vcpu);
> > > > 
> > > > ?
> > > 
> > > I won't insist, but I prefer the empty function to breaking the pattern
> > > of this if-sequence, and also to the calling of a function named "check"
> > > without considering its return value.
> > 
> > I like scratching our OCD as much as the next reviewer, but I think my
> > version more clearly expresses the purpose, which is more important than
> > having multiple identically-looking statements.
> > 
> > Thanks,
> > -Christoffer
> 
> I'll take your suggestion, and hopefully my OCD won't keep me up at night.
> I'll get v5 tested and sent tomorrow.
> 
Thanks Drew!
-Christoffer