Re: [PATCH] Add irqdevice interface + userint implementation

["Gregory Haskins" <ghaskins-Et1tbQHTxzrQT0dZR+AlfA@public.gmane.org>]

>>> On Tue, Apr 10, 2007 at 10:46 AM, in message <461BA363.5080308-atKUWr5tajBWk0Htik3J/w@public.gmane.org>,
Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org> wrote: 
> 
> Both the vcpu and the irqdevice are involved in the decision:
> 
> -  the vcpu holds the IF (or rather, the "interrupt window")
> -  the lapic holds the tpr
> -  but the vcpu contains the controls that allow exiting on tpr 
> modification (on 64- bit and with next- gen Intel processors)

Agreed

> 
> IIUC, you are suggesting (pseudocode):
> 
>   spin_lock(vcpu- >irq_lock)
>   nmi = !irq_window_open(vcpu)
>   if (vcpu- >irq- >pending(nmi))
>       inject(vcpu, vcpu- >irq- >read_vector(nmi))
>   spin_unlock(vcpu- >irq_lock)
> 

Your psuedo-code is spot on.  This would also be coupled with the locks inside the INTR callback, of course. 

> where - >pending() has to program a tpr exit if current tpr masks an 
> interrupt.

I wasn't planning on the irqdevice having this type of control because I assumed we would always have these exit types set, but you have highlighted an interesting point.  If we want to selectively enable these different types of exits (and I agree that it's probably a good idea), we need more intelligence from the irqdevice.  Now the question is how to handle/detect the different *types* of masking that can occur.  My current API is really just designed to return a boolean: true = "there are injectable vectors", false = "there are no injectable vectors (but there may be masked vectors).  

I don't like the idea of this programming being handled directly by the irqdevice because I think its potentially the wrong layer.  However, we do need to account for this somehow.  We could change the API to handle the pending condition as a tri-level: no-interrupts, masked-interrupts, injectable-interrupts?  For "no" and "injectable", the operation is as your psuedo-code indicates.  For the masked case, we could enumerate different classes of reasons (e.g. TPR, etc) that could be used to drive the programming.  So the pseudo-code would now be:

   spin_lock(vcpu- >irq_lock)
   nmi = !irq_window_open(vcpu)
   switch (vcpu- >irq- >pending(nmi, &mask_reasons)) {
   case NONE_PENDING:
      break;
   case MASKED_PENDING:
      if (mask_reasons & MASKREASON_TPR)
          vcpu->exit_reasons &= TPR;
      /* etc */
      break;
    case PENDING:
       inject(vcpu, vcpu- >irq- >read_vector(nmi))
       break;
   }
   spin_unlock(vcpu- >irq_lock)

Thoughts?

> 
> The alternative I'm suggesting is to move all this mess into the 
> irqdevice code in a single callback.  While this has less reuse 
> potential, in fact the hardware itself is messy and attempts to 
> generalize it may turn into a mess as well.
> 

I cant quite wrap my head around how I could work out all of the issues as I (think) have done with the current proposal, but I am open to suggestion.  Can you elaborate more?

>> I will put together a follow- on patch that implements my thinking to help 
> illustrate the concept.
>>   
> 
> That will be best.  After all, we're not trying to design a generic 
> interface, we have a know an limited set of users.  Seeing the users 
> will help evaluate the API.

Will do.

Regards,
-Greg




-------------------------------------------------------------------------
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