[PATCH kvmtool v3 0/3] vfio-pci: Support INTx mode re-enabling

[PATCH kvmtool v3 0/3] vfio-pci: Support INTx mode re-enabling

[Leo Yan]

When enable vfio-pci mode for NIC driver on Juno board, the IRQ is
failed to forward properly from host to guest, finally root caused this
issue is related with kvmtool cannot re-enable INTx mode properly.

So the basic working flow to reproduce this issue is as below:

    Host             Guest
-------------  --------------------
  INTx mode
                 MSI enable failed in NIC driver
                 MSI disable in NIC driver
                 Switch back to INTx mode --> kvmtool doesn't support

So this patch is to support INTx mode re-enabling; patch 0001 is one
minor fixing up for eventfd releasing; patch 0002 introduces a new
function vfio_pci_init_intx() which is used to finish INTx one-time
initialisation; patch 0003 is the core patch for support INTx mode
re-enabling, when kvmtool detects MSI is disabled it rollbacks to INTx
mode.

This patch set has been tested on Juno-r2 board.

== Changes for V3 ==
* Add new function vfio_pci_init_intx() for one-time initialisation.
* Simplized INTx re-enabling (don't change irq_line anymore at the
  runtime).


Leo Yan (3):
  vfio-pci: Release INTx's unmask eventfd properly
  vfio-pci: Add new function for INTx one-time initialisation
  vfio-pci: Re-enable INTx mode when disable MSI/MSIX

 include/kvm/vfio.h |   1 +
 vfio/pci.c         | 108 +++++++++++++++++++++++++++++----------------
 2 files changed, 72 insertions(+), 37 deletions(-)

-- 
2.19.1

[PATCH kvmtool v3 1/3] vfio-pci: Release INTx's unmask eventfd properly

[Leo Yan]

The PCI device INTx uses event fd 'unmask_fd' to signal the deassertion
of the line from guest to host; but this eventfd isn't released properly
when disable INTx.

This patch firstly adds field 'unmask_fd' in struct vfio_pci_device for
storing unmask eventfd and close it when disable INTx.

Reviewed-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 include/kvm/vfio.h | 1 +
 vfio/pci.c         | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/include/kvm/vfio.h b/include/kvm/vfio.h
index 60e6c54..28223cf 100644
--- a/include/kvm/vfio.h
+++ b/include/kvm/vfio.h
@@ -74,6 +74,7 @@ struct vfio_pci_device {
 
 	unsigned long			irq_modes;
 	int				intx_fd;
+	int				unmask_fd;
 	unsigned int			intx_gsi;
 	struct vfio_pci_msi_common	msi;
 	struct vfio_pci_msi_common	msix;
diff --git a/vfio/pci.c b/vfio/pci.c
index 03de3c1..5224fee 100644
--- a/vfio/pci.c
+++ b/vfio/pci.c
@@ -1008,6 +1008,7 @@ static void vfio_pci_disable_intx(struct kvm *kvm, struct vfio_device *vdev)
 	irq__del_irqfd(kvm, gsi, pdev->intx_fd);
 
 	close(pdev->intx_fd);
+	close(pdev->unmask_fd);
 }
 
 static int vfio_pci_enable_intx(struct kvm *kvm, struct vfio_device *vdev)
@@ -1095,6 +1096,7 @@ static int vfio_pci_enable_intx(struct kvm *kvm, struct vfio_device *vdev)
 	}
 
 	pdev->intx_fd = trigger_fd;
+	pdev->unmask_fd = unmask_fd;
 	/* Guest is going to ovewrite our irq_line... */
 	pdev->intx_gsi = gsi;
 
-- 
2.19.1

[PATCH kvmtool v3 2/3] vfio-pci: Add new function for INTx one-time initialisation

[Leo Yan]

To support INTx enabling for multiple times, we need firstly to extract
one-time initialisation and move the related code into a new function
vfio_pci_init_intx(); if later disable and re-enable the INTx, we can
skip these one-time operations.

This patch move below three main operations for INTx one-time
initialisation from function vfio_pci_enable_intx() into function
vfio_pci_init_intx():

- Reserve 2 FDs for INTx;
- Sanity check with ioctl VFIO_DEVICE_GET_IRQ_INFO;
- Setup pdev->intx_gsi.

Suggested-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 vfio/pci.c | 67 ++++++++++++++++++++++++++++++++----------------------
 1 file changed, 40 insertions(+), 27 deletions(-)

diff --git a/vfio/pci.c b/vfio/pci.c
index 5224fee..3c39844 100644
--- a/vfio/pci.c
+++ b/vfio/pci.c
@@ -1018,30 +1018,7 @@ static int vfio_pci_enable_intx(struct kvm *kvm, struct vfio_device *vdev)
 	struct vfio_irq_eventfd	trigger;
 	struct vfio_irq_eventfd	unmask;
 	struct vfio_pci_device *pdev = &vdev->pci;
