| Message ID | 1231324966-22286-6-git-send-email-sheng@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Wed, Jan 07, 2009 at 06:42:41PM +0800, Sheng Yang wrote: > Using kvm_set_irq to handle all interrupt injection. > > Signed-off-by: Sheng Yang <sheng@linux.intel.com> > --- > include/linux/kvm_host.h | 2 +- > virt/kvm/irq_comm.c | 79 +++++++++++++++++++++++++++++++++++++++++++-- > virt/kvm/kvm_main.c | 79 +++------------------------------------------- > 3 files changed, 81 insertions(+), 79 deletions(-) > > +static void gsi_dispatch(struct kvm *kvm, u32 gsi) > +{ > + int vcpu_id; > + struct kvm_vcpu *vcpu; > + struct kvm_ioapic *ioapic = ioapic_irqchip(kvm); > + struct kvm_gsi_route_entry *gsi_entry; > + int dest_id, vector, dest_mode, trig_mode, delivery_mode; > + u32 deliver_bitmask; > + > + BUG_ON(!ioapic); > + > + gsi_entry = kvm_find_gsi_route_entry(kvm, gsi); > + if (!gsi_entry) { > + printk(KERN_WARNING "kvm: fail to find correlated gsi entry\n"); > + return; > + } > + > +#ifdef CONFIG_X86 > + if (gsi_entry->type & KVM_GSI_ROUTE_MSI) { > + dest_id = (gsi_entry->msi.address_lo & MSI_ADDR_DEST_ID_MASK) > + >> MSI_ADDR_DEST_ID_SHIFT; > + vector = (gsi_entry->msi.data & MSI_DATA_VECTOR_MASK) > + >> MSI_DATA_VECTOR_SHIFT; > + dest_mode = test_bit(MSI_ADDR_DEST_MODE_SHIFT, > + (unsigned long *)&gsi_entry->msi.address_lo); > + trig_mode = test_bit(MSI_DATA_TRIGGER_SHIFT, > + (unsigned long *)&gsi_entry->msi.data); > + delivery_mode = test_bit(MSI_DATA_DELIVERY_MODE_SHIFT, > + (unsigned long *)&gsi_entry->msi.data); > + deliver_bitmask = kvm_ioapic_get_delivery_bitmask(ioapic, > + dest_id, dest_mode); > + /* IOAPIC delivery mode value is the same as MSI here */ > + switch (delivery_mode) { Sheng, This code seems to ignore the RH bit (MSI_ADDR_REDIRECTION_SHIFT): 4.Destination mode (DM) — This bit indicates whether the Destination ID field should be interpreted as logical or physical APIC ID for delivery of the lowest priority interrupt. If RH is 1 and DM is 0, the Destination ID field is in physical destination mode and only the processor in the system that has the matching APIC ID is considered for delivery of that interrupt (this means no re-direction). If RH is 1 and DM is 1, the Destination ID Field is interpreted as in logical destination mode and the redirection is limited to only those processors that are part of the logical group of processors based on the processor’s logical APIC ID and the Destination ID field in the message. The logical group of processors consists of those identified by matching the 8-bit Destination ID with the logical destination identified by the Destination Format Register and the Logical Destination Register in each local APIC. Is that intentional? -- 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 Thursday 08 January 2009 05:39:32 Marcelo Tosatti wrote: > On Wed, Jan 07, 2009 at 06:42:41PM +0800, Sheng Yang wrote: > > Using kvm_set_irq to handle all interrupt injection. > > > > Signed-off-by: Sheng Yang <sheng@linux.intel.com> > > --- > > include/linux/kvm_host.h | 2 +- > > virt/kvm/irq_comm.c | 79 > > +++++++++++++++++++++++++++++++++++++++++++-- virt/kvm/kvm_main.c | > > 79 +++------------------------------------------- 3 files changed, 81 > > insertions(+), 79 deletions(-) > > > > +static void gsi_dispatch(struct kvm *kvm, u32 gsi) > > +{ > > + int vcpu_id; > > + struct kvm_vcpu *vcpu; > > + struct kvm_ioapic *ioapic = ioapic_irqchip(kvm); > > + struct kvm_gsi_route_entry *gsi_entry; > > + int dest_id, vector, dest_mode, trig_mode, delivery_mode; > > + u32 deliver_bitmask; > > + > > + BUG_ON(!ioapic); > > + > > + gsi_entry = kvm_find_gsi_route_entry(kvm, gsi); > > + if (!gsi_entry) { > > + printk(KERN_WARNING "kvm: fail to find correlated gsi entry\n"); > > + return; > > + } > > + > > +#ifdef CONFIG_X86 > > + if (gsi_entry->type & KVM_GSI_ROUTE_MSI) { > > + dest_id = (gsi_entry->msi.address_lo & MSI_ADDR_DEST_ID_MASK) > > + >> MSI_ADDR_DEST_ID_SHIFT; > > + vector = (gsi_entry->msi.data & MSI_DATA_VECTOR_MASK) > > + >> MSI_DATA_VECTOR_SHIFT; > > + dest_mode = test_bit(MSI_ADDR_DEST_MODE_SHIFT, > > + (unsigned long *)&gsi_entry->msi.address_lo); > > + trig_mode = test_bit(MSI_DATA_TRIGGER_SHIFT, > > + (unsigned long *)&gsi_entry->msi.data); > > + delivery_mode = test_bit(MSI_DATA_DELIVERY_MODE_SHIFT, > > + (unsigned long *)&gsi_entry->msi.data); > > + deliver_bitmask = kvm_ioapic_get_delivery_bitmask(ioapic, > > + dest_id, dest_mode); > > + /* IOAPIC delivery mode value is the same as MSI here */ > > + switch (delivery_mode) { > > Sheng, > > This code seems to ignore the RH bit (MSI_ADDR_REDIRECTION_SHIFT): > > 4.Destination mode (DM) — This bit indicates whether the Destination > ID field should be interpreted as logical or physical APIC ID for > delivery of the lowest priority interrupt. If RH is 1 and DM is 0, > the Destination ID field is in physical destination mode and only the > processor in the system that has the matching APIC ID is considered > for delivery of that interrupt (this means no re-direction). If RH > is 1 and DM is 1, the Destination ID Field is interpreted as in > logical destination mode and the redirection is limited to only those > processors that are part of the logical group of processors based > on the processor’s logical APIC ID and the Destination ID field > in the message. The logical group of processors consists of those > identified by matching the 8-bit Destination ID with the logical > destination identified by the Destination Format Register and the > Logical Destination Register in each local APIC. > > Is that intentional? Um... Partly... For RH bits is almost always 1... OK, I would add another patch for this bit later.
On 07/01/09 18:42 +0800, Sheng Yang wrote: > Using kvm_set_irq to handle all interrupt injection. > > Signed-off-by: Sheng Yang <sheng@linux.intel.com> > --- > +static void gsi_dispatch(struct kvm *kvm, u32 gsi) ... > + case IOAPIC_FIXED: > + for (vcpu_id = 0; deliver_bitmask != 0; vcpu_id++) { > + if (!(deliver_bitmask & (1 << vcpu_id))) > + continue; > + deliver_bitmask &= ~(1 << vcpu_id); > + vcpu = ioapic->kvm->vcpus[vcpu_id]; > + if (vcpu) > + kvm_apic_set_irq(vcpu, vector, > + trig_mode); > + } > + break; > + default: > + break; > + } In cases such as the for() loop above, which are numerous in the patchset, I wonder if using bitops would be slightly better: case IOAPIC_FIXED: while (deliver_bitmask != 0) { vcpu_id = ffs(deliver_bitmask); __clear_bit(vcpu_id - 1, &deliver_bitmask); vcpu = ioapic->kvm->vcpus[vcpu_id - 1]; if (vcpu) kvm_apic_set_irq(vcpu, vector, trig_mode); } ; I did a quick check and the second example compiles to a more consise set of assembler instructions. The current code uses bitops in cases like this. Mike
On Thursday 08 January 2009 21:54:41 Mike Day wrote: > On 07/01/09 18:42 +0800, Sheng Yang wrote: > > Using kvm_set_irq to handle all interrupt injection. > > > > Signed-off-by: Sheng Yang <sheng@linux.intel.com> > > --- > > > > +static void gsi_dispatch(struct kvm *kvm, u32 gsi) > > ... > > > + case IOAPIC_FIXED: > > + for (vcpu_id = 0; deliver_bitmask != 0; vcpu_id++) { > > + if (!(deliver_bitmask & (1 << vcpu_id))) > > + continue; > > + deliver_bitmask &= ~(1 << vcpu_id); > > + vcpu = ioapic->kvm->vcpus[vcpu_id]; > > + if (vcpu) > > + kvm_apic_set_irq(vcpu, vector, > > + trig_mode); > > + } > > + break; > > + default: > > + break; > > + } > > In cases such as the for() loop above, which are numerous in the > patchset, I wonder if using bitops would be slightly better: > > case IOAPIC_FIXED: > while (deliver_bitmask != 0) { > vcpu_id = ffs(deliver_bitmask); > __clear_bit(vcpu_id - 1, &deliver_bitmask); > vcpu = ioapic->kvm->vcpus[vcpu_id - 1]; > if (vcpu) > kvm_apic_set_irq(vcpu, vector, > trig_mode); > } ; > > > I did a quick check and the second example compiles to a more > consise set of assembler instructions. The current code uses bitops in > cases like this. > Yes, that's what I did for bitmap changing in the following patches. Please refer to "[PATCH 10/10] KVM: bit ops for deliver_bitmap" I sent before.
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index eab9588..bfdaab9 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -351,7 +351,7 @@ struct kvm_gsi_route_entry { struct hlist_node link; }; -void kvm_set_irq(struct kvm *kvm, int irq_source_id, int irq, int level); +void kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 gsi, int level); void kvm_notify_acked_irq(struct kvm *kvm, unsigned gsi); void kvm_register_irq_ack_notifier(struct kvm *kvm, struct kvm_irq_ack_notifier *kian); diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c index f5e2d2c..e9fcd23 100644 --- a/virt/kvm/irq_comm.c +++ b/virt/kvm/irq_comm.c @@ -24,10 +24,81 @@ #include "ioapic.h" +#ifdef CONFIG_X86 +#include <asm/msidef.h> +#endif + +static void gsi_dispatch(struct kvm *kvm, u32 gsi) +{ + int vcpu_id; + struct kvm_vcpu *vcpu; + struct kvm_ioapic *ioapic = ioapic_irqchip(kvm); + struct kvm_gsi_route_entry *gsi_entry; + int dest_id, vector, dest_mode, trig_mode, delivery_mode; + u32 deliver_bitmask; + + BUG_ON(!ioapic); + + gsi_entry = kvm_find_gsi_route_entry(kvm, gsi); + if (!gsi_entry) { + printk(KERN_WARNING "kvm: fail to find correlated gsi entry\n"); + return; + } + +#ifdef CONFIG_X86 + if (gsi_entry->type & KVM_GSI_ROUTE_MSI) { + dest_id = (gsi_entry->msi.address_lo & MSI_ADDR_DEST_ID_MASK) + >> MSI_ADDR_DEST_ID_SHIFT; + vector = (gsi_entry->msi.data & MSI_DATA_VECTOR_MASK) + >> MSI_DATA_VECTOR_SHIFT; + dest_mode = test_bit(MSI_ADDR_DEST_MODE_SHIFT, + (unsigned long *)&gsi_entry->msi.address_lo); + trig_mode = test_bit(MSI_DATA_TRIGGER_SHIFT, + (unsigned long *)&gsi_entry->msi.data); + delivery_mode = test_bit(MSI_DATA_DELIVERY_MODE_SHIFT, + (unsigned long *)&gsi_entry->msi.data); + deliver_bitmask = kvm_ioapic_get_delivery_bitmask(ioapic, + dest_id, dest_mode); + /* IOAPIC delivery mode value is the same as MSI here */ + switch (delivery_mode) { + case IOAPIC_LOWEST_PRIORITY: + vcpu = kvm_get_lowest_prio_vcpu(ioapic->kvm, vector, + deliver_bitmask); + if (vcpu != NULL) + kvm_apic_set_irq(vcpu, vector, trig_mode); + else + printk(KERN_INFO + "kvm: null lowest priority vcpu!\n"); + break; + case IOAPIC_FIXED: + for (vcpu_id = 0; deliver_bitmask != 0; vcpu_id++) { + if (!(deliver_bitmask & (1 << vcpu_id))) + continue; + deliver_bitmask &= ~(1 << vcpu_id); + vcpu = ioapic->kvm->vcpus[vcpu_id]; + if (vcpu) + kvm_apic_set_irq(vcpu, vector, + trig_mode); + } + break; + default: + break; + } + } +#endif /* CONFIG_X86 */ +} + /* This should be called with the kvm->lock mutex held */ -void kvm_set_irq(struct kvm *kvm, int irq_source_id, int irq, int level) +void kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 gsi, int level) { - unsigned long *irq_state = (unsigned long *)&kvm->arch.irq_states[irq]; + unsigned long *irq_state; + + if (gsi & KVM_GSI_ROUTE_MASK) { + gsi_dispatch(kvm, gsi); + return; + } + + irq_state = (unsigned long *)&kvm->arch.irq_states[gsi]; /* Logical OR for level trig interrupt */ if (level) @@ -39,9 +110,9 @@ void kvm_set_irq(struct kvm *kvm, int irq_source_id, int irq, int level) * IOAPIC. So set the bit in both. The guest will ignore * writes to the unused one. */ - kvm_ioapic_set_irq(ioapic_irqchip(kvm), irq, !!(*irq_state)); + kvm_ioapic_set_irq(ioapic_irqchip(kvm), gsi, !!(*irq_state)); #ifdef CONFIG_X86 - kvm_pic_set_irq(pic_irqchip(kvm), irq, !!(*irq_state)); + kvm_pic_set_irq(pic_irqchip(kvm), gsi, !!(*irq_state)); #endif } diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 717e1b0..60352cf 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -47,10 +47,6 @@ #include <asm/uaccess.h> #include <asm/pgtable.h> -#ifdef CONFIG_X86 -#include <asm/msidef.h> -#endif - #ifdef KVM_COALESCED_MMIO_PAGE_OFFSET #include "coalesced_mmio.h" #endif @@ -85,69 +81,6 @@ static long kvm_vcpu_ioctl(struct file *file, unsigned int ioctl, static bool kvm_rebooting; #ifdef KVM_CAP_DEVICE_ASSIGNMENT - -#ifdef CONFIG_X86 -static void assigned_device_msi_dispatch(struct kvm_assigned_dev_kernel *dev, u32 gsi) -{ - int vcpu_id; - struct kvm_vcpu *vcpu; - struct kvm_ioapic *ioapic = ioapic_irqchip(dev->kvm); - struct kvm_gsi_route_entry *gsi_entry; - int dest_id, vector, dest_mode, trig_mode, delivery_mode; - u32 deliver_bitmask; - - BUG_ON(!ioapic); - - gsi_entry = kvm_find_gsi_route_entry(dev->kvm, gsi); - if (!gsi_entry) { - printk(KERN_WARNING "kvm: fail to find correlated gsi entry\n"); - return; - } - - if (gsi_entry->type & KVM_GSI_ROUTE_MSI) { - dest_id = (gsi_entry->msi.address_lo & MSI_ADDR_DEST_ID_MASK) - >> MSI_ADDR_DEST_ID_SHIFT; - vector = (gsi_entry->msi.data & MSI_DATA_VECTOR_MASK) - >> MSI_DATA_VECTOR_SHIFT; - dest_mode = test_bit(MSI_ADDR_DEST_MODE_SHIFT, - (unsigned long *)&gsi_entry->msi.address_lo); - trig_mode = test_bit(MSI_DATA_TRIGGER_SHIFT, - (unsigned long *)&gsi_entry->msi.data); - delivery_mode = test_bit(MSI_DATA_DELIVERY_MODE_SHIFT, - (unsigned long *)&gsi_entry->msi.data); - deliver_bitmask = kvm_ioapic_get_delivery_bitmask(ioapic, - dest_id, dest_mode); - /* IOAPIC delivery mode value is the same as MSI here */ - switch (delivery_mode) { - case IOAPIC_LOWEST_PRIORITY: - vcpu = kvm_get_lowest_prio_vcpu(ioapic->kvm, vector, - deliver_bitmask); - if (vcpu != NULL) - kvm_apic_set_irq(vcpu, vector, trig_mode); - else - printk(KERN_INFO - "kvm: null lowest priority vcpu!\n"); - break; - case IOAPIC_FIXED: - for (vcpu_id = 0; deliver_bitmask != 0; vcpu_id++) { - if (!(deliver_bitmask & (1 << vcpu_id))) - continue; - deliver_bitmask &= ~(1 << vcpu_id); - vcpu = ioapic->kvm->vcpus[vcpu_id]; - if (vcpu) - kvm_apic_set_irq(vcpu, vector, - trig_mode); - } - break; - default: - break; - } - } -} -#else -static void assigned_device_msi_dispatch(struct kvm_assigned_dev_kernel *dev, u32 gsi) {} -#endif - static struct kvm_assigned_dev_kernel *kvm_find_assigned_dev(struct list_head *head, int assigned_dev_id) { @@ -174,13 +107,11 @@ static void kvm_assigned_dev_interrupt_work_handler(struct work_struct *work) * finer-grained lock, update this */ mutex_lock(&assigned_dev->kvm->lock); - if (assigned_dev->irq_requested_type & KVM_ASSIGNED_DEV_GUEST_INTX) - kvm_set_irq(assigned_dev->kvm, - assigned_dev->irq_source_id, - assigned_dev->guest_irq, 1); - else if (assigned_dev->irq_requested_type & - KVM_ASSIGNED_DEV_GUEST_MSI) { - assigned_device_msi_dispatch(assigned_dev, assigned_dev->guest_irq); + + kvm_set_irq(assigned_dev->kvm, assigned_dev->irq_source_id, + assigned_dev->guest_irq, 1); + + if (assigned_dev->irq_requested_type & KVM_ASSIGNED_DEV_GUEST_MSI) { enable_irq(assigned_dev->host_irq); assigned_dev->host_irq_disabled = false; }
Using kvm_set_irq to handle all interrupt injection. Signed-off-by: Sheng Yang <sheng@linux.intel.com> --- include/linux/kvm_host.h | 2 +- virt/kvm/irq_comm.c | 79 +++++++++++++++++++++++++++++++++++++++++++-- virt/kvm/kvm_main.c | 79 +++------------------------------------------- 3 files changed, 81 insertions(+), 79 deletions(-)