On Friday 13 February 2009 03:51:18 Marcelo Tosatti wrote:
> Hi Sheng,
>
> On Wed, Feb 11, 2009 at 04:08:50PM +0800, Sheng Yang wrote:
> > We have to handle more than one interrupt with one handler for MSI-X. So
> > we need a bitmap to track the triggered interrupts.
> >
> > Signed-off-by: Sheng Yang <sheng@xxxxxxxxxxxxxxx>
> > ---
> > include/linux/kvm_host.h | 5 +-
> > virt/kvm/kvm_main.c | 103
> > ++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 103
> > insertions(+), 5 deletions(-)
> >
> >
> > @@ -335,6 +337,7 @@ struct kvm_assigned_dev_kernel {
> > #define KVM_ASSIGNED_DEV_GUEST_MSI (1 << 1)
> > #define KVM_ASSIGNED_DEV_HOST_INTX (1 << 8)
> > #define KVM_ASSIGNED_DEV_HOST_MSI (1 << 9)
> > +#define KVM_ASSIGNED_DEV_MSIX ((1 << 2) | (1 << 10))
>
> Can you explain the usage of the two bits?
Um... Just to keep consistent with formers(one for guest and one for host), at
cost of one bit.
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index ea96690..961603f 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -95,25 +95,113 @@ static struct kvm_assigned_dev_kernel
> > *kvm_find_assigned_dev(struct list_head *h return NULL;
> > }
> >
> > +static int find_host_irq_from_gsi(struct kvm_assigned_dev_kernel
> > *assigned_dev, + u32 gsi)
> > +{
> > + int i, entry, irq;
> > + struct msix_entry *host_msix_entries, *guest_msix_entries;
> > +
> > + host_msix_entries = assigned_dev->host_msix_entries;
> > + guest_msix_entries = assigned_dev->guest_msix_entries;
> > +
> > + entry = -1;
> > + irq = 0;
> > + for (i = 0; i < assigned_dev->entries_nr; i++)
> > + if (gsi == (guest_msix_entries + i)->vector) {
> > + entry = (guest_msix_entries + i)->entry;
> > + break;
> > + }
> > + if (entry < 0) {
> > + printk(KERN_WARNING "Fail to find correlated MSI-X entry!\n");
> > + return 0;
> > + }
> > + for (i = 0; i < assigned_dev->entries_nr; i++)
> > + if (entry == (host_msix_entries + i)->entry) {
> > + irq = (host_msix_entries + i)->vector;
> > + break;
> > + }
> > + if (irq == 0) {
> > + printk(KERN_WARNING "Fail to find correlated MSI-X irq!\n");
> > + return 0;
> > + }
>
> Can drop the printk's (also from find_gsi_from_host_irq).
Not that confident. In fact, I often triggered this during debug... IIRC,
userspace program shouldn't trigger this if kernel space works well. Maybe it
can be changed to WARN_ON() or BUG_ON() later.
>
> > struct kvm_assigned_dev_kernel *assigned_dev;
> > + struct kvm *kvm;
> > + u32 gsi;
> > + int irq;
> >
> > assigned_dev = container_of(work, struct kvm_assigned_dev_kernel,
> > interrupt_work);
> > + kvm = assigned_dev->kvm;
> >
> > /* 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(&assigned_dev->kvm->lock);
> > - kvm_set_irq(assigned_dev->kvm, assigned_dev->irq_source_id,
> > - assigned_dev->guest_irq, 1);
> > + mutex_lock(&kvm->lock);
> > +handle_irq:
> > + if (assigned_dev->irq_requested_type & KVM_ASSIGNED_DEV_MSIX) {
> > + gsi = find_first_bit(kvm->irq_routes_pending_bitmap,
> > + KVM_MAX_IRQ_ROUTES);
> > + BUG_ON(gsi >= KVM_MAX_IRQ_ROUTES);
> > + clear_bit(gsi, kvm->irq_routes_pending_bitmap);
> > + } else
> > + gsi = assigned_dev->guest_irq;
> > +
> > + kvm_set_irq(assigned_dev->kvm, assigned_dev->irq_source_id, gsi, 1);
> >
> > if (assigned_dev->irq_requested_type & KVM_ASSIGNED_DEV_GUEST_MSI) {
> > enable_irq(assigned_dev->host_irq);
> > assigned_dev->host_irq_disabled = false;
> > + } else if (assigned_dev->irq_requested_type & KVM_ASSIGNED_DEV_MSIX) {
> > + irq = find_host_irq_from_gsi(assigned_dev, gsi);
>
> Check the return value?
Yeah...
>
> > + enable_irq(irq);
>
> Do you guarantee that particular irq you're enable_irq'ing is not bogus?
> Its has been passed from userspace after all.
It isn't passed from userspace. This one is filled by pci_enable_msix(), which
should be OK.
>
> In a later patch you can assign KVM_ASSIGNED_DEV_MSIX if the irqchip is
> not in-kernel in assigned_device_update_msix:
>
> + adev->irq_requested_type |= KVM_ASSIGNED_DEV_MSIX;
> +
>
> Just trying to harden the code against bogosity elsewhere.
Yeah, of course. :)
>
> > + assigned_dev->host_irq_disabled = false;
> > + gsi = find_first_bit(kvm->irq_routes_pending_bitmap,
> > + KVM_MAX_IRQ_ROUTES);
> > + if (gsi < KVM_MAX_IRQ_ROUTES)
> > + goto handle_irq;
> > }
> > +
> > mutex_unlock(&assigned_dev->kvm->lock);
> > }
> >
> > @@ -121,6 +209,15 @@ static irqreturn_t kvm_assigned_dev_intr(int irq,
> > void *dev_id) {
> > struct kvm_assigned_dev_kernel *assigned_dev =
> > (struct kvm_assigned_dev_kernel *) dev_id;
> > + struct kvm *kvm = assigned_dev->kvm;
> > +
> > + if (assigned_dev->irq_requested_type == KVM_ASSIGNED_DEV_MSIX) {
> > + u32 gsi;
> > + gsi = find_gsi_from_host_irq(assigned_dev, irq);
> > + if (gsi == 0)
> > + return IRQ_HANDLED;
>
> So you chose GSI == 0 as invalid because of x86 assumptions? Or is there
> any other reason?
Yeah, it based on x86 and IA64 IRQ 0 can't be used by MSI-X. And only x86
support MSI-X now(and IA64 would follow later).
>
> IRQ sharing in the host side is not supported correct?
Um? Yeah... And seems we won't support it forever...
--
regards
Yang, Sheng
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
