Re: [PATCH 2/3] KVM: Add gsi_msg_pending_bitmap for MSI-X

[Marcelo Tosatti <mtosatti@redhat.com>]

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?