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

[Gleb Natapov <gleb@redhat.com>]

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.

--
			Gleb.