Re: [PATCH v7 12/15] s390/vfio-ap: Transition device migration state from STOP to RUNNING and vice versa

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The vfio_ap_set_state callback incorrectly returns success when attempting to change state while the device is in the VFIO_DEVICE_STATE_ERROR state.
- [High] Missing cleanup of the migration state buffer (ap_config) when migration is cancelled or completes without a device reset, leading to stale data on subsequent migrations.
- [Medium] The transition from VFIO_DEVICE_STATE_RUNNING to VFIO_DEVICE_STATE_STOP does not actually halt the physical AP queues, violating the VFIO migration specification.
--

--- Patch [12]: [PATCH v7 12/15] s390/vfio-ap: Transition device migration state from STOP to RUNNING and vice versa ---
commit 17303c3774ef0d4972c24647246d0a4f7d6c4f04
Author: Anthony Krowiak <[email protected]>

s390/vfio-ap: Transition device migration state from STOP to RUNNING and vice versa

This patch implements the state transitions between VFIO_DEVICE_STATE_STOP and
VFIO_DEVICE_STATE_RUNNING for the vfio-ap device. It assumes there is no
internal device state to incorporate and returns success for these transitions.

> diff --git a/drivers/s390/crypto/vfio_ap_migration.c b/drivers/s390/crypto/vfio_ap_migration.c
> index 30160e3dddedd..24955f70e13e2 100644
> --- a/drivers/s390/crypto/vfio_ap_migration.c
> +++ b/drivers/s390/crypto/vfio_ap_migration.c
> @@ -1524,12 +1524,17 @@ vfio_ap_transition_to_state(struct ap_matrix_mdev *matrix_mdev,
>  		return NULL;
>  	}
>  
> +	/*
> +	 * These states indicate migration has either not been initiated or
> +	 * has completed and the vfio-ap device is operating normally. Since the
> +	 * vfio-ap device does not virtualize a DMA device, there is no internal
> +	 * device state to incorporate into the vfio-ap device on the target.
> +	 */
>  	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);
> +		return NULL;

[Severity: Medium]
Does the transition from VFIO_DEVICE_STATE_RUNNING to VFIO_DEVICE_STATE_STOP
need to halt the physical AP queues?

Since the physical AP queues are not reset (e.g., via
vfio_ap_mdev_reset_queues) during this transition, could asynchronous
requests in flight continue executing?

If they complete, they might post adapter interrupts to the KVM GISA after
the guest state has been saved, altering external state and leading to lost
interrupts on the destination.

[Severity: High]
Does the transition from VFIO_DEVICE_STATE_STOP to VFIO_DEVICE_STATE_RUNNING
miss cleaning up the migration state buffer?

If a migration is cancelled, transitioning from STOP to RUNNING now returns
success but doesn't call vfio_ap_release_mig_files() to free ap_config.

On a subsequent migration attempt, vfio_ap_stop_copy_read() will see the
non-NULL ap_config and send the stale configuration instead of capturing a
fresh one:

drivers/s390/crypto/vfio_ap_migration.c:vfio_ap_stop_copy_read() {
    ...
	if (!mig_file->ap_config) {
		ret = vfio_ap_get_config(matrix_mdev);
    ...
}

>  	}
>  
>  	/* vfio_mig_get_next_state() does not use arcs other than the above */

[Severity: High]
With these new state transitions, does vfio_ap_set_state() still handle the
VFIO_DEVICE_STATE_ERROR state correctly?

If a migration error occurs and the device enters VFIO_DEVICE_STATE_ERROR,
userspace might attempt to transition to another state like STOP or RUNNING.

drivers/s390/crypto/vfio_ap_migration.c:vfio_ap_set_state() {
	struct file *filp = NULL;
    ...
	while (mig_data->mig_state != VFIO_DEVICE_STATE_ERROR &&
	       mig_data->mig_state != new_state) {
        ...
		filp = vfio_ap_transition_to_state(matrix_mdev, next_state);
    ...
	return filp;
}

If mig_state is VFIO_DEVICE_STATE_ERROR, the loop condition is false,
bypassing the transition logic. Because filp is initialized to NULL, the
function returns success instead of rejecting the transition.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=12