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

[Sheng Yang <sheng@linux.intel.com>]

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...

-- 
regards
Yang, Sheng