kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH kvmtool v2 0/3] vfio-pci: Support INTx mode re-enabling
@ 2019-03-20  6:20 Leo Yan
  2019-03-20  6:20 ` [PATCH kvmtool v2 1/3] vfio-pci: Release INTx's unmask eventfd properly Leo Yan
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Leo Yan @ 2019-03-20  6:20 UTC (permalink / raw)
  To: kvm, kvmarm, Will Deacon, Marc Zyngier, Jean-Philippe Brucker,
	Eric Auger, Robin Murphy
  Cc: 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; 0001/0002 patches
are only minor fixing up for eventfd releasing and remove useless FDs
reservation for INTx.  0003 patch is the core patch for supporting
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.

Leo Yan (3):
  vfio-pci: Release INTx's unmask eventfd properly
  vfio-pci: Remove useless FDs reservation in vfio_pci_enable_intx()
  vfio-pci: Re-enable INTx mode when disable MSI/MSIX

 include/kvm/vfio.h |  1 +
 vfio/pci.c         | 61 +++++++++++++++++++++++++++++++++++++---------
 2 files changed, 50 insertions(+), 12 deletions(-)

-- 
2.19.1

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH kvmtool v2 1/3] vfio-pci: Release INTx's unmask eventfd properly
  2019-03-20  6:20 [PATCH kvmtool v2 0/3] vfio-pci: Support INTx mode re-enabling Leo Yan
@ 2019-03-20  6:20 ` Leo Yan
  2019-03-25 18:27   ` Jean-Philippe Brucker
  2019-03-20  6:20 ` [PATCH kvmtool v2 2/3] vfio-pci: Remove useless FDs reservation in vfio_pci_enable_intx() Leo Yan
  2019-03-20  6:20 ` [PATCH kvmtool v2 3/3] vfio-pci: Re-enable INTx mode when disable MSI/MSIX Leo Yan
  2 siblings, 1 reply; 8+ messages in thread
From: Leo Yan @ 2019-03-20  6:20 UTC (permalink / raw)
  To: kvm, kvmarm, Will Deacon, Marc Zyngier, Jean-Philippe Brucker,
	Eric Auger, Robin Murphy
  Cc: 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.

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

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH kvmtool v2 2/3] vfio-pci: Remove useless FDs reservation in vfio_pci_enable_intx()
  2019-03-20  6:20 [PATCH kvmtool v2 0/3] vfio-pci: Support INTx mode re-enabling Leo Yan
  2019-03-20  6:20 ` [PATCH kvmtool v2 1/3] vfio-pci: Release INTx's unmask eventfd properly Leo Yan
@ 2019-03-20  6:20 ` Leo Yan
  2019-03-25 18:28   ` Jean-Philippe Brucker
  2019-03-20  6:20 ` [PATCH kvmtool v2 3/3] vfio-pci: Re-enable INTx mode when disable MSI/MSIX Leo Yan
  2 siblings, 1 reply; 8+ messages in thread
From: Leo Yan @ 2019-03-20  6:20 UTC (permalink / raw)
  To: kvm, kvmarm, Will Deacon, Marc Zyngier, Jean-Philippe Brucker,
	Eric Auger, Robin Murphy
  Cc: Leo Yan

Since INTx only uses 2 FDs, it's not particularly useful to reserve FDs
in function vfio_pci_enable_intx(); so this patch is to remove FDs
reservation in this function.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 vfio/pci.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/vfio/pci.c b/vfio/pci.c
index 5224fee..d025581 100644
--- a/vfio/pci.c
+++ b/vfio/pci.c
@@ -1025,8 +1025,6 @@ static int vfio_pci_enable_intx(struct kvm *kvm, struct vfio_device *vdev)
 		.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");
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH kvmtool v2 3/3] vfio-pci: Re-enable INTx mode when disable MSI/MSIX
  2019-03-20  6:20 [PATCH kvmtool v2 0/3] vfio-pci: Support INTx mode re-enabling Leo Yan
  2019-03-20  6:20 ` [PATCH kvmtool v2 1/3] vfio-pci: Release INTx's unmask eventfd properly Leo Yan
  2019-03-20  6:20 ` [PATCH kvmtool v2 2/3] vfio-pci: Remove useless FDs reservation in vfio_pci_enable_intx() Leo Yan
@ 2019-03-20  6:20 ` Leo Yan
  2019-03-25 18:28   ` Jean-Philippe Brucker
  2 siblings, 1 reply; 8+ messages in thread
From: Leo Yan @ 2019-03-20  6:20 UTC (permalink / raw)
  To: kvm, kvmarm, Will Deacon, Marc Zyngier, Jean-Philippe Brucker,
	Eric Auger, Robin Murphy
  Cc: 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.  The INTx mode has been
disabled and it has no chance to be enabled again, thus both INTx mode
and MSI/MSIX mode will not be enabled in vfio for this case.

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
we need to re-enable it so the device can fallback to use it.

In this patch, should note two minor changes:

- vfio_pci_disable_intx() may be called multiple times (each time the
  guest enables one MSI vector).  This patch changes to use
  'intx_fd == -1' to denote the INTx disabled, vfio_pci_disable_intx()
  and vfio_pci_enable_intx will directly bail out when detect INTx has
  been disabled and enabled respectively.

- Since pci_device_header will be corrupted after PCI configuration
  and all irq related info will be lost.  Before re-enabling INTx
  mode, this patch restores 'irq_pin' and 'irq_line' fields in struct
  pci_device_header.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 vfio/pci.c | 59 ++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 48 insertions(+), 11 deletions(-)

diff --git a/vfio/pci.c b/vfio/pci.c
index d025581..ba971eb 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,34 @@ 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) {
+		/*
+		 * Struct pci_device_header is not only used for header,
+		 * it also is used for PCI configuration; and in the function
+		 * vfio_pci_cfg_write() it firstly writes configuration space
+		 * and then read back the configuration space data into the
+		 * header structure; thus 'irq_pin' and 'irq_line' in the
+		 * header will be overwritten.
+		 *
+		 * If want to enable INTx mode properly, firstly needs to
+		 * restore 'irq_pin' and 'irq_line' values; we can simply set 1
+		 * to 'irq_pin', and 'pdev->intx_gsi' keeps gsi value when
+		 * enable INTx mode previously so we can simply use it to
+		 * recover irq line number by adding offset KVM_IRQ_OFFSET.
+		 */
+		pdev->hdr.irq_pin = 1;
+		pdev->hdr.irq_line = pdev->intx_gsi + KVM_IRQ_OFFSET;
+
+		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 +1027,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 +1038,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)
@@ -1025,6 +1055,10 @@ static int vfio_pci_enable_intx(struct kvm *kvm, struct vfio_device *vdev)
 		.index = VFIO_PCI_INTX_IRQ_INDEX,
 	};
 
+	/* INTx mode has been enabled */
+	if (pdev->intx_fd != -1)
+		return 0;
+
 	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");
@@ -1140,6 +1174,9 @@ static int vfio_pci_configure_dev_irqs(struct kvm *kvm, struct vfio_device *vdev
 			return ret;
 	}
 
+	/* Use intx_fd=-1 to denote INTx is disabled */
+	pdev->intx_fd = -1;
+
 	if (pdev->irq_modes & VFIO_PCI_IRQ_MODE_INTX)
 		ret = vfio_pci_enable_intx(kvm, vdev);
 
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH kvmtool v2 1/3] vfio-pci: Release INTx's unmask eventfd properly
  2019-03-20  6:20 ` [PATCH kvmtool v2 1/3] vfio-pci: Release INTx's unmask eventfd properly Leo Yan
@ 2019-03-25 18:27   ` Jean-Philippe Brucker
  0 siblings, 0 replies; 8+ messages in thread
From: Jean-Philippe Brucker @ 2019-03-25 18:27 UTC (permalink / raw)
  To: Leo Yan, kvm, kvmarm, Will Deacon, Marc Zyngier, Eric Auger,
	Robin Murphy

Hi Leo,

Thanks for the patches

On 20/03/2019 06:20, Leo Yan wrote:
> 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.
> 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>

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

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH kvmtool v2 2/3] vfio-pci: Remove useless FDs reservation in vfio_pci_enable_intx()
  2019-03-20  6:20 ` [PATCH kvmtool v2 2/3] vfio-pci: Remove useless FDs reservation in vfio_pci_enable_intx() Leo Yan
@ 2019-03-25 18:28   ` Jean-Philippe Brucker
  0 siblings, 0 replies; 8+ messages in thread
From: Jean-Philippe Brucker @ 2019-03-25 18:28 UTC (permalink / raw)
  To: Leo Yan, kvm, kvmarm, Will Deacon, Marc Zyngier, Eric Auger,
	Robin Murphy

On 20/03/2019 06:20, Leo Yan wrote:
> Since INTx only uses 2 FDs, it's not particularly useful to reserve FDs
> in function vfio_pci_enable_intx(); so this patch is to remove FDs
> reservation in this function.
> 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>

The main reason for this is that we want to call enable_intx() multiple
times and reserve_irq_fds() increments a static variable. That function
makes an approximation of the number of fds used by kvmtool and given
that we count a margin of 100 fds, the 2 INTx fds are already included
in that approximation (assuming we're not assigning hundreds of devices
to the guest).

But given that patch 3 highlights the need for a one-time init function,
maybe we can move the reserve_irq_fds() call there as well?

Thanks,
Jean

> ---
>  vfio/pci.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/vfio/pci.c b/vfio/pci.c
> index 5224fee..d025581 100644
> --- a/vfio/pci.c
> +++ b/vfio/pci.c
> @@ -1025,8 +1025,6 @@ static int vfio_pci_enable_intx(struct kvm *kvm, struct vfio_device *vdev)
>  		.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");
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH kvmtool v2 3/3] vfio-pci: Re-enable INTx mode when disable MSI/MSIX
  2019-03-20  6:20 ` [PATCH kvmtool v2 3/3] vfio-pci: Re-enable INTx mode when disable MSI/MSIX Leo Yan
@ 2019-03-25 18:28   ` Jean-Philippe Brucker
  2019-03-26  3:17     ` Leo Yan
  0 siblings, 1 reply; 8+ messages in thread
From: Jean-Philippe Brucker @ 2019-03-25 18:28 UTC (permalink / raw)
  To: Leo Yan, kvm, kvmarm, Will Deacon, Marc Zyngier, Eric Auger,
	Robin Murphy

On 20/03/2019 06:20, 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.  The INTx mode has been
> disabled and it has no chance to be enabled again, thus both INTx mode
> and MSI/MSIX mode will not be enabled in vfio for this case.
> 
> 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
> we need to re-enable it so the device can fallback to use it.
> 
> In this patch, should note two minor changes:
> 
> - vfio_pci_disable_intx() may be called multiple times (each time the
>   guest enables one MSI vector).  This patch changes to use
>   'intx_fd == -1' to denote the INTx disabled, vfio_pci_disable_intx()
>   and vfio_pci_enable_intx will directly bail out when detect INTx has
>   been disabled and enabled respectively.
> 
> - Since pci_device_header will be corrupted after PCI configuration
>   and all irq related info will be lost.  Before re-enabling INTx
>   mode, this patch restores 'irq_pin' and 'irq_line' fields in struct
>   pci_device_header.
> 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  vfio/pci.c | 59 ++++++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 48 insertions(+), 11 deletions(-)
> 
> diff --git a/vfio/pci.c b/vfio/pci.c
> index d025581..ba971eb 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,34 @@ 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) {
> +		/*
> +		 * Struct pci_device_header is not only used for header,
> +		 * it also is used for PCI configuration; and in the function
> +		 * vfio_pci_cfg_write() it firstly writes configuration space
> +		 * and then read back the configuration space data into the
> +		 * header structure; thus 'irq_pin' and 'irq_line' in the
> +		 * header will be overwritten.
> +		 *
> +		 * If want to enable INTx mode properly, firstly needs to
> +		 * restore 'irq_pin' and 'irq_line' values; we can simply set 1
> +		 * to 'irq_pin', and 'pdev->intx_gsi' keeps gsi value when
> +		 * enable INTx mode previously so we can simply use it to
> +		 * recover irq line number by adding offset KVM_IRQ_OFFSET.
> +		 */
> +		pdev->hdr.irq_pin = 1;
> +		pdev->hdr.irq_line = pdev->intx_gsi + KVM_IRQ_OFFSET;

That doesn't look right. We shouldn't change irq_line at runtime, it's
reserved to the guest (and irq_pin is static). Maybe move the bits that
should only be done once (intx_gsi setup, and also the
VFIO_DEVICE_GET_IRQ_INFO sanity check) outside of the enable() function,
like we do for msis?

Thanks,
Jean

> +
> +		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 +1027,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 +1038,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)
> @@ -1025,6 +1055,10 @@ static int vfio_pci_enable_intx(struct kvm *kvm, struct vfio_device *vdev)
>  		.index = VFIO_PCI_INTX_IRQ_INDEX,
>  	};
>  
> +	/* INTx mode has been enabled */
> +	if (pdev->intx_fd != -1)
> +		return 0;
> +
>  	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");
> @@ -1140,6 +1174,9 @@ static int vfio_pci_configure_dev_irqs(struct kvm *kvm, struct vfio_device *vdev
>  			return ret;
>  	}
>  
> +	/* Use intx_fd=-1 to denote INTx is disabled */
> +	pdev->intx_fd = -1;
> +
>  	if (pdev->irq_modes & VFIO_PCI_IRQ_MODE_INTX)
>  		ret = vfio_pci_enable_intx(kvm, vdev);
>  
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH kvmtool v2 3/3] vfio-pci: Re-enable INTx mode when disable MSI/MSIX
  2019-03-25 18:28   ` Jean-Philippe Brucker
@ 2019-03-26  3:17     ` Leo Yan
  0 siblings, 0 replies; 8+ messages in thread
From: Leo Yan @ 2019-03-26  3:17 UTC (permalink / raw)
  To: Jean-Philippe Brucker
  Cc: kvm, Marc Zyngier, Will Deacon, Robin Murphy, kvmarm

Hi Jean-Philippe,

On Mon, Mar 25, 2019 at 06:28:40PM +0000, Jean-Philippe Brucker wrote:

[...]

> > @@ -162,7 +160,34 @@ 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) {
> > +		/*
> > +		 * Struct pci_device_header is not only used for header,
> > +		 * it also is used for PCI configuration; and in the function
> > +		 * vfio_pci_cfg_write() it firstly writes configuration space
> > +		 * and then read back the configuration space data into the
> > +		 * header structure; thus 'irq_pin' and 'irq_line' in the
> > +		 * header will be overwritten.
> > +		 *
> > +		 * If want to enable INTx mode properly, firstly needs to
> > +		 * restore 'irq_pin' and 'irq_line' values; we can simply set 1
> > +		 * to 'irq_pin', and 'pdev->intx_gsi' keeps gsi value when
> > +		 * enable INTx mode previously so we can simply use it to
> > +		 * recover irq line number by adding offset KVM_IRQ_OFFSET.
> > +		 */
> > +		pdev->hdr.irq_pin = 1;
> > +		pdev->hdr.irq_line = pdev->intx_gsi + KVM_IRQ_OFFSET;
> 
> That doesn't look right. We shouldn't change irq_line at runtime, it's
> reserved to the guest (and irq_pin is static). Maybe move the bits that
> should only be done once (intx_gsi setup, and also the
> VFIO_DEVICE_GET_IRQ_INFO sanity check) outside of the enable() function,
> like we do for msis?

Agree this and another comment in patch 0002.  Will spin new patch
set with following these good suggestions.

Thanks a lot for reviewing!

Thanks,
Leo Yan

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2019-03-26  3:17 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-20  6:20 [PATCH kvmtool v2 0/3] vfio-pci: Support INTx mode re-enabling Leo Yan
2019-03-20  6:20 ` [PATCH kvmtool v2 1/3] vfio-pci: Release INTx's unmask eventfd properly Leo Yan
2019-03-25 18:27   ` Jean-Philippe Brucker
2019-03-20  6:20 ` [PATCH kvmtool v2 2/3] vfio-pci: Remove useless FDs reservation in vfio_pci_enable_intx() Leo Yan
2019-03-25 18:28   ` Jean-Philippe Brucker
2019-03-20  6:20 ` [PATCH kvmtool v2 3/3] vfio-pci: Re-enable INTx mode when disable MSI/MSIX Leo Yan
2019-03-25 18:28   ` Jean-Philippe Brucker
2019-03-26  3:17     ` Leo Yan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).