Re: [PATCH v4 2/2] KVM: VMX: Add Posted Interrupt supporting

[Marcelo Tosatti <mtosatti@redhat.com>]

On Sat, Feb 23, 2013 at 04:35:30PM +0200, Gleb Natapov wrote:
> On Sat, Feb 23, 2013 at 02:05:28PM +0000, Zhang, Yang Z wrote:
> > >> +	return test_bit(vector, (unsigned long *)pi_desc->pir);
> > >> +}
> > >> +
> > >> +static void pi_set_pir(int vector, struct pi_desc *pi_desc)
> > >> +{
> > >> +	__set_bit(vector, (unsigned long *)pi_desc->pir);
> > >> +}
> > > 
> > > This must be locked atomic operation (set_bit).
> > If using spinlock, pi_set_pir is not called concurrently. It safe to use _set_bit.
> > 
> HW still access it concurrently without bothering taking your lock.
> 
> >  
> > >> +
> > >>  struct vcpu_vmx {
> > >>  	struct kvm_vcpu       vcpu;
> > >>  	unsigned long         host_rsp;
> > >> @@ -429,6 +465,10 @@ struct vcpu_vmx {
> > >> 
> > >>  	bool rdtscp_enabled;
> > >> +	/* Posted interrupt descriptor */
> > >> +	struct pi_desc pi_desc;
> > >> +	spinlock_t pi_lock;
> > > 
> > > Don't see why the lock is necessary. Please use the following
> > > pseudocode for vmx_deliver_posted_interrupt:
> > > 
> > > vmx_deliver_posted_interrupt(), returns 'bool injected'.
> > > 
> > > 1. orig_irr = read irr from vapic page
> > > 2. if (orig_irr != 0)
> > > 3.	return false;
> > > 4. kvm_make_request(KVM_REQ_EVENT)
> > > 5. bool injected = !test_and_set_bit(PIR)
> > > 6. if (vcpu->guest_mode && injected)
> > > 7. 	if (test_and_set_bit(PIR notification bit))
> > > 8.		send PIR IPI
> > > 9. return injected
> > 
> > Consider follow case:
> > vcpu 0                      |                vcpu1
> > send intr to vcpu1
> > check irr
> >                                             receive a posted intr
> > 										pir->irr(pir is cleared, irr is set)
> > injected=test_and_set_bit=true
> > pir=set
> > 
> > Then both irr and pir have the interrupt pending, they may merge to one later, but injected reported as true. Wrong.
> > 
> I and Marcelo discussed the lockless logic that should be used here on
> the previous patch submission. All is left for you is to implement it.
> We worked hard to make irq injection path lockless, we will not going to
> add one now.

He is right, the scheme is still flawed (because of concurrent injectors
along with HW in VMX non-root). I'd said lets add a spinlock think about
lockless scheme in the meantime.