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@xxxxxxx>
> > Signed-off-by: Leo Yan <leo.yan@xxxxxxxxxx>
> > ---
> > 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@xxxxxxx>
Thanks for reviewing. I will spin a new patch set to drop unnecessary
comment to let changes as simple as possible.
Thanks,
Leo Yan
