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