Thanks for the comments, Kevin.
On Tuesday, March 5, 2024 3:38 PM, Tian, Kevin <kevin.tian@xxxxxxxxx> wrote:
> > From: Zeng, Xin <xin.zeng@xxxxxxxxx>
> > 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 = filp->private_data;
> > + struct qat_vf_core_device *qat_vdev = migf->qat_vdev;
> > + struct qat_mig_dev *mig_dev = qat_vdev->mdev;
> > + struct vfio_precopy_info info;
> > + loff_t *pos = &filp->f_pos;
> > + unsigned long minsz;
> > + int ret = 0;
> > +
> > + if (cmd != VFIO_MIG_GET_PRECOPY_INFO)
> > + return -ENOTTY;
> > +
> > + minsz = 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 != VFIO_DEVICE_STATE_PRE_COPY) {
> > + mutex_unlock(&qat_vdev->state_mutex);
> > + return -EINVAL;
> > + }
>
> what about PRE_COPY_P2P?
Good catch, will add it in next version.
>
> > +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 = 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);
> > +
> > + if (pre_copy)
> > + ret = qat_vf_save_setup(qat_vdev, migf);
> > + else
> > + ret = qat_vf_save_state(qat_vdev, migf);
> > + if (ret) {
> > + fput(migf->filp);
> > + return ERR_PTR(ret);
> > + }
>
> 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. :-)
>
> > +
> > +static struct file *qat_vf_pci_step_device_state(struct
> qat_vf_core_device
> > *qat_vdev, u32 new)
> > +{
> > + u32 cur = qat_vdev->mig_state;
> > + int ret;
> > +
> > + if ((cur == VFIO_DEVICE_STATE_RUNNING && new ==
> > VFIO_DEVICE_STATE_RUNNING_P2P) ||
> > + (cur == VFIO_DEVICE_STATE_PRE_COPY && new ==
> > VFIO_DEVICE_STATE_PRE_COPY_P2P)) {
> > + ret = qat_vfmig_suspend(qat_vdev->mdev);
> > + if (ret)
> > + return ERR_PTR(ret);
> > + return NULL;
> > + }
>
> 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.
>
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_AUTHOR("Intel Corporation");
>
> please use your name as the author.
Sure, will do.
