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 <[email protected]> > --- > 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, > _______________________________________________ kvmarm mailing list [email protected] https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
