Re: KVM: only abort guest entry if timer count goes from 0->1

[Avi Kivity <avi@qumranet.com>]

Marcelo Tosatti wrote:
> Only abort guest entry if the timer count went from 0->1, since for 1->2
> or larger the bit will either be set already or a timer irq will have
> been injected.
>
> Using atomic_inc_and_test() for it also introduces an SMP barrier
> to the LAPIC version (thought it was unecessary because of timer
> migration, but guest can be scheduled to a different pCPU between exit
> and kvm_vcpu_block(), so there is the possibility for a race).
>
> Noticed by Avi.
>
>   

Applied, thanks.

> diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
> index 9e3391e..c0f7872 100644
> --- a/arch/x86/kvm/i8254.c
> +++ b/arch/x86/kvm/i8254.c
> @@ -198,14 +198,11 @@ static int __pit_timer_fn(struct kvm_kpit_state *ps)
>  	struct kvm_vcpu *vcpu0 = ps->pit->kvm->vcpus[0];
>  	struct kvm_kpit_timer *pt = &ps->pit_timer;
>  
> -	atomic_inc(&pt->pending);
> -	smp_mb__after_atomic_inc();
> -	if (vcpu0) {
> +	if (!atomic_inc_and_test(&pt->pending))
>  		set_bit(KVM_REQ_PENDING_TIMER, &vcpu0->requests);
> -		if (waitqueue_active(&vcpu0->wq)) {
> -			vcpu0->arch.mp_state = KVM_MP_STATE_RUNNABLE;
> -			wake_up_interruptible(&vcpu0->wq);
> -		}
> +	if (vcpu0 && waitqueue_active(&vcpu0->wq)) {
> +		vcpu0->arch.mp_state = KVM_MP_STATE_RUNNABLE;
> +		wake_up_interruptible(&vcpu0->wq);
>  	}
>   

Semi-related: what business does the timer have waking up vcpu0?  The 
timer pin may be masked, or routed via the ioapic to vcpu13.  The code 
here needs to touch irq pin 0, not wake up random vcpus.

It can't do that immediately since we're in interrupt context and we 
can't take the appropriate locks.  So we need to go through a 
workqueue.  There's some code for this in the pci device assignment 
code, so we can just use that when it is merged.

Longer term we need to give the pic and ioapic their own locking, likely 
with spin_lock_irq() so we can touch them from interrupt context.

> index 180ba73..73f43de 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -945,8 +945,8 @@ static int __apic_timer_fn(struct kvm_lapic *apic)
>  	int result = 0;
>  	wait_queue_head_t *q = &apic->vcpu->wq;
>  
> -	atomic_inc(&apic->timer.pending);
> -	set_bit(KVM_REQ_PENDING_TIMER, &apic->vcpu->requests);
> +	if(!atomic_inc_and_test(&apic->timer.pending))
> +		set_bit(KVM_REQ_PENDING_TIMER, &apic->vcpu->requests);
>  	if (waitqueue_active(q)) {
>  		apic->vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
>  		wake_up_interruptible(q);
>   

The wakeup is okay since lapic is handled by its vcpu.  But do we need 
to change the mpstate?  The lapic code should do that, if it determines 
that an interrupt is actually generated.

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