-	int gsi = pdev->hdr.irq_line - KVM_IRQ_OFFSET;
-
-	struct vfio_irq_info irq_info = {
-		.argsz = sizeof(irq_info),
-		.index = VFIO_PCI_INTX_IRQ_INDEX,
-	};
-
-	vfio_pci_reserve_irq_fds(2);
-
-	ret = ioctl(vdev->fd, VFIO_DEVICE_GET_IRQ_INFO, &irq_info);
-	if (ret || irq_info.count == 0) {
-		vfio_dev_err(vdev, "no INTx reported by VFIO");
-		return -ENODEV;
-	}
-
-	if (!(irq_info.flags & VFIO_IRQ_INFO_EVENTFD)) {
-		vfio_dev_err(vdev, "interrupt not eventfd capable");
-		return -EINVAL;
-	}
-
-	if (!(irq_info.flags & VFIO_IRQ_INFO_AUTOMASKED)) {
-		vfio_dev_err(vdev, "INTx interrupt not AUTOMASKED");
-		return -EINVAL;
-	}
+	int gsi = pdev->intx_gsi;
 
 	/*
 	 * PCI IRQ is level-triggered, so we use two eventfds. trigger_fd
@@ -1097,8 +1074,6 @@ static int vfio_pci_enable_intx(struct kvm *kvm, struct vfio_device *vdev)
 
 	pdev->intx_fd = trigger_fd;
 	pdev->unmask_fd = unmask_fd;
-	/* Guest is going to ovewrite our irq_line... */
-	pdev->intx_gsi = gsi;
 
 	return 0;
 
@@ -1117,6 +1092,39 @@ err_close:
 	return ret;
 }
 
+static int vfio_pci_init_intx(struct kvm *kvm, struct vfio_device *vdev)
+{
+	int ret;
+	struct vfio_pci_device *pdev = &vdev->pci;
+	struct vfio_irq_info irq_info = {
+		.argsz = sizeof(irq_info),
+		.index = VFIO_PCI_INTX_IRQ_INDEX,
+	};
+
+	vfio_pci_reserve_irq_fds(2);
+
+	ret = ioctl(vdev->fd, VFIO_DEVICE_GET_IRQ_INFO, &irq_info);
+	if (ret || irq_info.count == 0) {
+		vfio_dev_err(vdev, "no INTx reported by VFIO");
+		return -ENODEV;
+	}
+
+	if (!(irq_info.flags & VFIO_IRQ_INFO_EVENTFD)) {
+		vfio_dev_err(vdev, "interrupt not eventfd capable");
+		return -EINVAL;
+	}
+
+	if (!(irq_info.flags & VFIO_IRQ_INFO_AUTOMASKED)) {
+		vfio_dev_err(vdev, "INTx interrupt not AUTOMASKED");
+		return -EINVAL;
+	}
+
+	/* Guest is going to ovewrite our irq_line... */
+	pdev->intx_gsi = pdev->hdr.irq_line - KVM_IRQ_OFFSET;
+
+	return 0;
+}
+
 static int vfio_pci_configure_dev_irqs(struct kvm *kvm, struct vfio_device *vdev)
 {
 	int ret = 0;
@@ -1142,8 +1150,13 @@ static int vfio_pci_configure_dev_irqs(struct kvm *kvm, struct vfio_device *vdev
 			return ret;
 	}
 
-	if (pdev->irq_modes & VFIO_PCI_IRQ_MODE_INTX)
+	if (pdev->irq_modes & VFIO_PCI_IRQ_MODE_INTX) {
+		ret = vfio_pci_init_intx(kvm, vdev);
+		if (ret)
+			return ret;
+
 		ret = vfio_pci_enable_intx(kvm, vdev);
+	}
 
 	return ret;
 }
-- 
2.19.1

Re: [PATCH kvmtool v3 2/3] vfio-pci: Add new function for INTx one-time initialisation

[Jean-Philippe Brucker]

On 26/03/2019 07:41, Leo Yan wrote:
> To support INTx enabling for multiple times, we need firstly to extract
> one-time initialisation and move the related code into a new function
> vfio_pci_init_intx(); if later disable and re-enable the INTx, we can
> skip these one-time operations.
> 
> This patch move below three main operations for INTx one-time
> initialisation from function vfio_pci_enable_intx() into function
> vfio_pci_init_intx():
> 
> - Reserve 2 FDs for INTx;
> - Sanity check with ioctl VFIO_DEVICE_GET_IRQ_INFO;
> - Setup pdev->intx_gsi.
> 
> Suggested-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>

Thanks for the patches

Reviewed-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>

