Re: [PATCH v4 05/15] s390-vfio-ap: Callback to get/set vfio device mig state during guest migration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The migration state machine is completely stubbed out, resulting in unconditional migration failures despite advertising migration capabilities.
- [High] A self-deadlock occurs in the error path of vfio_ap_mdev_probe() when vfio_register_emulated_iommu_dev() fails.
- [Medium] Migration capabilities are unconditionally advertised to userspace, but runtime transitions are blocked for Secure Execution (SE) guests.
--

--- Patch [5]: [PATCH v4 05/15] s390-vfio-ap: Callback to get/set vfio device mig state during guest migration ---
commit c722c4dee661c005442dd72e3de8888c3305b360
Author: Anthony Krowiak <akrowiak@linux.ibm.com>

s390-vfio-ap: Callback to get/set vfio device mig state during guest migration

This commit implements two callbacks, migration_set_state and
migration_get_state, for the vfio_migration_ops structure. These callbacks
manage the migration state of a vfio-ap device during live migration of
guests with pass-through access.

> diff --git a/drivers/s390/crypto/vfio_ap_migration.c b/drivers/s390/crypto/vfio_ap_migration.c
> index c7c71980bdbb8..8274e7b65eea4 100644
> --- a/drivers/s390/crypto/vfio_ap_migration.c
> +++ b/drivers/s390/crypto/vfio_ap_migration.c

[ ... ]

> @@ -56,16 +57,140 @@ struct vfio_ap_config {
>  	struct vfio_ap_queue_info	qinfo[] __counted_by(num_queues);
>  };
>  
> +static struct file *
> +vfio_ap_transition_to_state(struct ap_matrix_mdev *matrix_mdev,
> +			    enum vfio_device_mig_state new_state)
> +{
> +	struct vfio_ap_migration_data *mig_data;
> +	enum vfio_device_mig_state cur_state;
> +
> +	lockdep_assert_held(&matrix_dev->mdevs_lock);
> +	mig_data = matrix_mdev->mig_data;
> +	cur_state = mig_data->mig_state;
> +	dev_dbg(matrix_mdev->vdev.dev, "%s: %d -> %d\n", __func__, cur_state,
> +		new_state);
> +
> +	if (cur_state == VFIO_DEVICE_STATE_STOP &&
> +	    new_state == VFIO_DEVICE_STATE_STOP_COPY) {
> +		/* TODO */
> +		return ERR_PTR(-EOPNOTSUPP);
> +	}
> +
> +	if (cur_state == VFIO_DEVICE_STATE_STOP &&
> +	    new_state == VFIO_DEVICE_STATE_RESUMING) {
> +		/* TODO */
> +		return ERR_PTR(-EOPNOTSUPP);
> +	}
> +
> +	if ((cur_state == VFIO_DEVICE_STATE_RESUMING &&
> +	     new_state == VFIO_DEVICE_STATE_STOP) ||
> +	    (cur_state == VFIO_DEVICE_STATE_STOP_COPY &&
> +	     new_state == VFIO_DEVICE_STATE_STOP)) {
> +		/* TODO */
> +		return ERR_PTR(-EOPNOTSUPP);
> +	}
> +
> +	if ((cur_state == VFIO_DEVICE_STATE_STOP &&
> +	     new_state == VFIO_DEVICE_STATE_RUNNING) ||
> +	    (cur_state == VFIO_DEVICE_STATE_RUNNING &&
> +	     new_state == VFIO_DEVICE_STATE_STOP)) {
> +		/* TODO */
> +		return ERR_PTR(-EOPNOTSUPP);

[Severity: High]
Does this code cause a regression by unconditionally returning -EOPNOTSUPP?

Since all valid migration state transitions in vfio_ap_transition_to_state()
are caught and unconditionally return -EOPNOTSUPP, could this cause
orchestrators (like libvirt or QEMU) to fail at runtime? Orchestrators
querying the device will see migration as supported, but might unconditionally
fail when they attempt to change the state.

> +	}
> +
> +	/* vfio_mig_get_next_state() does not use arcs other than the above */
> +	WARN_ON(true);
> +
> +	return ERR_PTR(-EINVAL);
> +}
> +
>  static struct file *vfio_ap_set_state(struct vfio_device *vdev,
>  				      enum vfio_device_mig_state  new_state)
>  {
> -	return NULL;
> +	int ret;
> +	struct file *filp = NULL;
> +	struct ap_matrix_mdev *matrix_mdev;
> +	enum vfio_device_mig_state next_state;
> +	struct vfio_ap_migration_data *mig_data;
> +
> +	matrix_mdev = container_of(vdev, struct ap_matrix_mdev, vdev);
> +
> +	mutex_lock(&matrix_dev->mdevs_lock);
> +	if (ap_is_se_guest()) {
> +		dev_err(matrix_mdev->vdev.dev,
> +			"Migration not allowed from or to a Secure Execution guest\n");
> +		mutex_unlock(&matrix_dev->mdevs_lock);
> +		return ERR_PTR(-EPERM);

[Severity: Medium]
Could this runtime check cause a regression with orchestration software?

By advertising migration capabilities unconditionally to userspace, but
blocking transitions for Secure Execution (SE) guests here at runtime,
orchestrators might incorrectly believe migration is supported for SE guests. 

Could this be rejected earlier during capability checks rather than failing
during the actual migration attempt?

> +	}
> +
> +	mig_data = matrix_mdev->mig_data;

[ ... ]

[Severity: High]
Is there a regression involving a self-deadlock in the error path of
vfio_ap_mdev_probe() when vfio_register_emulated_iommu_dev() fails?

The error path in drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_mdev_probe()
holds the mdevs_lock while invoking vfio_put_device():

err_put_vdev:
	vfio_put_device(&matrix_mdev->vdev);
	mutex_unlock(&matrix_dev->mdevs_lock);

This triggers the synchronous release callback vfio_ap_mdev_release_dev()
which attempts to acquire the same lock:

	mutex_lock(&matrix_dev->mdevs_lock);
	matrix_mdev = container_of(vdev, struct ap_matrix_mdev, vdev);
	vfio_ap_release_migration_data(matrix_mdev);
	mutex_unlock(&matrix_dev->mdevs_lock);

Could this cause a kernel thread freeze when creating an mdev device fails?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707090226.3206695-1-akrowiak@linux.ibm.com?part=5