Hi, On 1/12/19 3:30 AM, Heyi Guo wrote: > Hi folks, > > I have some questions about vfio_msix_vector_do_use() in hw/vfio/pci.c, > could you help to explain? > > We can see that when guest tries to enable one specific MSIX vector by > unmasking MSIX Vector Control, the access will be trapped and then into > function vfio_msix_vector_do_use(). And we may go to the below branch in > line 525: > > > 520 > <https://git.qemu.org/?p=qemu.git;a=blob;f=hw/vfio/pci.c;h=c0cb1ec289084eb1593f24dc423e647f4b29eb74;hb=HEAD#l520> > /* > 521 > <https://git.qemu.org/?p=qemu.git;a=blob;f=hw/vfio/pci.c;h=c0cb1ec289084eb1593f24dc423e647f4b29eb74;hb=HEAD#l521> > * We don't want to have the host allocate all possible MSI vectors > 522 > <https://git.qemu.org/?p=qemu.git;a=blob;f=hw/vfio/pci.c;h=c0cb1ec289084eb1593f24dc423e647f4b29eb74;hb=HEAD#l522> > * for a device if they're not in use, so we shutdown and incrementally > 523 > <https://git.qemu.org/?p=qemu.git;a=blob;f=hw/vfio/pci.c;h=c0cb1ec289084eb1593f24dc423e647f4b29eb74;hb=HEAD#l523> > * increase them as needed. > 524 > <https://git.qemu.org/?p=qemu.git;a=blob;f=hw/vfio/pci.c;h=c0cb1ec289084eb1593f24dc423e647f4b29eb74;hb=HEAD#l524> > */ > 525 > <https://git.qemu.org/?p=qemu.git;a=blob;f=hw/vfio/pci.c;h=c0cb1ec289084eb1593f24dc423e647f4b29eb74;hb=HEAD#l525> > if (vdev->nr_vectors < nr + 1) { > 526 > <https://git.qemu.org/?p=qemu.git;a=blob;f=hw/vfio/pci.c;h=c0cb1ec289084eb1593f24dc423e647f4b29eb74;hb=HEAD#l526> > vfio_disable_irqindex(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX); > 527 > <https://git.qemu.org/?p=qemu.git;a=blob;f=hw/vfio/pci.c;h=c0cb1ec289084eb1593f24dc423e647f4b29eb74;hb=HEAD#l527> > vdev->nr_vectors = nr + 1; > 528 > <https://git.qemu.org/?p=qemu.git;a=blob;f=hw/vfio/pci.c;h=c0cb1ec289084eb1593f24dc423e647f4b29eb74;hb=HEAD#l528> > ret = vfio_enable_vectors(vdev, true); > 529 > <https://git.qemu.org/?p=qemu.git;a=blob;f=hw/vfio/pci.c;h=c0cb1ec289084eb1593f24dc423e647f4b29eb74;hb=HEAD#l529> > if (ret) { > 530 > <https://git.qemu.org/?p=qemu.git;a=blob;f=hw/vfio/pci.c;h=c0cb1ec289084eb1593f24dc423e647f4b29eb74;hb=HEAD#l530> > error_report("vfio: failed to enable vectors, %d", ret); > 531 > <https://git.qemu.org/?p=qemu.git;a=blob;f=hw/vfio/pci.c;h=c0cb1ec289084eb1593f24dc423e647f4b29eb74;hb=HEAD#l531> > } > > Here all MSIX vectors will be disabled first and then enabled, with one > more MSIX. The comment is there but I still don't quite understand. It > makes sense for not allocating all possible MSI vectors, but why shall > we shutdown the whole MSI when being requested to enable one specific > vector? Can't we just enable the user specified vector indexed by "nr"? This is not obvious to me either. vfio_enable_vectors() seems to handle the nr_vectors as a whole since the origin of the code. > > What's more, on ARM64 systems with GIC ITS, the kernel will issue an ITS > discard command when disabling a MSI vector, which will drop currently > pending MSI interrupt. If device driver in guest system enables some > MSIs first and interrupts may come at any time, and then it tries to > enable other MSIs, is it possible for the above code to cause interrupts > missing?
Seems originally we considered this case of losing interrupts. This was then removed by "vfio-pci: No spurious MSIs" (98cd5a5eaf). Does this prevent any particular device from working? Thanks Eric > > I may misunderstand the whole thing... Any comment is appreciated. > > Thanks, > > Heyi >
