Hi Jean-Philippe,
On Mon, Mar 25, 2019 at 06:28:40PM +0000, Jean-Philippe Brucker wrote:
[...]
> > @@ -162,7 +160,34 @@ 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) {
> > + /*
> > + * Struct pci_device_header is not only used for header,
> > + * it also is used for PCI configuration; and in the function
> > + * vfio_pci_cfg_write() it firstly writes configuration space
> > + * and then read back the configuration space data into the
> > + * header structure; thus 'irq_pin' and 'irq_line' in the
> > + * header will be overwritten.
> > + *
> > + * If want to enable INTx mode properly, firstly needs to
> > + * restore 'irq_pin' and 'irq_line' values; we can simply set 1
> > + * to 'irq_pin', and 'pdev->intx_gsi' keeps gsi value when
> > + * enable INTx mode previously so we can simply use it to
> > + * recover irq line number by adding offset KVM_IRQ_OFFSET.
> > + */
> > + pdev->hdr.irq_pin = 1;
> > + pdev->hdr.irq_line = pdev->intx_gsi + KVM_IRQ_OFFSET;
>
> That doesn't look right. We shouldn't change irq_line at runtime, it's
> reserved to the guest (and irq_pin is static). Maybe move the bits that
> should only be done once (intx_gsi setup, and also the
> VFIO_DEVICE_GET_IRQ_INFO sanity check) outside of the enable() function,
> like we do for msis?
Agree this and another comment in patch 0002. Will spin new patch
set with following these good suggestions.
Thanks a lot for reviewing!
Thanks,
Leo Yan
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
