Re: [PATCH vfio] vfio/pci: remove msi domain on msi disable

[Jason Gunthorpe]

On Mon, Sep 18, 2023 at 10:48:54AM -0700, Nelson, Shannon wrote:

> In our case, the VF device's msix count value found in PCI config space is
> changed by device configuration management outside of the baremetal host and
> read by the QEMU instance when it starts up, and then read by the vfio PCI
> core when QEMU requests the first IRQ.  

Oh, you definitely can't do that!

PCI config space is not allowed to change outside the OS's view and we
added sriov_set_msix_vec_count() specifically as a way to provide the
necessary synchronization between all the parts.

Randomly changing, what should be immutable, parts of the config space
from under a running OS is just non-compliant PCI behavior.

> The msix vectors are freed, but the msi_domain is not, and the msi_domain
> holds the MSIx count that it read when it was created.  If the device's MSIx
> count is increased, the next QEMU session will see the new number in PCI
> config space and try to use that new larger number, but the msi_domain is
> still using the smaller hwsize and the QEMU IRQ setup fails in
> msi_insert_desc().

Correct, devices are not allowed to change these parameters
autonomously, so there is no reason to accommodate this.

> This patch adds a msi_remove_device_irq_domain() call when the irqs are
> disabled in order to force a new read on the next IRQ allocation cycle. This
> is limited to only the vfio use of the msi_domain.

Definately no.
 
> I suppose we could add this to the trailing end of callbacks in our own
> driver, but this looks more like a generic vfio/msi issue than a driver
> specific thing.

Certainly not.
 
> The other possibility is to force the user to always do a bind cycle between
> QEMU sessions using the VF.  This seems to be unnecessary overhead and was
> not necessary when using the v6.1 kernel.  To the user, this looks like a
> regression - this is how it was reported to me.

You need to use sriov_set_msix_vec_count() and only
sriov_set_msix_vec_count() to change this parameter or I expect you
will constantly experience problems.

Jason