Re: [PATCH v2] KVM: s390: Fall back to short-term pinning in MAP ioctl

[Matthew Rosato <mjrosato@linux.ibm.com>]

On 7/23/26 2:10 PM, Jaehoon Kim wrote:
> FOLL_LONGTERM pinning fails for some memory types, such as file-backed
> guest memory. As a result, kvm_s390_adapter_map() returns -EINVAL and
> irqfd adapter registration fails even though interrupt delivery could
> still work via the existing non-atomic path.
> 
> When FOLL_LONGTERM pinning fails, verify that the page is accessible
> using a short-term pin instead. If the short-term pin succeeds, unpin
> the page and add a map entry with pinned=false to preserve MAP/UNMAP
> symmetry. The non-atomic irqfd path already performs short-term pinning
> for interrupt delivery, so this restores the previous behavior for
> memory that cannot be pinned long-term.
> 
> get_map_info() is updated to return NULL for unpinned entries so that
> the atomic irqfd fast path falls back to the non-atomic path.
> kvm_s390_adapter_unmap() and kvm_s390_unmap_all_adapters() skip dirty
> marking and unpin for unpinned entries.
> 
> Update Documentation/virt/kvm/devices/s390_flic.rst to reflect the
> new MAP/UNMAP behavior.
> 
> Fixes: adcd5b3e758b ("KVM: s390: Add map/unmap ioctl and clean mappings post-guest")

cc stable?

> Signed-off-by: Jaehoon Kim <jhkim@linux.ibm.com>
> Reviewed-by: Douglas Freimuth <freimuth@linux.ibm.com>
> ---
>  Documentation/virt/kvm/devices/s390_flic.rst | 11 +++-
>  arch/s390/include/asm/kvm_host.h             |  5 ++
>  arch/s390/kvm/interrupt.c                    | 59 +++++++++++++++-----
>  3 files changed, 58 insertions(+), 17 deletions(-)
> 
> diff --git a/Documentation/virt/kvm/devices/s390_flic.rst b/Documentation/virt/kvm/devices/s390_flic.rst
> index b784f8016748..ec9aaae04e22 100644
> --- a/Documentation/virt/kvm/devices/s390_flic.rst
> +++ b/Documentation/virt/kvm/devices/s390_flic.rst
> @@ -112,9 +112,16 @@ Groups:
>        mask or unmask the adapter, as specified in mask
>  
>      KVM_S390_IO_ADAPTER_MAP
> -      This is now a no-op. The mapping is purely done by the irq route.
> +      Map an adapter indicator or summary page for long-term pinning so that
> +      interrupt injection can be performed in atomic context. If long-term
> +      pinning is not possible (e.g. file-backed memory), the page is verified
> +      via a short-term pin and the ioctl returns success; interrupt injection
> +      will use the non-atomic irqfd path with short-term pinning on each
> +      interrupt. In Secure Execution mode this is a no-op.

Nit:
... this is a no-op and the ioctl returns success.

>      KVM_S390_IO_ADAPTER_UNMAP
> -      This is now a no-op. The mapping is purely done by the irq route.
> +      Unmap a previously mapped adapter indicator or summary page and release
> +      the long-term pin. If the page was not long-term pinned (e.g. file-backed
> +      memory or Secure Execution mode), this is a no-op and returns success.

Technically not a no-op in the file-backed case, you will still remove a
list entry.

How about a little re-wording:

KVM_S390_IO_ADAPTER_UNMAP
  Unmap a previously mapped adapter indicator or summary page and release
  the long-term pin. If the page was not long-term pinned (e.g. file-backed
  memory), the map entry is removed and success is returned; if no prior
  map entry exists, -ENOENT is returned. In Secure Execution mode this is
  a no-op and the ioctl returns success.

Otherwise, code looks good to me.  Thanks Jaehoon!

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>

