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 04:49:20PM +0100, Jan Kiszka wrote:
> PCI 2.3 allows to generically disable IRQ sources at device level. This
> enables us to share IRQs of such devices between on the host side when
> passing them to a guest.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  include/linux/kvm_host.h |    1 +
>  virt/kvm/assigned-dev.c  |  194 ++++++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 180 insertions(+), 15 deletions(-)
> 
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 46120da..fdc2bd9 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -466,6 +466,7 @@ struct kvm_assigned_dev_kernel {
>  	unsigned int entries_nr;
>  	int host_irq;
>  	bool host_irq_disabled;
> +	bool pci_2_3;
>  	struct msix_entry *host_msix_entries;
>  	int guest_irq;
>  	struct msix_entry *guest_msix_entries;
> diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
> index ca402ed..91fe9c8 100644
> --- a/virt/kvm/assigned-dev.c
> +++ b/virt/kvm/assigned-dev.c
> @@ -55,17 +55,145 @@ static int find_index_from_host_irq(struct kvm_assigned_dev_kernel
>  	return index;
>  }
>  
> +/*
> + * Verify that the device supports Interrupt Disable bit in command register,
> + * per PCI 2.3, by flipping this bit and reading it back: this bit was readonly
> + * in PCI 2.2.
> + */
> +static bool pci_2_3_supported(struct pci_dev *pdev)
> +{
> +	bool supported = false;
> +	u16 orig, new;
> +
> +	pci_block_user_cfg_access(pdev);
> +	pci_read_config_word(pdev, PCI_COMMAND, &orig);
> +	pci_write_config_word(pdev, PCI_COMMAND,
> +			      orig ^ PCI_COMMAND_INTX_DISABLE);
> +	pci_read_config_word(pdev, PCI_COMMAND, &new);
> +
> +	/*
> +	 * There's no way to protect against
> +	 * hardware bugs or detect them reliably, but as long as we know
> +	 * what the value should be, let's go ahead and check it.
> +	 */
> +	if ((new ^ orig) & ~PCI_COMMAND_INTX_DISABLE) {
> +		dev_err(&pdev->dev, "Command changed from 0x%x to 0x%x: "
> +			"driver or HW bug?\n", orig, new);
> +		goto out;
> +	}
> +	if (!((new ^ orig) & PCI_COMMAND_INTX_DISABLE)) {
> +		dev_warn(&pdev->dev, "Device does not support "
> +			 "disabling interrupts: unable to bind.\n");
> +		goto out;
> +	}
> +	supported = true;
> +
> +	/* Now restore the original value. */
> +	pci_write_config_word(pdev, PCI_COMMAND, orig);
> +
> +out:
> +	pci_unblock_user_cfg_access(pdev);
> +	return supported;
> +}
> +
> +static unsigned int
> +pci_2_3_set_irq_mask(struct pci_dev *dev, bool mask, bool check_status)
> +{
> +	u32 cmd_status_dword;
> +	u16 origcmd, newcmd;
> +	unsigned int status;
> +
> +	/*
> +	 * We do a single dword read to retrieve both command and status.
> +	 * Document assumptions that make this possible.
> +	 */
> +	BUILD_BUG_ON(PCI_COMMAND % 4);
> +	BUILD_BUG_ON(PCI_COMMAND + 2 != PCI_STATUS);
> +
> +	pci_block_user_cfg_access(dev);
> +
> +	/*
> +	 * Read both command and status registers in a single 32-bit operation.
> +	 * Note: we could cache the value for command and move the status read
> +	 * out of the lock if there was a way to get notified of user changes
> +	 * to command register through sysfs. Should be good for shared irqs.
> +	 */
> +	pci_read_config_dword(dev, PCI_COMMAND, &cmd_status_dword);
> +	origcmd = cmd_status_dword;
> +	status = cmd_status_dword >> 16;
> +
> +	if (check_status) {
> +		bool irq_pending = status & PCI_STATUS_INTERRUPT;
> +
> +		/*
> +		 * Check interrupt status register to see whether our device
> +		 * triggered the interrupt (when masking) or the next IRQ is
> +		 * already pending (when unmasking).
> +		 */
> +		if (!(mask == irq_pending))

Same as mask != irq_pending?

> +			goto done;
> +	}
> +
> +	newcmd = origcmd & ~PCI_COMMAND_INTX_DISABLE;
> +	if (mask)
> +		newcmd |= PCI_COMMAND_INTX_DISABLE;
> +	if (newcmd != origcmd)
> +		pci_write_config_word(dev, PCI_COMMAND, newcmd);
> +
> +done:
> +	pci_unblock_user_cfg_access(dev);
> +	return status;
> +}
> +
> +static void pci_2_3_irq_mask(struct pci_dev *dev)
> +{
> +	pci_2_3_set_irq_mask(dev, true, false);
> +}
> +
> +static unsigned int pci_2_3_irq_check_and_mask(struct pci_dev *dev)
> +{
> +	return pci_2_3_set_irq_mask(dev, true, true);
> +}
> +
> +static void pci_2_3_irq_unmask(struct pci_dev *dev)
> +{
> +	pci_2_3_set_irq_mask(dev, false, false);
> +}
> +
> +static unsigned int pci_2_3_irq_check_and_unmask(struct pci_dev *dev)
> +{
> +	return pci_2_3_set_irq_mask(dev, false, true);
> +}
> +

IMO this is not a terribly good interface: all users check the pending bit
(PCI_STATUS_INTERRUPT) which is what the function pci_2_3_set_irq_mask
did anyway.  I'd suggest returning irqreturn_t or bool and not unsigned
int.


> +static irqreturn_t kvm_assigned_dev_intr(int irq, void *dev_id)
> +{
> +	struct kvm_assigned_dev_kernel *assigned_dev = dev_id;
> +	int ret = IRQ_WAKE_THREAD;
> +
> +	spin_lock(&assigned_dev->intx_lock);
> +	if (assigned_dev->host_irq_disabled ||
> +	    !(pci_2_3_irq_check_and_mask(assigned_dev->dev) &
> +			PCI_STATUS_INTERRUPT))
> +		ret = IRQ_NONE;
> +	else
> +		assigned_dev->host_irq_disabled = true;

This is a bug I think.  For pci 2.3 we should never track interrupt
state in kvm IMO.  For example, if userspace unmasks an interrupt
through a config write, we will get an interrupt while host_irq_disabled
is set. If we then fail to mask it, kaboom.

> +	spin_unlock(&assigned_dev->intx_lock);
> +
> +	return ret;
> +}
> +
>  static irqreturn_t kvm_assigned_dev_thread(int irq, void *dev_id)
>  {
>  	struct kvm_assigned_dev_kernel *assigned_dev = dev_id;
>  	u32 vector;
>  	int index;
>  
> -	if (assigned_dev->irq_requested_type & KVM_DEV_IRQ_HOST_INTX) {
> -		spin_lock(&assigned_dev->intx_lock);
> +	if (assigned_dev->irq_requested_type & KVM_DEV_IRQ_HOST_INTX &&
> +	    !assigned_dev->pci_2_3) {
> +		spin_lock_irq(&assigned_dev->intx_lock);
>  		disable_irq_nosync(irq);
>  		assigned_dev->host_irq_disabled = true;
> -		spin_unlock(&assigned_dev->intx_lock);
> +		spin_unlock_irq(&assigned_dev->intx_lock);
>  	}
>  
>  	if (assigned_dev->irq_requested_type & KVM_DEV_IRQ_HOST_MSIX) {
> @@ -87,6 +215,7 @@ static irqreturn_t kvm_assigned_dev_thread(int irq, void *dev_id)
>  static void kvm_assigned_dev_ack_irq(struct kvm_irq_ack_notifier *kian)
>  {
>  	struct kvm_assigned_dev_kernel *dev;
> +	bool reassert = false;
>  
>  	if (kian->gsi == -1)
>  		return;
> @@ -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.

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

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

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

> +	if (request_threaded_irq(dev->host_irq, irq_handler,
> +				 kvm_assigned_dev_thread, flags,
> +				 dev->irq_name, (void *)dev))
>  		return -EIO;
> +
> +	if (dev->pci_2_3)
> +		pci_2_3_irq_unmask(dev->dev);
>  	return 0;
>  }
>  
> @@ -308,7 +473,6 @@ static int assigned_device_enable_guest_msi(struct kvm *kvm,
>  {
>  	dev->guest_irq = irq->guest_irq;
>  	dev->ack_notifier.gsi = -1;
> -	dev->host_irq_disabled = false;
>  	return 0;
>  }
>  #endif
> @@ -320,7 +484,6 @@ static int assigned_device_enable_guest_msix(struct kvm *kvm,
>  {
>  	dev->guest_irq = irq->guest_irq;
>  	dev->ack_notifier.gsi = -1;
> -	dev->host_irq_disabled = false;
>  	return 0;
>  }
>  #endif
> @@ -354,6 +517,7 @@ static int assign_host_irq(struct kvm *kvm,
>  	default:
>  		r = -EINVAL;
>  	}
> +	dev->host_irq_disabled = false;
>  
>  	if (!r)
>  		dev->irq_requested_type |= host_irq_type;
> -- 
> 1.7.1