Received-SPF: pass (google.com: domain of linux-crypto+bounces-2507-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
From: "Tian, Kevin" <kevin.tian@intel.com>
To: "Zeng, Xin" <xin.zeng@intel.com>, Alex Williamson
	<alex.williamson@redhat.com>
CC: "herbert@gondor.apana.org.au" <herbert@gondor.apana.org.au>,
	"jgg@nvidia.com" <jgg@nvidia.com>, "yishaih@nvidia.com" <yishaih@nvidia.com>,
	"shameerali.kolothum.thodi@huawei.com"
	<shameerali.kolothum.thodi@huawei.com>, "linux-crypto@vger.kernel.org"
	<linux-crypto@vger.kernel.org>, "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	qat-linux <qat-linux@intel.com>, "Cao, Yahui" <yahui.cao@intel.com>
Subject: RE: [PATCH v4 10/10] vfio/qat: Add vfio_pci driver for Intel QAT VF
 devices
Thread-Topic: [PATCH v4 10/10] vfio/qat: Add vfio_pci driver for Intel QAT VF
 devices
Thread-Index: AQHaalR95YlyUWWE9kKxaDXGuT4g97EjlCQAgAPgaQCAAVbSwA==
Date: Tue, 5 Mar 2024 07:43:06 +0000
Message-ID: <BN9PR11MB52765004B838A18B46C43C328C222@BN9PR11MB5276.namprd11.prod.outlook.com>
References: <20240228143402.89219-1-xin.zeng@intel.com>
	<20240228143402.89219-11-xin.zeng@intel.com>
 <20240301165848.37cfffaf.alex.williamson@redhat.com>
 <BL1PR11MB549484AC60C56912F260933488232@BL1PR11MB5494.namprd11.prod.outlook.com>
In-Reply-To: <BL1PR11MB549484AC60C56912F260933488232@BL1PR11MB5494.namprd11.prod.outlook.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?WBq55n5bYOInwQXPg17Z/rUDzUn8ftupcl0um42mUVkwHoIXhhG8SM25/S+r?=
 =?us-ascii?Q?ZXAGskYsLNllNngZKu+44tR4v3a0wa6lGFqm0LZPQcU9a4m7haXnhWzUdheg?=
 =?us-ascii?Q?pjhoD+GIsCwmlbmBoX5LRaeKdVWfSt5vRYzk9M/+UfDAdP7HENR2sW2Mt9b6?=
 =?us-ascii?Q?x4rz6NAk/qRjpYVSrp5mKa43WKSwz9t/BN/3nihnuOmg/u5LRXUzCoaNnOJ6?=
 =?us-ascii?Q?nfL2NA1MketUcG9WK5bh3rqy7guizax2ApqOYs4CK3oKGGoBc5YS5u6h/Yh8?=
 =?us-ascii?Q?xpDexkmzJkto8xXeSgagi1II5Dnk7RUy7Ad7XurdymV50ri5I1qqUe91W02G?=
 =?us-ascii?Q?7eqeH07hp5viyjbhQtcqjViLLVfTHy3YJmkt3V0F/Q6Eg0T5UdhW84BLM0rg?=
 =?us-ascii?Q?7G9MfFxtal1baON+j/2JW5YCGu+l9NHO5+pQMzlR5rdyISl0CVw8QC+fZzPn?=
 =?us-ascii?Q?4ocFlJKeK/6Rit9+88c3elZNjO40HHhJiX1MYn3KnANu6Q8SDfR2RXOBriUK?=
 =?us-ascii?Q?34E3uZ/ich7bAoeKaBgjJKF/YK+mVzQyNEXY7gHyMt18ekzoDKJcYeOZqJiL?=
 =?us-ascii?Q?5Giy2EbrsZiH8YofMjs3PzCGHikfdPCH5OZn+8TL86lGT7PlaokgWUFmrmlA?=
 =?us-ascii?Q?ZlHntlEOm07vM63qkKpjh8vwpaxOXSQtGykJ+oF4SpQTjY2d9cmM7Sxs7czG?=
 =?us-ascii?Q?8Fcu7XUkc5D/Ow7lhEnpJ4PSsf/5QO5PWPd3HxMq3beNCYOIT/AjrGJlj6A9?=
 =?us-ascii?Q?5xbeLMcUSYNj9ulWLuzX3DgqY3PX/Qmu8qWpCCJEuEq+Otvne/eHsIHbL1EX?=
 =?us-ascii?Q?Q0K0w8ozInuTOSfVK0w+f2CHIt1cvKqTHJhuiVkzYL0TwdqmRsLlXxN6jNUh?=
 =?us-ascii?Q?V7jm+5A7UFsM4mh5qmzwxHnKwGBeHgF4oxGivVQZgYRruTqrctVgTUVdJZi/?=
 =?us-ascii?Q?EVIWzI91BL/jB/4wd8t6fYCOUX1MiqtCRwrJ5DP1rO/nwh25pFH90wABU9YA?=
 =?us-ascii?Q?Iet2/hIbv4fCEktQZED7SwdCWX9zDiOkD7OWuQVZJlIE49/H0nW9+Qsa8mLC?=
 =?us-ascii?Q?1ThNfLp4DXDns41WsilEpwIGnxKDrQ0L6gSzpbonljUoOr9ak+dE5AX+YHVn?=
 =?us-ascii?Q?qkn2AgKncUag/wtzD4VUnYugk10kEYBAkhOeiQ/aY8xwaXx1LDCYU2DoDV53?=
 =?us-ascii?Q?LEmd70HmUF5XZ4poRLyaqvr4p7g+9Qmt1o1KCbDQQ8mQBinJ3SNshsPIh8PX?=
 =?us-ascii?Q?dsuct+qE2c5GpTLndRbsoHbWE41x8uD24VvIXENTdWbwugptwtwaXLvGZAUP?=
 =?us-ascii?Q?DzcxqYtZ6fl2NGyc6fJkWrjEoIEK/JaRQh27MeBmeyYovKP2N/pR8dl6s6yw?=
 =?us-ascii?Q?RPEO6ywT9CWfnZxAt18qXZxs96rIXUdE0OSDvhOIjjn9n9MH0HayFabjJkIB?=
 =?us-ascii?Q?Mi8qmLHRfJaYGwTAUpTZ96j9zwHYlYdixEApc93Kqm9QTYJy/EBfOLz9eBBW?=
 =?us-ascii?Q?bw72ZK5oUvigwPHGept4Zan4M0FKFadN5Ck1dK41fzbwYdkgwxRiU5aOR7is?=
 =?us-ascii?Q?75f2Zf9jhCJ3//rUxUHlt7o3SwV5FINWgUYyMzdH?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-crypto@vger.kernel.org
List-Id: <linux-crypto.vger.kernel.org>
List-Subscribe: <mailto:linux-crypto+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-crypto+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BN9PR11MB5276.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 803ff90b-908b-4de6-075a-08dc3ce7e58c
X-MS-Exchange-CrossTenant-originalarrivaltime: 05 Mar 2024 07:43:06.5698
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: L7DtM5RlhhW9p27KgWXUQzlGKTzLG5IeqSc9MdZK2Wk/kyTZ5JyHzbp0BYncca69TvbbHpVXyTwMA55D/ez+7g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB4959
X-OriginatorOrg: intel.com

> From: Zeng, Xin <xin.zeng@intel.com>
> Sent: Monday, March 4, 2024 7:11 PM
>=20
> On Saturday, March 2, 2024 7:59 AM, Alex Williamson
> <alex.williamson@redhat.com> wrote:
> > On Wed, 28 Feb 2024 22:34:02 +0800
> > Xin Zeng <xin.zeng@intel.com> wrote:
> > > +static int
> > > +qat_vf_vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_=
id
> > *id)
> > > +{
> > > +	struct device *dev =3D &pdev->dev;
> > > +	struct qat_vf_core_device *qat_vdev;
> > > +	int ret;
> > > +
> > > +	if (!pci_match_id(id, pdev)) {
> > > +		pci_err(pdev, "Incompatible device, disallowing
> > driver_override\n");
> > > +		return -ENODEV;
> > > +	}
> >
> > I think the question of whether this is the right thing to do is still
> > up for debate, but as noted in the thread where I raised the question,
> > this mechanism doesn't actually work.
> >
> > The probe callback is passed the matching ID from the set of dynamic
> > IDs added via new_id, the driver id_table, or pci_device_id_any for a
> > strictly driver_override match.  Any of those would satisfy
> > pci_match_id().
> >
> > If we wanted the probe function to exclusively match devices in the
> > id_table, we should call this as:
> >
> > 	if (!pci_match_id(qat_vf_vfio_pci_table, pdev))...
> >
> > If we wanted to still allow dynamic IDs, the test might be more like:
> >
> > 	if (id =3D=3D &pci_device_id_any)...
> >
> > Allowing dynamic IDs but failing driver_override requires a slightly
> > more sophisticated user, but is inconsistent.  Do we have any
> > consensus on this?  Thanks,
> >
>=20
> As migration support is required to be reported at probe time in the
> variant driver, and migration support is device specific, the variant
> driver should be considered as device specific driver, not a general
> purpose vfio-pci stand-in.
> From this point of view, maybe it's not necessary to allow dynamic id
> arbitrarily, matching devices in the id_table provided by the variant
> driver exclusively would be the preference. If this sound good to you,
> I'll update the implementation (to use the correct id table)  in next
> version.

I'm leaning toward this view too. Somehow driver_override is more
restrictive than dynamic IDs based on its description then it sounds
a bit weird that on one hand we are doing strict match upon static
table but on the other hand lifting the bar for dynamic IDs.

If consensus cannot be reached quickly I prefer to not adding
the match and wait to change all drivers together when it
is made later.