Re: [PATCH v5 4/4] KVM: x86: Add support for local interrupt requests from userspace

[Steve Rutherford <srutherford@google.com>]

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