Re: [PATCH v7 14/15] s390/vfio-ap: Add 'migratable' feature to sysfs 'features' attribute

["Jason J. Herne" <jjherne@linux.ibm.com>]

On 8/7/26 6:18 PM, Anthony Krowiak wrote:
> Add the 'migratable' feature to the features provided by the sysfs
> /sys/devices/vfio_ap/matrix/feature attribute to indicate that migration
> of vfio devices is supported. Since live guest migration is not supported
> for SE guests, the 'migratable' feature will not be included when the
> feature attribute is displayed.
> 
> Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com>
> ---
>   drivers/s390/crypto/vfio_ap_drv.c | 14 +++++++++++++-
>   1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
> index 8e69ed286bb9..abfb34421d74 100644
> --- a/drivers/s390/crypto/vfio_ap_drv.c
> +++ b/drivers/s390/crypto/vfio_ap_drv.c
> @@ -26,9 +26,21 @@ MODULE_LICENSE("GPL v2");
>   struct ap_matrix_dev *matrix_dev;
>   debug_info_t *vfio_ap_dbf_info;
>   
> +#define VFIO_AP_DRV_FEATURES	"guest_matrix hotplug ap_config migratable"
> +#define SE_GUEST_FEATURES	"guest_matrix hotplug ap_config"
> +
>   static ssize_t features_show(struct device *dev, struct device_attribute *attr, char *buf)
>   {
> -	return sysfs_emit(buf, "guest_matrix hotplug ap_config\n");
> +	/*
> +	 * If the system running is an SE guest, it will support a different
> +	 * set of features; for example, it will not support live guest
> +	 * migration
> +	 */
> +	if (ap_is_se_guest())
> +		return sysfs_emit(buf, "%s\n", SE_GUEST_FEATURES);

ap_is_se_guest() is defined like this:
is_prot_virt_guest() && ap_sb_available();

is_prot_virt_guest() makes sense, this tells is this is actually a 
secured guest.

But ap_sb_available() "Returns 1 if secure binding facility is available".

What do we want to happen here if is_prot_virt_guest() = TRUE while 
ap_sb_available() = FALSE? Have you considered this scenario?

Also, the code would be a bit more extendible if you emitted the static 
string of always supported features, and then if(se) emit the string of 
se support features prepended with a space. Then new feature updates 
only have a single string to modify, so no risk of accidentally missing one.

Also, I'd consider using "migrate" or "migration" as they match existing 
feature designations better. We did not use hotpluggable :)

> +
> +	/* Return all features supported by the vfio_ap device driver */
> +	return sysfs_emit(buf, "%s\n", VFIO_AP_DRV_FEATURES);
>   }
>   static DEVICE_ATTR_RO(features);
>