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@xxxxxxxxxx>
> > ---
> > 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,
> >
>
