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

Reply via email to