> ---
>  vfio/pci.c | 67 ++++++++++++++++++++++++++++++++----------------------
>  1 file changed, 40 insertions(+), 27 deletions(-)
> 
> diff --git a/vfio/pci.c b/vfio/pci.c
> index 5224fee..3c39844 100644
> --- a/vfio/pci.c
> +++ b/vfio/pci.c
> @@ -1018,30 +1018,7 @@ static int vfio_pci_enable_intx(struct kvm *kvm, struct vfio_device *vdev)
>  	struct vfio_irq_eventfd	trigger;
>  	struct vfio_irq_eventfd	unmask;
>  	struct vfio_pci_device *pdev = &vdev->pci;
> -	int gsi = pdev->hdr.irq_line - KVM_IRQ_OFFSET;
> -
> -	struct vfio_irq_info irq_info = {
> -		.argsz = sizeof(irq_info),
> -		.index = VFIO_PCI_INTX_IRQ_INDEX,
> -	};
> -
> -	vfio_pci_reserve_irq_fds(2);
> -
> -	ret = ioctl(vdev->fd, VFIO_DEVICE_GET_IRQ_INFO, &irq_info);
> -	if (ret || irq_info.count == 0) {
> -		vfio_dev_err(vdev, "no INTx reported by VFIO");
> -		return -ENODEV;
> -	}
> -
> -	if (!(irq_info.flags & VFIO_IRQ_INFO_EVENTFD)) {
> -		vfio_dev_err(vdev, "interrupt not eventfd capable");
> -		return -EINVAL;
> -	}
> -
> -	if (!(irq_info.flags & VFIO_IRQ_INFO_AUTOMASKED)) {
> -		vfio_dev_err(vdev, "INTx interrupt not AUTOMASKED");
> -		return -EINVAL;
> -	}
> +	int gsi = pdev->intx_gsi;
>  
>  	/*
>  	 * PCI IRQ is level-triggered, so we use two eventfds. trigger_fd
> @@ -1097,8 +1074,6 @@ static int vfio_pci_enable_intx(struct kvm *kvm, struct vfio_device *vdev)
>  
>  	pdev->intx_fd = trigger_fd;
>  	pdev->unmask_fd = unmask_fd;
> -	/* Guest is going to ovewrite our irq_line... */
> -	pdev->intx_gsi = gsi;
>  
>  	return 0;
>  
> @@ -1117,6 +1092,39 @@ err_close:
>  	return ret;
>  }
>  
> +static int vfio_pci_init_intx(struct kvm *kvm, struct vfio_device *vdev)
> +{
> +	int ret;
> +	struct vfio_pci_device *pdev = &vdev->pci;
> +	struct vfio_irq_info irq_info = {
> +		.argsz = sizeof(irq_info),
> +		.index = VFIO_PCI_INTX_IRQ_INDEX,
> +	};
> +
> +	vfio_pci_reserve_irq_fds(2);
> +
> +	ret = ioctl(vdev->fd, VFIO_DEVICE_GET_IRQ_INFO, &irq_info);
> +	if (ret || irq_info.count == 0) {
> +		vfio_dev_err(vdev, "no INTx reported by VFIO");
> +		return -ENODEV;
> +	}
> +
> +	if (!(irq_info.flags & VFIO_IRQ_INFO_EVENTFD)) {
> +		vfio_dev_err(vdev, "interrupt not eventfd capable");
> +		return -EINVAL;
> +	}
> +
> +	if (!(irq_info.flags & VFIO_IRQ_INFO_AUTOMASKED)) {
> +		vfio_dev_err(vdev, "INTx interrupt not AUTOMASKED");
> +		return -EINVAL;
> +	}
> +
> +	/* Guest is going to ovewrite our irq_line... */
> +	pdev->intx_gsi = pdev->hdr.irq_line - KVM_IRQ_OFFSET;
> +
> +	return 0;
> +}
> +
>  static int vfio_pci_configure_dev_irqs(struct kvm *kvm, struct vfio_device *vdev)
>  {
>  	int ret = 0;
> @@ -1142,8 +1150,13 @@ static int vfio_pci_configure_dev_irqs(struct kvm *kvm, struct vfio_device *vdev
>  			return ret;
>  	}
>  
> -	if (pdev->irq_modes & VFIO_PCI_IRQ_MODE_INTX)
> +	if (pdev->irq_modes & VFIO_PCI_IRQ_MODE_INTX) {
> +		ret = vfio_pci_init_intx(kvm, vdev);
> +		if (ret)
> +			return ret;
> +
>  		ret = vfio_pci_enable_intx(kvm, vdev);
> +	}
>  
>  	return ret;
>  }
> 

[PATCH kvmtool v3 3/3] vfio-pci: Re-enable INTx mode when disable MSI/MSIX

[Leo Yan]

Since PCI forbids enabling INTx, MSI or MSIX at the same time, it's by
default to disable INTx mode when enable MSI/MSIX mode; but this logic is
easily broken if the guest PCI driver detects the MSI/MSIX cannot work as
expected and tries to rollback to use INTx mode.  In this case, the INTx
mode has been disabled and has no chance to re-enable it, thus both INTx
mode and MSI/MSIX mode cannot work in vfio.

Below shows the detailed flow for introducing this issue:

  vfio_pci_configure_dev_irqs()
    `-> vfio_pci_enable_intx()

  vfio_pci_enable_msis()
    `-> vfio_pci_disable_intx()

  vfio_pci_disable_msis()   => Guest PCI driver disables MSI

To fix this issue, when disable MSI/MSIX we need to check if INTx mode
is available for this device or not; if the device can support INTx then
re-enable it so that the device can fallback to use it.

Since vfio_pci_disable_intx() / vfio_pci_enable_intx() pair functions
may be called for multiple times, this patch uses 'intx_fd == -1' to
denote the INTx is disabled, the pair functions can directly bail out
when detect INTx has been disabled and enabled respectively.

Suggested-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 vfio/pci.c | 41 ++++++++++++++++++++++++++++++-----------
 1 file changed, 30 insertions(+), 11 deletions(-)

