Re: [PATCH v3 2/5] KVM: Switch assigned device IRQ forwarding to threaded handler

[Marcelo Tosatti <mtosatti@redhat.com>]

On Wed, Nov 03, 2010 at 09:11:13AM +0100, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> This improves the IRQ forwarding for assigned devices: By using the
> kernel's threaded IRQ scheme, we can get rid of the latency-prone work
> queue and simplify the code in the same run.
> 
> Moreover, we no longer have to hold assigned_dev_lock while raising the
> guest IRQ, which can be a lenghty operation as we may have to iterate
> over all VCPUs. The lock is now only used for synchronizing masking vs.
> unmasking of INTx-type IRQs, thus is renames to intx_lock.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  include/linux/kvm_host.h |   12 +----
>  virt/kvm/assigned-dev.c  |  106 ++++++++++++++-------------------------------
>  2 files changed, 35 insertions(+), 83 deletions(-)
> 
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index bcf71c7..eaacb5d 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -457,16 +457,8 @@ struct kvm_irq_ack_notifier {
>  	void (*irq_acked)(struct kvm_irq_ack_notifier *kian);
>  };
>  
> -#define KVM_ASSIGNED_MSIX_PENDING		0x1
> -struct kvm_guest_msix_entry {
> -	u32 vector;
> -	u16 entry;
> -	u16 flags;
> -};
> -
>  struct kvm_assigned_dev_kernel {
>  	struct kvm_irq_ack_notifier ack_notifier;
> -	struct work_struct interrupt_work;
>  	struct list_head list;
>  	int assigned_dev_id;
>  	int host_segnr;
> @@ -477,13 +469,13 @@ struct kvm_assigned_dev_kernel {
>  	bool host_irq_disabled;
>  	struct msix_entry *host_msix_entries;
>  	int guest_irq;
> -	struct kvm_guest_msix_entry *guest_msix_entries;
> +	struct msix_entry *guest_msix_entries;
>  	unsigned long irq_requested_type;
>  	int irq_source_id;
>  	int flags;
>  	struct pci_dev *dev;
>  	struct kvm *kvm;
> -	spinlock_t assigned_dev_lock;
> +	spinlock_t intx_lock;
>  };
>  
>  struct kvm_irq_mask_notifier {
> diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
> index ecc4419..d0b07ea 100644
> --- a/virt/kvm/assigned-dev.c
> +++ b/virt/kvm/assigned-dev.c
> @@ -55,58 +55,31 @@ static int find_index_from_host_irq(struct kvm_assigned_dev_kernel
>  	return index;
>  }
>  
> -static void kvm_assigned_dev_interrupt_work_handler(struct work_struct *work)
> +static irqreturn_t kvm_assigned_dev_thread(int irq, void *dev_id)
>  {
> -	struct kvm_assigned_dev_kernel *assigned_dev;
> -	int i;
> +	struct kvm_assigned_dev_kernel *assigned_dev = dev_id;
> +	u32 vector;
> +	int index;
>  
> -	assigned_dev = container_of(work, struct kvm_assigned_dev_kernel,
> -				    interrupt_work);
> +	if (assigned_dev->irq_requested_type & KVM_DEV_IRQ_HOST_INTX) {
> +		spin_lock(&assigned_dev->intx_lock);
> +		disable_irq_nosync(irq);
> +		assigned_dev->host_irq_disabled = true;
> +		spin_unlock(&assigned_dev->intx_lock);
> +	}

Shouldnt this happen in the hardirq handler? Otherwise host will receive
interrupts between ->eoi and this point.