Re: [PATCH v4 5/9] s390/vfio-ap: Fix control domain removal in vfio_ap_mdev_cfg_remove

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] vfio_ap_mdev_hot_unplug_cfg uses the return value of bitmap_andnot to decide whether to update the guest APCB, bypassing the update when the last device or domain is removed.
--

--- Patch [5]: [PATCH v4 5/9] s390/vfio-ap: Fix control domain removal in vfio_ap_mdev_cfg_remove ---
commit 10842a74fdddf431bdf00cf897b82e3096df6c33
Author: Anthony Krowiak <[email protected]>

s390/vfio-ap: Fix control domain removal in vfio_ap_mdev_cfg_remove
    
The vfio_ap_config_remove function uses the bitmap_andnot function to clear
bits from the matrix_mdev->matrix.adm bitmap. This prevents the explicitly
unplugged control domains from being removed from the KVM guest. The
bitmap_and function is used instead.

Fixes: eeb386aeb5b7c ("s390/vfio-ap: handle config changed and scan complete notification")
Cc: [email protected]

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 21c502598f8ca..0f3537aadea87 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -2636,9 +2636,9 @@ static void vfio_ap_mdev_cfg_remove(unsigned long *ap_remove,
>  		do_remove |= bitmap_and(aqrem, aq_remove,
>  					  matrix_mdev->matrix.aqm,
>  					  AP_DOMAINS);
> -		do_remove |= bitmap_andnot(cdrem, cd_remove,
> -					     matrix_mdev->matrix.adm,
> -					     AP_DOMAINS);
> +		do_remove |= bitmap_and(cdrem, cd_remove,
> +					matrix_mdev->matrix.adm,
> +					AP_DOMAINS);
>  
>  		if (do_remove)
>  			vfio_ap_mdev_hot_unplug_cfg(matrix_mdev, aprem, aqrem,

[Severity: High]
This is a pre-existing issue, but does vfio_ap_mdev_hot_unplug_cfg() fail to
update the guest APCB when the last device or domain is removed?

In drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_mdev_hot_unplug_cfg(), the return
value of bitmap_andnot() is used to determine if the hotplug update should
occur:

	if (!bitmap_empty(aprem, AP_DEVICES)) {
		do_hotplug |= bitmap_andnot(matrix_mdev->shadow_apcb.apm,
					    matrix_mdev->shadow_apcb.apm,
					    aprem, AP_DEVICES);
	}
...
	if (do_hotplug)
		vfio_ap_mdev_update_guest_apcb(matrix_mdev);

Because bitmap_andnot() returns true only if the resulting destination bitmap
is non-empty, removing the final remaining bits will cause bitmap_andnot() to
return 0.

If a host administrator removes the last remaining assigned AP adapter or
domain from the host configuration for a specific mdev, do_hotplug remains 0.
Will this skip the vfio_ap_mdev_update_guest_apcb() call and allow the KVM
guest to retain hardware access to the unplugged resources?

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