diff --git a/vfio/pci.c b/vfio/pci.c
index 3c39844..3b2b1e7 100644
--- a/vfio/pci.c
+++ b/vfio/pci.c
@@ -28,6 +28,7 @@ struct vfio_irq_eventfd {
 	msi_update_state(state, val, VFIO_PCI_MSI_STATE_EMPTY)
 
 static void vfio_pci_disable_intx(struct kvm *kvm, struct vfio_device *vdev);
+static int vfio_pci_enable_intx(struct kvm *kvm, struct vfio_device *vdev);
 
 static int vfio_pci_enable_msis(struct kvm *kvm, struct vfio_device *vdev,
 				bool msix)
@@ -50,17 +51,14 @@ static int vfio_pci_enable_msis(struct kvm *kvm, struct vfio_device *vdev,
 	if (!msi_is_enabled(msis->virt_state))
 		return 0;
 
-	if (pdev->irq_modes & VFIO_PCI_IRQ_MODE_INTX) {
-		/*
-		 * PCI (and VFIO) forbids enabling INTx, MSI or MSIX at the same
-		 * time. Since INTx has to be enabled from the start (we don't
-		 * have a reliable way to know when the user starts using it),
-		 * disable it now.
-		 */
+	/*
+	 * PCI (and VFIO) forbids enabling INTx, MSI or MSIX at the same
+	 * time. Since INTx has to be enabled from the start (after enabling
+	 * 'pdev->intx_fd' will be assigned to an eventfd and doesn't equal
+	 * to the init value -1), disable it now.
+	 */
+	if (pdev->irq_modes & VFIO_PCI_IRQ_MODE_INTX)
 		vfio_pci_disable_intx(kvm, vdev);
-		/* Permanently disable INTx */
-		pdev->irq_modes &= ~VFIO_PCI_IRQ_MODE_INTX;
-	}
 
 	eventfds = (void *)msis->irq_set + sizeof(struct vfio_irq_set);
 
@@ -162,7 +160,16 @@ static int vfio_pci_disable_msis(struct kvm *kvm, struct vfio_device *vdev,
 	msi_set_enabled(msis->phys_state, false);
 	msi_set_empty(msis->phys_state, true);
 
-	return 0;
+	/*
+	 * When MSI or MSIX is disabled, this might be called when
+	 * PCI driver detects the MSI interrupt failure and wants to
+	 * rollback to INTx mode.  Thus enable INTx if the device
+	 * supports INTx mode in this case.
+	 */
+	if (pdev->irq_modes & VFIO_PCI_IRQ_MODE_INTX)
+		ret = vfio_pci_enable_intx(kvm, vdev);
+
+	return ret >= 0 ? 0 : ret;
 }
 
 static int vfio_pci_update_msi_entry(struct kvm *kvm, struct vfio_device *vdev,
@@ -1002,6 +1009,10 @@ static void vfio_pci_disable_intx(struct kvm *kvm, struct vfio_device *vdev)
 		.index	= VFIO_PCI_INTX_IRQ_INDEX,
 	};
 
+	/* INTx mode has been disabled */
+	if (pdev->intx_fd == -1)
+		return;
+
 	pr_debug("user requested MSI, disabling INTx %d", gsi);
 
 	ioctl(vdev->fd, VFIO_DEVICE_SET_IRQS, &irq_set);
@@ -1009,6 +1020,7 @@ static void vfio_pci_disable_intx(struct kvm *kvm, struct vfio_device *vdev)
 
 	close(pdev->intx_fd);
 	close(pdev->unmask_fd);
+	pdev->intx_fd = -1;
 }
 
 static int vfio_pci_enable_intx(struct kvm *kvm, struct vfio_device *vdev)
@@ -1020,6 +1032,10 @@ static int vfio_pci_enable_intx(struct kvm *kvm, struct vfio_device *vdev)
 	struct vfio_pci_device *pdev = &vdev->pci;
 	int gsi = pdev->intx_gsi;
 
+	/* INTx mode has been enabled */
+	if (pdev->intx_fd != -1)
+		return 0;
+
 	/*
 	 * PCI IRQ is level-triggered, so we use two eventfds. trigger_fd
 	 * signals an interrupt from host to guest, and unmask_fd signals the
@@ -1122,6 +1138,9 @@ static int vfio_pci_init_intx(struct kvm *kvm, struct vfio_device *vdev)
 	/* Guest is going to ovewrite our irq_line... */
 	pdev->intx_gsi = pdev->hdr.irq_line - KVM_IRQ_OFFSET;
 
+	/* Use intx_fd = -1 to denote INTx is disabled */
+	pdev->intx_fd = -1;
+
 	return 0;
 }
 
-- 
2.19.1

Re: [PATCH kvmtool v3 3/3] vfio-pci: Re-enable INTx mode when disable MSI/MSIX

[Jean-Philippe Brucker]

