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
