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

[Anthony Krowiak <akrowiak@linux.ibm.com>]

On 8/7/26 6:43 PM, sashiko-bot@kernel.org wrote:
> 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 <akrowiak@linux.ibm.com>
>
> 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.

The vfio_ap driver's only role is managing the guest's AP
configuration (shadow_apcb). It has no involvement in AP operations
on the guest; the AP instructions are sent directly to the hardware.
Any in-flight AP requests on the source are part of the guest and AP
bus state that is migrated; it is the responsibility of the guest's
crypto software on the destination to handle responses from the hardware
there. No queue reset is required at this transition point.

>
> [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.

That will be fixed in the next version of the series

>
> 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.

An ERROR check after the transition loop in vfio_ap_set_state() will be
added: filp will be set to ERR_PTR(-EIO).