On 26/03/2019 07:41, Leo Yan wrote:
> Since PCI forbids enabling INTx, MSI or MSIX at the same time, it's by
> default to disable INTx mode when enable MSI/MSIX mode; but this logic is
> easily broken if the guest PCI driver detects the MSI/MSIX cannot work as
> expected and tries to rollback to use INTx mode.  In this case, the INTx
> mode has been disabled and has no chance to re-enable it, thus both INTx
> mode and MSI/MSIX mode cannot work in vfio.
> 
> Below shows the detailed flow for introducing this issue:
> 
>   vfio_pci_configure_dev_irqs()
>     `-> vfio_pci_enable_intx()
> 
>   vfio_pci_enable_msis()
>     `-> vfio_pci_disable_intx()
> 
>   vfio_pci_disable_msis()   => Guest PCI driver disables MSI
> 
> To fix this issue, when disable MSI/MSIX we need to check if INTx mode
> is available for this device or not; if the device can support INTx then
> re-enable it so that the device can fallback to use it.
> 
> Since vfio_pci_disable_intx() / vfio_pci_enable_intx() pair functions
> may be called for multiple times, this patch uses 'intx_fd == -1' to
> denote the INTx is disabled, the pair functions can directly bail out
> when detect INTx has been disabled and enabled respectively.
> 
> Suggested-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  vfio/pci.c | 41 ++++++++++++++++++++++++++++++-----------
>  1 file changed, 30 insertions(+), 11 deletions(-)
> 
> diff --git a/vfio/pci.c b/vfio/pci.c
> index 3c39844..3b2b1e7 100644
> --- a/vfio/pci.c
> +++ b/vfio/pci.c
> @@ -28,6 +28,7 @@ struct vfio_irq_eventfd {
>  	msi_update_state(state, val, VFIO_PCI_MSI_STATE_EMPTY)
>  
>  static void vfio_pci_disable_intx(struct kvm *kvm, struct vfio_device *vdev);
> +static int vfio_pci_enable_intx(struct kvm *kvm, struct vfio_device *vdev);
>  
>  static int vfio_pci_enable_msis(struct kvm *kvm, struct vfio_device *vdev,
>  				bool msix)
> @@ -50,17 +51,14 @@ static int vfio_pci_enable_msis(struct kvm *kvm, struct vfio_device *vdev,
>  	if (!msi_is_enabled(msis->virt_state))
>  		return 0;
>  
> -	if (pdev->irq_modes & VFIO_PCI_IRQ_MODE_INTX) {
> -		/*
> -		 * PCI (and VFIO) forbids enabling INTx, MSI or MSIX at the same
> -		 * time. Since INTx has to be enabled from the start (we don't
> -		 * have a reliable way to know when the user starts using it),
> -		 * disable it now.
> -		 */
> +	/*
> +	 * PCI (and VFIO) forbids enabling INTx, MSI or MSIX at the same
> +	 * time. Since INTx has to be enabled from the start (after enabling
> +	 * 'pdev->intx_fd' will be assigned to an eventfd and doesn't equal
> +	 * to the init value -1), disable it now.
> +	 */

I don't think the comment change is useful, we don't need that much
detail. The text that you replaced was trying to explain why we enable
INTx from the start, and would still apply (although it should have been
s/user/guest/)

> +	if (pdev->irq_modes & VFIO_PCI_IRQ_MODE_INTX)
>  		vfio_pci_disable_intx(kvm, vdev);
> -		/* Permanently disable INTx */
> -		pdev->irq_modes &= ~VFIO_PCI_IRQ_MODE_INTX;
> -	}
>  
>  	eventfds = (void *)msis->irq_set + sizeof(struct vfio_irq_set);
>  
> @@ -162,7 +160,16 @@ static int vfio_pci_disable_msis(struct kvm *kvm, struct vfio_device *vdev,
>  	msi_set_enabled(msis->phys_state, false);
>  	msi_set_empty(msis->phys_state, true);
>  
> -	return 0;
> +	/*
> +	 * When MSI or MSIX is disabled, this might be called when
> +	 * PCI driver detects the MSI interrupt failure and wants to
> +	 * rollback to INTx mode.  Thus enable INTx if the device
> +	 * supports INTx mode in this case.
> +	 */
> +	if (pdev->irq_modes & VFIO_PCI_IRQ_MODE_INTX)
> +		ret = vfio_pci_enable_intx(kvm, vdev);
> +
> +	return ret >= 0 ? 0 : ret;
>  }
>  
>  static int vfio_pci_update_msi_entry(struct kvm *kvm, struct vfio_device *vdev,
> @@ -1002,6 +1009,10 @@ static void vfio_pci_disable_intx(struct kvm *kvm, struct vfio_device *vdev)
>  		.index	= VFIO_PCI_INTX_IRQ_INDEX,
>  	};
>  
> +	/* INTx mode has been disabled */

Here as well, the comments on intx_fd seem unnecessary. But these are
only nits, the code is fine and I tested for regressions on my hardware, so:

Reviewed-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>

> +	if (pdev->intx_fd == -1)
> +		return;
> +
>  	pr_debug("user requested MSI, disabling INTx %d", gsi);
>  
>  	ioctl(vdev->fd, VFIO_DEVICE_SET_IRQS, &irq_set);
> @@ -1009,6 +1020,7 @@ static void vfio_pci_disable_intx(struct kvm *kvm, struct vfio_device *vdev)
>  
>  	close(pdev->intx_fd);
>  	close(pdev->unmask_fd);
> +	pdev->intx_fd = -1;
>  }
>  
>  static int vfio_pci_enable_intx(struct kvm *kvm, struct vfio_device *vdev)
> @@ -1020,6 +1032,10 @@ static int vfio_pci_enable_intx(struct kvm *kvm, struct vfio_device *vdev)
>  	struct vfio_pci_device *pdev = &vdev->pci;
>  	int gsi = pdev->intx_gsi;
>  
> +	/* INTx mode has been enabled */
> +	if (pdev->intx_fd != -1)
> +		return 0;
> +
>  	/*
>  	 * PCI IRQ is level-triggered, so we use two eventfds. trigger_fd
>  	 * signals an interrupt from host to guest, and unmask_fd signals the
> @@ -1122,6 +1138,9 @@ static int vfio_pci_init_intx(struct kvm *kvm, struct vfio_device *vdev)
>  	/* Guest is going to ovewrite our irq_line... */
>  	pdev->intx_gsi = pdev->hdr.irq_line - KVM_IRQ_OFFSET;
>  
> +	/* Use intx_fd = -1 to denote INTx is disabled */
> +	pdev->intx_fd = -1;
> +
>  	return 0;
>  }
>  
> 

Re: [PATCH kvmtool v3 3/3] vfio-pci: Re-enable INTx mode when disable MSI/MSIX

[Leo Yan]

