Re: [PATCH v2 4/4] KVM: Allow host IRQ sharing for passed-through PCI 2.3 devices

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Nov 02, 2010 at 06:56:14PM +0100, Jan Kiszka wrote:
> >> @@ -99,12 +228,23 @@ static void kvm_assigned_dev_ack_irq(struct kvm_irq_ack_notifier *kian)
> >>  	/* The guest irq may be shared so this ack may be
> >>  	 * from another device.
> >>  	 */
> >> -	spin_lock(&dev->intx_lock);
> >> +	spin_lock_irq(&dev->intx_lock);
> >>  	if (dev->host_irq_disabled) {
> >> -		enable_irq(dev->host_irq);
> >> +		if (dev->pci_2_3) {
> >> +			if (pci_2_3_irq_check_and_unmask(dev->dev) &
> >> +			    PCI_STATUS_INTERRUPT) {
> >> +				reassert = true;
> >> +				goto out;
> >> +			}
> >> +		} else
> >> +			enable_irq(dev->host_irq);
> > 
> > Or
> > 
> > 		if (!dev->pci_2_3)
> > 			enable_irq(dev->host_irq);
> > 		else if (pci_2_3_irq_check_and_unmask(dev->dev) & PCI_STATUS_INTERRUPT) {
> > 			reassert = true;
> > 			goto out;
> > 		}
> > 
> > to reduce nesting.
> 
> Yeah.
> 
> > 
> >>  		dev->host_irq_disabled = false;
> >>  	}
> >> -	spin_unlock(&dev->intx_lock);
> >> +out:
> >> +	spin_unlock_irq(&dev->intx_lock);
> >> +
> >> +	if (reassert)
> >> +		kvm_set_irq(dev->kvm, dev->irq_source_id, dev->guest_irq, 1);
> > 
> > Hmm, I think this still has more overhead than it needs to have.
> > Instead of setting level to 0 and then back to 1, can't we just
> > avoid set to 1 in the first place? This would need a different
> > interface than pci_2_3_irq_check_and_unmask to avoid a race
> > where interrupt is received while we are acking another one:
> > 
> > 	block userspace access
> > 	check pending bit
> > 	if (!pending)
> > 		set irq (0)
> > 	clear pending
> > 	block userspace access
> > 
> > Would be worth it for high volume devices.
> 
> The problem is that we can't reorder guest IRQ line clearing and host
> IRQ line enabling without taking a lock across host IRQ disable + guest
> IRQ raise - and that is now distributed across hard and threaded IRQ
> handlers and we don't want to hold and IRQ-safe lock during kvm_set_irq.

Oh I think I confused you.
What I mean is:

 	block userspace access
 	check interrupt status bit
 	if (!interrupt status bit set)
 		set irq (0)
 	clear interrupt disable bit
 	block userspace access

This way we enable interrupt after set irq so not need for
extra locks I think.

Hmm one thing I noticed is that pci_block_user_cfg_access
will BUG_ON if it was already blocked. So I think we have
a bug here when interrupt handler kicks in right after
we unmask interrupts.

Probably need some kind of lock to protect against this.


> > 
> >>  }
> >>  
> >>  static void deassign_guest_irq(struct kvm *kvm,
> >> @@ -151,7 +291,11 @@ static void deassign_host_irq(struct kvm *kvm,
> >>  		pci_disable_msix(assigned_dev->dev);
> >>  	} else {
> >>  		/* Deal with MSI and INTx */
> >> -		disable_irq(assigned_dev->host_irq);
> >> +		if (assigned_dev->pci_2_3) {
> >> +			pci_2_3_irq_mask(assigned_dev->dev);
> >> +			synchronize_irq(assigned_dev->host_irq);
> >> +		} else
> >> +			disable_irq(assigned_dev->host_irq);
> >>  
> >>  		free_irq(assigned_dev->host_irq, (void *)assigned_dev);
> >>  
> >> @@ -199,6 +343,13 @@ static void kvm_free_assigned_device(struct kvm *kvm,
> >>  
> >>  	pci_reset_function(assigned_dev->dev);
> >>  
> >> +	/*
> >> +	 * Unmask the IRQ at PCI level once the reset is done - the next user
> >> +	 * may not expect the IRQ being masked.
> >> +	 */
> >> +	if (assigned_dev->pci_2_3)
> >> +		pci_2_3_irq_unmask(assigned_dev->dev);
> >> +
> > 
> > Doesn't pci_reset_function clear mask bit? It seems to ...
> 
> I was left with non-functional devices for the host here if I was not
> doing this. Need to recheck, but I think it was required.

Interesting. Could you check why please?


> > 
> >>  	pci_release_regions(assigned_dev->dev);
> >>  	pci_disable_device(assigned_dev->dev);
> >>  	pci_dev_put(assigned_dev->dev);
> >> @@ -224,15 +375,29 @@ void kvm_free_all_assigned_devices(struct kvm *kvm)
> >>  static int assigned_device_enable_host_intx(struct kvm *kvm,
> >>  					    struct kvm_assigned_dev_kernel *dev)
> >>  {
> >> +	irq_handler_t irq_handler = NULL;
> >> +	unsigned long flags = 0;
> >> +
> >>  	dev->host_irq = dev->dev->irq;
> >> -	/* Even though this is PCI, we don't want to use shared
> >> -	 * interrupts. Sharing host devices with guest-assigned devices
> >> -	 * on the same interrupt line is not a happy situation: there
> >> -	 * are going to be long delays in accepting, acking, etc.
> >> +
> >> +	/*
> >> +	 * We can only share the IRQ line with other host devices if we are
> >> +	 * able to disable the IRQ source at device-level - independently of
> >> +	 * the guest driver. Otherwise host devices may suffer from unbounded
> >> +	 * IRQ latencies when the guest keeps the line asserted.
> >>  	 */
> >> -	if (request_threaded_irq(dev->host_irq, NULL, kvm_assigned_dev_thread,
> >> -				 0, dev->irq_name, (void *)dev))
> >> +	dev->pci_2_3 = pci_2_3_supported(dev->dev);
> >> +	if (dev->pci_2_3) {
> >> +		irq_handler = kvm_assigned_dev_intr;
> >> +		flags = IRQF_SHARED;
> >> +	}
> > 
> > I would prefer and else clause here instead of initializing
> > variables at the top and overwriting here. Makes it easier
> > to see which value is valid when.
> 
> OK.
> 
> Thanks,
> Jan
> 
> -- 
> Siemens AG, Corporate Technology, CT T DE IT 1
> Corporate Competence Center Embedded Linux