Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
Date:   Mon, 18 Sep 2023 20:32:19 -0300
From:   Jason Gunthorpe <jgg@ziepe.ca>
To:     "Nelson, Shannon" <shannon.nelson@amd.com>
Cc:     alex.williamson@redhat.com, kevin.tian@intel.com,
        reinette.chatre@intel.com, tglx@linutronix.de, kvm@vger.kernel.org,
        brett.creeley@amd.com, linux-kernel@vger.kernel.org,
        Leon Romanovsky <leonro@nvidia.com>
Subject: Re: [PATCH vfio] vfio/pci: remove msi domain on msi disable
Message-ID: <20230918233219.GO13795@ziepe.ca>
References: <20230914191406.54656-1-shannon.nelson@amd.com>
 <20230918141705.GE13795@ziepe.ca>
 <acfa5d59-242b-4b31-a3ef-b4163972f26b@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <acfa5d59-242b-4b31-a3ef-b4163972f26b@amd.com>
Precedence: bulk

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