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

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

>>> On Mon, Apr 16, 2007 at 10:07 AM, in message <46238338.8070503-atKUWr5tajBWk0Htik3J/w@public.gmane.org>,
Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org> wrote: 
> Gregory Haskins wrote:
>> From: None <None>
>>
>> The current code is geared towards using a user- mode (A)PIC.  This patch adds
>> an "irqdevice" abstraction, and implements a "userint" model to handle the
>> duties of the original code.  Later, we can develop other irqdevice models 
>> to handle objects like LAPIC, IOAPIC, i8259, etc, as appropriate
>>
>>   
> 
> Viewed in light of 3/3, various races are exposed.
> 
> 
>> @@ - 2044,13 +2043,11 @@ static int kvm_vcpu_ioctl_set_sregs(struct kvm_vcpu 
> *vcpu,
>>  	if (mmu_reset_needed)
>>  		kvm_mmu_reset_context(vcpu);
>>  
>> -	memcpy(vcpu- >irq_pending, sregs- >interrupt_bitmap,
>> -	       sizeof vcpu- >irq_pending);
>> -	vcpu- >irq_summary = 0;
>> -	for (i = 0; i < NR_IRQ_WORDS; ++i)
>> -		if (vcpu- >irq_pending[i])
>> -			__set_bit(i, &vcpu- >irq_summary);
>> -
>> +	/* walk the interrupt- bitmap and inject an IRQ for each bit found */
>> +	for (i = 0; i < 256; ++i)
>> +		if (test_bit(i, &sregs- >interrupt_bitmap[0]))
>> +			kvm_irqdevice_set_pin(&vcpu- >irq_dev, i, 1);
>> + 
>>   
> 
> You need to lower a pin here if it was previously set.

Hmm.  Good find.  It also means my assumption about being able to ignore de-asserts in the userint model is false.  I will fix this.

> 
> 
>> diff -- git a/drivers/kvm/vmx.c b/drivers/kvm/vmx.c
>> index 61a6116..a0fdf02 100644
>> ---  a/drivers/kvm/vmx.c
>> +++ b/drivers/kvm/vmx.c
>> @@ - 1219,13 +1219,8 @@ static void inject_rmode_irq(struct kvm_vcpu *vcpu, 
> int irq)
>>  
>>  static void kvm_do_inject_irq(struct kvm_vcpu *vcpu)
>>  {
>> -	int word_index = __ffs(vcpu- >irq_summary);
>> -	int bit_index = __ffs(vcpu- >irq_pending[word_index]);
>> -	int irq = word_index * BITS_PER_LONG + bit_index;
>> -
>> -	clear_bit(bit_index, &vcpu- >irq_pending[word_index]);
>> -	if (!vcpu- >irq_pending[word_index])
>> -		clear_bit(word_index, &vcpu- >irq_summary);
>> +	int irq = kvm_irqdevice_read_vector(&vcpu- >irq_dev, 0);
>> +	BUG_ON(irq < 0);
>>   
> 
> This BUG can trigger.  A level- triggered irq was asserted, then deasserted.

Ditto for this.

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



> or here? possibly a good answer is "don't rely on r_f_i_i if not using 
> userint".

Yeah, exactly.  Once the user turns on in-kernel interrupts, some of the kvm-run interface (TBD) will be disabled/ignored due to being redundant/overlapping.

Since userspace will have to ack the in-kernel interrupt feature, it will obviously know when to stop using those features as well.

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