Hi Jean-Philippe,

On Thu, Apr 04, 2019 at 12:06:51PM +0100, Jean-Philippe Brucker wrote:
> On 26/03/2019 07:41, Leo Yan wrote:
> > Since PCI forbids enabling INTx, MSI or MSIX at the same time, it's by
> > default to disable INTx mode when enable MSI/MSIX mode; but this logic is
> > easily broken if the guest PCI driver detects the MSI/MSIX cannot work as
> > expected and tries to rollback to use INTx mode.  In this case, the INTx
> > mode has been disabled and has no chance to re-enable it, thus both INTx
> > mode and MSI/MSIX mode cannot work in vfio.
> > 
> > Below shows the detailed flow for introducing this issue:
> > 
> >   vfio_pci_configure_dev_irqs()
> >     `-> vfio_pci_enable_intx()
> > 
> >   vfio_pci_enable_msis()
> >     `-> vfio_pci_disable_intx()
> > 
> >   vfio_pci_disable_msis()   => Guest PCI driver disables MSI
> > 
> > To fix this issue, when disable MSI/MSIX we need to check if INTx mode
> > is available for this device or not; if the device can support INTx then
> > re-enable it so that the device can fallback to use it.
> > 
> > Since vfio_pci_disable_intx() / vfio_pci_enable_intx() pair functions
> > may be called for multiple times, this patch uses 'intx_fd == -1' to
> > denote the INTx is disabled, the pair functions can directly bail out
> > when detect INTx has been disabled and enabled respectively.
> > 
> > Suggested-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > ---
> >  vfio/pci.c | 41 ++++++++++++++++++++++++++++++-----------
> >  1 file changed, 30 insertions(+), 11 deletions(-)
> > 
> > diff --git a/vfio/pci.c b/vfio/pci.c
> > index 3c39844..3b2b1e7 100644
> > --- a/vfio/pci.c
> > +++ b/vfio/pci.c
> > @@ -28,6 +28,7 @@ struct vfio_irq_eventfd {
> >  	msi_update_state(state, val, VFIO_PCI_MSI_STATE_EMPTY)
> >  
> >  static void vfio_pci_disable_intx(struct kvm *kvm, struct vfio_device *vdev);
> > +static int vfio_pci_enable_intx(struct kvm *kvm, struct vfio_device *vdev);
> >  
> >  static int vfio_pci_enable_msis(struct kvm *kvm, struct vfio_device *vdev,
> >  				bool msix)
> > @@ -50,17 +51,14 @@ static int vfio_pci_enable_msis(struct kvm *kvm, struct vfio_device *vdev,
> >  	if (!msi_is_enabled(msis->virt_state))
> >  		return 0;
> >  
> > -	if (pdev->irq_modes & VFIO_PCI_IRQ_MODE_INTX) {
> > -		/*
> > -		 * PCI (and VFIO) forbids enabling INTx, MSI or MSIX at the same
> > -		 * time. Since INTx has to be enabled from the start (we don't
> > -		 * have a reliable way to know when the user starts using it),
> > -		 * disable it now.
> > -		 */
> > +	/*
> > +	 * PCI (and VFIO) forbids enabling INTx, MSI or MSIX at the same
> > +	 * time. Since INTx has to be enabled from the start (after enabling
> > +	 * 'pdev->intx_fd' will be assigned to an eventfd and doesn't equal
> > +	 * to the init value -1), disable it now.
> > +	 */
> 
> I don't think the comment change is useful, we don't need that much
> detail. The text that you replaced was trying to explain why we enable
> INTx from the start, and would still apply (although it should have been
> s/user/guest/)
> 
> > +	if (pdev->irq_modes & VFIO_PCI_IRQ_MODE_INTX)
> >  		vfio_pci_disable_intx(kvm, vdev);
> > -		/* Permanently disable INTx */
> > -		pdev->irq_modes &= ~VFIO_PCI_IRQ_MODE_INTX;
> > -	}
> >  
> >  	eventfds = (void *)msis->irq_set + sizeof(struct vfio_irq_set);
> >  
> > @@ -162,7 +160,16 @@ static int vfio_pci_disable_msis(struct kvm *kvm, struct vfio_device *vdev,
> >  	msi_set_enabled(msis->phys_state, false);
> >  	msi_set_empty(msis->phys_state, true);
> >  
> > -	return 0;
> > +	/*
> > +	 * When MSI or MSIX is disabled, this might be called when
> > +	 * PCI driver detects the MSI interrupt failure and wants to
> > +	 * rollback to INTx mode.  Thus enable INTx if the device
> > +	 * supports INTx mode in this case.
> > +	 */
> > +	if (pdev->irq_modes & VFIO_PCI_IRQ_MODE_INTX)
> > +		ret = vfio_pci_enable_intx(kvm, vdev);
> > +
> > +	return ret >= 0 ? 0 : ret;
> >  }
> >  
> >  static int vfio_pci_update_msi_entry(struct kvm *kvm, struct vfio_device *vdev,
> > @@ -1002,6 +1009,10 @@ static void vfio_pci_disable_intx(struct kvm *kvm, struct vfio_device *vdev)
> >  		.index	= VFIO_PCI_INTX_IRQ_INDEX,
> >  	};
> >  
> > +	/* INTx mode has been disabled */
> 
> Here as well, the comments on intx_fd seem unnecessary. But these are
> only nits, the code is fine and I tested for regressions on my hardware, so:
> 
> Reviewed-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>

Thanks for reviewing.  I will spin a new patch set to drop unnecessary
comment to let changes as simple as possible.

Thanks,
Leo Yan

Re: [PATCH kvmtool v3 3/3] vfio-pci: Re-enable INTx mode when disable MSI/MSIX

[Leo Yan]

Hi Jean-Philippe,

On Thu, Apr 04, 2019 at 12:06:51PM +0100, Jean-Philippe Brucker wrote:
> On 26/03/2019 07:41, Leo Yan wrote:
> > Since PCI forbids enabling INTx, MSI or MSIX at the same time, it's by
> > default to disable INTx mode when enable MSI/MSIX mode; but this logic is
> > easily broken if the guest PCI driver detects the MSI/MSIX cannot work as
> > expected and tries to rollback to use INTx mode.  In this case, the INTx
> > mode has been disabled and has no chance to re-enable it, thus both INTx
> > mode and MSI/MSIX mode cannot work in vfio.
> > 
> > Below shows the detailed flow for introducing this issue:
> > 
> >   vfio_pci_configure_dev_irqs()
> >     `-> vfio_pci_enable_intx()
> > 
> >   vfio_pci_enable_msis()
> >     `-> vfio_pci_disable_intx()
> > 
> >   vfio_pci_disable_msis()   => Guest PCI driver disables MSI
> > 
> > To fix this issue, when disable MSI/MSIX we need to check if INTx mode
> > is available for this device or not; if the device can support INTx then
> > re-enable it so that the device can fallback to use it.
> > 
> > Since vfio_pci_disable_intx() / vfio_pci_enable_intx() pair functions
> > may be called for multiple times, this patch uses 'intx_fd == -1' to
> > denote the INTx is disabled, the pair functions can directly bail out
> > when detect INTx has been disabled and enabled respectively.
> > 
> > Suggested-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > ---
> >  vfio/pci.c | 41 ++++++++++++++++++++++++++++++-----------
> >  1 file changed, 30 insertions(+), 11 deletions(-)
> > 
> > diff --git a/vfio/pci.c b/vfio/pci.c
> > index 3c39844..3b2b1e7 100644
> > --- a/vfio/pci.c
> > +++ b/vfio/pci.c
> > @@ -28,6 +28,7 @@ struct vfio_irq_eventfd {
> >  	msi_update_state(state, val, VFIO_PCI_MSI_STATE_EMPTY)
> >  
> >  static void vfio_pci_disable_intx(struct kvm *kvm, struct vfio_device *vdev);
> > +static int vfio_pci_enable_intx(struct kvm *kvm, struct vfio_device *vdev);
> >  
> >  static int vfio_pci_enable_msis(struct kvm *kvm, struct vfio_device *vdev,
> >  				bool msix)
> > @@ -50,17 +51,14 @@ static int vfio_pci_enable_msis(struct kvm *kvm, struct vfio_device *vdev,
> >  	if (!msi_is_enabled(msis->virt_state))
> >  		return 0;
> >  
> > -	if (pdev->irq_modes & VFIO_PCI_IRQ_MODE_INTX) {
> > -		/*
> > -		 * PCI (and VFIO) forbids enabling INTx, MSI or MSIX at the same
> > -		 * time. Since INTx has to be enabled from the start (we don't
> > -		 * have a reliable way to know when the user starts using it),
> > -		 * disable it now.
> > -		 */
> > +	/*
> > +	 * PCI (and VFIO) forbids enabling INTx, MSI or MSIX at the same
> > +	 * time. Since INTx has to be enabled from the start (after enabling
> > +	 * 'pdev->intx_fd' will be assigned to an eventfd and doesn't equal
> > +	 * to the init value -1), disable it now.
> > +	 */
> 
> I don't think the comment change is useful, we don't need that much
> detail. The text that you replaced was trying to explain why we enable
> INTx from the start, and would still apply (although it should have been
> s/user/guest/)
> 
> > +	if (pdev->irq_modes & VFIO_PCI_IRQ_MODE_INTX)
> >  		vfio_pci_disable_intx(kvm, vdev);
> > -		/* Permanently disable INTx */
> > -		pdev->irq_modes &= ~VFIO_PCI_IRQ_MODE_INTX;
> > -	}
> >  
> >  	eventfds = (void *)msis->irq_set + sizeof(struct vfio_irq_set);
> >  
> > @@ -162,7 +160,16 @@ static int vfio_pci_disable_msis(struct kvm *kvm, struct vfio_device *vdev,
> >  	msi_set_enabled(msis->phys_state, false);
> >  	msi_set_empty(msis->phys_state, true);
> >  
> > -	return 0;
> > +	/*
> > +	 * When MSI or MSIX is disabled, this might be called when
> > +	 * PCI driver detects the MSI interrupt failure and wants to
> > +	 * rollback to INTx mode.  Thus enable INTx if the device
> > +	 * supports INTx mode in this case.
> > +	 */
> > +	if (pdev->irq_modes & VFIO_PCI_IRQ_MODE_INTX)
> > +		ret = vfio_pci_enable_intx(kvm, vdev);
> > +
> > +	return ret >= 0 ? 0 : ret;
> >  }
> >  
> >  static int vfio_pci_update_msi_entry(struct kvm *kvm, struct vfio_device *vdev,
> > @@ -1002,6 +1009,10 @@ static void vfio_pci_disable_intx(struct kvm *kvm, struct vfio_device *vdev)
> >  		.index	= VFIO_PCI_INTX_IRQ_INDEX,
> >  	};
> >  
> > +	/* INTx mode has been disabled */
> 
> Here as well, the comments on intx_fd seem unnecessary. But these are
> only nits, the code is fine and I tested for regressions on my hardware, so:
> 
> Reviewed-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>

Thanks for reviewing.  I will spin a new patch set to drop unnecessary
comment to let changes as simple as possible.

Thanks,
Leo Yan

Re: [PATCH kvmtool v3 0/3] vfio-pci: Support INTx mode re-enabling

[Will Deacon]

Hi Leo,

On Tue, Mar 26, 2019 at 03:41:28PM +0800, Leo Yan wrote:
> When enable vfio-pci mode for NIC driver on Juno board, the IRQ is
> failed to forward properly from host to guest, finally root caused this
> issue is related with kvmtool cannot re-enable INTx mode properly.
> 
> So the basic working flow to reproduce this issue is as below:
> 
>     Host             Guest
> -------------  --------------------
>   INTx mode
>                  MSI enable failed in NIC driver
>                  MSI disable in NIC driver
>                  Switch back to INTx mode --> kvmtool doesn't support
> 
> So this patch is to support INTx mode re-enabling; patch 0001 is one
> minor fixing up for eventfd releasing; patch 0002 introduces a new
> function vfio_pci_init_intx() which is used to finish INTx one-time
> initialisation; patch 0003 is the core patch for support INTx mode
> re-enabling, when kvmtool detects MSI is disabled it rollbacks to INTx
> mode.
> 
> This patch set has been tested on Juno-r2 board.
> 
> == Changes for V3 ==
> * Add new function vfio_pci_init_intx() for one-time initialisation.
> * Simplized INTx re-enabling (don't change irq_line anymore at the
>   runtime).

Just checking, but is this series intended to supersede this one:

https://www.spinics.net/lists/kvm/msg183750.html

?

Thanks,

Will

Re: [PATCH kvmtool v3 0/3] vfio-pci: Support INTx mode re-enabling

[Leo Yan]

Hi Will,

On Tue, Apr 02, 2019 at 05:50:25PM +0100, Will Deacon wrote:
> Hi Leo,
> 
> On Tue, Mar 26, 2019 at 03:41:28PM +0800, Leo Yan wrote:
> > When enable vfio-pci mode for NIC driver on Juno board, the IRQ is
> > failed to forward properly from host to guest, finally root caused this
> > issue is related with kvmtool cannot re-enable INTx mode properly.
> > 
> > So the basic working flow to reproduce this issue is as below:
> > 
> >     Host             Guest
> > -------------  --------------------
> >   INTx mode
> >                  MSI enable failed in NIC driver
> >                  MSI disable in NIC driver
> >                  Switch back to INTx mode --> kvmtool doesn't support
> > 
> > So this patch is to support INTx mode re-enabling; patch 0001 is one
> > minor fixing up for eventfd releasing; patch 0002 introduces a new
> > function vfio_pci_init_intx() which is used to finish INTx one-time
> > initialisation; patch 0003 is the core patch for support INTx mode
> > re-enabling, when kvmtool detects MSI is disabled it rollbacks to INTx
> > mode.
> > 
> > This patch set has been tested on Juno-r2 board.
> > 
> > == Changes for V3 ==
> > * Add new function vfio_pci_init_intx() for one-time initialisation.
> > * Simplized INTx re-enabling (don't change irq_line anymore at the
> >   runtime).
> 
> Just checking, but is this series intended to supersede this one:
> 
> https://www.spinics.net/lists/kvm/msg183750.html
> 
> ?

Yes, this patch series is v3 and the link you pointed out is the old
patch series v1.  Sorry for confusion.

Thanks,
Leo Yan

Re: [PATCH kvmtool v3 0/3] vfio-pci: Support INTx mode re-enabling

[Will Deacon]

On Wed, Apr 03, 2019 at 11:58:17AM +0800, Leo Yan wrote:
> On Tue, Apr 02, 2019 at 05:50:25PM +0100, Will Deacon wrote:
> > Just checking, but is this series intended to supersede this one:
> > 
> > https://www.spinics.net/lists/kvm/msg183750.html
> > 
> > ?
> 
> Yes, this patch series is v3 and the link you pointed out is the old
> patch series v1.  Sorry for confusion.

No, that's fine and what I was assuming. Just wanted to confirm due to the
change in $subject, so I don't miss anything.

Will

Re: [PATCH kvmtool v3 0/3] vfio-pci: Support INTx mode re-enabling

[Leo Yan]

Hi Will,

On Wed, Apr 03, 2019 at 01:43:31PM +0100, Will Deacon wrote:
> On Wed, Apr 03, 2019 at 11:58:17AM +0800, Leo Yan wrote:
> > On Tue, Apr 02, 2019 at 05:50:25PM +0100, Will Deacon wrote:
> > > Just checking, but is this series intended to supersede this one:
> > > 
> > > https://www.spinics.net/lists/kvm/msg183750.html
> > > 
> > > ?
> > 
> > Yes, this patch series is v3 and the link you pointed out is the old
> > patch series v1.  Sorry for confusion.
> 
> No, that's fine and what I was assuming. Just wanted to confirm due to the
> change in $subject, so I don't miss anything.

Yes.

Gently ping Jean-Philippe if I can get green light for this patch set?
Otherwise, please let me know if I need to follow up anything; thanks
for reviewing.

Thanks,
Leo Yan