From mboxrd@z Thu Jan  1 00:00:00 1970
From: Steve Rutherford <srutherford@google.com>
Subject: Re: [PATCH v5 4/4] KVM: x86: Add support for local interrupt
 requests from userspace
Date: Tue, 28 Jul 2015 12:06:35 -0700
Message-ID: <20150728190635.GA7460@google.com>
References: <1438039062-3168-1-git-send-email-srutherford@google.com>
 <1438039062-3168-4-git-send-email-srutherford@google.com>
 <55B7A6AE.5070001@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: kvm@vger.kernel.org
To: Paolo Bonzini <pbonzini@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mail-pd0-f181.google.com ([209.85.192.181]:35954 "EHLO
	mail-pd0-f181.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1752246AbbG1TGl (ORCPT <rfc822;kvm@vger.kernel.org>);
	Tue, 28 Jul 2015 15:06:41 -0400
Received: by pdjr16 with SMTP id r16so76593011pdj.3
        for <kvm@vger.kernel.org>; Tue, 28 Jul 2015 12:06:41 -0700 (PDT)
Content-Disposition: inline
In-Reply-To: <55B7A6AE.5070001@redhat.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On Tue, Jul 28, 2015 at 05:58:38PM +0200, Paolo Bonzini wrote:
> 
> 
> On 28/07/2015 01:17, Steve Rutherford wrote:
> >  	return kvm->arch.vpic;
> >  }
> >  
> > +static inline int pic_in_kernel(struct kvm *kvm)
> > +{
> > +	int ret;
> > +
> > +	ret = (pic_irqchip(kvm) != NULL);
> > +	smp_rmb();
> 
> What does this memory barrier pair with?  I don't think it's necessary.

To be honest, it's probably not necessary. I couldn't find why
irqchip_in_kernel (which this function is more or less a copy of)
needed it's memory barrier, so I cargo culted this one in.

> 
> > +	return ret;
> > +}
> > +
> >  static inline int irqchip_split(struct kvm *kvm)
> >  {
> >  	return kvm->arch.irqchip_split;
> 
> 
> > @@ -5819,13 +5828,24 @@ static void post_kvm_run_save(struct kvm_vcpu *vcpu)
> >  	kvm_run->flags = is_smm(vcpu) ? KVM_RUN_X86_SMM : 0;
> >  	kvm_run->cr8 = kvm_get_cr8(vcpu);
> >  	kvm_run->apic_base = kvm_get_apic_base(vcpu);
> > -	if (irqchip_in_kernel(vcpu->kvm))
> > +	if (irqchip_in_kernel(vcpu->kvm) && pic_in_kernel(vcpu->kvm))
> >  		kvm_run->ready_for_interrupt_injection = 1;
> > -	else
> > +	else if (irqchip_in_kernel(vcpu->kvm)) {
> > +		int ready_for_interrupt_injection =
> > +				kvm_apic_accept_pic_intr(vcpu);
> > +
> > +		if (!kvm_run->ready_for_interrupt_injection &&
> > +		    ready_for_interrupt_injection)
> > +			kvm_make_request(KVM_REQ_PIC_UNMASK_EXIT, vcpu);
> > +
> > +		kvm_run->ready_for_interrupt_injection =
> > +				ready_for_interrupt_injection;
> > +	} else {
> >  		kvm_run->ready_for_interrupt_injection =
> >  			kvm_arch_interrupt_allowed(vcpu) &&
> >  			!kvm_cpu_has_interrupt(vcpu) &&
> >  			!kvm_event_needs_reinjection(vcpu);
> > +	}
> >  }
> >  
> >  static void update_cr8_intercept(struct kvm_vcpu *vcpu)
> 
> Why is this necessary?  Could it just set
> kvm_run->ready_for_interrupt_injection as in the pic_in_kernel case?

The goal is to couple the interrupt ack cycle as closely as possible
with the injection of the local interrupt (which occur more or less
atomically on real hardware). The idea is to only ever attempt to
inject local interrupts when the CPU/APIC is ready to immediately
accept. 

If the CPU is ignoring the PIC, the interrupt acknowledge cycle should
not be performed, even if the PIC is high. This patch uses the
ready_for_interrupt_injection flag to let userspace whether or not the
cpu is paying attention to the PIC at the moment. 

When the PIC is high and the CPU transitions from ignoring the PIC to
paying attention to the PIC, it should (per real hardware)
immediately trigger an interrupt acknowledge cycle (which requires
bouncing up to userspace).

Steve
> 
> Paolo