2023-05-11 15:53:10

by Reinette Chatre

[permalink] [raw]
Subject: [PATCH V5 00/11] vfio/pci: Support dynamic allocation of MSI-X interrupts

Changes since V4:
- V4: https://lore.kernel.org/lkml/[email protected]/
- Add Kevin's Reviewed-by tag as applicable.
- Treat non-existing INTx interrupt context as kernel bug with WARN. This
exposed an issue in the scenario where INTx mask/unmask may occur without
INTx enabled. This is fixed by obtaining the interrupt context later
(right before use) within impacted functions: vfio_pci_intx_mask() and
vfio_pci_intx_unmask_handler(). (Kevin)
- Treat pci_irq_vector() returning '0' for a MSI/MSI-X interrupt as a kernel
bug via a WARN instead of ignoring this value. (Kevin)
- Improve accuracy of comments. (Kevin)
- Please refer to individual patches for local changes.

Changes since V3:
- V3: https://lore.kernel.org/lkml/[email protected]/
- Be considerate about layout and size with changes to
struct vfio_pci_core_device. Keep flags together and transition all to
use bitfields. (Alex and Jason)
- Do not free dynamically allocated interrupts on error path. (Alex)
- Please refer to individual patches for localized changes.

Changes since V2:
- V2: https://lore.kernel.org/lkml/[email protected]/
- During testing of V2 "kernel test robot" reported issues resulting from
include/linux/pci.h missing a stub for pci_msix_can_alloc_dyn() when
CONFIG_PCI_MSI=n. A separate fix was sent to address this. The fix can
be found in the kernel (since v6.3-rc7) as
commit 195d8e5da3ac ("PCI/MSI: Provide missing stub for pci_msix_can_alloc_dyn()")
- Biggest change is the transition to "active contexts" for both MSI and MSI-X.
Interrupt contexts have always been allocated when the interrupts are
allocated while they are only used while interrupts are
enabled. In this series interrupt contexts are made dynamic, while doing
so their allocation is moved to match how they are used: allocated when
interrupts are enabled. Whether a Linux interrupt number exists determines
whether an interrupt can be enabled.
Previous policy (up to V2) that an allocated interrupt has an interrupt
context no longer applies. Instead, an interrupt context has a
handler/trigger, aka "active contexts". (Alex)
- Re-ordered patches in support of "active contexts".
- Only free interrupts on MSI-X teardown and otherwise use the
allocated interrupts as a cache. (Alex)
- Using unsigned int for the vector broke the unwind loop within
vfio_msi_set_block(). (Alex)
- Introduce new "has_dyn_msix" property of virtual device instead of
querying support every time. (Alex)
- Some smaller changes, please refer to individual patches.

Changes since RFC V1:
- RFC V1: https://lore.kernel.org/lkml/[email protected]/
- Improved changelogs.
- Simplify interface so that vfio_irq_ctx_alloc_single() returns pointer to
allocated context. (Alex)
- Remove vfio_irq_ctx_range_allocated() and associated attempts to maintain
invalid error path behavior. (Alex and Kevin)
- Add pointer to interrupt context as function parameter to
vfio_irq_ctx_free(). (Alex)
- Ensure variables are initialized. (Dan Carpenter)
- Only support dynamic allocation if device supports it. (Alex)

Qemu allocates interrupts incrementally at the time the guest unmasks an
interrupt, for example each time a Linux guest runs request_irq().

Dynamic allocation of MSI-X interrupts was not possible until v6.2 [1].
This prompted Qemu to, when allocating a new interrupt, first release all
previously allocated interrupts (including disable of MSI-X) followed
by re-allocation of all interrupts that includes the new interrupt.
Please see [2] for a detailed discussion about this issue.

Releasing and re-allocating interrupts may be acceptable if all
interrupts are unmasked during device initialization. If unmasking of
interrupts occur during runtime this may result in lost interrupts.
For example, consider an accelerator device with multiple work queues,
each work queue having a dedicated interrupt. A work queue can be
enabled at any time with its associated interrupt unmasked while other
work queues are already active. Having all interrupts released and MSI-X
disabled to enable the new work queue will impact active work queues.

This series builds on the recent interrupt sub-system core changes
that added support for dynamic MSI-X allocation after initial MSI-X
enabling.

Add support for dynamic MSI-X allocation to vfio-pci. A flag
indicating lack of support for dynamic allocation already exist:
VFIO_IRQ_INFO_NORESIZE and has always been set for MSI and MSI-X. With
support for dynamic MSI-X the flag is cleared for MSI-X when supported,
enabling Qemu to modify its behavior.

Any feedback is appreciated

Reinette

