Re: [PATCH mlx5-next v1 2/5] PCI: Add SR-IOV sysfs entry to read number of MSI-X vectors

From: Alexander Duyck <alexander.duyck@gmail.com>
To: Jason Gunthorpe <jgg@nvidia.com>
Cc: Leon Romanovsky <leon@kernel.org>,
	Bjorn Helgaas <bhelgaas@google.com>,
	Saeed Mahameed <saeedm@nvidia.com>,
	Jakub Kicinski <kuba@kernel.org>,
	linux-pci <linux-pci@vger.kernel.org>,
	linux-rdma@vger.kernel.org, Netdev <netdev@vger.kernel.org>,
	Don Dutile <ddutile@redhat.com>,
	Alex Williamson <alex.williamson@redhat.com>
Subject: Re: [PATCH mlx5-next v1 2/5] PCI: Add SR-IOV sysfs entry to read number of MSI-X vectors
Date: Thu, 14 Jan 2021 09:55:24 -0800	[thread overview]
Message-ID: <CAKgT0UcKqt=EgE+eitB8-u8LvxqHBDfF+u2ZSi5urP_Aj0Btvg@mail.gmail.com> (raw)
In-Reply-To: <20210114164857.GN4147@nvidia.com>

On Thu, Jan 14, 2021 at 8:49 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> On Thu, Jan 14, 2021 at 08:40:14AM -0800, Alexander Duyck wrote:
>
> > Where I think you and I disagree is that I really think the MSI-X
> > table size should be fixed at one value for the life of the VF.
> > Instead of changing the table size it should be the number of vectors
> > that are functional that should be the limit. Basically there should
> > be only some that are functional and some that are essentially just
> > dummy vectors that you can write values into that will never be used.
>
> Ignoring the PCI config space to learn the # of available MSI-X
> vectors is big break on the how the device's programming ABI works.
>
> Or stated another way, that isn't compatible with any existing drivers
> so it is basically not a useful approach as it can't be deployed.
>
> I don't know why you think that is better.
>
> Jason

First off, this is technically violating the PCIe spec section 7.7.2.2
because this is the device driver modifying the Message Control
register for a device, even if it is the PF firmware modifying the VF.
The table size is something that should be set and fixed at device
creation and not changed.

The MSI-X table is essentially just an MMIO resource, and I believe it
should not be resized, just as you wouldn't expect any MMIO BAR to be
dynamically resized. Many drivers don't make use of the full MSI-X
table nor do they bother reading the size. We just populate a subset
of the table based on the number of interrupt causes we will need to
associate to interrupt handlers. You can check for yourself. There are
only a handful of drivers such as vfio that ever bother reading at the
offset "PCI_MSIX_FLAGS". Normally it is the number of interrupt causes
that determine how many we need, not the size of the table. In
addition the OS may restrict us further since there are only so many
MSI-X interrupts that are supported per system/socket.

As far as the programming ABI, having support for some number of MSI-X
vectors isn't the same as having that number of MSI-X vectors. The
MSI-X vector table entry is nothing more than a spot to write an
address/data pair with the ability to mask the value to prevent it
from being triggered. The driver/OS will associate some handler to
that address/data pair. Populating an entry doesn't guarantee the
interrupt will ever be used. The programming model for the device is
what defines what trigger will be associated with it and when/how it
will be used.

What I see this patch doing is trying to push driver PF policy onto
the VF PCIe device configuration space dynamically. Having some
limited number of interrupt causes should really be what is limiting
things here. I see that being mostly a thing between the firmware and
the VF in terms of configuration and not something that necessarily
has to be pushed down onto the PCIe configuration space itself.