Re: [PATCH 3/8] KVM: Handle device assignment to guests

Ben-Ami Yassour
2008-07-17T08:25:45Z

On Thu, 2008-07-17 at 09:02 +0300, Avi Kivity wrote:
> Han, Weidong wrote:
> > Avi Kivity wrote:
> >
> >>> +static void kvm_pci_pt_work_fn(struct work_struct *work) +{
> >>> + struct kvm_pci_pt_dev_list *match;
> >>> + struct kvm_pci_pt_work *int_work;
> >>> + int source;
> >>> + unsigned long flags;
> >>> + int guest_irq;
> >>> + int host_irq;
> >>> +
> >>> + int_work = container_of(work, struct kvm_pci_pt_work, work); +
> >>> + source = int_work->source ? KVM_PT_SOURCE_IRQ_ACK :
> >>> KVM_PT_SOURCE_IRQ; + + /* This is taken to safely inject irq
> >>>
> > inside
> >
> >>> the guest. When + * the interrupt injection (or the ioapic code)
> >>> uses a + * finer-grained lock, update this
> >>> + */
> >>> + mutex_lock(&int_work->kvm->lock);
> >>> + read_lock_irqsave(&kvm_pci_pt_lock, flags);
> >>> + match =
> >>>
> > kvm_find_pci_pt_dev(&int_work->kvm->arch.pci_pt_dev_head,
> >
> >>> NULL, + int_work->irq, source);
> >>> + if (!match) {
> >>> + printk(KERN_ERR "%s: no matching device assigned to
> >>>
> > guest "
> >
> >>> + "found for irq %d, source = %d!\n",
> >>> + __func__, int_work->irq, int_work->source);
> >>> + read_unlock_irqrestore(&kvm_pci_pt_lock, flags); +
> >>>
> > goto out;
> >
> >>> + }
> >>> + guest_irq = match->pt_dev.guest.irq;
> >>> + host_irq = match->pt_dev.host.irq;
> >>> + read_unlock_irqrestore(&kvm_pci_pt_lock, flags);
> >>> +
> >>> + if (source == KVM_PT_SOURCE_IRQ)
> >>> + kvm_set_irq(int_work->kvm, guest_irq, 1);
> >>> + else {
> >>> + kvm_set_irq(int_work->kvm, int_work->irq, 0);
> >>> + enable_irq(host_irq);
> >>> + }
> >>> +out:
> >>> + mutex_unlock(&int_work->kvm->lock);
> >>> + kvm_put_kvm(int_work->kvm);
> >>> +}
> >>>
> >>> +
> >>> +/* FIXME: Implement the OR logic needed to make shared interrupts
> >>> on + * this line behave properly + */
> >>>
> >>>
> >> Isn't this a showstopper? There is no easy way for a user to avoid
> >> sharing, especially as we have only three pci irqs at present.
What do you mean by only 3 (for passthrough we use whatever interrupt
the host is using for that device)?

> >>
> >>
> >
> > Currently it's not easy to avoid sharing. I think we can support MSI for
> > assgined device to solve sharing problem.
> >
>
> MSI is definitely the right direction, but we also need to support
> the
> OR logic for guests that do not support MSI.
>

The main problems that I am ware of with shared interrupt are as
follows:

1. Protection
A bad guest can keep the interrupt line up, and if its shared then it
interferes with other devices that might not be assigned to this guest.

﻿2. Performance
The current implementation disables the irq when the interrupt is
received on the host and re-enables it when the guest acks the interrupt
on the virtual ioapic.

Clearly its not nice to do that when other devices uses the same irq...

When removing the enable-disable of the irq, it causes serious
performance degradation.
The throughput goes down from 700 to 400 Mb/sec.
The number of interrupts per second on the host (from the NIC) is 130000
(compared to 22200 before).

Regardless, I think that the right way to go would be to merge the
current code (with the non-shared interrupt limitation) and then
continue to work on additional features like shared interrupts support.
Do you agree?

Regards,
Ben