[1] commit 34026364df8e ("PCI/MSI: Provide post-enable dynamic allocation interfaces for MSI-X")
[2] https://lore.kernel.org/kvm/MWHPR11MB188603D0D809C1079F5817DC8C099@MWHPR11MB1886.namprd11.prod.outlook.com/#t

Reinette Chatre (11):
vfio/pci: Consolidate irq cleanup on MSI/MSI-X disable
vfio/pci: Remove negative check on unsigned vector
vfio/pci: Prepare for dynamic interrupt context storage
vfio/pci: Move to single error path
vfio/pci: Use xarray for interrupt context storage
vfio/pci: Remove interrupt context counter
vfio/pci: Update stale comment
vfio/pci: Use bitfield for struct vfio_pci_core_device flags
vfio/pci: Probe and store ability to support dynamic MSI-X
vfio/pci: Support dynamic MSI-X
vfio/pci: Clear VFIO_IRQ_INFO_NORESIZE for MSI-X

drivers/vfio/pci/vfio_pci_core.c | 8 +-
drivers/vfio/pci/vfio_pci_intrs.c | 305 ++++++++++++++++++++----------
include/linux/vfio_pci_core.h | 26 +--
include/uapi/linux/vfio.h | 3 +
4 files changed, 229 insertions(+), 113 deletions(-)


base-commit: 457391b0380335d5e9a5babdec90ac53928b23b4
--
2.34.1



2023-05-11 15:53:37

by Reinette Chatre

[permalink] [raw]
Subject: [PATCH V5 07/11] vfio/pci: Update stale comment

In preparation for surrounding code change it is helpful to
ensure that existing comments are accurate.

Remove inaccurate comment about direct access and update
the rest of the comment to reflect the purpose of writing
the cached MSI message to the device.

Suggested-by: Alex Williamson <[email protected]>
Link: https://lore.kernel.org/lkml/[email protected]/
Signed-off-by: Reinette Chatre <[email protected]>
---
Changes since V4:
- Restore text about backdoor reset as example to indicate that
the restore may not be a severe problem but instead done in
collaboration with user space. (Kevin)

Changes since V2:
- New patch.

drivers/vfio/pci/vfio_pci_intrs.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
index e40eca69a293..867327e159c1 100644
--- a/drivers/vfio/pci/vfio_pci_intrs.c
+++ b/drivers/vfio/pci/vfio_pci_intrs.c
@@ -428,11 +428,9 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_core_device *vdev,
}

