Re: [PATCH] Add irqdevice interface + userint implementation

[Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org>]

Gregory Haskins wrote:
>>> +
>>> +struct kvm_irqdevice;
>>> +
>>> +struct kvm_irqsink {
>>> +	void (*raise_intr)(struct kvm_irqsink *this, 
>>> +			   struct kvm_irqdevice *dev);
>>> +
>>> +	void *private;
>>> +};
>>> +
>>> +struct kvm_irqdevice {
>>> +	int  (*pending)(struct kvm_irqdevice *this, int flags);
>>> +	int  (*read_vector)(struct kvm_irqdevice *this, int flags); 
>>>   
>>>       
>> I'm a bit confused here.  The kvm_irqsink mechanism implies a push 
>> mechanism.  - >pending and - >read_vector imply a pull mechanism.
>>     
>
> What I am modeling is the concept of an PIC INTR + INTA.  INTR serves to just cause the processor to realize it needs to service an interrupt.  The INTA cycle is what then gets invoked after INTR to figure out which vector is appropriate.  Finally, the vector is called out of the IDT.  
>
> However, there is no actual INTA cycle on a virtualized guest.  We emulate an INTA cycle by calling our pending/read routines and injecting the vector into the VMCS.  Whats missing from the current HEAD is that there is no concept of INTR.  This is whats new, and its why we have both a push and a pull mechanism.  The old way only had the pull.  Does this make sense?  
>   

Hmm.  The current code is synchronous in nature (the vcpu is never
executing while we raise an interrupt, so the INTA is never needed, as
we can ensure the cpu can process the interrupt when we inject it.  With
smp/paravirt/whatever (I realize now), this is no longer true.  The NIC
raises an interrupt, we have to interrupt the guest to see if its
current IF/cr8 permit interrupt injection, and if not,  we have to keep
the interrupt in the irqdevice and request an exit when IF/cr8 permit
injection.

This means that ->pending() and ->read_vector() have to be called in a
critical section, otherwise the pending interrupt might be change
between the first and second call, resulting in an injected interrupt to
software that is not ready to accept it.  If we replace read_vector by
->ack(this, int vector) we avoid this need (and also save a pointless
recalculation of the vector).

  

>>> +static inline int bitarray_findhighest(struct bitarray *this)
>>> +{
>>> +	if (!this- >summary)
>>> +		return - 1;
>>> +	else {
>>> +	    int word_index = __ffs(this- >summary);
>>> +	    int bit_index  = __ffs(this- >pending[word_index]);
>>> +
>>> +	    return word_index * BITS_PER_LONG + bit_index;	
>>> +	}
>>> +}
>>>   
>>>       
>> Um, this is the lowest?
>>     
>
> Opps...carry over from the original code.  I forgot to reverse the polarity.
>
>   

Note that the original code did not work as intended.  Because of the
IF/tpr issues, there is at most interrupt queued, and only when it is
ready to be accepted.  With the in-kernel apic that changes of course.

>   
>>> +
>>> +static int userint_pending(struct kvm_irqdevice *this, int flags)
>>> +{
>>> +	kvm_userint *s = (kvm_userint*)this- >private;
>>> +	int ret;
>>> +
>>> +	spin_lock_irq(&s- >lock);
>>> +
>>> +	if (flags & KVM_IRQFLAGS_NMI)
>>> +		ret = s- >nmi_pending;
>>> +	else
>>> +		ret = bitarray_pending(&s- >irq_pending);
>>>   
>>>       
>> You probably want:
>>
>>     ret = s- >nmi_pending;
>>     if (!(flags & KVM_IRQFLAGS_NMI))
>>           ret |= bitarray_pending(...);
>>
>> To avoid calling this twice when interrupts are enabled.
>>     
>
> I think there is a misunderstanding.  The return value from this function should just be 0 for false, and non-zero for true.  The bitarray still holds a bit for IRQ2 so you technically wouldn't need to call this twice when interrupts are enabled.  Simply calling this without the flag set will give you the proper behavior, so I think it is fine as written.  It also doesn't hurt to do it the way you wrote it either.  Let me know if I misunderstood you.
>
>   

Ah, I forgot that nmi is also vector 2 here.



-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV