Re: [PATCH v4] KVM: Allow host IRQ sharing for assigned PCI 2.3 devices

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

On 2012-02-29 16:22, Alex Williamson wrote:
> On Tue, 2012-02-28 at 14:19 +0100, Jan Kiszka wrote:
>> PCI 2.3 allows to generically disable IRQ sources at device level. This
>> enables us to share legacy IRQs of such devices with other host devices
>> when passing them to a guest.
>>
>> The new IRQ sharing feature introduced here is optional, user space has
>> to request it explicitly.
> 
> Is this really true?  Looks like it's automatic.

It is true: no IRQ sharing without userspace setting
KVM_DEV_ASSIGN_PCI_2_3 during KVM_ASSIGN_PCI_DEVICE. My qemu-kvm patches
will allow to control this, but make it default on (reasons for this
will be provided in that context).

> 
>>  Moreover, user space can inform us about its
>> view of PCI_COMMAND_INTX_DISABLE so that we can avoid unmasking the
>> interrupt and signaling it if the guest masked it via the virtualized
>> PCI config space.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>
>> Changes in v4:
>>  - Integrated doc changes as proposed by Alex
>>  - Fixed deassign_host_irq /wrt MSI
>>  - Fixed kvm_vm_ioctl_set_pci_irq_mask /wrt INTx unmasking of non-2.3
>>    devices
>>
>>  Documentation/virtual/kvm/api.txt |   41 +++++++
>>  arch/x86/kvm/x86.c                |    1 +
>>  include/linux/kvm.h               |    6 +
>>  include/linux/kvm_host.h          |    2 +
>>  virt/kvm/assigned-dev.c           |  209 +++++++++++++++++++++++++++++++-----
>>  5 files changed, 230 insertions(+), 29 deletions(-)
> [snip]
>> +static int kvm_vm_ioctl_set_pci_irq_mask(struct kvm *kvm,
>> +		struct kvm_assigned_pci_dev *assigned_dev)
>> +{
>> +	int r = 0;
>> +	struct kvm_assigned_dev_kernel *match;
>> +
>> +	mutex_lock(&kvm->lock);
>> +
>> +	match = kvm_find_assigned_dev(&kvm->arch.assigned_dev_head,
>> +				      assigned_dev->assigned_dev_id);
>> +	if (!match) {
>> +		r = -ENODEV;
>> +		goto out;
>> +	}
>> +
>> +	mutex_lock(&match->intx_mask_lock);
>> +
>> +	match->flags &= ~KVM_DEV_ASSIGN_MASK_INTX;
>> +	match->flags |= assigned_dev->flags & KVM_DEV_ASSIGN_MASK_INTX;
>> +
>> +	if (match->irq_requested_type & KVM_DEV_IRQ_GUEST_INTX) {
>> +		if (assigned_dev->flags & KVM_DEV_ASSIGN_MASK_INTX) {
>> +			kvm_set_irq(match->kvm, match->irq_source_id,
>> +				    match->guest_irq, 0);
>> +			/*
>> +			 * Masking at hardware-level is performed on demand,
>> +			 * i.e. when an IRQ actually arrives at the host.
>> +			 */
>> +		} else if (!(assigned_dev->flags & KVM_DEV_ASSIGN_PCI_2_3)) {
>> +			/*
>> +			 * Unmask the IRQ line if required. Unmasking at
>> +			 * device level will be performed by user space.
>> +			 */
>> +			spin_lock_irq(&match->intx_lock);
>> +			if (match->host_irq_disabled) {
>> +				enable_irq(match->host_irq);
>> +				match->host_irq_disabled = false;
>> +			}
>> +			spin_unlock_irq(&match->intx_lock);
> 
> This still looks broken.  If we start with a PCI 2.3 device with INTx
> disabled, how does this ever kick start to get another interrupt?
> Shouldn't we just call kvm_assigned_dev_ack_irq() here and handle both
> INTx modes?  Thanks,

No, that would be wrong. The IRQ must be delivered to the guest first.

And that will happen with 2.3 once userspace unmasks the device INTx
and, thus, triggers another host-side IRQ. Same for non-2.3 devices,
just that this happens on enable_irq.

Jan

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