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.

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to