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?
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
