>>> On Mon, Apr 16, 2007 at 10:07 AM, in message <[EMAIL PROTECTED]>,
Avi Kivity <[EMAIL PROTECTED]> 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/
_______________________________________________
kvm-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel
