Received-SPF: pass (google.com: domain of linux-crypto+bounces-2396-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Thu, 29 Feb 2024 10:22:15 -0400
From: Jason Gunthorpe <jgg@nvidia.com>
To: Alex Williamson <alex.williamson@redhat.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: <20240229142215.GG9179@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>
 <20240226124107.4317b3c3.alex.williamson@redhat.com>
 <20240226194952.GO13330@nvidia.com>
 <20240226152458.2b8a0f83.alex.williamson@redhat.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240226152458.2b8a0f83.alex.williamson@redhat.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?us-ascii?Q?uY3rBBbXE618x+1s+0SlgPJd8cZXxrzWVZi7FMxXBwVuu9w/5hRnzapoSqjh?=
 =?us-ascii?Q?2+vl++u5GngR0KInZffAxQ33FibAWky8l5SLN0H9aqAkQW+TbC3OuoYu8zi8?=
 =?us-ascii?Q?3iMeNaHBzjlQr1/IOCNGlbriAOuy71Mtvb0jQvPz+PbhB+05JXYa9l5wzsy8?=
 =?us-ascii?Q?i9tgsd3iN7pgg9zRoTWFkQn6xYkXg3V2K3cAjNwlbFbmIo+BY+Alu5Rg44Vt?=
 =?us-ascii?Q?ZkcGIqsyC8e7QBPS6qf876bnSmN/Q33f3BxTaroQg7LT97GPh4EyLOJgVYxx?=
 =?us-ascii?Q?w+82Sp1gaFzLUB9ySlMC8GkRAr18YaOSa6Ga1FwXIXWGdWiUPJa9b3siEdZK?=
 =?us-ascii?Q?FwOROKdvLmAqbdyQ51XyZg3L73SYNz2+ArKLTZJuyM35jPjTd1jzudNGDiEb?=
 =?us-ascii?Q?V5+BmTErHieLRylmFiGc5C8ntcB21rxRfD7efPbPKK+UAxfkWV5C4qxciQIf?=
 =?us-ascii?Q?DzKqjqWJOPGrKMOuqWeg0SJvZ1HYaLZCdzL/eOCYM7TCRSpPr8TBaum+FBBi?=
 =?us-ascii?Q?utxQS9+S5/5aKtAg/TCEap78HcL4+mGIc+F1I3y0BVikpl/zWPF7fBC61D/p?=
 =?us-ascii?Q?wdAOPurZVeqDv+Fle2UgfCkhYRNd3CmIVXctyVYhEMONTW+SGTv7FA90j54j?=
 =?us-ascii?Q?Ywenk5WLV7TN3q7i2ceAzkmlp5F0HBwuYPHYqlzR9c7npNm0pGcfoyDQJuQw?=
 =?us-ascii?Q?bAPC7dbd+XrKzjWSjNRqcv9uJGdPcuG33wX2j6pQYU+4asuoXtbEwb0rXZq9?=
 =?us-ascii?Q?z3Lk7pBHwJobQKHoAsudaw0qzsv3vFxZ8wqDIseRR1pKLj7T5eRwPX4NxpOp?=
 =?us-ascii?Q?yy4y5hah/WVTJhhtpw40WpmBe6Dwal/tNuege3/fbjH4t6JINRlZEZ3CV9wE?=
 =?us-ascii?Q?bsN9XmLTBZpDc3XwkPcibzq8Lfi31psXQEwNMkKsRqsr2wtL8cHJzuWm4Axb?=
 =?us-ascii?Q?BEdeHmGm5JD38ZpHCYQvK1TUmzDpku/EnhBC/3njWxC3acCnllFL2NbECQ1i?=
 =?us-ascii?Q?OEJGOEFyR/fonIU2xhWm2VgIyaSFQjb8IHh91G8f7XOAMdx3LQFn4LFs8BOp?=
 =?us-ascii?Q?6Ty+HkoLMnC45MbSl3yGL6vfUU8CYjcdIQgqQ1g9hHHwRcMHjz08O29Miu6N?=
 =?us-ascii?Q?lsYhVNQSE1d6u1Xiq+MhIUoknj5mmJetp2um+/r6haoZJ7SeIs8D/QxcIzDx?=
 =?us-ascii?Q?HxILdLSkOrKsmSVx9nUnxEiKbqpww5Wal1pYbjvvTGtNVIe6a1eRSXw9BcgV?=
 =?us-ascii?Q?8tcZyyK3n2ReQWBWd7+V3d7je0DIHnJRFqXHJfmcHGxZb03lebl/hWF4VYOP?=
 =?us-ascii?Q?anzW+zXm6JYOL/Hfx4xgRBIcOaUO1VWDd0gYlyXa+AkdOQdyFbQZQUdxLur+?=
 =?us-ascii?Q?G+9hmQi6DP1/qVB/rpQZZiANn6bE4go8T8Ww/9SWFfI9v/e2uZCGA9FPtGnJ?=
 =?us-ascii?Q?yC1B3oq9CclWnC8TrBC0/loEQraVzPEszC/1QCsxXD0vsktHmDrrsm8LVisy?=
 =?us-ascii?Q?OK++n9UQ0P+kxZ5T9Q46WS1Fq4tUUS1MtGdLrisytA+KJMj6S2JrYqOh8Kkh?=
 =?us-ascii?Q?2e25sanbKK3NyaDZEXa7GI8C58fcp1osfuvznK/m?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ac03497b-195b-4f8b-afe5-08dc3931d4ab
X-MS-Exchange-CrossTenant-AuthSource: DM6PR12MB3849.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Feb 2024 14:22:16.5037
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 1/IAgPE2tZ2dOsBfgXlL8zvUeKRQszy8daXlYcknQrh9aYYVobtviOKW4maS6uEY
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR12MB8537

On Mon, Feb 26, 2024 at 03:24:58PM -0700, Alex Williamson wrote:
> On Mon, 26 Feb 2024 15:49:52 -0400
> Jason Gunthorpe <jgg@nvidia.com> wrote:
> 
> > On Mon, Feb 26, 2024 at 12:41:07PM -0700, Alex Williamson wrote:
> > > 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 think the intent was to bind it to all the devices in its ID table
> > automatically for VFIO use and it should always be OK to do that. Not
> > doing so is a micro optimization.
> 
> Automatic in what sense?  We use PCI_DRIVER_OVERRIDE_DEVICE_VFIO to set
> the id_table entry to override_only, so any binding requires the user
> to specify a driver_override.  Nothing automatically performs that
> driver_override except the recent support in libvirt for "managed"
> devices.

I mean in the sense the user would run some command it would find the
right driver..
 
> Effectively override_only negates the id_table to little more than a
> suggestion to userspace of how the driver should be used.

We once talked about having a "flavor" sysfs in the driver core where
instead of using driver_override you'd switch flavours to vfio and
then it would use the driver binding logic to get the right driver. I
forget why we abandoned that direction..

> > checks that ID table.. If we lack a usable tool for that then it that
> > is the problemm.
> 
> These are the sort of questions I'm currently fielding and yes, we
> don't really have any tools to make this automatic outside of the
> recent libvirt support.

I see. Having people do this manually was not my vision at a least.

> > > 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.  
> > 
> > I don't think this is fully accurate (or at least not what was
> > intended), the VFIO device can be configured any time up until the VM
> > mlx5 driver reaches the device startup.
> > 
> > Is something preventing this? Did we accidentally cache the migratable
> > flag in vfio or something??
> 
> I don't think so, I think this was just the policy we had decided
> relative to profiling VFs when they're created rather than providing a
> means to do it though a common vfio variant driver interface[1].