>  
>    KVM_DEV_FLIC_AISM
>      modify the adapter-interruption-suppression mode for a given isc if the
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index eaa34c5bd3c1..c172f9b212d1 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -476,6 +476,11 @@ struct s390_map_info {
>  	__u64 guest_addr;
>  	__u64 addr;
>  	struct page *page;
> +	/*
> +	 * True if the page is long-term pinned. False if long-term pinning
> +	 * failed and this entry exists only to preserve MAP/UNMAP symmetry.
> +	 */
> +	bool pinned;
>  };
>  
>  struct s390_io_adapter {
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 9e3e6b0d72ad..75eacd1578ef 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -2520,8 +2520,22 @@ static int kvm_s390_adapter_map(struct kvm *kvm, unsigned int id, __u64 addr)
>  	map->addr = host_addr;
>  	map->page = pin_map_page(kvm, host_addr, FOLL_LONGTERM);
>  	if (!map->page) {
> -		ret = -EINVAL;
> -		goto out;
> +		/*
> +		 * Long-term pinning may fail for memory types such as file-backed
> +		 * memory. Verify that short-term pinning succeeds so that the
> +		 * non-atomic irqfd path can handle interrupt injection.
> +		 */
> +		map->page = pin_map_page(kvm, host_addr, 0);
> +		if (!map->page) {
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +		unpin_user_page(map->page);
> +		map->page = NULL;
> +		map->pinned = false;
> +		/* Add an entry to preserve MAP/UNMAP symmetry. */
> +	} else {
> +		map->pinned = true;
>  	}
>  	spin_lock_irqsave(&adapter->maps_lock, flags);
>  	if (adapter->nr_maps < MAX_S390_ADAPTER_MAPS) {
> @@ -2532,7 +2546,7 @@ static int kvm_s390_adapter_map(struct kvm *kvm, unsigned int id, __u64 addr)
>  		ret = -EINVAL;
>  	}
>  	spin_unlock_irqrestore(&adapter->maps_lock, flags);
> -	if (ret)
> +	if (ret && map->page)
>  		unpin_user_page(map->page);
>  out:
>  	if (ret)
> @@ -2546,6 +2560,7 @@ static int kvm_s390_adapter_unmap(struct kvm *kvm, unsigned int id, __u64 addr)
>  	struct s390_map_info *map, *tmp, *map_to_free;
>  	struct page *map_page_to_put = NULL;
>  	u64 map_addr_to_mark = 0;
> +	bool map_pinned = false;
>  	unsigned long flags;
>  	int found = 0, idx;
>  
> @@ -2560,6 +2575,7 @@ static int kvm_s390_adapter_unmap(struct kvm *kvm, unsigned int id, __u64 addr)
>  			list_del(&map->list);
>  			map_page_to_put = map->page;
>  			map_addr_to_mark = map->guest_addr;
> +			map_pinned = map->pinned;
>  			map_to_free = map;
>  			break;
>  		}
> @@ -2568,11 +2584,18 @@ static int kvm_s390_adapter_unmap(struct kvm *kvm, unsigned int id, __u64 addr)
>  
>  	if (found) {
>  		kfree(map_to_free);
> -		idx = srcu_read_lock(&kvm->srcu);
> -		mark_page_dirty(kvm, map_addr_to_mark >> PAGE_SHIFT);
> -		set_page_dirty_lock(map_page_to_put);
> -		srcu_read_unlock(&kvm->srcu, idx);
> -		unpin_user_page(map_page_to_put);
> +		if (map_pinned) {
> +			/*
> +			 * Only long-term pinned pages need to be marked dirty
> +			 * and released. Fallback entries exist only for
> +			 * MAP/UNMAP symmetry.
> +			 */
> +			idx = srcu_read_lock(&kvm->srcu);
> +			mark_page_dirty(kvm, map_addr_to_mark >> PAGE_SHIFT);
> +			set_page_dirty_lock(map_page_to_put);
> +			srcu_read_unlock(&kvm->srcu, idx);
> +			unpin_user_page(map_page_to_put);
> +		}
>  	}
>  
>  	return found ? 0 : -ENOENT;
> @@ -2598,11 +2621,13 @@ void kvm_s390_unmap_all_adapters(struct kvm *kvm)
>  
>  		list_for_each_entry_safe(map, tmp, &local_list, list) {
>  			list_del(&map->list);
> -			idx = srcu_read_lock(&kvm->srcu);
> -			mark_page_dirty(kvm, map->guest_addr >> PAGE_SHIFT);
> -			set_page_dirty_lock(map->page);
> -			srcu_read_unlock(&kvm->srcu, idx);
> -			unpin_user_page(map->page);
> +			if (map->pinned) {
> +				idx = srcu_read_lock(&kvm->srcu);
> +				mark_page_dirty(kvm, map->guest_addr >> PAGE_SHIFT);
> +				set_page_dirty_lock(map->page);
> +				srcu_read_unlock(&kvm->srcu, idx);
> +				unpin_user_page(map->page);
> +			}
>  			kfree(map);
>  		}
>  	}
> @@ -2929,8 +2954,12 @@ static struct s390_map_info *get_map_info(struct s390_io_adapter *adapter,
>  		return NULL;
>  
>  	list_for_each_entry(map, &adapter->maps, list) {
> -		if (map->addr == addr)
> -			return map;
> +		if (map->addr == addr) {
> +			if (map->pinned)
> +				return map;
> +			else
> +				return NULL;
> +		}
>  	}
>  	return NULL;
>  }