RE: [PATCH 10/10] vfio/qat: Add vfio_pci driver for Intel QAT VF devices

["Zeng, Xin" <xin.zeng@intel.com>]

Thanks for the comments, Shameer.

> -----Original Message-----
> From: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Sent: Thursday, February 8, 2024 8:17 PM
> To: Zeng, Xin <xin.zeng@intel.com>; herbert@gondor.apana.org.au;
> alex.williamson@redhat.com; jgg@nvidia.com; yishaih@nvidia.com; Tian, Kevin
> <kevin.tian@intel.com>
> Cc: linux-crypto@vger.kernel.org; kvm@vger.kernel.org; qat-linux <qat-
> linux@intel.com>; Cao, Yahui <yahui.cao@intel.com>
> Subject: RE: [PATCH 10/10] vfio/qat: Add vfio_pci driver for Intel QAT VF devices
> 
> 
> > +static struct qat_vf_migration_file *
> > +qat_vf_save_device_data(struct qat_vf_core_device *qat_vdev)
> > +{
> > +	struct qat_vf_migration_file *migf;
> > +	int ret;
> > +
> > +	migf = kzalloc(sizeof(*migf), GFP_KERNEL);
> > +	if (!migf)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	migf->filp = anon_inode_getfile("qat_vf_mig", &qat_vf_save_fops,
> > migf, O_RDONLY);
> > +	ret = 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);
> > +
> > +	ret = qat_vdev->mdev->ops->save_state(qat_vdev->mdev);
> > +	if (ret) {
> > +		fput(migf->filp);
> > +		kfree(migf);
> 
> Probably don't need that kfree(migf) here as fput() -->  qat_vf_release_file () will
> do that.

Thanks, it's redundant, will update it in next version.

> 
> > +static int qat_vf_pci_get_data_size(struct vfio_device *vdev,
> > +				    unsigned long *stop_copy_length)
> > +{
> > +	struct qat_vf_core_device *qat_vdev = container_of(vdev,
> > +			struct qat_vf_core_device, core_device.vdev);
> > +
> > +	*stop_copy_length = qat_vdev->mdev->state_size;
> 
> Do we need a lock here or this is not changing?

Yes, will update it in next version.

> 
> > +	return 0;
> > +}
> > +
> > +static const struct vfio_migration_ops qat_vf_pci_mig_ops = {
> > +	.migration_set_state = qat_vf_pci_set_device_state,
> > +	.migration_get_state = qat_vf_pci_get_device_state,
> > +	.migration_get_data_size = qat_vf_pci_get_data_size,
> > +};
> > +
> > +static void qat_vf_pci_release_dev(struct vfio_device *core_vdev)
> > +{
> > +	struct qat_vf_core_device *qat_vdev = container_of(core_vdev,
> > +			struct qat_vf_core_device, core_device.vdev);
> > +
> > +	qat_vdev->mdev->ops->cleanup(qat_vdev->mdev);
> > +	qat_vfmig_destroy(qat_vdev->mdev);
> > +	mutex_destroy(&qat_vdev->state_mutex);
> > +	vfio_pci_core_release_dev(core_vdev);
> > +}
> > +
> > +static int qat_vf_pci_init_dev(struct vfio_device *core_vdev)
> > +{
> > +	struct qat_vf_core_device *qat_vdev = container_of(core_vdev,
> > +			struct qat_vf_core_device, core_device.vdev);
> > +	struct qat_migdev_ops *ops;
> > +	struct qat_mig_dev *mdev;
> > +	struct pci_dev *parent;
> > +	int ret, vf_id;
> > +
> > +	core_vdev->migration_flags = VFIO_MIGRATION_STOP_COPY |
> > VFIO_MIGRATION_P2P;
> > +	core_vdev->mig_ops = &qat_vf_pci_mig_ops;
> > +
> > +	ret = vfio_pci_core_init_dev(core_vdev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	mutex_init(&qat_vdev->state_mutex);
> > +	spin_lock_init(&qat_vdev->reset_lock);
> > +
> > +	parent = qat_vdev->core_device.pdev->physfn;
> 
> Can we use pci_physfn() here?

Sure, will update it in next version.

> 
> > +	vf_id = pci_iov_vf_id(qat_vdev->core_device.pdev);
> > +	if (!parent || vf_id < 0) {
> 
> Also if the pci_iov_vf_id() return success I don't think you need to
> check for parent and can use directly below.

OK, will update it in next version.

> 
> > +		ret = -ENODEV;
> > +		goto err_rel;
> > +	}
> > +
> > +	mdev = qat_vfmig_create(parent, vf_id);
> > +	if (IS_ERR(mdev)) {
> > +		ret = PTR_ERR(mdev);
> > +		goto err_rel;
> > +	}
> > +
> > +	ops = mdev->ops;
> > +	if (!ops || !ops->init || !ops->cleanup ||
> > +	    !ops->open || !ops->close ||
> > +	    !ops->save_state || !ops->load_state ||
> > +	    !ops->suspend || !ops->resume) {
> > +		ret = -EIO;
> > +		dev_err(&parent->dev, "Incomplete device migration ops
> > structure!");
> > +		goto err_destroy;
> > +	}
> 
> If all these ops are a must why cant we move the check inside the
> qat_vfmig_create()?
> Or rather call them explicitly as suggested by Jason.

We can do it, but it might make sense to leave the check to the APIs' user
as some of these ops interfaces might be optional for other QAT variant driver
in future.

Thanks,
Xin