Received-SPF: pass (google.com: domain of linux-crypto+bounces-2327-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Mon, 26 Feb 2024 12:41:07 -0700
From: Alex Williamson <alex.williamson@redhat.com>
To: Jason Gunthorpe <jgg@nvidia.com>
Cc: Xin Zeng <xin.zeng@intel.com>, herbert@gondor.apana.org.au,
 yishaih@nvidia.com, shameerali.kolothum.thodi@huawei.com,
 kevin.tian@intel.com, linux-crypto@vger.kernel.org, kvm@vger.kernel.org,
 qat-linux@intel.com, Yahui Cao <yahui.cao@intel.com>
Subject: Re: [PATCH v3 10/10] vfio/qat: Add vfio_pci driver for Intel QAT VF
 devices
Message-ID: <20240226124107.4317b3c3.alex.williamson@redhat.com>
In-Reply-To: <20240226191220.GM13330@nvidia.com>
References: <20240221155008.960369-1-xin.zeng@intel.com>
	<20240221155008.960369-11-xin.zeng@intel.com>
	<20240226115556.3f494157.alex.williamson@redhat.com>
	<20240226191220.GM13330@nvidia.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Mon, 26 Feb 2024 15:12:20 -0400
Jason Gunthorpe <jgg@nvidia.com> wrote:

> On Mon, Feb 26, 2024 at 11:55:56AM -0700, Alex Williamson wrote:
> > This will be the first intel vfio-pci variant driver, I don't think we
> > need an intel sub-directory just yet.
> > 
> > Tangentially, I think an issue we're running into with
> > PCI_DRIVER_OVERRIDE_DEVICE_VFIO is that we require driver_override to
> > bind the device and therefore the id_table becomes little more than a
> > suggestion.  Our QE is already asking, for example, if they should use
> > mlx5-vfio-pci for all mlx5 compatible devices.  
> 
> They don't have to, but it works fine, there is no reason not to.

But there's also no reason to.  None of the metadata exposed by the
driver suggests it should be a general purpose vfio-pci stand-in.

> I imagined that users would always bind the variant driver, it is why
> the drivers all have "disabled" fallbacks to just be normal vfio-pci.
> 
> > I wonder if all vfio-pci variant drivers that specify an id_table
> > shouldn't include in their probe function:
> > 
> > 	if (!pci_match_id(pdev, id)) {
> > 		pci_info(pdev, "Incompatible device, disallowing driver_override\n");
> > 		return -ENODEV;
> > 	}  
> 
> Certainly an interesting idea, doesn't that completely defeat driver
> binding and new_id though?

I guess we always send a compatible id, so it'd be more a matter of
exporting and testing id against pci_device_id_any, that would be the
footprint of just a driver_override match (or an extremely liberal
dynamic id).

> You are worried about someone wrongly loading a mlx5 driver on, say,
> an Intel device?

That's sort of where we're headed if we consider it valid to bind a CX5
to mlx5-vfio-pci just because they have a host driver with a similar
name in common.  It's essentially a free for all.  I worry about test
matrices, user confusion, and being on guard for arbitrary devices at
every turn in variant drivers if our policy is that they should all work
equivalent to a basic vfio-pci-core implementation for anything.

> > (And yes, I see the irony that vfio introduced driver_override and
> > we've created variant drivers that require driver_override and now we
> > want to prevent driver_overrides)  
> 
> Heh
>  
> > Jason, are you seeing any of this as well and do you have a better
> > suggestion how we might address the issue?  Thanks,  
> 
> I haven't heard of confusion here.. People who don't care just use
> vfio-pci like the internet tells them, people who do care seem to be
> very sophisticated right now..
> 
> Did the userspace tool Max sketched out to automatically parse the id
> tables ever get completed? That seems like the best solution, just
> automate it and remove the decision from the user.

libvirt recently implemented support for managed="yes" with variant
drivers where it will find the best "vfio_pci" driver for a device
using an algorithm like Max suggested, but in practice it's not clear
how useful that will be considering devices likes CX7 require
configuration before binding to the variant driver.  libvirt has no
hooks to specify or perform configuration at that point.

The driverctl script also exists and could maybe consider the
"vfio-pci" driver name to be a fungible alias for the best matching
vfio_pci class driver, but I'm not sure if driverctl has a sufficient
following to make a difference.  Thanks,

Alex