Re: [Qemu-devel] Questions about VFIO enabling MSIX vector

[Auger Eric <eric.auger@redhat.com>]

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
>