| Message ID | 1234339731-3195-3-git-send-email-sheng@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
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@linux.intel.com> > --- > 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? > 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). > 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? > + 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. 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. > + 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? IRQ sharing in the host side is not supported correct? -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@linux.intel.com> > > --- > > 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...
On Fri, Feb 13, 2009 at 11:37:45AM +0800, Sheng Yang wrote: > > > +#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. OK > > 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. OK > > 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. Alright. > > 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). OK. > > > > IRQ sharing in the host side is not supported correct? > > Um? Yeah... And seems we won't support it forever... OK. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Sheng Yang wrote: >>> + 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). > gsi != irq... as used in kvm it is just a cookie and is never visible to the guest. I'd prefer an illegal value here. Also, please repost the entire patchset so I can be sure I apply the latest; too many versions floating around.
On Wednesday 18 February 2009 02:09:49 Avi Kivity wrote: > Sheng Yang wrote: > >>> + 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). > > gsi != irq... as used in kvm it is just a cookie and is never visible to > the guest. I'd prefer an illegal value here. > > > Also, please repost the entire patchset so I can be sure I apply the > latest; too many versions floating around. OK. And please don't forget there is a patchset named "Optimize and unify IOAPIC/MSI delivery" before this one. :)
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index a7d6123..c081867 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -144,6 +144,8 @@ struct kvm { #ifdef CONFIG_HAVE_KVM_IRQCHIP struct list_head irq_routing; /* of kvm_kernel_irq_routing_entry */ struct hlist_head mask_notifier_list; +#define KVM_MAX_IRQ_ROUTES 1024 + DECLARE_BITMAP(irq_routes_pending_bitmap, KVM_MAX_IRQ_ROUTES); #endif #ifdef KVM_ARCH_WANT_MMU_NOTIFIER @@ -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)) unsigned long irq_requested_type; int irq_source_id; int flags; @@ -502,8 +505,6 @@ static inline int mmu_notifier_retry(struct kvm_vcpu *vcpu, unsigned long mmu_se #ifdef CONFIG_HAVE_KVM_IRQCHIP -#define KVM_MAX_IRQ_ROUTES 1024 - int kvm_setup_default_irq_routing(struct kvm *kvm); int kvm_set_irq_routing(struct kvm *kvm, const struct kvm_irq_routing_entry *entries, 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; + } + + return irq; +} + +static u32 find_gsi_from_host_irq(struct kvm_assigned_dev_kernel *assigned_dev, + int irq) +{ + int i, entry; + u32 gsi; + 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; + gsi = 0; + for (i = 0; i < assigned_dev->entries_nr; i++) + if (irq == (host_msix_entries + i)->vector) { + entry = (host_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 == (guest_msix_entries + i)->entry) { + gsi = (guest_msix_entries + i)->vector; + break; + } + if (gsi == 0) { + printk(KERN_WARNING "Fail to find correlated MSI-X gsi!\n"); + return 0; + } + + return gsi; +} + static void kvm_assigned_dev_interrupt_work_handler(struct work_struct *work) { 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); + enable_irq(irq); + 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; + set_bit(gsi, kvm->irq_routes_pending_bitmap); + } schedule_work(&assigned_dev->interrupt_work);
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@linux.intel.com> --- include/linux/kvm_host.h | 5 +- virt/kvm/kvm_main.c | 103 ++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 103 insertions(+), 5 deletions(-)