Re: FW: [kvm-commits] KVM: Move interrupt injection out of interruptdisabled section

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

Dong, Eddie wrote:
>
>> benefit is
>> removing the lapic calculations from the critical section.
>>
>> But I still don't understand your object.  The interrupt has to be
>> committed sometime.  We move the commit point earlier by the time it
>> takes to ->prepare_guest_switch() (normally zero, but sometimes some
>> thousand cycles), and kvm_load_guest_fpu() (usually zero as
>> well).  The
>> timing is sometimes worse, but there was always a window where a high
>> priority interrupt is ignored over low priority interrupts.
>>     
>
> Yes even in rare native when 2 irqs arrives at almost same time.
> But now we have frequent happenings. Guest frequently see
> higher irq pending. Let us keep an eye to see if it will hurt something.
>
> Then probably need this patch?
>
> diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c
> index aab465d..46ad3b7 100644
> --- a/drivers/kvm/kvm_main.c
> +++ b/drivers/kvm/kvm_main.c
> @@ -2165,6 +2165,7 @@ again:
>         if (unlikely(r))
>                 goto out;
>
> +       vcpu->guest_mode = 1;
>         kvm_inject_pending_timer_irqs(vcpu);
>         clear_bit(KVM_REQ_INTR, &vcpu->requests);
>         if (irqchip_in_kernel(vcpu->kvm))
> @@ -2183,6 +2184,7 @@ again:
>                 local_irq_enable();
>                 preempt_enable();
>                 r = -EINTR;
> +               vcpu->guest_mode = 0;
>                 kvm_run->exit_reason = KVM_EXIT_INTR;
>                 ++vcpu->stat.signal_exits;
>                 goto out;
> @@ -2192,6 +2194,7 @@ again:
>                 if (test_and_clear_bit(KVM_REQ_TLB_FLUSH,
> &vcpu->requests))
>                         kvm_x86_ops->tlb_flush(vcpu);
>                 if (test_bit(KVM_REQ_INTR, &vcpu->requests)) {
> +                       vcpu->guest_mode = 0;
>                         local_irq_enable();
>                         preempt_enable();
>                         r = 1;
> @@ -2199,7 +2202,6 @@ again:
>                 }
>         }
>
> -       vcpu->guest_mode = 1;
>         kvm_guest_enter();
>
>         kvm_x86_ops->run(vcpu, kvm_run);
>
>   

I don't see why this is needed.  The interrupt code will 
set_bit(KVM_REQ_INTR) even if the vcpu is not in guest mode.


>   
>> [we can try to improve it by using vm86 interrupt redirection
>> which may
>> allow event injection using VT instead of writing to the guest stack].
>>     
>
> :) Love this much more personaly, or lock pages.
>
>   

We could lock the pages like this:

- do the interrupt injection
    - if a real mode interrupt, abort, and set a flag requesting to lock 
the guest pages
        - exit the critical section
        - notice the flag is set, lock guest stack
        - retry
    - otherwise, proceed with injection

but using vm86 interrupt redirection is better.


>>> Another serious issue in current code is that due to previous
>>> injection, the irq is already consumed, (IRR->ISR). So if there
>>> are higher priority IRQs arrive later after the previous injection,
>>> now guest will see 2 IRQs consumed, i.e. 2 ISR bits are set.
>>>
>>>
>>>       
>> I don't understand why.  If an event is pending in
>> VM_ENTRY_INTR_INFO_FIELD, then we don't call
>> kvm_cpu_get_interrupt() at all.
>>
>>     
> This is for protect mode. real mode is not setting this one,
> and thus double consumed.
>
>   

Good point.  That is also fixed by using vm86 interrupt redirection.  
I'll read some more about it.

> But this is fixable with other additional logic. My point is that
> we may see more potential issues now :-(
>
>
>   

You're right.  Well, that's the price of progress (swapping).

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


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/