Re: [PATCH v12 05/14] 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 1 potential
issue to consider:
- [Medium] s390/vfio-ap: wrong SE guest check for migration in
  vfio_ap_set_state()
--

commit 2bdeb1a9bd0e390837450d39cca0f224a21db0ec
Author: Anthony Krowiak <akrowiak@linux.ibm.com>

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

This commit adds the migration_set_state and migration_get_state callbacks
for vfio_ap devices to manage device migration states during guest live
migration.

> diff --git a/drivers/s390/crypto/vfio_ap_migration.c b/drivers/s390/crypto/vfio_ap_migration.c
> index 94772cab1e855..df012f80ed0d7 100644
> --- a/drivers/s390/crypto/vfio_ap_migration.c
> +++ b/drivers/s390/crypto/vfio_ap_migration.c
> @@ -143,6 +143,150 @@ static void vfio_ap_release_resuming_file(struct vfio_ap_migration_data *mig_dat

[ ... ]

> +static struct file *vfio_ap_set_state(struct vfio_device *vdev,
> +				      enum vfio_device_mig_state  new_state)
> +{
> +	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;
> +
> +	mutex_lock(&matrix_dev->mdevs_lock);
> +
> +	matrix_mdev = container_of(vdev, struct ap_matrix_mdev, vdev);
> +	mig_data = matrix_mdev->mig_data;
> +
> +	/*
> +	 * The mig_data pointer is set in the vfio_ap_init_migration_data
> +	 * function which is called when the vfio-ap device fd is opened.
> +	 * Since the implicit pre-open state is RUNNING, a request to set
> +	 * RUNNING is a no-op. Any other state transition is invalid before
> +	 * open_device.
> +	 */
> +	if (!mig_data) {
> +		mutex_unlock(&matrix_dev->mdevs_lock);
> +		if (new_state == VFIO_DEVICE_STATE_RUNNING)
> +			return NULL;
> +		return ERR_PTR(-ENODEV);
> +	}

[Severity: Medium]
Does vfio_ap_set_state() allow migration state transitions for Secure
Execution (SE) KVM guests?

vfio_ap_init_migration_data() tries to skip allocating mig_data for SE
guests by checking ap_is_se_guest(), which tests is_prot_virt_guest():

drivers/s390/crypto/ap_bus.c:ap_is_se_guest() {
	return is_prot_virt_guest() && ap_sb_available();
}

On a KVM host, is_prot_virt_guest() is always false because it checks
whether the running host kernel itself is an SE guest rather than whether
the KVM guest attached to matrix_mdev is protected.  In addition, a KVM
guest only enters SE mode later during boot via DIAG 308 subcode 10
(KVM_PV_ENABLE), after open_device has already called
vfio_ap_init_migration_data().  As a result, mig_data is always allocated
and vfio_ap_set_state() does not reject migration state changes on SE KVM
guests.

Should vfio_ap_set_state() check kvm_s390_pv_is_protected(matrix_mdev->kvm)
(under kvm->lock) to reject migration when the attached KVM guest is in SE
mode?

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