Re: [PATCH][RFC] Use return value from kvm_set_irq() to re-inject PIT interrupts.

[Marcelo Tosatti <mtosatti@redhat.com>]

On Mon, Aug 24, 2009 at 03:06:23PM +0300, Gleb Natapov wrote:
> Use return value from kvm_set_irq() to track coalesced PIT interrupts
> instead of ack/mask notifiers.

Gleb,

What is the advantage of doing so?

Ack notifiers are asynchronous notifications. Using the return value
from kvm_set_irq implies that timer emulation is based on a "tick
generating device" on the host side.

What I mean is that the ack notifications are useful, since they are
asynchronous.

Supposing your goal is to get rid of ack notifiers, due to their burden 
in irqchip code?

> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
> index b857ca3..0b63991 100644
> --- a/arch/x86/kvm/i8254.c
> +++ b/arch/x86/kvm/i8254.c
> @@ -231,20 +231,7 @@ int pit_has_pending_timer(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm_pit *pit = vcpu->kvm->arch.vpit;
>  
> -	if (pit && kvm_vcpu_is_bsp(vcpu) && pit->pit_state.irq_ack)
> -		return atomic_read(&pit->pit_state.pit_timer.pending);
> -	return 0;
> -}
> -
> -static void kvm_pit_ack_irq(struct kvm_irq_ack_notifier *kian)
> -{
> -	struct kvm_kpit_state *ps = container_of(kian, struct kvm_kpit_state,
> -						 irq_ack_notifier);
> -	spin_lock(&ps->inject_lock);
> -	if (atomic_dec_return(&ps->pit_timer.pending) < 0)
> -		atomic_inc(&ps->pit_timer.pending);
> -	ps->irq_ack = 1;
> -	spin_unlock(&ps->inject_lock);
> +	return atomic_read(&pit->pit_state.pit_timer.pending);
>  }
>  
>  void __kvm_migrate_pit_timer(struct kvm_vcpu *vcpu)
> @@ -297,7 +284,6 @@ static void create_pit_timer(struct kvm_kpit_state *ps, u32 val, int is_period)
>  	pt->vcpu = pt->kvm->bsp_vcpu;
>  
>  	atomic_set(&pt->pending, 0);
> -	ps->irq_ack = 1;
>  
>  	hrtimer_start(&pt->timer, ktime_add_ns(ktime_get(), interval),
>  		      HRTIMER_MODE_ABS);
> @@ -577,17 +563,6 @@ void kvm_pit_reset(struct kvm_pit *pit)
>  	mutex_unlock(&pit->pit_state.lock);
>  
>  	atomic_set(&pit->pit_state.pit_timer.pending, 0);
> -	pit->pit_state.irq_ack = 1;
> -}
> -
> -static void pit_mask_notifer(struct kvm_irq_mask_notifier *kimn, bool mask)
> -{
> -	struct kvm_pit *pit = container_of(kimn, struct kvm_pit, mask_notifier);
> -
> -	if (!mask) {
> -		atomic_set(&pit->pit_state.pit_timer.pending, 0);
> -		pit->pit_state.irq_ack = 1;
> -	}
>  }
>  
>  static const struct kvm_io_device_ops pit_dev_ops = {
> @@ -619,7 +594,6 @@ struct kvm_pit *kvm_create_pit(struct kvm *kvm, u32 flags)
>  
>  	mutex_init(&pit->pit_state.lock);
>  	mutex_lock(&pit->pit_state.lock);
> -	spin_lock_init(&pit->pit_state.inject_lock);
>  
>  	kvm->arch.vpit = pit;
>  	pit->kvm = kvm;
> @@ -628,17 +602,11 @@ struct kvm_pit *kvm_create_pit(struct kvm *kvm, u32 flags)
>  	pit_state->pit = pit;
>  	hrtimer_init(&pit_state->pit_timer.timer,
>  		     CLOCK_MONOTONIC, HRTIMER_MODE_ABS);
> -	pit_state->irq_ack_notifier.gsi = 0;
> -	pit_state->irq_ack_notifier.irq_acked = kvm_pit_ack_irq;
> -	kvm_register_irq_ack_notifier(kvm, &pit_state->irq_ack_notifier);
>  	pit_state->pit_timer.reinject = true;
>  	mutex_unlock(&pit->pit_state.lock);
>  
>  	kvm_pit_reset(pit);
>  
> -	pit->mask_notifier.func = pit_mask_notifer;
> -	kvm_register_irq_mask_notifier(kvm, 0, &pit->mask_notifier);
> -
>  	kvm_iodevice_init(&pit->dev, &pit_dev_ops);
>  	ret = __kvm_io_bus_register_dev(&kvm->pio_bus, &pit->dev);
>  	if (ret < 0)
> @@ -670,10 +638,6 @@ void kvm_free_pit(struct kvm *kvm)
>  	struct hrtimer *timer;
>  
>  	if (kvm->arch.vpit) {
> -		kvm_unregister_irq_mask_notifier(kvm, 0,
> -					       &kvm->arch.vpit->mask_notifier);
> -		kvm_unregister_irq_ack_notifier(kvm,
> -				&kvm->arch.vpit->pit_state.irq_ack_notifier);
>  		mutex_lock(&kvm->arch.vpit->pit_state.lock);
>  		timer = &kvm->arch.vpit->pit_state.pit_timer.timer;
>  		hrtimer_cancel(timer);
> @@ -683,12 +647,12 @@ void kvm_free_pit(struct kvm *kvm)
>  	}
>  }
>  
> -static void __inject_pit_timer_intr(struct kvm *kvm)
> +static int __inject_pit_timer_intr(struct kvm *kvm)
>  {
>  	struct kvm_vcpu *vcpu;
> -	int i;
> +	int i, r;
>  
> -	kvm_set_irq(kvm, kvm->arch.vpit->irq_source_id, 0, 1);
> +	r = kvm_set_irq(kvm, kvm->arch.vpit->irq_source_id, 0, 1);
>  	kvm_set_irq(kvm, kvm->arch.vpit->irq_source_id, 0, 0);
>  
>  	/*
> @@ -703,6 +667,8 @@ static void __inject_pit_timer_intr(struct kvm *kvm)
>  	if (kvm->arch.vapics_in_nmi_mode > 0)
>  		kvm_for_each_vcpu(i, vcpu, kvm)
>  			kvm_apic_nmi_wd_deliver(vcpu);
> +
> +	return r;
>  }
>  
>  void kvm_inject_pit_timer_irqs(struct kvm_vcpu *vcpu)
> @@ -711,20 +677,14 @@ void kvm_inject_pit_timer_irqs(struct kvm_vcpu *vcpu)
>  	struct kvm *kvm = vcpu->kvm;
>  	struct kvm_kpit_state *ps;
>  
> -	if (pit) {
> -		int inject = 0;
> -		ps = &pit->pit_state;
> -
> -		/* Try to inject pending interrupts when
> -		 * last one has been acked.
> -		 */
> -		spin_lock(&ps->inject_lock);
> -		if (atomic_read(&ps->pit_timer.pending) && ps->irq_ack) {
> -			ps->irq_ack = 0;
> -			inject = 1;
> -		}
> -		spin_unlock(&ps->inject_lock);
> -		if (inject)
> -			__inject_pit_timer_intr(kvm);
> -	}
> +	if (!pit)
> +		return;
> +
> +	ps = &pit->pit_state;
> +
> +	if (!atomic_read(&ps->pit_timer.pending))
> +		return;
> +
> +	if (__inject_pit_timer_intr(kvm))
> +		atomic_dec(&ps->pit_timer.pending);
>  }
> diff --git a/arch/x86/kvm/i8254.h b/arch/x86/kvm/i8254.h
> index d4c1c7f..19937da 100644
> --- a/arch/x86/kvm/i8254.h
> +++ b/arch/x86/kvm/i8254.h
> @@ -28,8 +28,6 @@ struct kvm_kpit_state {
>  	struct mutex lock;
>  	struct kvm_pit *pit;
>  	spinlock_t inject_lock;
> -	unsigned long irq_ack;
> -	struct kvm_irq_ack_notifier irq_ack_notifier;
>  };
>  
>  struct kvm_pit {
> @@ -39,7 +37,6 @@ struct kvm_pit {
>  	struct kvm *kvm;
>  	struct kvm_kpit_state pit_state;
>  	int irq_source_id;
> -	struct kvm_irq_mask_notifier mask_notifier;
>  };
>  
>  #define KVM_PIT_BASE_ADDRESS	    0x40
> --
> 			Gleb.