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

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

On 3/26/26 02:42, 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.
> 
> 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.
> 
> Signed-off-by: Douglas Freimuth <freimuth@linux.ibm.com>
> ---

Looks good, two nits below.

[...]
>   		if (ret > 0)
>   			ret = 0;
>   		break;
> -	/*
> -	 * The following operations are no longer needed and therefore no-ops.
> -	 * The gpa to hva translation is done when an IRQ route is set up. The
> -	 * set_irq code uses get_user_pages_remote() to do the actual write.
> -	 */
>   	case KVM_S390_IO_ADAPTER_MAP:
>   	case KVM_S390_IO_ADAPTER_UNMAP:
> -		ret = 0;
> +		/* If in Secure Execution mode do not long term pin. */
> +		mutex_lock(&dev->kvm->lock);
> +		if (kvm_s390_pv_is_protected(dev->kvm)) {
> +			mutex_unlock(&dev->kvm->lock);
> +			return 0;
> +		}
> +		mutex_unlock(&dev->kvm->lock);
> +		idx = srcu_read_lock(&dev->kvm->srcu);
> +		host_addr = gpa_to_hva(dev->kvm, req.addr);
> +		if (kvm_is_error_hva(host_addr)) {
> +			srcu_read_unlock(&dev->kvm->srcu, idx);
> +			return -EFAULT;
> +			}

Alignment issue

[...]

>   static int kvm_s390_handle_pv(struct kvm *kvm, struct kvm_pv_cmd *cmd)
>   {
>   	const bool need_lock = (cmd->cmd != KVM_PV_ASYNC_CLEANUP_PERFORM);
> @@ -2503,6 +2522,7 @@ static int kvm_s390_handle_pv(struct kvm *kvm, struct kvm_pv_cmd *cmd)
>   
>   	switch (cmd->cmd) {
>   	case KVM_PV_ENABLE: {
> +		kvm_s390_unmap_all_adapters_pv(kvm);

Shouldn't this be located after the check that's below?

>   		r = -EINVAL;
>   		if (kvm_s390_pv_is_protected(kvm))
>   			break;