Re: [PATCH v9 1/3] KVM: s390: Add map/unmap ioctl and clean mappings post-guest

[Janosch Frank <frankja@linux.ibm.com>]

On 5/31/26 21:03, Douglas Freimuth wrote:
> s390 needs map/unmap ioctls, which map the adapter set
> indicator pages, so the pages can be accessed when interrupts are
> disabled. The mappings are cleaned up when the guest is removed.
> pin_user_pages_remote is used for both the ioctl as well
> as the pin-on-demand logic in adapter_indicators_set().
> 
> Map/Unmap ioctls are fenced in order to avoid the longterm pinning
> in Secure Execution environments. In Secure Execution
> environments the path of execution available before this patch is followed.
> 
> Statistical counters to count map/unmap functions for adapter indicator
> pages are added. The counters can be used to analyze
> map/unmap functions in non-Secure Execution environments and similarly
> can be used to analyze Secure Execution environments where the counters
> will not be incremented as the adapter indicator pages are not mapped.
> 
> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
> Signed-off-by: Douglas Freimuth <freimuth@linux.ibm.com>
> ---
>   arch/s390/include/asm/kvm_host.h |   5 +
>   arch/s390/kvm/interrupt.c        | 226 +++++++++++++++++++++++++------
>   arch/s390/kvm/kvm-s390.c         |   3 +
>   arch/s390/kvm/kvm-s390.h         |   2 +
>   4 files changed, 195 insertions(+), 41 deletions(-)
> 
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index 8a4f4a39f7a2..0056cc9414a0 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -448,6 +448,8 @@ struct kvm_vcpu_arch {
>   struct kvm_vm_stat {
>   	struct kvm_vm_stat_generic generic;
>   	u64 inject_io;
> +	u64 io_390_adapter_map;
> +	u64 io_390_adapter_unmap;
>   	u64 inject_float_mchk;
>   	u64 inject_pfault_done;
>   	u64 inject_service_signal;
> @@ -479,6 +481,9 @@ struct s390_io_adapter {
>   	bool masked;
>   	bool swap;
>   	bool suppressible;
> +	spinlock_t maps_lock;
> +	struct list_head maps;
> +	unsigned int nr_maps;
>   };
>   
>   #define MAX_S390_IO_ADAPTERS ((MAX_ISC + 1) * 8)
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 3bcdbbbb6891..5ad0b29c8c1b 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -2411,24 +2411,34 @@ static int register_io_adapter(struct kvm_device *dev,
>   {
>   	struct s390_io_adapter *adapter;
>   	struct kvm_s390_io_adapter adapter_info;
> +	int rc = 0;

Why do you even set it to 0 if you return 0 on the success case instead 
of return rc?

>   
> +	mutex_lock(&dev->kvm->lock);
>   	if (copy_from_user(&adapter_info,
> -			   (void __user *)attr->addr, sizeof(adapter_info)))
> -		return -EFAULT;
> -
> -	if (adapter_info.id >= MAX_S390_IO_ADAPTERS)
> -		return -EINVAL;
> -
> +			   (void __user *)attr->addr, sizeof(adapter_info))) {
> +		rc = -EFAULT;
> +		goto out;
> +	}
> +	if (adapter_info.id >= MAX_S390_IO_ADAPTERS) {
> +		rc = -EINVAL;
> +		goto out;
> +	}
>   	adapter_info.id = array_index_nospec(adapter_info.id,
>   					     MAX_S390_IO_ADAPTERS);
>   
> -	if (dev->kvm->arch.adapters[adapter_info.id] != NULL)
> -		return -EINVAL;
> -
> +	if (dev->kvm->arch.adapters[adapter_info.id] != NULL) {
> +		rc = -EINVAL;

You could move the rc assignment outside of the braces and have one less 
assignment. But if you like it more this way then keep it, I'm still 
able to read it :)

> +		goto out;
> +	}
>   	adapter = kzalloc_obj(*adapter, GFP_KERNEL_ACCOUNT);
> -	if (!adapter)
> -		return -ENOMEM;
> +	if (!adapter) {
> +		rc = -ENOMEM;
> +		goto out;
> +	}
>   
> +	INIT_LIST_HEAD(&adapter->maps);
> +	spin_lock_init(&adapter->maps_lock);
> +	adapter->nr_maps = 0;
>   	adapter->id = adapter_info.id;
>   	adapter->isc = adapter_info.isc;
>   	adapter->maskable = adapter_info.maskable;
> @@ -2437,8 +2447,13 @@ static int register_io_adapter(struct kvm_device *dev,
>   	adapter->suppressible = (adapter_info.flags) &

Why the braces?

>   				KVM_S390_ADAPTER_SUPPRESSIBLE;
>   	dev->kvm->arch.adapters[adapter->id] = adapter;
> +	mutex_unlock(&dev->kvm->lock);
>   
>   	return 0;
> +
> +out:
> +	mutex_unlock(&dev->kvm->lock);
> +	return rc;

If you set rc to 0 you can actually have one return for all cases.

>   }
>   
>   int kvm_s390_mask_adapter(struct kvm *kvm, unsigned int id, bool masked)
> @@ -2453,12 +2468,151 @@ int kvm_s390_mask_adapter(struct kvm *kvm, unsigned int id, bool masked)
>   	return ret;
>   }
>