Re: [PATCH] Add irqdevice interface + userint implementation

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

Gregory Haskins wrote:
>>>> On Tue, Apr 10, 2007 at  8:01 AM, in message <461B7C81.7040102-atKUWr5tajBWk0Htik3J/w@public.gmane.org>,
>>>>         
> Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org> wrote: 
>   
>> Gregory Haskins wrote:
>>     
>>> And if we replace read_vector() with ack(), how do we learn of the vector in 
>>>       
>> the first place?  I am guessing that you are thinking that both pending and 
>> read_vector return a vector and thus the confusion.  Otherwise, I am missing 
>> your point completely :)  Could you elaborate?
>>     
>>>   
>>>       
>> That's what I thought, but it doesn't really matter:  what's important 
>> is that accepting an interrupt has several stages, and that they must 
>> act as a transaction.
>>
>> The stages are:
>> 1. determine which interrupt is to be injected (if not, abort)
>> 2. check if IF and tpr allow injection
>> 3. one of
>>      -  inject the interrupt and mark it as in- service
>>      -  request an exit on IF and/or tpr threshold
>>
>> Simply indicating that an interrupt may be pending is already performed 
>> by the irq_sink callback.
>>     
>
> Yes, but the operative words here are "may be".  INTR tells the system that an interrupt *may be* pending, but we don't yet actually know which vector (if any) will be eligible for injection.  Thinking down the road when we will have SMP/PV, we must assume that the CPU could be actually running concurrently.  If so, it must be stopped first before we can evaluate the state (e.g., IF, TPR, etc.) to know which vector (if any) can be dispatched.  The eligibility of a vector (or the vector itself if a new, higher-pri line is raised in between) may change up until the point the EXIT actually occurs.  
>
> This is analogous to how the 8259 INTR/INTA cycle works, IIUC.  The INTR line is raised to get the CPUs attention asynchronously independent of CPU state, but its the processor controlled INTA cycle that retrieves the vector to use.  IMHO, we want to do the same thing here.   The thread that invokes the set_pin and thus indirectly the INTR is one context.  You can think of this as the PIC.  The thread that is running the guest and VMEXITs is another context.  You can think of this as the processor.  The PIC signals the processor that it needs attention, but the vector is conveyed on the processor's terms when its ready.  In cases where there is no parallelism (e.g., todays sync design, or of the vcpu happens to not be executing), the INTR is a no-op. 
>
> Now as far as the rest of the API is concerned, pending() tells us definitively (and efficiently) after we have stopped that there really is something pending given the current state of the system.  read_vector() tells us what that vector actually is.  Really, read_vector() is the authoritative source of data and acts like our processor controlled INTA cycle.  The important thing is that this read_vector() method is called in the same context as the guest CPU thread.  The other two (irq_sink, pending) are just helpers to get us to that point.
>
> Where I really think we need to have a critical section is between the vcpu-INTR callback, and the part of the vcpu logic that evaluates whether interrupts are pending (to avoid missing a newly injected interrupt while we are processing).  However, I really see this as an vcpu issue more than an issue against the irqdevice interface.  Does this make sense?
>   

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)

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)

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

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


-- 
error compiling committee.c: too many arguments to function


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