Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <cover.1683740667.git.reinette.chatre@intel.com> <20230523164301.14620a69.alex.williamson@redhat.com>
In-Reply-To: <20230523164301.14620a69.alex.williamson@redhat.com>
From:   YangHang Liu <yanghliu@redhat.com>
Date:   Wed, 24 May 2023 10:43:48 +0800
Message-ID: <CAGYh1E8Yk-eVNOMQrx6SUgNou5fY6=jFzHR=rQ9diwXbJAaKVg@mail.gmail.com>
Subject: Re: [PATCH V5 00/11] vfio/pci: Support dynamic allocation of MSI-X interrupts
To:     Alex Williamson <alex.williamson@redhat.com>
Cc:     Reinette Chatre <reinette.chatre@intel.com>, jgg@nvidia.com,
        yishaih@nvidia.com, shameerali.kolothum.thodi@huawei.com,
        kevin.tian@intel.com, tglx@linutronix.de, darwi@linutronix.de,
        kvm@vger.kernel.org, dave.jiang@intel.com, jing2.liu@intel.com,
        ashok.raj@intel.com, fenghua.yu@intel.com,
        tom.zanussi@linux.intel.com, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

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@redhat.com>


On Wed, May 24, 2023 at 6:46=E2=80=AFAM Alex Williamson
<alex.williamson@redhat.com> wrote:
>
> On Thu, 11 May 2023 08:44:27 -0700
> Reinette Chatre <reinette.chatre@intel.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. Th=
is
> >   exposed an issue in the scenario where INTx mask/unmask may occur wit=
hout
> >   INTx enabled. This is fixed by obtaining the interrupt context later
> >   (right before use) within impacted functions: vfio_pci_intx_mask() an=
d
> >   vfio_pci_intx_unmask_handler(). (Kevin)
> > - Treat pci_irq_vector() returning '0' for a MSI/MSI-X interrupt as a k=
ernel
> >   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 t=
o
> >   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 fr=
om
> >   include/linux/pci.h missing a stub for pci_msix_can_alloc_dyn() when
> >   CONFIG_PCI_MSI=3Dn. 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 an=
d 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 do=
ing
> >   so their allocation is moved to match how they are used: allocated wh=
en
> >   interrupts are enabled. Whether a Linux interrupt number exists deter=
mines
> >   whether an interrupt can be enabled.
> >   Previous policy (up to V2) that an allocated interrupt has an interru=
pt
> >   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.ch=
atre@intel.com/
> > - Improved changelogs.
> > - Simplify interface so that vfio_irq_ctx_alloc_single() returns pointe=
r to
> >   allocated context. (Alex)
> > - Remove vfio_irq_ctx_range_allocated() and associated attempts to main=
tain
> >   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 a=
n
> > 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 a=
ll
> > 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 allocati=
on 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
>