Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 24 Jun 2021 16:22:34 -0600
From:   Alex Williamson <alex.williamson@redhat.com>
To:     Thomas Gleixner <tglx@linutronix.de>
Cc:     "Tian\, Kevin" <kevin.tian@intel.com>,
        Jason Gunthorpe <jgg@nvidia.com>,
        "Dey\, Megha" <megha.dey@intel.com>,
        "Raj\, Ashok" <ashok.raj@intel.com>,
        "Pan\, Jacob jun" <jacob.jun.pan@intel.com>,
        "Jiang\, Dave" <dave.jiang@intel.com>,
        "Liu\, Yi L" <yi.l.liu@intel.com>,
        "Lu\, Baolu" <baolu.lu@intel.com>,
        "Williams\, Dan J" <dan.j.williams@intel.com>,
        "Luck\, Tony" <tony.luck@intel.com>,
        "Kumar\, Sanjay K" <sanjay.k.kumar@intel.com>,
        LKML <linux-kernel@vger.kernel.org>, KVM <kvm@vger.kernel.org>,
        Kirti Wankhede <kwankhede@nvidia.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Marc Zyngier <maz@kernel.org>,
        Bjorn Helgaas <helgaas@kernel.org>
Subject: Re: [PATCH] vfio/pci: Document the MSI[X] resize side effects
 properly
Message-ID: <20210624162234.6476712c.alex.williamson@redhat.com>
In-Reply-To: <87im23bh72.ffs@nanos.tec.linutronix.de>
References: <20210622131217.76b28f6f.alex.williamson@redhat.com>
        <87o8bxcuxv.ffs@nanos.tec.linutronix.de>
        <20210623091935.3ab3e378.alex.williamson@redhat.com>
        <MWHPR11MB18864420ACE88E060203F7818C079@MWHPR11MB1886.namprd11.prod.outlook.com>
        <87mtrgatqo.ffs@nanos.tec.linutronix.de>
        <20210623204828.2bc7e6dc.alex.williamson@redhat.com>
        <87im23bh72.ffs@nanos.tec.linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Thu, 24 Jun 2021 14:06:09 +0200
Thomas Gleixner <tglx@linutronix.de> wrote:

> The documentation of VFIO_IRQ_INFO_NORESIZE is inaccurate as it suggests
> that it is safe to dynamically add new MSI-X vectors even when
> previously allocated vectors are already in use and enabled.
> 
> Enabling additional vectors is possible according the MSI-X specification,
> but the kernel does not have any mechanisms today to do that safely.
> 
> The only available mechanism is to teardown the already active vectors
> and to setup the full vector set afterwards.
> 
> This requires to temporarily disable MSI-X which redirects any interrupt
> raised by the device during this time to the legacy PCI/INTX which is
> not handled and the interrupt is therefore lost.
> 
> Update the documentation of VFIO_IRQ_INFO_NORESIZE accordingly.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  include/uapi/linux/vfio.h |   17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -699,10 +699,19 @@ struct vfio_region_info_cap_nvlink2_lnks
>   * disabling the entire index.  This is used for interrupts like PCI MSI
>   * and MSI-X where the driver may only use a subset of the available
>   * indexes, but VFIO needs to enable a specific number of vectors
> - * upfront.  In the case of MSI-X, where the user can enable MSI-X and
> - * then add and unmask vectors, it's up to userspace to make the decision
> - * whether to allocate the maximum supported number of vectors or tear
> - * down setup and incrementally increase the vectors as each is enabled.
> + * upfront.
> + *
> + * MSI cannot be resized safely when interrupts are in use already because
> + * resizing requires temporary disablement of MSI for updating the relevant
> + * PCI config space entries. Disabling MSI redirects an interrupt raised by
> + * the device during this time to the unhandled legacy PCI/INTX, which
> + * means the interrupt is lost.
> + *
> + * Enabling additional vectors for MSI-X is possible at least from the
> + * perspective of the MSI-X specification, but not supported by the
> + * exisiting PCI/MSI-X mechanisms in the kernel. The kernel provides
> + * currently only a full teardown/setup cycle which requires to disable
> + * MSI-X temporarily with the same side effects as for MSI.
>   */
>  struct vfio_irq_info {
>  	__u32	argsz;
> 

There's good information here, but as per my other reply I think
NORESIZE might be only a host implementation issue for both MSI and
MSI/X.

I'd also rather not focus on that existing implementation in this
header, which is essentially the uAPI spec, because that implementation
can change and we're unlikely to remember to update the description
here.  We might even be describing a device that emulates MSI/X in some
way that it's not bound by this limitation.  For example maybe Intel's
emulation of MSI-X backed by IMS wouldn't need this flag and we could
update QEMU to finally have a branch that avoids the teardown/setup.
We have a flag to indicate this behavior, consequences should be
relative to the presence of that flag.

Finally a nit, I don't really see a strong case that the existing text
is actually inaccurate or implying some safety against lost interrupts.
It's actually making note of the issue here already, though the more
explicit description is welcome.  Thanks,

Alex