Received-SPF: pass (google.com: domain of linux-crypto+bounces-2517-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
From: "Zeng, Xin" <xin.zeng@intel.com>
To: "Tian, Kevin" <kevin.tian@intel.com>, "herbert@gondor.apana.org.au"
	<herbert@gondor.apana.org.au>, "alex.williamson@redhat.com"
	<alex.williamson@redhat.com>, "jgg@nvidia.com" <jgg@nvidia.com>,
	"yishaih@nvidia.com" <yishaih@nvidia.com>,
	"shameerali.kolothum.thodi@huawei.com" <shameerali.kolothum.thodi@huawei.com>
CC: "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: AQHaalR9StzovMarUEC10fvX40lRFLEoy0sAgAAnj9A=
Date: Wed, 6 Mar 2024 02:47:36 +0000
Message-ID: <DM4PR11MB550263F49D2EA8755F3CCFA588212@DM4PR11MB5502.namprd11.prod.outlook.com>
References: <20240228143402.89219-1-xin.zeng@intel.com>
 <20240228143402.89219-11-xin.zeng@intel.com>
 <BN9PR11MB5276492EDE7F9CA57117FE518C222@BN9PR11MB5276.namprd11.prod.outlook.com>
In-Reply-To: <BN9PR11MB5276492EDE7F9CA57117FE518C222@BN9PR11MB5276.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?2OTsKs5oYk50VhSF+EIXZ1Co3SbIsi42GmLOO772AQsrbmk5ZtNwRUaOKlcE?=
 =?us-ascii?Q?sqvgvssdBrKKii/hBmrMOtCd7RKLH6fhwvRI9dY233r1vDR4qm7TT8IMFnMN?=
 =?us-ascii?Q?k01TLSby3HznMR5Ix8/Bx9p/pnshW00v9DkcpaeFM6UD/brtrTiO0ZpHKkU1?=
 =?us-ascii?Q?iVbg9UoQzzEhRlSb8PWX3aQZjDZ7RYoItuxpIuUnvFo6dHSvYfosS1X1+usO?=
 =?us-ascii?Q?uCJbKV0YzhV0gTYBgarxFFVD53DqdZXrvgMKFpWO2eydhMESiUKALnG/X8iG?=
 =?us-ascii?Q?NvFNkzjvHny04BHG/jyDY6FbwacS7f2gwAASVgZLnDNz2U9BVIREbGGdur4W?=
 =?us-ascii?Q?AEO1cYhP8iHwtBESTikUccTi8j6eQwdeoWL7YrH9jxe+afIYOv/fpWvOhSdi?=
 =?us-ascii?Q?4aGdwulDQ/U91e2sdsfrXcXNlloqXqwbVuA3pkxNVRYOOfAMxPbIzdwG0/sU?=
 =?us-ascii?Q?KJGJrBXTHnrpnsvRDKFNNk9zATgEMA4I6s63KjFI0o7TUHJ0TI6wjKki+hYv?=
 =?us-ascii?Q?DZ7qnBzJVDxM44Bjhi6EcehFynCvr+l8QR9CyXmNeUJZlrUYMx4ZCbrO876P?=
 =?us-ascii?Q?gtn9P4i1rn1IUySMqdTrLHujCpK6CKxufidNxlq1/4MIOtiDx+gnBgRB6xJE?=
 =?us-ascii?Q?EXASYZdOCun03BfTt4PJvGakv2H4n7aoeQRSMaKAZYOQLzfDJxcl1ClskVQq?=
 =?us-ascii?Q?dyfUZMbg41fNPkp+I0H92fdkoZn/xfH57remF/t1BvKRjrstzcC4ImWENoY8?=
 =?us-ascii?Q?e1aoCl5xNnJ7Lru/tzpBEGhDRUbcmYiJgg151NqpOO2NjB+bicSmfU1vt1kz?=
 =?us-ascii?Q?/MoX8mcK9OwBU4Ntip/MwVCbINnhvHi/aayysisI9luVhzVOaN7bXnLiazRx?=
 =?us-ascii?Q?B/ZyD5poTGy/5a4K7zROVjEVbf25dA1uR7mBPxwfhL7Gg1M33I7OWTdQZrbW?=
 =?us-ascii?Q?ulAESH+qncMkna+xsF61TP/CkA93nynuflfk6d3/2Z5EJzIbfXLmMuaksDlx?=
 =?us-ascii?Q?kRYKDnY6/55J3zEfTKjwab1iFCgZDETJ59k15DTt3u5z1Rksi0MPWK4RVtZT?=
 =?us-ascii?Q?RtJgnZGPaghBWpEpAJUc12LnTJtuzunJqAgtdDq9Bk82sYXj9jpgzJTEylcx?=
 =?us-ascii?Q?vJYNYz4XzMyHzaIHbDTDCkA8ufU3PsVVQxTWcgASsDp7Ou8u4bCtZxD1LKOi?=
 =?us-ascii?Q?MvL8+R3LpCh44XWuGV/kGRO9R1+Y8cONXh+nTrWMZIU1o+y3o3TdoaEd2iCG?=
 =?us-ascii?Q?79o+pgcsptLTKH0oxMS9M4SmqSbmR1Es0NInRyQjGhL0VGjgTQQXvK87b3vr?=
 =?us-ascii?Q?fnLHft6x+54av4Z3x3yJoVr33sXpT5QFXh+1z/UNQ4QOSA5utGcBVOoUMrG/?=
 =?us-ascii?Q?meWWQdYDaEawiB7cJbKH5YrpSZwxSg4OUgSNVU84nB8u6EvVjvwvhcybmvrP?=
 =?us-ascii?Q?ZdOjshG3KVLK4uOj0L8Sc17AROYiWkr7eN5aaEWrv7iplI5pTkLuQaEPKCsE?=
 =?us-ascii?Q?VlhQisf3oUPHE3pxS47sx3VH6OSUhcMiONjr2s8x8aMoqzD/O+yhUjueOT6h?=
 =?us-ascii?Q?lbkNsRWylPFQNm50QveT0Ja0oGLKI/SSy+p+fH7r?=
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: DM4PR11MB5502.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: da9b25af-8f17-40a4-2e5a-08dc3d87c7f1
X-MS-Exchange-CrossTenant-originalarrivaltime: 06 Mar 2024 02:47:36.3893
 (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: 6ZYrHE66lY+Mtm38UESi1d/+JeXkodn94EDQ1pQ7wNAr0CwAlCxew8FQr66aBWuCY5B4qrWRJMlLe6QwPjKTAQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB8501
X-OriginatorOrg: intel.com

Thanks for the comments, Kevin.
On Tuesday, March 5, 2024 3:38 PM, Tian, Kevin <kevin.tian@intel.com> wrote=
:
> > From: Zeng, Xin <xin.zeng@intel.com>
> > Sent: Wednesday, February 28, 2024 10:34 PM
> > +
> > +static long qat_vf_precopy_ioctl(struct file *filp, unsigned int cmd,
> > +				 unsigned long arg)
> > +{
> > +	struct qat_vf_migration_file *migf =3D filp->private_data;
> > +	struct qat_vf_core_device *qat_vdev =3D migf->qat_vdev;
> > +	struct qat_mig_dev *mig_dev =3D qat_vdev->mdev;
> > +	struct vfio_precopy_info info;
> > +	loff_t *pos =3D &filp->f_pos;
> > +	unsigned long minsz;
> > +	int ret =3D 0;
> > +
> > +	if (cmd !=3D VFIO_MIG_GET_PRECOPY_INFO)
> > +		return -ENOTTY;
> > +
> > +	minsz =3D offsetofend(struct vfio_precopy_info, dirty_bytes);
> > +
> > +	if (copy_from_user(&info, (void __user *)arg, minsz))
> > +		return -EFAULT;
> > +	if (info.argsz < minsz)
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&qat_vdev->state_mutex);
> > +	if (qat_vdev->mig_state !=3D VFIO_DEVICE_STATE_PRE_COPY) {
> > +		mutex_unlock(&qat_vdev->state_mutex);
> > +		return -EINVAL;
> > +	}
>=20
> what about PRE_COPY_P2P?

Good catch, will add it in next version.

>=20
> > +static struct qat_vf_migration_file *
> > +qat_vf_save_device_data(struct qat_vf_core_device *qat_vdev, bool
> > pre_copy)
> > +{
> > +	struct qat_vf_migration_file *migf;
> > +	int ret;
> > +
> > +	migf =3D kzalloc(sizeof(*migf), GFP_KERNEL);
> > +	if (!migf)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	migf->filp =3D anon_inode_getfile("qat_vf_mig", &qat_vf_save_fops,
> > +					migf, O_RDONLY);
> > +	ret =3D PTR_ERR_OR_ZERO(migf->filp);
> > +	if (ret) {
> > +		kfree(migf);
> > +		return ERR_PTR(ret);
> > +	}
> > +
> > +	stream_open(migf->filp->f_inode, migf->filp);
> > +	mutex_init(&migf->lock);
> > +
> > +	if (pre_copy)
> > +		ret =3D qat_vf_save_setup(qat_vdev, migf);
> > +	else
> > +		ret =3D qat_vf_save_state(qat_vdev, migf);
> > +	if (ret) {
> > +		fput(migf->filp);
> > +		return ERR_PTR(ret);
> > +	}
>=20
> lack of kfree(migf). Using goto can avoid such typo in error unwind.

kfree(migf) will be invoked in fput. This was pointed out by
Shameer and I updated it in v2. :-)

>=20
> > +
> > +static struct file *qat_vf_pci_step_device_state(struct
> qat_vf_core_device
> > *qat_vdev, u32 new)
> > +{
> > +	u32 cur =3D qat_vdev->mig_state;
> > +	int ret;
> > +
> > +	if ((cur =3D=3D VFIO_DEVICE_STATE_RUNNING && new =3D=3D
> > VFIO_DEVICE_STATE_RUNNING_P2P) ||
> > +	    (cur =3D=3D VFIO_DEVICE_STATE_PRE_COPY && new =3D=3D
> > VFIO_DEVICE_STATE_PRE_COPY_P2P)) {
> > +		ret =3D qat_vfmig_suspend(qat_vdev->mdev);
> > +		if (ret)
> > +			return ERR_PTR(ret);
> > +		return NULL;
> > +	}
>=20
> could you arrange the transitions in pair as mlx does e.g. following
> above is the reverse one calling qat_vfmig_resume()?

Sure, will update it in next version.

>=20
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_AUTHOR("Intel Corporation");
>=20
> please use your name as the author.

Sure, will do.