| Message ID | 20230914191406.54656-1-shannon.nelson@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [vfio] vfio/pci: remove msi domain on msi disable | expand |
On Thu, Sep 14, 2023 at 12:14:06PM -0700, Shannon Nelson wrote: > The new MSI dynamic allocation machinery is great for making the irq > management more flexible. It includes caching information about the > MSI domain which gets reused on each new open of a VFIO fd. However, > this causes an issue when the underlying hardware has flexible MSI-x > configurations, as a changed configuration doesn't get seen between > new opens, and is only refreshed between PCI unbind/bind cycles. > > In our device we can change the per-VF MSI-x resource allocation > without the need for rebooting or function reset. For example, > > 1. Initial power up and kernel boot: > # lspci -s 2e:00.1 -vv | grep MSI-X > Capabilities: [a0] MSI-X: Enable+ Count=8 Masked- > > 2. Device VF configuration change happens with no reset Is this an out of tree driver problem? The intree way to alter the MSI configuration is via sriov_set_msix_vec_count, and there is only one in-tree driver that uses it right now. If something is going wrong here it should be fixed in the sriov_set_msix_vec_count() machinery, possibly in the pci core to synchronize the msi_domain view of the world. Jason
On 9/18/2023 7:17 AM, Jason Gunthorpe wrote: > > On Thu, Sep 14, 2023 at 12:14:06PM -0700, Shannon Nelson wrote: >> The new MSI dynamic allocation machinery is great for making the irq >> management more flexible. It includes caching information about the >> MSI domain which gets reused on each new open of a VFIO fd. However, >> this causes an issue when the underlying hardware has flexible MSI-x >> configurations, as a changed configuration doesn't get seen between >> new opens, and is only refreshed between PCI unbind/bind cycles. >> >> In our device we can change the per-VF MSI-x resource allocation >> without the need for rebooting or function reset. For example, >> >> 1. Initial power up and kernel boot: >> # lspci -s 2e:00.1 -vv | grep MSI-X >> Capabilities: [a0] MSI-X: Enable+ Count=8 Masked- >> >> 2. Device VF configuration change happens with no reset > > Is this an out of tree driver problem? No, not an out-of-tree driver, this is the vfio pci core. > > The intree way to alter the MSI configuration is via > sriov_set_msix_vec_count, and there is only one in-tree driver that > uses it right now. > > If something is going wrong here it should be fixed in the > sriov_set_msix_vec_count() machinery, possibly in the pci core to > synchronize the msi_domain view of the world. > > Jason The sriov_set_msix_vec_count method assumes (a) the unbind/bind cycle on the VF, and (b) VF MSIx count change configured from the host neither of which are the case in our situation. 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. The core code enables the msix range on first IRQ request, and disables it when the vfio fd is closed. It creates the msi_domain on the first call if it doesn't exist, but it does not remove the msi_domain when the irqs are disabled. The IRQ request call trace looks like: QEMU: vfio_msix_vector_do_use->ioctl() (driver.vfio_device_ops.ioctl = vfio_pci_core_ioctl) vfio_pci_core_ioctl vfio_pci_ioctl_set_irqs vfio_pci_set_irqs_ioctl vfio_pci_set_msi_trigger vfio_msi_enable pci_alloc_irq_vectors pci_alloc_irq_vectors_affinity __pci_enable_msix_range pci_setup_msix_device_domain return if msi_domain exists pci_create_device_domain msi_create_device_irq_domain __msi_create_irq_domain - sets info->hwsize msi_capability_init msi_setup_msi_desc msi_insert_msi_desc msi_domain_insert_msi_desc msi_insert_desc fail if index >= hwsize On close of the vfio fd, the trace is: QEMU: close() driver.vfio_device_ops.close vfio_pci_core_close_device vfio_pci_core_disable vfio_pci_set_irqs_ioctl(vdev, VFIO_IRQ_SET_DATA_NONE | VFIO_IRQ_SET_ACTION_TRIGGER, vdev->irq_type, 0, 0, NULL); vfio_pci_set_msi_trigger vfio_msi_disable pci_free_irq_vectors 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(). 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. 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. 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. sln
On Mon, Sep 18 2023 at 11:17, Jason Gunthorpe wrote: > On Thu, Sep 14, 2023 at 12:14:06PM -0700, Shannon Nelson wrote: >> The new MSI dynamic allocation machinery is great for making the irq >> management more flexible. It includes caching information about the >> MSI domain which gets reused on each new open of a VFIO fd. However, >> this causes an issue when the underlying hardware has flexible MSI-x >> configurations, as a changed configuration doesn't get seen between >> new opens, and is only refreshed between PCI unbind/bind cycles. >> >> In our device we can change the per-VF MSI-x resource allocation >> without the need for rebooting or function reset. For example, >> >> 1. Initial power up and kernel boot: >> # lspci -s 2e:00.1 -vv | grep MSI-X >> Capabilities: [a0] MSI-X: Enable+ Count=8 Masked- >> >> 2. Device VF configuration change happens with no reset > > Is this an out of tree driver problem? > > The intree way to alter the MSI configuration is via > sriov_set_msix_vec_count, and there is only one in-tree driver that > uses it right now. Right, but that only addresses the driver specific issues. > If something is going wrong here it should be fixed in the > sriov_set_msix_vec_count() machinery, possibly in the pci core to > synchronize the msi_domain view of the world. Right, we should definitely not do that on a per driver basis. Thanks, tglx
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
On Mon, Sep 18, 2023 at 08:43:21PM +0200, Thomas Gleixner wrote: > On Mon, Sep 18 2023 at 11:17, Jason Gunthorpe wrote: > > On Thu, Sep 14, 2023 at 12:14:06PM -0700, Shannon Nelson wrote: > >> The new MSI dynamic allocation machinery is great for making the irq > >> management more flexible. It includes caching information about the > >> MSI domain which gets reused on each new open of a VFIO fd. However, > >> this causes an issue when the underlying hardware has flexible MSI-x > >> configurations, as a changed configuration doesn't get seen between > >> new opens, and is only refreshed between PCI unbind/bind cycles. > >> > >> In our device we can change the per-VF MSI-x resource allocation > >> without the need for rebooting or function reset. For example, > >> > >> 1. Initial power up and kernel boot: > >> # lspci -s 2e:00.1 -vv | grep MSI-X > >> Capabilities: [a0] MSI-X: Enable+ Count=8 Masked- > >> > >> 2. Device VF configuration change happens with no reset > > > > Is this an out of tree driver problem? > > > > The intree way to alter the MSI configuration is via > > sriov_set_msix_vec_count, and there is only one in-tree driver that > > uses it right now. > > Right, but that only addresses the driver specific issues. Sort of.. sriov_vf_msix_count_store() is intended to be the entry point for this and if the kernel grows places that cache the value or something then this function should flush those caches too. I suppose flushing happens implicitly because Shannon reports that things work fine if the driver is rebound. Since sriov_vf_msix_count_store() ensures there is no driver bound before proceeding it probe/unprobe must be flushing out everything? Jason
On Mon, Sep 18 2023 at 20:37, Jason Gunthorpe wrote: > On Mon, Sep 18, 2023 at 08:43:21PM +0200, Thomas Gleixner wrote: >> On Mon, Sep 18 2023 at 11:17, Jason Gunthorpe wrote: >> > On Thu, Sep 14, 2023 at 12:14:06PM -0700, Shannon Nelson wrote: >> >> The new MSI dynamic allocation machinery is great for making the irq >> >> management more flexible. It includes caching information about the >> >> MSI domain which gets reused on each new open of a VFIO fd. However, >> >> this causes an issue when the underlying hardware has flexible MSI-x >> >> configurations, as a changed configuration doesn't get seen between >> >> new opens, and is only refreshed between PCI unbind/bind cycles. >> >> >> >> In our device we can change the per-VF MSI-x resource allocation >> >> without the need for rebooting or function reset. For example, >> >> >> >> 1. Initial power up and kernel boot: >> >> # lspci -s 2e:00.1 -vv | grep MSI-X >> >> Capabilities: [a0] MSI-X: Enable+ Count=8 Masked- >> >> >> >> 2. Device VF configuration change happens with no reset >> > >> > Is this an out of tree driver problem? >> > >> > The intree way to alter the MSI configuration is via >> > sriov_set_msix_vec_count, and there is only one in-tree driver that >> > uses it right now. >> >> Right, but that only addresses the driver specific issues. > > Sort of.. sriov_vf_msix_count_store() is intended to be the entry > point for this and if the kernel grows places that cache the value or > something then this function should flush those caches too. Sorry. What I wanted to say is that the driver callback is not the right place to reload the MSI domains after the change. > I suppose flushing happens implicitly because Shannon reports that > things work fine if the driver is rebound. Since > sriov_vf_msix_count_store() ensures there is no driver bound before > proceeding it probe/unprobe must be flushing out everything? Correct. So sriov_set_msix_vec_count() could just do: ret = pdev->driver->sriov_set_msix_vec_count(vf_dev, val); if (!ret) teardown_msi_domain(pdev); Right? Thanks, tglx
On Tue, Sep 19, 2023 at 01:47:37AM +0200, Thomas Gleixner wrote: > On Mon, Sep 18 2023 at 20:37, Jason Gunthorpe wrote: > > On Mon, Sep 18, 2023 at 08:43:21PM +0200, Thomas Gleixner wrote: > >> On Mon, Sep 18 2023 at 11:17, Jason Gunthorpe wrote: > >> > On Thu, Sep 14, 2023 at 12:14:06PM -0700, Shannon Nelson wrote: > >> >> The new MSI dynamic allocation machinery is great for making the irq > >> >> management more flexible. It includes caching information about the > >> >> MSI domain which gets reused on each new open of a VFIO fd. However, > >> >> this causes an issue when the underlying hardware has flexible MSI-x > >> >> configurations, as a changed configuration doesn't get seen between > >> >> new opens, and is only refreshed between PCI unbind/bind cycles. > >> >> > >> >> In our device we can change the per-VF MSI-x resource allocation > >> >> without the need for rebooting or function reset. For example, > >> >> > >> >> 1. Initial power up and kernel boot: > >> >> # lspci -s 2e:00.1 -vv | grep MSI-X > >> >> Capabilities: [a0] MSI-X: Enable+ Count=8 Masked- > >> >> > >> >> 2. Device VF configuration change happens with no reset > >> > > >> > Is this an out of tree driver problem? > >> > > >> > The intree way to alter the MSI configuration is via > >> > sriov_set_msix_vec_count, and there is only one in-tree driver that > >> > uses it right now. > >> > >> Right, but that only addresses the driver specific issues. > > > > Sort of.. sriov_vf_msix_count_store() is intended to be the entry > > point for this and if the kernel grows places that cache the value or > > something then this function should flush those caches too. > > Sorry. What I wanted to say is that the driver callback is not the right > place to reload the MSI domains after the change. Oh, that isn't even what Shannon's patch does, it patched VFIO's main PCI driver - not a sriov_set_msix_vec_count() callback :( Shannon's scenario doesn't even use sriov_vf_msix_count_store() at all - the AMD device just randomly changes its MSI count whenever it likes. > > I suppose flushing happens implicitly because Shannon reports that > > things work fine if the driver is rebound. Since > > sriov_vf_msix_count_store() ensures there is no driver bound before > > proceeding it probe/unprobe must be flushing out everything? > > Correct. So sriov_set_msix_vec_count() could just do: > > ret = pdev->driver->sriov_set_msix_vec_count(vf_dev, val); > if (!ret) > teardown_msi_domain(pdev); > > Right? It subtly isn't needed, sriov_vf_msix_count_store() already requires no driver is associated with the device and this: int msi_setup_device_data(struct device *dev) { struct msi_device_data *md; int ret, i; if (dev->msi.data) return 0; md = devres_alloc(msi_device_data_release, sizeof(*md), GFP_KERNEL); if (!md) return -ENOMEM; Already ensured that msi_remove_device_irq_domain() was called via msi_device_data_release() triggering as part of the devm shutdown of the bound driver. So, the intree mechanism to change the MSI vector size works. The crazy mechanism where the device just changes its value without synchronizing to the OS does not. I don't think we need to try and fix that.. Jason
On 9/18/2023 4:32 PM, Jason Gunthorpe wrote: > > 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. Hmmm... I guess I need to have a little chat with my friendly HW/FW folks. Thanks, sln
On Mon, Sep 18 2023 at 21:02, Jason Gunthorpe wrote: > On Tue, Sep 19, 2023 at 01:47:37AM +0200, Thomas Gleixner wrote: >> >> > The intree way to alter the MSI configuration is via >> >> > sriov_set_msix_vec_count, and there is only one in-tree driver that >> >> > uses it right now. >> >> >> >> Right, but that only addresses the driver specific issues. >> > >> > Sort of.. sriov_vf_msix_count_store() is intended to be the entry >> > point for this and if the kernel grows places that cache the value or >> > something then this function should flush those caches too. >> >> Sorry. What I wanted to say is that the driver callback is not the right >> place to reload the MSI domains after the change. > > Oh, that isn't even what Shannon's patch does, it patched VFIO's main > PCI driver - not a sriov_set_msix_vec_count() callback :( Shannon's > scenario doesn't even use sriov_vf_msix_count_store() at all - the AMD > device just randomly changes its MSI count whenever it likes. Ooops. When real hardware changes things behind the kernels back we consider it a hardware bug. The same applies to virtualization muck. So all we should do is add some code which yells when the "hardware" plays silly buggers. >> > I suppose flushing happens implicitly because Shannon reports that >> > things work fine if the driver is rebound. Since >> > sriov_vf_msix_count_store() ensures there is no driver bound before >> > proceeding it probe/unprobe must be flushing out everything? >> >> Correct. So sriov_set_msix_vec_count() could just do: >> >> ret = pdev->driver->sriov_set_msix_vec_count(vf_dev, val); >> if (!ret) >> teardown_msi_domain(pdev); >> >> Right? > > It subtly isn't needed, sriov_vf_msix_count_store() already requires > no driver is associated with the device and this: > > int msi_setup_device_data(struct device *dev) > { > struct msi_device_data *md; > int ret, i; > > if (dev->msi.data) > return 0; > > md = devres_alloc(msi_device_data_release, sizeof(*md), GFP_KERNEL); > if (!md) > return -ENOMEM; > > Already ensured that msi_remove_device_irq_domain() was called via > msi_device_data_release() triggering as part of the devm shutdown of > the bound driver. Indeed. > So, the intree mechanism to change the MSI vector size works. The > crazy mechanism where the device just changes its value without > synchronizing to the OS does not. > > I don't think we need to try and fix that.. We might want to detect it and yell about it, right? Thanks, tglx
On Tue, Sep 19, 2023 at 02:25:32AM +0200, Thomas Gleixner wrote: > > I don't think we need to try and fix that.. > > We might want to detect it and yell about it, right? It strikes me as a good idea, yes. If it doesn't cost anything. Jason
On Mon, Sep 18 2023 at 21:32, Jason Gunthorpe wrote: > On Tue, Sep 19, 2023 at 02:25:32AM +0200, Thomas Gleixner wrote: > >> > I don't think we need to try and fix that.. >> >> We might want to detect it and yell about it, right? > > It strikes me as a good idea, yes. If it doesn't cost anything. It should not be expensive in the interrupt allocation/deallocation path, where hardware needs to be accessed anyway. So one extra read is not the end of the world. Thanks, tglx
diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c index cbb4bcbfbf83..f66d5e7e078b 100644 --- a/drivers/vfio/pci/vfio_pci_intrs.c +++ b/drivers/vfio/pci/vfio_pci_intrs.c @@ -538,6 +538,7 @@ static void vfio_msi_disable(struct vfio_pci_core_device *vdev, bool msix) cmd = vfio_pci_memory_lock_and_enable(vdev); pci_free_irq_vectors(pdev); vfio_pci_memory_unlock_and_restore(vdev, cmd); + msi_remove_device_irq_domain(&pdev->dev, MSI_DEFAULT_DOMAIN); /* * Both disable paths above use pci_intx_for_msi() to clear DisINTx
The new MSI dynamic allocation machinery is great for making the irq management more flexible. It includes caching information about the MSI domain which gets reused on each new open of a VFIO fd. However, this causes an issue when the underlying hardware has flexible MSI-x configurations, as a changed configuration doesn't get seen between new opens, and is only refreshed between PCI unbind/bind cycles. In our device we can change the per-VF MSI-x resource allocation without the need for rebooting or function reset. For example, 1. Initial power up and kernel boot: # lspci -s 2e:00.1 -vv | grep MSI-X Capabilities: [a0] MSI-X: Enable+ Count=8 Masked- 2. Device VF configuration change happens with no reset 3. New MSI-x count value seen: # lspci -s 2e:00.1 -vv | grep MSI-X Capabilities: [a0] MSI-X: Enable- Count=64 Masked- This allows for per-VF dynamic reconfiguration of interrupt resources for the VMs using the VFIO devices supported by our hardware. The problem comes where the dynamic IRQ management created the MSI domain when the VFIO device creates the first IRQ in the first ioctl() VFIO_DEVICE_SET_IRQS request. The current MSI-x count (hwsize) is read when setting up the irq vectors under pci_alloc_irq_vectors_affinity(), and the MSI domain information is set up, which includes the hwsize. When the VFIO fd is closed, the IRQs are removed, but the MSI domain information is kept for later use since we're only closing the current VFIO fd, not unbinding the PCI device connection. When a new VFIO fd open happens and a new VFIO_DEVICE_SET_IRQS request comes down, the cycle starts again, reusing the existing MSI domain with the previous hwsize. This is fine until this new QEMU instance has read the new larger MSI-x count from PCI config space (QEMU:vfio_msi_enable()) and tries to create more IRQs than were available before. We fail in msi_insert_desc() because the MSI domain still is set up for the earlier hwsize and has no room for the n+1 IRQ. This can be easily fixed by simply adding msi_remove_device_irq_domain() into vfio_msi_disable() which is called when the VFIO IRQs are removed either by an ioctl() call from QEMU or from the VFIO fd close. This forces the MSI domain to be recreated with the new MSI-x count on the next VFIO_DEVICE_SET_IRQS request. Link: https://lore.kernel.org/all/cover.1683740667.git.reinette.chatre@intel.com/ Link: https://lore.kernel.org/r/20221124232325.798556374@linutronix.de Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> --- drivers/vfio/pci/vfio_pci_intrs.c | 1 + 1 file changed, 1 insertion(+)