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

[Jan Kiszka <jan.kiszka@siemens.com>]

Am 02.11.2010 19:24, Michael S. Tsirkin wrote:
> On Tue, Nov 02, 2010 at 06:56:14PM +0100, Jan Kiszka wrote:
>>>>  		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.

OK. Would require some serious refactoring again.

But what about edge IRQs? Don't we need to toggle the bit for them? And
as we do not differentiate between level and edge, we currently have to
do this unconditionally.

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

Or an atomic counter. Will have a look.

Alex, does VFIO take care of this already?

> 
>>>
>>>>  }
>>>>  
>>>>  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?
> 

Can't reproduce anymore. This was early code, maybe affected by some
bits or buts that no longer exist.

Spec says it's cleared on reset, so I removed those lines now.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux