[PATCH kvmtool v1 1/2] vfio-pci: Release INTx's guest to host eventfd properly

[PATCH kvmtool v1 1/2] vfio-pci: Release INTx's guest to host 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.

When disable INTx this patch firstly unbinds interrupt signal by calling
ioctl VFIO_DEVICE_SET_IRQS and then it uses the new added field
'unmask_fd' in struct vfio_pci_device to close event fd.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 include/kvm/vfio.h |  1 +
 vfio/pci.c         | 15 ++++++++++++---
 2 files changed, 13 insertions(+), 3 deletions(-)

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..c0683f6 100644
--- a/vfio/pci.c
+++ b/vfio/pci.c
@@ -996,18 +996,26 @@ static void vfio_pci_disable_intx(struct kvm *kvm, struct vfio_device *vdev)
 {
 	struct vfio_pci_device *pdev = &vdev->pci;
 	int gsi = pdev->intx_gsi;
-	struct vfio_irq_set irq_set = {
-		.argsz	= sizeof(irq_set),
+	struct vfio_irq_set trigger_irq = {
+		.argsz	= sizeof(trigger_irq),
 		.flags	= VFIO_IRQ_SET_DATA_NONE | VFIO_IRQ_SET_ACTION_TRIGGER,
 		.index	= VFIO_PCI_INTX_IRQ_INDEX,
 	};
 
+	struct vfio_irq_set unmask_irq = {
+		.argsz	= sizeof(unmask_irq),
+		.flags	= VFIO_IRQ_SET_DATA_NONE | VFIO_IRQ_SET_ACTION_UNMASK,
+		.index	= VFIO_PCI_INTX_IRQ_INDEX,
+	};
+
 	pr_debug("user requested MSI, disabling INTx %d", gsi);
 
-	ioctl(vdev->fd, VFIO_DEVICE_SET_IRQS, &irq_set);
+	ioctl(vdev->fd, VFIO_DEVICE_SET_IRQS, &trigger_irq);
+	ioctl(vdev->fd, VFIO_DEVICE_SET_IRQS, &unmask_irq);
 	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 +1103,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 v1 2/2] vfio-pci: Fallback to 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.  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.

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

diff --git a/vfio/pci.c b/vfio/pci.c
index c0683f6..44727bb 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,7 +51,7 @@ 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) {
+	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
@@ -58,9 +59,6 @@ static int vfio_pci_enable_msis(struct kvm *kvm, struct vfio_device *vdev,
 		 * disable it now.
 		 */
 		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;
+	if (pdev->irq_modes & VFIO_PCI_IRQ_MODE_INTX)
+		/*
+		 * 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.
+		 */
+		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,
-- 
2.19.1

Re: [PATCH kvmtool v1 1/2] vfio-pci: Release INTx's guest to host eventfd properly

[Jean-Philippe Brucker]

Hi,

On 15/03/2019 08:33, 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.
> 
> When disable INTx this patch firstly unbinds interrupt signal by calling
> ioctl VFIO_DEVICE_SET_IRQS and then it uses the new added field
> 'unmask_fd' in struct vfio_pci_device to close event fd.
> 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  include/kvm/vfio.h |  1 +
>  vfio/pci.c         | 15 ++++++++++++---
>  2 files changed, 13 insertions(+), 3 deletions(-)
> 
> 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..c0683f6 100644
> --- a/vfio/pci.c
> +++ b/vfio/pci.c
> @@ -996,18 +996,26 @@ static void vfio_pci_disable_intx(struct kvm *kvm, struct vfio_device *vdev)
>  {
>  	struct vfio_pci_device *pdev = &vdev->pci;
>  	int gsi = pdev->intx_gsi;
> -	struct vfio_irq_set irq_set = {
> -		.argsz	= sizeof(irq_set),
> +	struct vfio_irq_set trigger_irq = {
> +		.argsz	= sizeof(trigger_irq),
>  		.flags	= VFIO_IRQ_SET_DATA_NONE | VFIO_IRQ_SET_ACTION_TRIGGER,
>  		.index	= VFIO_PCI_INTX_IRQ_INDEX,
>  	};
>  
> +	struct vfio_irq_set unmask_irq = {
> +		.argsz	= sizeof(unmask_irq),
> +		.flags	= VFIO_IRQ_SET_DATA_NONE | VFIO_IRQ_SET_ACTION_UNMASK,
> +		.index	= VFIO_PCI_INTX_IRQ_INDEX,
> +	};
> +
>  	pr_debug("user requested MSI, disabling INTx %d", gsi);
>  
> -	ioctl(vdev->fd, VFIO_DEVICE_SET_IRQS, &irq_set);
> +	ioctl(vdev->fd, VFIO_DEVICE_SET_IRQS, &trigger_irq);
> +	ioctl(vdev->fd, VFIO_DEVICE_SET_IRQS, &unmask_irq);

The patch makes sense, we do need to close unmask_fd, but I don't think
we need the additional ioctl. VFIO removes the unmask trigger when we
disable INTx in the first ioctl, so an additional ioctl to remove the
unmask trigger will return EINVAL.

Thanks,
Jean

>  	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 +1103,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;
>  
> 

Re: [PATCH kvmtool v1 2/2] vfio-pci: Fallback to INTx mode when disable MSI/MSIX

[Jean-Philippe Brucker]

On 15/03/2019 08:33, 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.
> 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  vfio/pci.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/vfio/pci.c b/vfio/pci.c
> index c0683f6..44727bb 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,7 +51,7 @@ 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) {
> +	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
> @@ -58,9 +59,6 @@ static int vfio_pci_enable_msis(struct kvm *kvm, struct vfio_device *vdev,
>  		 * disable it now.
>  		 */
>  		vfio_pci_disable_intx(kvm, vdev);
> -		/* Permanently disable INTx */
> -		pdev->irq_modes &= ~VFIO_PCI_IRQ_MODE_INTX;

As a result vfio_pci_disable_intx() may be called multiple times (each
time the guest enables one MSI vector). Could you make
vfio_pci_disable_intx() safe against that (maybe use intx_fd == -1 to
denote the INTx state)?

> -	}
>  
>  	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;
> +	if (pdev->irq_modes & VFIO_PCI_IRQ_MODE_INTX)
> +		/*
> +		 * 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.
> +		 */
> +		ret = vfio_pci_enable_intx(kvm, vdev);

Let's remove vfio_pci_reserve_irq_fds(2) from vfio_pci_enable_intx(), it
should only called once per run, and isn't particularly useful here
since INTx only uses 2 fds. It's used to bump the fd rlimit when a
device needs ~2048 file descriptors for MSI-X.

Thanks,
Jean

> +
> +	return ret >= 0 ? 0 : ret;
>  }
>  
>  static int vfio_pci_update_msi_entry(struct kvm *kvm, struct vfio_device *vdev,
> 

Re: [PATCH kvmtool v1 2/2] vfio-pci: Fallback to INTx mode when disable MSI/MSIX

[Leo Yan]

Hi Jean-Philippe,

On Fri, Mar 15, 2019 at 02:20:07PM +0000, Jean-Philippe Brucker wrote:
> On 15/03/2019 08:33, 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.
> > 
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > ---
> >  vfio/pci.c | 17 ++++++++++++-----
> >  1 file changed, 12 insertions(+), 5 deletions(-)
> > 
> > diff --git a/vfio/pci.c b/vfio/pci.c
> > index c0683f6..44727bb 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,7 +51,7 @@ 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) {
> > +	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
> > @@ -58,9 +59,6 @@ static int vfio_pci_enable_msis(struct kvm *kvm, struct vfio_device *vdev,
> >  		 * disable it now.
> >  		 */
> >  		vfio_pci_disable_intx(kvm, vdev);
> > -		/* Permanently disable INTx */
> > -		pdev->irq_modes &= ~VFIO_PCI_IRQ_MODE_INTX;
> 
> As a result vfio_pci_disable_intx() may be called multiple times (each
> time the guest enables one MSI vector). Could you make
> vfio_pci_disable_intx() safe against that (maybe use intx_fd == -1 to
> denote the INTx state)?

Will do this.

> > -	}
> >  
> >  	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;
> > +	if (pdev->irq_modes & VFIO_PCI_IRQ_MODE_INTX)
> > +		/*
> > +		 * 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.
> > +		 */
> > +		ret = vfio_pci_enable_intx(kvm, vdev);
> 
> Let's remove vfio_pci_reserve_irq_fds(2) from vfio_pci_enable_intx(), it
> should only called once per run, and isn't particularly useful here
> since INTx only uses 2 fds. It's used to bump the fd rlimit when a
> device needs ~2048 file descriptors for MSI-X.

Understand; will do it.

Thanks a lot for very detailed suggestions.

> > +
> > +	return ret >= 0 ? 0 : ret;
> >  }
> >  
> >  static int vfio_pci_update_msi_entry(struct kvm *kvm, struct vfio_device *vdev,
> > 
>