Am 01.11.2010 15:08, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@xxxxxxxxxxx>
>
> The complete work handler runs with assigned_dev_lock acquired and
> interrupts disabled, so there is nothing to gain pushing this work out
> of the actually IRQ handler. Fold them together.
Err, forget it. kvm_set_irq pulls in the famous pic lock, and that one
is not prepared to be called from IRQ context (lockdep just complained).
I will try to clean this up via a threaded IRQ handler, maybe using the
slow-path only for INTx-type IRQs and pushing MSIs directly from the
hard handler.
Jan
>
> Signed-off-by: Jan Kiszka <jan.kiszka@xxxxxxxxxxx>
> ---
> include/linux/kvm_host.h | 1 -
> virt/kvm/assigned-dev.c | 71 +++++++++++++++-------------------------------
> 2 files changed, 23 insertions(+), 49 deletions(-)
>
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 462b982..df5497f 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -465,7 +465,6 @@ struct kvm_guest_msix_entry {
>
> 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;
> diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
> index 7c98928..5c1b56a 100644
> --- a/virt/kvm/assigned-dev.c
> +++ b/virt/kvm/assigned-dev.c
> @@ -55,18 +55,24 @@ 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_intr(int irq, void *dev_id)
> {
> - struct kvm_assigned_dev_kernel *assigned_dev;
> - int i;
> -
> - assigned_dev = container_of(work, struct kvm_assigned_dev_kernel,
> - interrupt_work);
> + struct kvm_assigned_dev_kernel *assigned_dev =
> + (struct kvm_assigned_dev_kernel *) dev_id;
> + unsigned long flags;
>
> - spin_lock_irq(&assigned_dev->assigned_dev_lock);
> + spin_lock_irqsave(&assigned_dev->assigned_dev_lock, flags);
> if (assigned_dev->irq_requested_type & KVM_DEV_IRQ_HOST_MSIX) {
> struct kvm_guest_msix_entry *guest_entries =
> assigned_dev->guest_msix_entries;
> + int index = find_index_from_host_irq(assigned_dev, irq);
> + int i;
> +
> + if (index < 0)
> + goto out;
> +
> + guest_entries[index].flags |= KVM_ASSIGNED_MSIX_PENDING;
> +
> for (i = 0; i < assigned_dev->entries_nr; i++) {
> if (!(guest_entries[i].flags &
> KVM_ASSIGNED_MSIX_PENDING))
> @@ -76,33 +82,15 @@ static void kvm_assigned_dev_interrupt_work_handler(struct work_struct *work)
> assigned_dev->irq_source_id,
> guest_entries[i].vector, 1);
> }
> - } else
> + } else {
> kvm_set_irq(assigned_dev->kvm, assigned_dev->irq_source_id,
> assigned_dev->guest_irq, 1);
>
> - spin_unlock_irq(&assigned_dev->assigned_dev_lock);
> -}
> -
> -static irqreturn_t kvm_assigned_dev_intr(int irq, void *dev_id)
> -{
> - unsigned long flags;
> - struct kvm_assigned_dev_kernel *assigned_dev =
> - (struct kvm_assigned_dev_kernel *) dev_id;
> -
> - spin_lock_irqsave(&assigned_dev->assigned_dev_lock, flags);
> - if (assigned_dev->irq_requested_type & KVM_DEV_IRQ_HOST_MSIX) {
> - int index = find_index_from_host_irq(assigned_dev, irq);
> - if (index < 0)
> - goto out;
> - assigned_dev->guest_msix_entries[index].flags |=
> - KVM_ASSIGNED_MSIX_PENDING;
> - }
> -
> - schedule_work(&assigned_dev->interrupt_work);
> -
> - if (assigned_dev->irq_requested_type & KVM_DEV_IRQ_GUEST_INTX) {
> - disable_irq_nosync(irq);
> - assigned_dev->host_irq_disabled = true;
> + if (assigned_dev->irq_requested_type &
> + KVM_DEV_IRQ_GUEST_INTX) {
> + disable_irq_nosync(irq);
> + assigned_dev->host_irq_disabled = true;
> + }
> }
>
> out:
> @@ -147,33 +135,23 @@ static void deassign_guest_irq(struct kvm *kvm,
> assigned_dev->irq_requested_type &= ~(KVM_DEV_IRQ_GUEST_MASK);
> }
>
> -/* The function implicit hold kvm->lock mutex due to cancel_work_sync() */
> static void deassign_host_irq(struct kvm *kvm,
> struct kvm_assigned_dev_kernel *assigned_dev)
> {
> /*
> - * In kvm_free_device_irq, cancel_work_sync return true if:
> - * 1. work is scheduled, and then cancelled.
> - * 2. work callback is executed.
> - *
> - * The first one ensured that the irq is disabled and no more events
> - * would happen. But for the second one, the irq may be enabled (e.g.
> - * for MSI). So we disable irq here to prevent further events.
> + * We disable irq here to prevent further events.
> *
> * Notice this maybe result in nested disable if the interrupt type is
> * INTx, but it's OK for we are going to free it.
> *
> * If this function is a part of VM destroy, please ensure that till
> * now, the kvm state is still legal for probably we also have to wait
> - * interrupt_work done.
> + * on a currently running IRQ handler.
> */
> if (assigned_dev->irq_requested_type & KVM_DEV_IRQ_HOST_MSIX) {
> int i;
> for (i = 0; i < assigned_dev->entries_nr; i++)
> - disable_irq_nosync(assigned_dev->
> - host_msix_entries[i].vector);
> -
> - cancel_work_sync(&assigned_dev->interrupt_work);
> + disable_irq(assigned_dev->host_msix_entries[i].vector);
>
> for (i = 0; i < assigned_dev->entries_nr; i++)
> free_irq(assigned_dev->host_msix_entries[i].vector,
> @@ -185,8 +163,7 @@ static void deassign_host_irq(struct kvm *kvm,
> pci_disable_msix(assigned_dev->dev);
> } else {
> /* Deal with MSI and INTx */
> - disable_irq_nosync(assigned_dev->host_irq);
> - cancel_work_sync(&assigned_dev->interrupt_work);
> + disable_irq(assigned_dev->host_irq);
>
> free_irq(assigned_dev->host_irq, (void *)assigned_dev);
>
> @@ -558,8 +535,6 @@ static int kvm_vm_ioctl_assign_device(struct kvm *kvm,
> match->irq_source_id = -1;
> match->kvm = kvm;
> match->ack_notifier.irq_acked = kvm_assigned_dev_ack_irq;
> - INIT_WORK(&match->interrupt_work,
> - kvm_assigned_dev_interrupt_work_handler);
>
> list_add(&match->list, &kvm->arch.assigned_dev_head);
>
Attachment:
signature.asc
Description: OpenPGP digital signature
