Re: [PATCH 1/6] KVM: Check for pending events before attempting injection

[Avi Kivity <avi@redhat.com>]

  On 07/28/2010 07:21 PM, Marcelo Tosatti wrote:
> On Tue, Jul 27, 2010 at 04:19:35PM +0300, Avi Kivity wrote:
>> Instead of blindly attempting to inject an event before each guest entry,
>> check for a possible event first in vcpu->requests.  Sites that can trigger
>> event injection are modified to set KVM_REQ_EVENT:
>>
>> - interrupt, nmi window opening
>> - ppr updates
>> - i8259 output changes
>> - local apic irr changes
>> - rflags updates
>> - gif flag set
>> - event set on exit
>>
>> This improves non-injecting entry performance, and sets the stage for
>> non-atomic injection.
>>
>> Signed-off-by: Avi Kivity<avi@redhat.com>
>> ---
>>   arch/x86/kvm/i8259.c     |    1 +
>>   arch/x86/kvm/lapic.c     |   12 ++++++++++--
>>   arch/x86/kvm/svm.c       |    8 +++++++-
>>   arch/x86/kvm/vmx.c       |    6 ++++++
>>   arch/x86/kvm/x86.c       |   35 ++++++++++++++++++++++++++---------
>>   include/linux/kvm_host.h |    1 +
>>   6 files changed, 51 insertions(+), 12 deletions(-)
>>
>> @@ -4731,17 +4737,19 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>>   		goto out;
>>   	}
>>
>> -	inject_pending_event(vcpu);
>> +	if (kvm_check_request(KVM_REQ_EVENT, vcpu)) {
>> +		inject_pending_event(vcpu);
>>
>> -	/* enable NMI/IRQ window open exits if needed */
>> -	if (vcpu->arch.nmi_pending)
>> -		kvm_x86_ops->enable_nmi_window(vcpu);
>> -	else if (kvm_cpu_has_interrupt(vcpu) || req_int_win)
>> -		kvm_x86_ops->enable_irq_window(vcpu);
>> +		/* enable NMI/IRQ window open exits if needed */
>> +		if (vcpu->arch.nmi_pending)
>> +			kvm_x86_ops->enable_nmi_window(vcpu);
>> +		else if (kvm_cpu_has_interrupt(vcpu) || req_int_win)
>> +			kvm_x86_ops->enable_irq_window(vcpu);
> Problem is it might not be possible to inject the event signalled by
> KVM_REQ_EVENT, say an interrupt from an irqchip, if there is an event
> that needs reinjection (or an exception).

That can happen event now, no?  A pending exception, interrupt comes 
along, injection picks up the exception but leaves the interrupt.

Now the situation can be more complicated:

- pending exception
- injection
- interrupt, sets KVM_REQ_EVENT
- notices KVM_REQ_EVENT
- drops KVM_REQ_EVENT, cancels exception (made pending again)
- goes back
- injection (injects exception again, interrupt is pending)

as far as I can tell, this is all fine.

> Perhaps moving atomic_set(&vcpu->guest_mode, 1) up to preemptible
> section is safe, because kvm_vcpu_kick can only IPI stale vcpu->cpu
> while preemption is enabled. In that case, it will hit
>
> if (!atomic_read(&vcpu->guest_mode)
>
> later.
>

I don't really follow.

> Although the KVM_REQ_EVENT idea is nice. Can you think of a way
> to fix the issue?



-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.