Re: [PATCH 2/3] KVM: Add irqdevice object

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

>>> On Mon, Apr 16, 2007 at 10:55 AM, in message <46238E55.80401-atKUWr5tajBWk0Htik3J/w@public.gmane.org>, Avi
Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org> wrote: 
> Gregory Haskins wrote:
>>   
>>>>  
>>>>  	if (vcpu-  >rmode.active) {
>>>>  		inject_rmode_irq(vcpu, irq);
>>>> @@ -  1246,7 +1241,7 @@ static void do_interrupt_requests(struct kvm_vcpu 
>>>>       
>>> *vcpu,
>>>     
>>>>  		 (vmcs_read32(GUEST_INTERRUPTIBILITY_INFO) & 3) == 0);
>>>>  
>>>>  	if (vcpu-  >interrupt_window_open &&
>>>> -	    vcpu-  >irq_summary &&
>>>> +	    kvm_irqdevice_pending(&vcpu-  >irq_dev, 0) &&
>>>>  	    !(vmcs_read32(VM_ENTRY_INTR_INFO_FIELD) & INTR_INFO_VALID_MASK))
>>>>   
>>>>       
>>> What if an irq is made pending here?
>>>     
>>
>> The only race I see is related to what you pointed out previously:  A 
> level- sensitive interrupt could be asserted when pending() is read, and 
> deasserted when read_vector() is read.  Handling the irq == - 1 from 
> read_vector() should fix the race.  Or are you pointing out something else?
>>   
> 
> That one.
> 
> I think there are probably a few more hiding in there.  To reduce the 
> number of combinations, I'd suggest putting the irq and the inter- vcpu 
> communication things under the same lock, and to make sure - >pending() 
> and - >read_vector() are always called in the same critical section (or 
> even better, to unify them into one function).

I think you will see that this has been addressed downstream in my series.  Its just not polished up enough to share yet.  Probably some time this week, however.

Thanks for the review!

-Greg


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/