/*
- * The MSIx vector table resides in device memory which may be cleared
- * via backdoor resets. We don't allow direct access to the vector
- * table so even if a userspace driver attempts to save/restore around
- * such a reset it would be unsuccessful. To avoid this, restore the
- * cached value of the message prior to enabling.
+ * If the vector was previously allocated, refresh the on-device
+ * message data before enabling in case it had been cleared or
+ * corrupted (e.g. due to backdoor resets) since writing.
*/
cmd = vfio_pci_memory_lock_and_enable(vdev);
if (msix) {
--
2.34.1


2023-05-16 22:57:57

by Alex Williamson

[permalink] [raw]
Subject: Re: [PATCH V5 00/11] vfio/pci: Support dynamic allocation of MSI-X interrupts

On Thu, 11 May 2023 08:44:27 -0700
Reinette Chatre <[email protected]> wrote:

> Changes since V4:
> - V4: https://lore.kernel.org/lkml/[email protected]/
> - Add Kevin's Reviewed-by tag as applicable.
> - Treat non-existing INTx interrupt context as kernel bug with WARN. This
> exposed an issue in the scenario where INTx mask/unmask may occur without
> INTx enabled. This is fixed by obtaining the interrupt context later
> (right before use) within impacted functions: vfio_pci_intx_mask() and
> vfio_pci_intx_unmask_handler(). (Kevin)
> - Treat pci_irq_vector() returning '0' for a MSI/MSI-X interrupt as a kernel
> bug via a WARN instead of ignoring this value. (Kevin)
> - Improve accuracy of comments. (Kevin)
> - Please refer to individual patches for local changes.

Looks good to me.

Kevin, do you want to add any additional reviews or check the changes
made based on your previous comments?

Thanks,
Alex


> Changes since V3:
> - V3: https://lore.kernel.org/lkml/[email protected]/
> - Be considerate about layout and size with changes to
> struct vfio_pci_core_device. Keep flags together and transition all to
> use bitfields. (Alex and Jason)
> - Do not free dynamically allocated interrupts on error path. (Alex)
> - Please refer to individual patches for localized changes.
>
> Changes since V2:
> - V2: https://lore.kernel.org/lkml/[email protected]/
> - During testing of V2 "kernel test robot" reported issues resulting from
> include/linux/pci.h missing a stub for pci_msix_can_alloc_dyn() when
> CONFIG_PCI_MSI=n. A separate fix was sent to address this. The fix can
> be found in the kernel (since v6.3-rc7) as
> commit 195d8e5da3ac ("PCI/MSI: Provide missing stub for pci_msix_can_alloc_dyn()")
> - Biggest change is the transition to "active contexts" for both MSI and MSI-X.
> Interrupt contexts have always been allocated when the interrupts are
> allocated while they are only used while interrupts are
> enabled. In this series interrupt contexts are made dynamic, while doing
> so their allocation is moved to match how they are used: allocated when
> interrupts are enabled. Whether a Linux interrupt number exists determines
> whether an interrupt can be enabled.
> Previous policy (up to V2) that an allocated interrupt has an interrupt
> context no longer applies. Instead, an interrupt context has a
> handler/trigger, aka "active contexts". (Alex)
> - Re-ordered patches in support of "active contexts".
> - Only free interrupts on MSI-X teardown and otherwise use the
> allocated interrupts as a cache. (Alex)
> - Using unsigned int for the vector broke the unwind loop within
> vfio_msi_set_block(). (Alex)
> - Introduce new "has_dyn_msix" property of virtual device instead of
> querying support every time. (Alex)
> - Some smaller changes, please refer to individual patches.
>
> Changes since RFC V1:
> - RFC V1: https://lore.kernel.org/lkml/[email protected]/
> - Improved changelogs.
> - Simplify interface so that vfio_irq_ctx_alloc_single() returns pointer to
> allocated context. (Alex)
> - Remove vfio_irq_ctx_range_allocated() and associated attempts to maintain
> invalid error path behavior. (Alex and Kevin)
> - Add pointer to interrupt context as function parameter to
> vfio_irq_ctx_free(). (Alex)
> - Ensure variables are initialized. (Dan Carpenter)
> - Only support dynamic allocation if device supports it. (Alex)
>
> Qemu allocates interrupts incrementally at the time the guest unmasks an
> interrupt, for example each time a Linux guest runs request_irq().
>
> Dynamic allocation of MSI-X interrupts was not possible until v6.2 [1].
> This prompted Qemu to, when allocating a new interrupt, first release all
> previously allocated interrupts (including disable of MSI-X) followed
> by re-allocation of all interrupts that includes the new interrupt.
> Please see [2] for a detailed discussion about this issue.
>
> Releasing and re-allocating interrupts may be acceptable if all
> interrupts are unmasked during device initialization. If unmasking of
> interrupts occur during runtime this may result in lost interrupts.
> For example, consider an accelerator device with multiple work queues,
> each work queue having a dedicated interrupt. A work queue can be
> enabled at any time with its associated interrupt unmasked while other
> work queues are already active. Having all interrupts released and MSI-X
> disabled to enable the new work queue will impact active work queues.
>
> This series builds on the recent interrupt sub-system core changes
> that added support for dynamic MSI-X allocation after initial MSI-X
> enabling.
>
> Add support for dynamic MSI-X allocation to vfio-pci. A flag
> indicating lack of support for dynamic allocation already exist:
> VFIO_IRQ_INFO_NORESIZE and has always been set for MSI and MSI-X. With
> support for dynamic MSI-X the flag is cleared for MSI-X when supported,
> enabling Qemu to modify its behavior.
>
> Any feedback is appreciated
>
> Reinette
>
> [1] commit 34026364df8e ("PCI/MSI: Provide post-enable dynamic allocation interfaces for MSI-X")
> [2] https://lore.kernel.org/kvm/MWHPR11MB188603D0D809C1079F5817DC8C099@MWHPR11MB1886.namprd11.prod.outlook.com/#t
>
> Reinette Chatre (11):
> vfio/pci: Consolidate irq cleanup on MSI/MSI-X disable
> vfio/pci: Remove negative check on unsigned vector
> vfio/pci: Prepare for dynamic interrupt context storage
> vfio/pci: Move to single error path
> vfio/pci: Use xarray for interrupt context storage
> vfio/pci: Remove interrupt context counter
> vfio/pci: Update stale comment
> vfio/pci: Use bitfield for struct vfio_pci_core_device flags
> vfio/pci: Probe and store ability to support dynamic MSI-X
> vfio/pci: Support dynamic MSI-X
> vfio/pci: Clear VFIO_IRQ_INFO_NORESIZE for MSI-X
>
> drivers/vfio/pci/vfio_pci_core.c | 8 +-
> drivers/vfio/pci/vfio_pci_intrs.c | 305 ++++++++++++++++++++----------
> include/linux/vfio_pci_core.h | 26 +--
> include/uapi/linux/vfio.h | 3 +
> 4 files changed, 229 insertions(+), 113 deletions(-)
>
>
> base-commit: 457391b0380335d5e9a5babdec90ac53928b23b4


2023-05-17 02:19:29

by Tian, Kevin

[permalink] [raw]
Subject: RE: [PATCH V5 07/11] vfio/pci: Update stale comment

> From: Chatre, Reinette <[email protected]>
> Sent: Thursday, May 11, 2023 11:45 PM
>
> In preparation for surrounding code change it is helpful to
> ensure that existing comments are accurate.
>
> Remove inaccurate comment about direct access and update
> the rest of the comment to reflect the purpose of writing
> the cached MSI message to the device.
>
> Suggested-by: Alex Williamson <[email protected]>
> Link:
> https://lore.kernel.org/lkml/20230330164050.0069e2a5.alex.williamson@re
> dhat.com/
> Signed-off-by: Reinette Chatre <[email protected]>

Reviewed-by: Kevin Tian <[email protected]>

2023-05-17 02:28:11

by Tian, Kevin

[permalink] [raw]
Subject: RE: [PATCH V5 00/11] vfio/pci: Support dynamic allocation of MSI-X interrupts

> From: Alex Williamson <[email protected]>
> Sent: Wednesday, May 17, 2023 6:53 AM
>
> On Thu, 11 May 2023 08:44:27 -0700
> Reinette Chatre <[email protected]> wrote:
>
> > Changes since V4:
> > - V4:
> https://lore.kernel.org/lkml/[email protected]
> /
> > - Add Kevin's Reviewed-by tag as applicable.
> > - Treat non-existing INTx interrupt context as kernel bug with WARN. This
> > exposed an issue in the scenario where INTx mask/unmask may occur
> without
> > INTx enabled. This is fixed by obtaining the interrupt context later
> > (right before use) within impacted functions: vfio_pci_intx_mask() and
> > vfio_pci_intx_unmask_handler(). (Kevin)
> > - Treat pci_irq_vector() returning '0' for a MSI/MSI-X interrupt as a kernel
> > bug via a WARN instead of ignoring this value. (Kevin)
> > - Improve accuracy of comments. (Kevin)
> > - Please refer to individual patches for local changes.
>
> Looks good to me.
>
> Kevin, do you want to add any additional reviews or check the changes
> made based on your previous comments?
>

Good to me too. I've given the remaining reviewed-by's.

2023-05-17 14:42:05

by Jason Gunthorpe

[permalink] [raw]
Subject: Re: [PATCH V5 00/11] vfio/pci: Support dynamic allocation of MSI-X interrupts

On Thu, May 11, 2023 at 08:44:27AM -0700, Reinette Chatre wrote:
>
> Qemu allocates interrupts incrementally at the time the guest unmasks an
> interrupt, for example each time a Linux guest runs request_irq().
>
> Dynamic allocation of MSI-X interrupts was not possible until v6.2 [1].
> This prompted Qemu to, when allocating a new interrupt, first release all
> previously allocated interrupts (including disable of MSI-X) followed
> by re-allocation of all interrupts that includes the new interrupt.
> Please see [2] for a detailed discussion about this issue.
>
> Releasing and re-allocating interrupts may be acceptable if all
> interrupts are unmasked during device initialization. If unmasking of
> interrupts occur during runtime this may result in lost interrupts.
> For example, consider an accelerator device with multiple work queues,
> each work queue having a dedicated interrupt. A work queue can be
> enabled at any time with its associated interrupt unmasked while other
> work queues are already active. Having all interrupts released and MSI-X
> disabled to enable the new work queue will impact active work queues.
>
> This series builds on the recent interrupt sub-system core changes
> that added support for dynamic MSI-X allocation after initial MSI-X
> enabling.
>
> Add support for dynamic MSI-X allocation to vfio-pci. A flag
> indicating lack of support for dynamic allocation already exist:
> VFIO_IRQ_INFO_NORESIZE and has always been set for MSI and MSI-X. With
> support for dynamic MSI-X the flag is cleared for MSI-X when supported,
> enabling Qemu to modify its behavior.
>
> Any feedback is appreciated

Reviewed-by: Jason Gunthorpe <[email protected]>

Jason

2023-05-17 15:48:19

by Reinette Chatre

[permalink] [raw]
Subject: Re: [PATCH V5 00/11] vfio/pci: Support dynamic allocation of MSI-X interrupts


On 5/16/2023 7:14 PM, Tian, Kevin wrote:
>> From: Alex Williamson <[email protected]>
>> Sent: Wednesday, May 17, 2023 6:53 AM
>>
>> On Thu, 11 May 2023 08:44:27 -0700
>> Reinette Chatre <[email protected]> wrote:
>>
>>> Changes since V4:
>>> - V4:
>> https://lore.kernel.org/lkml/[email protected]
>> /
>>> - Add Kevin's Reviewed-by tag as applicable.
>>> - Treat non-existing INTx interrupt context as kernel bug with WARN. This
>>> exposed an issue in the scenario where INTx mask/unmask may occur
>> without
>>> INTx enabled. This is fixed by obtaining the interrupt context later
>>> (right before use) within impacted functions: vfio_pci_intx_mask() and
>>> vfio_pci_intx_unmask_handler(). (Kevin)
>>> - Treat pci_irq_vector() returning '0' for a MSI/MSI-X interrupt as a kernel
>>> bug via a WARN instead of ignoring this value. (Kevin)
>>> - Improve accuracy of comments. (Kevin)
>>> - Please refer to individual patches for local changes.
>>
>> Looks good to me.
>>
>> Kevin, do you want to add any additional reviews or check the changes
>> made based on your previous comments?
>>
>
> Good to me too. I've given the remaining reviewed-by's.

Thank you very much Alex and Kevin.

Reinette

2023-05-17 15:59:20

by Reinette Chatre

[permalink] [raw]
Subject: Re: [PATCH V5 00/11] vfio/pci: Support dynamic allocation of MSI-X interrupts



On 5/17/2023 7:25 AM, Jason Gunthorpe wrote:
>
> Reviewed-by: Jason Gunthorpe <[email protected]>
>

Thank you very much Jason.

Reinette

2023-05-22 22:47:27

by Thomas Gleixner

[permalink] [raw]
Subject: Re: [PATCH V5 00/11] vfio/pci: Support dynamic allocation of MSI-X interrupts

Reinette!

On Thu, May 11 2023 at 08:44, Reinette Chatre wrote:
> Changes since V4:
> - V4: https://lore.kernel.org/lkml/[email protected]/
> - Add Kevin's Reviewed-by tag as applicable.
> - Treat non-existing INTx interrupt context as kernel bug with WARN. This
> exposed an issue in the scenario where INTx mask/unmask may occur without
> INTx enabled. This is fixed by obtaining the interrupt context later
> (right before use) within impacted functions: vfio_pci_intx_mask() and
> vfio_pci_intx_unmask_handler(). (Kevin)
> - Treat pci_irq_vector() returning '0' for a MSI/MSI-X interrupt as a kernel
> bug via a WARN instead of ignoring this value. (Kevin)
> - Improve accuracy of comments. (Kevin)
> - Please refer to individual patches for local changes.

I only skimmed the series for obvious hickups vs. the PCI/MSI core (my
virt[io] foo is limited) and I did not find anything to complain about.

Aside of that I really like how this series is built up to restructure
and cleanup things first so that the new functionality falls in place
instead of the popular 'glue it in no matter what' approach.

That's a pleasure to read even for the virt[io] illiterate!

Acked-by: Thomas Gleixner <[email protected]>

Thanks,

tglx

2023-05-22 23:06:18

by Reinette Chatre

[permalink] [raw]
Subject: Re: [PATCH V5 00/11] vfio/pci: Support dynamic allocation of MSI-X interrupts



On 5/22/2023 3:25 PM, Thomas Gleixner wrote:
>
> Acked-by: Thomas Gleixner <[email protected]>
>

Thank you very much Thomas.

Reinette

2023-05-23 22:53:51

by Alex Williamson

[permalink] [raw]
Subject: Re: [PATCH V5 00/11] vfio/pci: Support dynamic allocation of MSI-X interrupts

On Thu, 11 May 2023 08:44:27 -0700
Reinette Chatre <[email protected]> wrote:

> Changes since V4:
> - V4: https://lore.kernel.org/lkml/[email protected]/
> - Add Kevin's Reviewed-by tag as applicable.
> - Treat non-existing INTx interrupt context as kernel bug with WARN. This
> exposed an issue in the scenario where INTx mask/unmask may occur without
> INTx enabled. This is fixed by obtaining the interrupt context later
> (right before use) within impacted functions: vfio_pci_intx_mask() and
> vfio_pci_intx_unmask_handler(). (Kevin)
> - Treat pci_irq_vector() returning '0' for a MSI/MSI-X interrupt as a kernel
> bug via a WARN instead of ignoring this value. (Kevin)
> - Improve accuracy of comments. (Kevin)
> - Please refer to individual patches for local changes.
>
> Changes since V3:
> - V3: https://lore.kernel.org/lkml/[email protected]/
> - Be considerate about layout and size with changes to
> struct vfio_pci_core_device. Keep flags together and transition all to
> use bitfields. (Alex and Jason)
> - Do not free dynamically allocated interrupts on error path. (Alex)
> - Please refer to individual patches for localized changes.
>
> Changes since V2:
> - V2: https://lore.kernel.org/lkml/[email protected]/
> - During testing of V2 "kernel test robot" reported issues resulting from
> include/linux/pci.h missing a stub for pci_msix_can_alloc_dyn() when
> CONFIG_PCI_MSI=n. A separate fix was sent to address this. The fix can
> be found in the kernel (since v6.3-rc7) as
> commit 195d8e5da3ac ("PCI/MSI: Provide missing stub for pci_msix_can_alloc_dyn()")
> - Biggest change is the transition to "active contexts" for both MSI and MSI-X.
> Interrupt contexts have always been allocated when the interrupts are
> allocated while they are only used while interrupts are
> enabled. In this series interrupt contexts are made dynamic, while doing
> so their allocation is moved to match how they are used: allocated when
> interrupts are enabled. Whether a Linux interrupt number exists determines
> whether an interrupt can be enabled.
> Previous policy (up to V2) that an allocated interrupt has an interrupt
> context no longer applies. Instead, an interrupt context has a
> handler/trigger, aka "active contexts". (Alex)
> - Re-ordered patches in support of "active contexts".
> - Only free interrupts on MSI-X teardown and otherwise use the
> allocated interrupts as a cache. (Alex)
> - Using unsigned int for the vector broke the unwind loop within
> vfio_msi_set_block(). (Alex)
> - Introduce new "has_dyn_msix" property of virtual device instead of
> querying support every time. (Alex)
> - Some smaller changes, please refer to individual patches.
>
> Changes since RFC V1:
> - RFC V1: https://lore.kernel.org/lkml/[email protected]/
> - Improved changelogs.
> - Simplify interface so that vfio_irq_ctx_alloc_single() returns pointer to
> allocated context. (Alex)
> - Remove vfio_irq_ctx_range_allocated() and associated attempts to maintain
> invalid error path behavior. (Alex and Kevin)
> - Add pointer to interrupt context as function parameter to
> vfio_irq_ctx_free(). (Alex)
> - Ensure variables are initialized. (Dan Carpenter)
> - Only support dynamic allocation if device supports it. (Alex)
>
> Qemu allocates interrupts incrementally at the time the guest unmasks an
> interrupt, for example each time a Linux guest runs request_irq().
>
> Dynamic allocation of MSI-X interrupts was not possible until v6.2 [1].
> This prompted Qemu to, when allocating a new interrupt, first release all
> previously allocated interrupts (including disable of MSI-X) followed
> by re-allocation of all interrupts that includes the new interrupt.
> Please see [2] for a detailed discussion about this issue.
>
> Releasing and re-allocating interrupts may be acceptable if all
> interrupts are unmasked during device initialization. If unmasking of
> interrupts occur during runtime this may result in lost interrupts.
> For example, consider an accelerator device with multiple work queues,
> each work queue having a dedicated interrupt. A work queue can be
> enabled at any time with its associated interrupt unmasked while other
> work queues are already active. Having all interrupts released and MSI-X
> disabled to enable the new work queue will impact active work queues.
>
> This series builds on the recent interrupt sub-system core changes
> that added support for dynamic MSI-X allocation after initial MSI-X
> enabling.
>
> Add support for dynamic MSI-X allocation to vfio-pci. A flag
> indicating lack of support for dynamic allocation already exist:
> VFIO_IRQ_INFO_NORESIZE and has always been set for MSI and MSI-X. With
> support for dynamic MSI-X the flag is cleared for MSI-X when supported,
> enabling Qemu to modify its behavior.
>
> Any feedback is appreciated
>
> Reinette
>
> [1] commit 34026364df8e ("PCI/MSI: Provide post-enable dynamic allocation interfaces for MSI-X")
> [2] https://lore.kernel.org/kvm/MWHPR11MB188603D0D809C1079F5817DC8C099@MWHPR11MB1886.namprd11.prod.outlook.com/#t
>
> Reinette Chatre (11):
> vfio/pci: Consolidate irq cleanup on MSI/MSI-X disable
> vfio/pci: Remove negative check on unsigned vector
> vfio/pci: Prepare for dynamic interrupt context storage
> vfio/pci: Move to single error path
> vfio/pci: Use xarray for interrupt context storage
> vfio/pci: Remove interrupt context counter
> vfio/pci: Update stale comment
> vfio/pci: Use bitfield for struct vfio_pci_core_device flags
> vfio/pci: Probe and store ability to support dynamic MSI-X
> vfio/pci: Support dynamic MSI-X
> vfio/pci: Clear VFIO_IRQ_INFO_NORESIZE for MSI-X
>
> drivers/vfio/pci/vfio_pci_core.c | 8 +-
> drivers/vfio/pci/vfio_pci_intrs.c | 305 ++++++++++++++++++++----------
> include/linux/vfio_pci_core.h | 26 +--
> include/uapi/linux/vfio.h | 3 +
> 4 files changed, 229 insertions(+), 113 deletions(-)
>
>
> base-commit: 457391b0380335d5e9a5babdec90ac53928b23b4

Applied to vfio next branch for v6.5. Thanks!

Alex


2023-05-24 03:01:38

by YangHang Liu

[permalink] [raw]
Subject: Re: [PATCH V5 00/11] vfio/pci: Support dynamic allocation of MSI-X interrupts

Running regression tests in the following test matrix:

i40e PF + INTx interrupt + RHEL9 guest -- PASS
bnx2x PF + MSI-X + RHEL9 guest -- PASS
iavf VF + MSI-X + Win2019 guest -- PASS
mlx5_core VF + MSI-X + Win2022 guest -- PASS
ixgbe PF + INTx + RHEL9 guest -- PASS
i40e PF + MSIX + Win2019 guest -- PASS
qede VF + MSIX + RHEL9 guest -- PASS

Tested-by: YangHang Liu <[email protected]>



On Wed, May 24, 2023 at 6:46 AM Alex Williamson
<[email protected]> wrote:
>
> On Thu, 11 May 2023 08:44:27 -0700
> Reinette Chatre <[email protected]> wrote:
>
> > Changes since V4:
> > - V4: https://lore.kernel.org/lkml/[email protected]/
> > - Add Kevin's Reviewed-by tag as applicable.
> > - Treat non-existing INTx interrupt context as kernel bug with WARN. This
> > exposed an issue in the scenario where INTx mask/unmask may occur without
> > INTx enabled. This is fixed by obtaining the interrupt context later
> > (right before use) within impacted functions: vfio_pci_intx_mask() and
> > vfio_pci_intx_unmask_handler(). (Kevin)
> > - Treat pci_irq_vector() returning '0' for a MSI/MSI-X interrupt as a kernel
> > bug via a WARN instead of ignoring this value. (Kevin)
> > - Improve accuracy of comments. (Kevin)
> > - Please refer to individual patches for local changes.
> >
> > Changes since V3:
> > - V3: https://lore.kernel.org/lkml/[email protected]/
> > - Be considerate about layout and size with changes to
> > struct vfio_pci_core_device. Keep flags together and transition all to
> > use bitfields. (Alex and Jason)
> > - Do not free dynamically allocated interrupts on error path. (Alex)
> > - Please refer to individual patches for localized changes.
> >
> > Changes since V2:
> > - V2: https://lore.kernel.org/lkml/[email protected]/
> > - During testing of V2 "kernel test robot" reported issues resulting from
> > include/linux/pci.h missing a stub for pci_msix_can_alloc_dyn() when
> > CONFIG_PCI_MSI=n. A separate fix was sent to address this. The fix can
> > be found in the kernel (since v6.3-rc7) as
> > commit 195d8e5da3ac ("PCI/MSI: Provide missing stub for pci_msix_can_alloc_dyn()")
> > - Biggest change is the transition to "active contexts" for both MSI and MSI-X.
> > Interrupt contexts have always been allocated when the interrupts are
> > allocated while they are only used while interrupts are
> > enabled. In this series interrupt contexts are made dynamic, while doing
> > so their allocation is moved to match how they are used: allocated when
> > interrupts are enabled. Whether a Linux interrupt number exists determines
> > whether an interrupt can be enabled.
> > Previous policy (up to V2) that an allocated interrupt has an interrupt
> > context no longer applies. Instead, an interrupt context has a
> > handler/trigger, aka "active contexts". (Alex)
> > - Re-ordered patches in support of "active contexts".
> > - Only free interrupts on MSI-X teardown and otherwise use the
> > allocated interrupts as a cache. (Alex)
> > - Using unsigned int for the vector broke the unwind loop within
> > vfio_msi_set_block(). (Alex)
> > - Introduce new "has_dyn_msix" property of virtual device instead of
> > querying support every time. (Alex)
> > - Some smaller changes, please refer to individual patches.
> >
> > Changes since RFC V1:
> > - RFC V1: https://lore.kernel.org/lkml/[email protected]/
> > - Improved changelogs.
> > - Simplify interface so that vfio_irq_ctx_alloc_single() returns pointer to
> > allocated context. (Alex)
> > - Remove vfio_irq_ctx_range_allocated() and associated attempts to maintain
> > invalid error path behavior. (Alex and Kevin)
> > - Add pointer to interrupt context as function parameter to
> > vfio_irq_ctx_free(). (Alex)
> > - Ensure variables are initialized. (Dan Carpenter)
> > - Only support dynamic allocation if device supports it. (Alex)
> >
> > Qemu allocates interrupts incrementally at the time the guest unmasks an
> > interrupt, for example each time a Linux guest runs request_irq().
> >
> > Dynamic allocation of MSI-X interrupts was not possible until v6.2 [1].
> > This prompted Qemu to, when allocating a new interrupt, first release all
> > previously allocated interrupts (including disable of MSI-X) followed
> > by re-allocation of all interrupts that includes the new interrupt.
> > Please see [2] for a detailed discussion about this issue.
> >
> > Releasing and re-allocating interrupts may be acceptable if all
> > interrupts are unmasked during device initialization. If unmasking of
> > interrupts occur during runtime this may result in lost interrupts.
> > For example, consider an accelerator device with multiple work queues,
> > each work queue having a dedicated interrupt. A work queue can be
> > enabled at any time with its associated interrupt unmasked while other
> > work queues are already active. Having all interrupts released and MSI-X
> > disabled to enable the new work queue will impact active work queues.
> >
> > This series builds on the recent interrupt sub-system core changes
> > that added support for dynamic MSI-X allocation after initial MSI-X
> > enabling.
> >
> > Add support for dynamic MSI-X allocation to vfio-pci. A flag
> > indicating lack of support for dynamic allocation already exist:
> > VFIO_IRQ_INFO_NORESIZE and has always been set for MSI and MSI-X. With
> > support for dynamic MSI-X the flag is cleared for MSI-X when supported,
> > enabling Qemu to modify its behavior.
> >
> > Any feedback is appreciated
> >
> > Reinette
> >
> > [1] commit 34026364df8e ("PCI/MSI: Provide post-enable dynamic allocation interfaces for MSI-X")
> > [2] https://lore.kernel.org/kvm/MWHPR11MB188603D0D809C1079F5817DC8C099@MWHPR11MB1886.namprd11.prod.outlook.com/#t
> >
> > Reinette Chatre (11):
> > vfio/pci: Consolidate irq cleanup on MSI/MSI-X disable
> > vfio/pci: Remove negative check on unsigned vector
> > vfio/pci: Prepare for dynamic interrupt context storage
> > vfio/pci: Move to single error path
> > vfio/pci: Use xarray for interrupt context storage
> > vfio/pci: Remove interrupt context counter
> > vfio/pci: Update stale comment
> > vfio/pci: Use bitfield for struct vfio_pci_core_device flags
> > vfio/pci: Probe and store ability to support dynamic MSI-X
> > vfio/pci: Support dynamic MSI-X
> > vfio/pci: Clear VFIO_IRQ_INFO_NORESIZE for MSI-X
> >
> > drivers/vfio/pci/vfio_pci_core.c | 8 +-
> > drivers/vfio/pci/vfio_pci_intrs.c | 305 ++++++++++++++++++++----------
> > include/linux/vfio_pci_core.h | 26 +--
> > include/uapi/linux/vfio.h | 3 +
> > 4 files changed, 229 insertions(+), 113 deletions(-)
> >
> >
> > base-commit: 457391b0380335d5e9a5babdec90ac53928b23b4
>
> Applied to vfio next branch for v6.5. Thanks!
>
> Alex
>


2023-05-24 14:41:14

by Reinette Chatre

[permalink] [raw]
Subject: Re: [PATCH V5 00/11] vfio/pci: Support dynamic allocation of MSI-X interrupts

Hi YangHang,

On 5/23/2023 7:43 PM, YangHang Liu wrote:
> Running regression tests in the following test matrix:
>
> i40e PF + INTx interrupt + RHEL9 guest -- PASS
> bnx2x PF + MSI-X + RHEL9 guest -- PASS
> iavf VF + MSI-X + Win2019 guest -- PASS
> mlx5_core VF + MSI-X + Win2022 guest -- PASS
> ixgbe PF + INTx + RHEL9 guest -- PASS
> i40e PF + MSIX + Win2019 guest -- PASS
> qede VF + MSIX + RHEL9 guest -- PASS
>
> Tested-by: YangHang Liu <[email protected]>
>

Thank you very much for this thorough testing.

Reinette