Message-ID: <CAGYh1E8Yk-eVNOMQrx6SUgNou5fY6=jFzHR=rQ9diwXbJAaKVg@mail.gmail.com>
Date:   Wed, 24 May 2023 10:43:48 +0800
From:   YangHang Liu <yanghliu@...hat.com>
To:     Alex Williamson <alex.williamson@...hat.com>
Cc:     Reinette Chatre <reinette.chatre@...el.com>, jgg@...dia.com,
        yishaih@...dia.com, shameerali.kolothum.thodi@...wei.com,
        kevin.tian@...el.com, tglx@...utronix.de, darwi@...utronix.de,
        kvm@...r.kernel.org, dave.jiang@...el.com, jing2.liu@...el.com,
        ashok.raj@...el.com, fenghua.yu@...el.com,
        tom.zanussi@...ux.intel.com, linux-kernel@...r.kernel.org
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 <yanghliu@...hat.com>



On Wed, May 24, 2023 at 6:46 AM Alex Williamson
<alex.williamson@...hat.com> wrote:
>
> On Thu, 11 May 2023 08:44:27 -0700
> Reinette Chatre <reinette.chatre@...el.com> wrote:
>
> > Changes since V4:
> > - V4: https://lore.kernel.org/lkml/cover.1682615447.git.reinette.chatre@intel.com/
> > - 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/cover.1681837892.git.reinette.chatre@intel.com/
> > - 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/cover.1680038771.git.reinette.chatre@intel.com/
> > - 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/cover.1678911529.git.reinette.chatre@intel.com/
> > - 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
>

Powered by blists - more mailing lists