I didn't quite mean it so literally though, I imagined we could bind
the driver in VFIO, profile the VF, then open the VFIO cdev.

> Nothing necessarily prevents libvirt from configuring devices after
> binding to a vfio-pci variant driver, but per the noted thread the
> upstream policy is to have the device configured prior to giving it to
> vfio and any configuration mechanisms are specific to the device and
> variant driver, therefore what more is libvirt to do?

I admit I don't have a clear sense what the plan here is on the
userspace side. Somehow this all has to be threaded together and a
real environment needs a step to profile and provision these complex
VFIO devices.

For k8s I think we are doing this with the operator plugins. I don't
know of a plan for "raw" libvirt..

> OTOH, libvirt is more targeted and while it will work automatically for
> a "managed" device specified via domain xml, it's also available as a
> utility through virsh, ex:
> 
>  # virsh nodedev-detach pci_0000_01_10_0
> 
> Which should trigger the code to bind to vfio-pci or the best variant
> driver.

Maybe we just need to more clearly document this?

> Relative to variant driver probe() re-checking the id_table, I know we
> generally don't try to set policy in the kernel and this sort of
> behavior has been around for a long, long time with new_id, but the
> barrier to entry has been substantially lowered with things like
> driverctl that I'd still entertain the idea.  Thanks,

I'm not adverse to it, but maybe we should try to push the userspace
forward some more before putting kernel protections in?

Jason