[PATCH v4 0/3] KVM: s390: Introducing kvm_arch_set_irq_inatomic Fast Inject

[PATCH v4 0/3] KVM: s390: Introducing kvm_arch_set_irq_inatomic Fast Inject

[Douglas Freimuth]

S390 needs this series of three patches in order to enable a non-blocking
path for irqfd injection on s390 via kvm_arch_set_irq_inatomic(). Before
these changes, kvm_arch_set_irq_inatomic() would just return -EWOULDBLOCK
and place all interrupts on the global work queue, which must subsequently
be processed by a different thread. This series of patches implements an
s390 version of inatomic and is relevant to virtio-blk and virtio-net and
was tested against virtio-pci and virtio-ccw.

The inatomic fast path cannot lose control since it is running with
interrupts disabled. This meant making the following changes that exist on
the slow path today. First, the adapter_indicators page needs to be mapped
since it is accessed with interrupts disabled, so we added map/unmap
functions. Second, access to shared resources between the fast and slow
paths needed to be changed from mutex and semaphores to raw_spin_lock's.
Finally, the memory allocation on the slow path utilizes GFP_KERNEL_ACCOUNT
but we had to implement the fast path with GFP_ATOMIC allocation. Each of
these enhancements were required to prevent blocking on the fast inject
path.

Statistical counters have been added to enable analysis of irq injection on
the fast path and slow path including io_390_inatomic, io_flic_inject_airq,
io_set_adapter_int and io_390_inatomic_adapter_masked. And counters have
been added to analyze map/unmap of the adapter_indicator
pages in non-Secure Execution environments and to track fencing of Fast
Inject in Secure Execution environments. In order to take advantage of this
kernel series with virtio-pci, a QEMU that includes the
's390x/pci: set kvm_msi_via_irqfd_allowed' fix is needed.  Additionally,
the guest xml needs a thread pool and threads explicitly assigned per disk
device using the common way of defining threads for disks.

Patch 1 enables map/unmap of adapter indicator pages but for Secure
Execution environments it avoids the long term mapping.

v3->v4: In kvm_s390_adapter_unmap mark_page_dirty using guest address.
v3->v4: In kvm_s390_unmap_all_adapters_pv mark_page_dirty using guest addr.
v3->v4: In kvm_s390_unmap_all_adapters_pv decrement nr_maps.
v3->v4: In kvm_s390_adapter_unmap hold maps->lock through list deletion.
v3->v4: In kvm_s390_destroy_adapters hold the maps->lock until list empty.
v3->v4: In kvm_s390_unmap_all_adapters_pv hold maps->lock to empty list.
v3->v4: Change maps_lock and ais_lock to raw_spinlock for RT kernel case.

Douglas Freimuth (3):
  KVM: s390: Add map/unmap ioctl and clean mappings post-guest
  KVM: s390: Enable adapter_indicators_set to use mapped pages
  KVM: s390: Introducing kvm_arch_set_irq_inatomic fast inject

 arch/s390/include/asm/kvm_host.h |  11 +-
 arch/s390/kvm/interrupt.c        | 401 ++++++++++++++++++++++++++-----
 arch/s390/kvm/kvm-s390.c         |  56 ++++-
 arch/s390/kvm/kvm-s390.h         |   3 +-
 4 files changed, 402 insertions(+), 69 deletions(-)

-- 
2.52.0

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

[Douglas Freimuth]

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>
---
 arch/s390/include/asm/kvm_host.h |   5 +
 arch/s390/kvm/interrupt.c        | 159 ++++++++++++++++++++++++++-----
 arch/s390/kvm/kvm-s390.c         |  30 ++++++
 3 files changed, 169 insertions(+), 25 deletions(-)

diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index 3039c88daa63..e84532eca75f 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;
+	raw_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 7cb8ce833b62..49c95c1eb9b1 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -2426,6 +2426,9 @@ static int register_io_adapter(struct kvm_device *dev,
 	if (!adapter)
 		return -ENOMEM;
 
+	INIT_LIST_HEAD(&adapter->maps);
+	raw_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;
@@ -2450,12 +2453,128 @@ int kvm_s390_mask_adapter(struct kvm *kvm, unsigned int id, bool masked)
 	return ret;
 }
 
+static struct page *get_map_page(struct kvm *kvm, u64 uaddr)
+{
+	struct mm_struct *mm = kvm->mm;
+	struct page *page = NULL;
+	int locked = 1;
+
+	if (mmget_not_zero(mm)) {
+		mmap_read_lock(mm);
+		get_user_pages_remote(mm, uaddr, 1, FOLL_WRITE,
+				      &page, &locked);
+		if (locked)
+			mmap_read_unlock(mm);
+		mmput(mm);
+	}
+
+	return page;
+}
+
+static int kvm_s390_adapter_map(struct kvm *kvm, unsigned int id, __u64 addr)
+{
+	struct s390_io_adapter *adapter = get_io_adapter(kvm, id);
+	struct s390_map_info *map;
+	unsigned long flags;
+	__u64 host_addr;
+	int ret, idx;
+
+	if (!adapter || !addr)
+		return -EINVAL;
+
+	map = kzalloc_obj(*map, GFP_KERNEL_ACCOUNT);
+	if (!map)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(&map->list);
+	idx = srcu_read_lock(&kvm->srcu);
+	host_addr = gpa_to_hva(kvm, addr);
+	if (kvm_is_error_hva(host_addr)) {
+		srcu_read_unlock(&kvm->srcu, idx);
+		kfree(map);
+		ret = -EFAULT;
+		goto out;
+	}
+	srcu_read_unlock(&kvm->srcu, idx);
+	map->guest_addr = addr;
+	map->addr = host_addr;
+	map->page = get_map_page(kvm, host_addr);
+	if (!map->page) {
+		ret = -EINVAL;
+		goto out;
+	}
+	raw_spin_lock_irqsave(&adapter->maps_lock, flags);
+	if (adapter->nr_maps < MAX_S390_ADAPTER_MAPS) {
+		list_add_tail(&map->list, &adapter->maps);
+		adapter->nr_maps++;
+		ret = 0;
+	} else {
+		put_page(map->page);
+		ret = -EINVAL;
+	}
+	raw_spin_unlock_irqrestore(&adapter->maps_lock, flags);
+out:
+	if (ret)
+		kfree(map);
+	return ret;
+}
+
+static int kvm_s390_adapter_unmap(struct kvm *kvm, unsigned int id, __u64 addr)
+{
+	struct s390_io_adapter *adapter = get_io_adapter(kvm, id);
+	struct s390_map_info *map, *tmp;
+	struct page *map_page_to_put = NULL;
+	u64 map_addr_to_mark = 0;
+	unsigned long flags;
+	int found = 0, idx;
+
+	if (!adapter || !addr)
+		return -EINVAL;
+
+	raw_spin_lock_irqsave(&adapter->maps_lock, flags);
+	list_for_each_entry_safe(map, tmp, &adapter->maps, list) {
+		if (map->guest_addr == addr) {
+			found = 1;
+			adapter->nr_maps--;
+			list_del(&map->list);
+			map_page_to_put = map->page;
+			map_addr_to_mark = map->guest_addr;
+			kfree(map);
+			break;
+		}
+	}
+	raw_spin_unlock_irqrestore(&adapter->maps_lock, flags);
+
+	if (found) {
+		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);
+		put_page(map_page_to_put);
+	}
+
+	return found ? 0 : -ENOENT;
+}
+
 void kvm_s390_destroy_adapters(struct kvm *kvm)
 {
 	int i;
+	struct s390_map_info *map, *tmp;
+	unsigned long flags;
 
-	for (i = 0; i < MAX_S390_IO_ADAPTERS; i++)
+	for (i = 0; i < MAX_S390_IO_ADAPTERS; i++) {
+		if (!kvm->arch.adapters[i])
+			continue;
+		raw_spin_lock_irqsave(&kvm->arch.adapters[i]->maps_lock, flags);
+		list_for_each_entry_safe(map, tmp,
+					 &kvm->arch.adapters[i]->maps, list) {
+			list_del(&map->list);
+			put_page(map->page);
+			kfree(map);
+		}
+		raw_spin_unlock_irqrestore(&kvm->arch.adapters[i]->maps_lock, flags);
 		kfree(kvm->arch.adapters[i]);
+	}
 }
 
 static int modify_io_adapter(struct kvm_device *dev,
@@ -2477,14 +2596,22 @@ static int modify_io_adapter(struct kvm_device *dev,
 		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;
+		}
+		if (req.type == KVM_S390_IO_ADAPTER_MAP) {
+			dev->kvm->stat.io_390_adapter_map++;
+			ret = kvm_s390_adapter_map(dev->kvm, req.id, req.addr);
+		} else {
+			dev->kvm->stat.io_390_adapter_unmap++;
+			ret = kvm_s390_adapter_unmap(dev->kvm, req.id, req.addr);
+		}
+		mutex_unlock(&dev->kvm->lock);
 		break;
 	default:
 		ret = -EINVAL;
@@ -2730,24 +2857,6 @@ static unsigned long get_ind_bit(__u64 addr, unsigned long bit_nr, bool swap)
 	return swap ? (bit ^ (BITS_PER_LONG - 1)) : bit;
 }
 
-static struct page *get_map_page(struct kvm *kvm, u64 uaddr)
-{
-	struct mm_struct *mm = kvm->mm;
-	struct page *page = NULL;
-	int locked = 1;
-
-	if (mmget_not_zero(mm)) {
-		mmap_read_lock(mm);
-		get_user_pages_remote(mm, uaddr, 1, FOLL_WRITE,
-				      &page, &locked);
-		if (locked)
-			mmap_read_unlock(mm);
-		mmput(mm);
-	}
-
-	return page;
-}
-
 static int adapter_indicators_set(struct kvm *kvm,
 				  struct s390_io_adapter *adapter,
 				  struct kvm_s390_adapter_int *adapter_int)
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index d7838334a338..d1e1bed42c79 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -68,6 +68,8 @@
 const struct kvm_stats_desc kvm_vm_stats_desc[] = {
 	KVM_GENERIC_VM_STATS(),
 	STATS_DESC_COUNTER(VM, inject_io),
+	STATS_DESC_COUNTER(VM, io_390_adapter_map),
+	STATS_DESC_COUNTER(VM, io_390_adapter_unmap),
 	STATS_DESC_COUNTER(VM, inject_float_mchk),
 	STATS_DESC_COUNTER(VM, inject_pfault_done),
 	STATS_DESC_COUNTER(VM, inject_service_signal),
@@ -2491,6 +2493,33 @@ static int kvm_s390_pv_dmp(struct kvm *kvm, struct kvm_pv_cmd *cmd,
 	return r;
 }
 
+static void kvm_s390_unmap_all_adapters_pv(struct kvm *kvm)
+{
+	struct s390_map_info *map, *tmp;
+	LIST_HEAD(local_list);
+	unsigned long flags;
+	int i, idx;
+
+	for (i = 0; i < MAX_S390_IO_ADAPTERS; i++) {
+		if (!kvm->arch.adapters[i])
+			continue;
+		raw_spin_lock_irqsave(&kvm->arch.adapters[i]->maps_lock, flags);
+		list_splice_init(&kvm->arch.adapters[i]->maps, &local_list);
+		kvm->arch.adapters[i]->nr_maps = 0;
+		raw_spin_unlock_irqrestore(&kvm->arch.adapters[i]->maps_lock, flags);
+
+		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);
+			put_page(map->page);
+			kfree(map);
+		}
+	}
+}
+
 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);
@@ -2507,6 +2536,7 @@ static int kvm_s390_handle_pv(struct kvm *kvm, struct kvm_pv_cmd *cmd)
 		if (kvm_s390_pv_is_protected(kvm))
 			break;
 
+		kvm_s390_unmap_all_adapters_pv(kvm);
 		mmap_write_lock(kvm->mm);
 		/*
 		 * Disable creation of new THPs. Existing THPs can stay, they
-- 
2.52.0

[PATCH v4 2/3] KVM: s390: Enable adapter_indicators_set to use mapped pages

[Douglas Freimuth]

The S390 adapter_indicators_set function needs to be able to use mapped
pages so that worked can be processed,on a fast path when interrupts are
disabled. If adapter indicator pages are not mapped then local mapping is
done on a slow path as it is prior to this patch. For example, Secure
Execution environments will take the local mapping path as it does prior to
this patch.

Signed-off-by: Douglas Freimuth <freimuth@linux.ibm.com>
---
 arch/s390/kvm/interrupt.c | 94 ++++++++++++++++++++++++++++-----------
 1 file changed, 69 insertions(+), 25 deletions(-)

diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index 49c95c1eb9b1..c75fb3f19bb0 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -2857,41 +2857,85 @@ static unsigned long get_ind_bit(__u64 addr, unsigned long bit_nr, bool swap)
 	return swap ? (bit ^ (BITS_PER_LONG - 1)) : bit;
 }
 
+static struct s390_map_info *get_map_info(struct s390_io_adapter *adapter,
+					  u64 addr)
+{
+	struct s390_map_info *map;
+
+	if (!adapter)
+		return NULL;
+
+	list_for_each_entry(map, &adapter->maps, list) {
+		if (map->addr == addr)
+			return map;
+	}
+	return NULL;
+}
+
 static int adapter_indicators_set(struct kvm *kvm,
 				  struct s390_io_adapter *adapter,
 				  struct kvm_s390_adapter_int *adapter_int)
 {
 	unsigned long bit;
 	int summary_set, idx;
-	struct page *ind_page, *summary_page;
+	struct s390_map_info *ind_info, *summary_info;
 	void *map;
+	struct page *ind_page, *summary_page;
+	unsigned long flags;
 
-	ind_page = get_map_page(kvm, adapter_int->ind_addr);
-	if (!ind_page)
-		return -1;
-	summary_page = get_map_page(kvm, adapter_int->summary_addr);
-	if (!summary_page) {
-		put_page(ind_page);
-		return -1;
+	raw_spin_lock_irqsave(&adapter->maps_lock, flags);
+	ind_info = get_map_info(adapter, adapter_int->ind_addr);
+	if (!ind_info) {
+		raw_spin_unlock_irqrestore(&adapter->maps_lock, flags);
+		ind_page = get_map_page(kvm, adapter_int->ind_addr);
+		if (!ind_page)
+			return -1;
+		idx = srcu_read_lock(&kvm->srcu);
+		map = page_address(ind_page);
+		bit = get_ind_bit(adapter_int->ind_addr,
+				  adapter_int->ind_offset, adapter->swap);
+		set_bit(bit, map);
+		mark_page_dirty(kvm, adapter_int->ind_gaddr >> PAGE_SHIFT);
+		set_page_dirty_lock(ind_page);
+		srcu_read_unlock(&kvm->srcu, idx);
+	} else {
+		map = page_address(ind_info->page);
+		bit = get_ind_bit(ind_info->addr, adapter_int->ind_offset, adapter->swap);
+		set_bit(bit, map);
+		raw_spin_unlock_irqrestore(&adapter->maps_lock, flags);
+	}
+	raw_spin_lock_irqsave(&adapter->maps_lock, flags);
+	summary_info = get_map_info(adapter, adapter_int->summary_addr);
+	if (!summary_info) {
+		raw_spin_unlock_irqrestore(&adapter->maps_lock, flags);
+		summary_page = get_map_page(kvm, adapter_int->summary_addr);
+		if (!summary_page) {
+			if (!ind_info) {
+				WARN_ON_ONCE(!ind_page);
+				put_page(ind_page);
+			}
+			return -1;
+		}
+		idx = srcu_read_lock(&kvm->srcu);
+		map = page_address(summary_page);
+		bit = get_ind_bit(adapter_int->summary_addr,
+				  adapter_int->summary_offset, adapter->swap);
+		summary_set = test_and_set_bit(bit, map);
+		mark_page_dirty(kvm, adapter_int->summary_gaddr >> PAGE_SHIFT);
+		set_page_dirty_lock(summary_page);
+		srcu_read_unlock(&kvm->srcu, idx);
+	} else {
+		map = page_address(summary_info->page);
+		bit = get_ind_bit(summary_info->addr, adapter_int->summary_offset,
+				  adapter->swap);
+		summary_set = test_and_set_bit(bit, map);
+		raw_spin_unlock_irqrestore(&adapter->maps_lock, flags);
 	}
 
-	idx = srcu_read_lock(&kvm->srcu);
-	map = page_address(ind_page);
-	bit = get_ind_bit(adapter_int->ind_addr,
-			  adapter_int->ind_offset, adapter->swap);
-	set_bit(bit, map);
-	mark_page_dirty(kvm, adapter_int->ind_gaddr >> PAGE_SHIFT);
-	set_page_dirty_lock(ind_page);
-	map = page_address(summary_page);
-	bit = get_ind_bit(adapter_int->summary_addr,
-			  adapter_int->summary_offset, adapter->swap);
-	summary_set = test_and_set_bit(bit, map);
-	mark_page_dirty(kvm, adapter_int->summary_gaddr >> PAGE_SHIFT);
-	set_page_dirty_lock(summary_page);
-	srcu_read_unlock(&kvm->srcu, idx);
-
-	put_page(ind_page);
-	put_page(summary_page);
+	if (!ind_info)
+		put_page(ind_page);
+	if (!summary_info)
+		put_page(summary_page);
 	return summary_set ? 0 : 1;
 }
 
-- 
2.52.0

[PATCH v4 3/3] KVM: s390: Introducing kvm_arch_set_irq_inatomic fast inject

[Douglas Freimuth]

S390 needs a fast path for irq injection, and along those lines we
introduce kvm_arch_set_irq_inatomic. Instead of placing all interrupts on
the global work queue as it does today, this patch provides a fast path for
irq injection.

The inatomic fast path cannot lose control since it is running with
interrupts disabled. This meant making the following changes that exist on
the slow path today. First, the adapter_indicators page needs to be mapped
since it is accessed with interrupts disabled, so we added map/unmap
functions. Second, access to shared resources between the fast and slow
paths needed to be changed from mutex and semaphores to raw_spin_lock's.
Finally, the memory allocation on the slow path utilizes GFP_KERNEL_ACCOUNT
but we had to implement the fast path with GFP_ATOMIC allocation. Each of
these enhancements were required to prevent blocking on the fast inject
path.

Fencing of Fast Inject in Secure Execution environments is enabled in the
patch series by not mapping adapter indicator pages. In Secure Execution
environments the path of execution available before this patch is followed.

Statistical counters have been added to enable analysis of irq injection on
the fast path and slow path including io_390_inatomic, io_flic_inject_airq,
io_set_adapter_int and io_390_inatomic_adapter_masked.

Signed-off-by: Douglas Freimuth <freimuth@linux.ibm.com>
---
 arch/s390/include/asm/kvm_host.h |   6 +-
 arch/s390/kvm/interrupt.c        | 160 +++++++++++++++++++++++++++----
 arch/s390/kvm/kvm-s390.c         |  26 +++--
 arch/s390/kvm/kvm-s390.h         |   3 +-
 4 files changed, 170 insertions(+), 25 deletions(-)

diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index e84532eca75f..69f456e264b0 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -359,7 +359,7 @@ struct kvm_s390_float_interrupt {
 	struct kvm_s390_mchk_info mchk;
 	struct kvm_s390_ext_info srv_signal;
 	int last_sleep_cpu;
-	struct mutex ais_lock;
+	raw_spinlock_t ais_lock;
 	u8 simm;
 	u8 nimm;
 };
@@ -450,6 +450,10 @@ struct kvm_vm_stat {
 	u64 inject_io;
 	u64 io_390_adapter_map;
 	u64 io_390_adapter_unmap;
+	u64 io_390_inatomic;
+	u64 io_flic_inject_airq;
+	u64 io_set_adapter_int;
+	u64 io_390_inatomic_adapter_masked;
 	u64 inject_float_mchk;
 	u64 inject_pfault_done;
 	u64 inject_service_signal;
diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index c75fb3f19bb0..f714a3bb7f93 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -1963,15 +1963,10 @@ static int __inject_vm(struct kvm *kvm, struct kvm_s390_interrupt_info *inti)
 }
 
 int kvm_s390_inject_vm(struct kvm *kvm,
-		       struct kvm_s390_interrupt *s390int)
+		       struct kvm_s390_interrupt *s390int, struct kvm_s390_interrupt_info *inti)
 {
-	struct kvm_s390_interrupt_info *inti;
 	int rc;
 
-	inti = kzalloc_obj(*inti, GFP_KERNEL_ACCOUNT);
-	if (!inti)
-		return -ENOMEM;
-
 	inti->type = s390int->type;
 	switch (inti->type) {
 	case KVM_S390_INT_VIRTIO:
@@ -2007,6 +2002,7 @@ int kvm_s390_inject_vm(struct kvm *kvm,
 				 2);
 
 	rc = __inject_vm(kvm, inti);
+	/* memory allocation is done by the caller and inti is passed in, we free it here */
 	if (rc)
 		kfree(inti);
 	return rc;
@@ -2284,6 +2280,7 @@ static int flic_ais_mode_get_all(struct kvm *kvm, struct kvm_device_attr *attr)
 {
 	struct kvm_s390_float_interrupt *fi = &kvm->arch.float_int;
 	struct kvm_s390_ais_all ais;
+	unsigned long flags;
 
 	if (attr->attr < sizeof(ais))
 		return -EINVAL;
@@ -2291,10 +2288,10 @@ static int flic_ais_mode_get_all(struct kvm *kvm, struct kvm_device_attr *attr)
 	if (!test_kvm_facility(kvm, 72))
 		return -EOPNOTSUPP;
 
-	mutex_lock(&fi->ais_lock);
+	raw_spin_lock_irqsave(&fi->ais_lock, flags);
 	ais.simm = fi->simm;
 	ais.nimm = fi->nimm;
-	mutex_unlock(&fi->ais_lock);
+	raw_spin_unlock_irqrestore(&fi->ais_lock, flags);
 
 	if (copy_to_user((void __user *)attr->addr, &ais, sizeof(ais)))
 		return -EFAULT;
@@ -2648,6 +2645,7 @@ static int modify_ais_mode(struct kvm *kvm, struct kvm_device_attr *attr)
 	struct kvm_s390_float_interrupt *fi = &kvm->arch.float_int;
 	struct kvm_s390_ais_req req;
 	int ret = 0;
+	unsigned long flags;
 
 	if (!test_kvm_facility(kvm, 72))
 		return -EOPNOTSUPP;
@@ -2664,7 +2662,7 @@ static int modify_ais_mode(struct kvm *kvm, struct kvm_device_attr *attr)
 				       2 : KVM_S390_AIS_MODE_SINGLE :
 				       KVM_S390_AIS_MODE_ALL, req.mode);
 
-	mutex_lock(&fi->ais_lock);
+	raw_spin_lock_irqsave(&fi->ais_lock, flags);
 	switch (req.mode) {
 	case KVM_S390_AIS_MODE_ALL:
 		fi->simm &= ~AIS_MODE_MASK(req.isc);
@@ -2677,7 +2675,7 @@ static int modify_ais_mode(struct kvm *kvm, struct kvm_device_attr *attr)
 	default:
 		ret = -EINVAL;
 	}
-	mutex_unlock(&fi->ais_lock);
+	raw_spin_unlock_irqrestore(&fi->ais_lock, flags);
 
 	return ret;
 }
@@ -2691,25 +2689,33 @@ static int kvm_s390_inject_airq(struct kvm *kvm,
 		.parm = 0,
 		.parm64 = isc_to_int_word(adapter->isc),
 	};
+	struct kvm_s390_interrupt_info *inti;
+	unsigned long flags;
+
 	int ret = 0;
 
+	inti = kzalloc_obj(*inti, GFP_KERNEL_ACCOUNT);
+	if (!inti)
+		return -ENOMEM;
+
 	if (!test_kvm_facility(kvm, 72) || !adapter->suppressible)
-		return kvm_s390_inject_vm(kvm, &s390int);
+		return kvm_s390_inject_vm(kvm, &s390int, inti);
 
-	mutex_lock(&fi->ais_lock);
+	raw_spin_lock_irqsave(&fi->ais_lock, flags);
 	if (fi->nimm & AIS_MODE_MASK(adapter->isc)) {
 		trace_kvm_s390_airq_suppressed(adapter->id, adapter->isc);
+		kfree(inti);
 		goto out;
 	}
 
-	ret = kvm_s390_inject_vm(kvm, &s390int);
+	ret = kvm_s390_inject_vm(kvm, &s390int, inti);
 	if (!ret && (fi->simm & AIS_MODE_MASK(adapter->isc))) {
 		fi->nimm |= AIS_MODE_MASK(adapter->isc);
 		trace_kvm_s390_modify_ais_mode(adapter->isc,
 					       KVM_S390_AIS_MODE_SINGLE, 2);
 	}
 out:
-	mutex_unlock(&fi->ais_lock);
+	raw_spin_unlock_irqrestore(&fi->ais_lock, flags);
 	return ret;
 }
 
@@ -2718,6 +2724,8 @@ static int flic_inject_airq(struct kvm *kvm, struct kvm_device_attr *attr)
 	unsigned int id = attr->attr;
 	struct s390_io_adapter *adapter = get_io_adapter(kvm, id);
 
+	kvm->stat.io_flic_inject_airq++;
+
 	if (!adapter)
 		return -EINVAL;
 
@@ -2728,6 +2736,7 @@ static int flic_ais_mode_set_all(struct kvm *kvm, struct kvm_device_attr *attr)
 {
 	struct kvm_s390_float_interrupt *fi = &kvm->arch.float_int;
 	struct kvm_s390_ais_all ais;
+	unsigned long flags;
 
 	if (!test_kvm_facility(kvm, 72))
 		return -EOPNOTSUPP;
@@ -2735,10 +2744,10 @@ static int flic_ais_mode_set_all(struct kvm *kvm, struct kvm_device_attr *attr)
 	if (copy_from_user(&ais, (void __user *)attr->addr, sizeof(ais)))
 		return -EFAULT;
 
-	mutex_lock(&fi->ais_lock);
+	raw_spin_lock_irqsave(&fi->ais_lock, flags);
 	fi->simm = ais.simm;
 	fi->nimm = ais.nimm;
-	mutex_unlock(&fi->ais_lock);
+	raw_spin_unlock_irqrestore(&fi->ais_lock, flags);
 
 	return 0;
 }
@@ -2904,6 +2913,7 @@ static int adapter_indicators_set(struct kvm *kvm,
 		set_bit(bit, map);
 		raw_spin_unlock_irqrestore(&adapter->maps_lock, flags);
 	}
+
 	raw_spin_lock_irqsave(&adapter->maps_lock, flags);
 	summary_info = get_map_info(adapter, adapter_int->summary_addr);
 	if (!summary_info) {
@@ -2939,6 +2949,44 @@ static int adapter_indicators_set(struct kvm *kvm,
 	return summary_set ? 0 : 1;
 }
 
+static int adapter_indicators_set_fast(struct kvm *kvm,
+				       struct s390_io_adapter *adapter,
+				       struct kvm_s390_adapter_int *adapter_int,
+				       int setbit)
+{
+	unsigned long bit;
+	int summary_set;
+	struct s390_map_info *ind_info, *summary_info;
+	void *map;
+
+	raw_spin_lock(&adapter->maps_lock);
+	ind_info = get_map_info(adapter, adapter_int->ind_addr);
+	if (!ind_info) {
+		raw_spin_unlock(&adapter->maps_lock);
+		return -EWOULDBLOCK;
+	}
+	map = page_address(ind_info->page);
+	bit = get_ind_bit(ind_info->addr, adapter_int->ind_offset, adapter->swap);
+	if (setbit)
+		set_bit(bit, map);
+	else
+		clear_bit(bit, map);
+	summary_info = get_map_info(adapter, adapter_int->summary_addr);
+	if (!summary_info) {
+		raw_spin_unlock(&adapter->maps_lock);
+		return -EWOULDBLOCK;
+	}
+	map = page_address(summary_info->page);
+	bit = get_ind_bit(summary_info->addr, adapter_int->summary_offset,
+			  adapter->swap);
+	if (setbit)
+		summary_set = test_and_set_bit(bit, map);
+	else
+		summary_set = test_and_clear_bit(bit, map);
+	raw_spin_unlock(&adapter->maps_lock);
+	return summary_set ? 0 : 1;
+}
+
 /*
  * < 0 - not injected due to error
  * = 0 - coalesced, summary indicator already active
@@ -2951,6 +2999,8 @@ static int set_adapter_int(struct kvm_kernel_irq_routing_entry *e,
 	int ret;
 	struct s390_io_adapter *adapter;
 
+	kvm->stat.io_set_adapter_int++;
+
 	/* We're only interested in the 0->1 transition. */
 	if (!level)
 		return 0;
@@ -3019,7 +3069,6 @@ int kvm_set_routing_entry(struct kvm *kvm,
 	int idx;
 
 	switch (ue->type) {
-	/* we store the userspace addresses instead of the guest addresses */
 	case KVM_IRQ_ROUTING_S390_ADAPTER:
 		if (kvm_is_ucontrol(kvm))
 			return -EINVAL;
@@ -3610,3 +3659,80 @@ int __init kvm_s390_gib_init(u8 nisc)
 out:
 	return rc;
 }
+
+/*
+ * kvm_arch_set_irq_inatomic: fast-path for irqfd injection
+ */
+int kvm_arch_set_irq_inatomic(struct kvm_kernel_irq_routing_entry *e,
+			      struct kvm *kvm, int irq_source_id, int level,
+			      bool line_status)
+{
+	int ret, setbit;
+	struct s390_io_adapter *adapter;
+	struct kvm_s390_float_interrupt *fi = &kvm->arch.float_int;
+	struct kvm_s390_interrupt_info *inti;
+	struct kvm_s390_interrupt s390int = {
+			.type = KVM_S390_INT_IO(1, 0, 0, 0),
+			.parm = 0,
+	};
+
+	kvm->stat.io_390_inatomic++;
+
+	/* We're only interested in the 0->1 transition. */
+	if (!level)
+		return -EWOULDBLOCK;
+	if (e->type != KVM_IRQ_ROUTING_S390_ADAPTER)
+		return -EWOULDBLOCK;
+
+	adapter = get_io_adapter(kvm, e->adapter.adapter_id);
+	if (!adapter)
+		return -EWOULDBLOCK;
+
+	s390int.parm64 = isc_to_int_word(adapter->isc);
+	setbit = 1;
+	ret = adapter_indicators_set_fast(kvm, adapter, &e->adapter, setbit);
+	if (ret < 0)
+		return -EWOULDBLOCK;
+	if (!ret || adapter->masked) {
+		kvm->stat.io_390_inatomic_adapter_masked++;
+		return 0;
+	}
+
+	inti = kzalloc_obj(*inti, GFP_ATOMIC);
+	if (!inti)
+		return -EWOULDBLOCK;
+
+	if (!test_kvm_facility(kvm, 72) || !adapter->suppressible) {
+		ret = kvm_s390_inject_vm(kvm, &s390int, inti);
+		if (ret == 0) {
+			return ret;
+		} else {
+			setbit = 0;
+			adapter_indicators_set_fast(kvm, adapter, &e->adapter, setbit);
+			return -EWOULDBLOCK;
+		}
+	}
+
+	raw_spin_lock(&fi->ais_lock);
+	if (fi->nimm & AIS_MODE_MASK(adapter->isc)) {
+		trace_kvm_s390_airq_suppressed(adapter->id, adapter->isc);
+		kfree(inti);
+		goto out;
+	}
+
+	ret = kvm_s390_inject_vm(kvm, &s390int, inti);
+	if (!ret && (fi->simm & AIS_MODE_MASK(adapter->isc))) {
+		fi->nimm |= AIS_MODE_MASK(adapter->isc);
+		trace_kvm_s390_modify_ais_mode(adapter->isc,
+					       KVM_S390_AIS_MODE_SINGLE, 2);
+	} else if (ret) {
+		raw_spin_unlock(&fi->ais_lock);
+		setbit = 0;
+		adapter_indicators_set_fast(kvm, adapter, &e->adapter, setbit);
+		return -EWOULDBLOCK;
+	}
+
+out:
+	raw_spin_unlock(&fi->ais_lock);
+	return 0;
+}
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index d1e1bed42c79..0b3e44f6fa79 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -70,6 +70,10 @@ const struct kvm_stats_desc kvm_vm_stats_desc[] = {
 	STATS_DESC_COUNTER(VM, inject_io),
 	STATS_DESC_COUNTER(VM, io_390_adapter_map),
 	STATS_DESC_COUNTER(VM, io_390_adapter_unmap),
+	STATS_DESC_COUNTER(VM, io_390_inatomic),
+	STATS_DESC_COUNTER(VM, io_flic_inject_airq),
+	STATS_DESC_COUNTER(VM, io_set_adapter_int),
+	STATS_DESC_COUNTER(VM, io_390_inatomic_adapter_masked),
 	STATS_DESC_COUNTER(VM, inject_float_mchk),
 	STATS_DESC_COUNTER(VM, inject_pfault_done),
 	STATS_DESC_COUNTER(VM, inject_service_signal),
@@ -2872,6 +2876,7 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
 	void __user *argp = (void __user *)arg;
 	struct kvm_device_attr attr;
 	int r;
+	struct kvm_s390_interrupt_info *inti;
 
 	switch (ioctl) {
 	case KVM_S390_INTERRUPT: {
@@ -2880,7 +2885,10 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
 		r = -EFAULT;
 		if (copy_from_user(&s390int, argp, sizeof(s390int)))
 			break;
-		r = kvm_s390_inject_vm(kvm, &s390int);
+		inti = kzalloc_obj(*inti, GFP_KERNEL_ACCOUNT);
+		if (!inti)
+			return -ENOMEM;
+		r = kvm_s390_inject_vm(kvm, &s390int, inti);
 		break;
 	}
 	case KVM_CREATE_IRQCHIP: {
@@ -3278,7 +3286,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
 		mutex_unlock(&kvm->lock);
 	}
 
-	mutex_init(&kvm->arch.float_int.ais_lock);
+	raw_spin_lock_init(&kvm->arch.float_int.ais_lock);
 	spin_lock_init(&kvm->arch.float_int.lock);
 	for (i = 0; i < FIRQ_LIST_COUNT; i++)
 		INIT_LIST_HEAD(&kvm->arch.float_int.lists[i]);
@@ -4399,28 +4407,34 @@ int kvm_s390_try_set_tod_clock(struct kvm *kvm, const struct kvm_s390_vm_tod_clo
 	return 1;
 }
 
-static void __kvm_inject_pfault_token(struct kvm_vcpu *vcpu, bool start_token,
-				      unsigned long token)
+static int __kvm_inject_pfault_token(struct kvm_vcpu *vcpu, bool start_token,
+				     unsigned long token)
 {
 	struct kvm_s390_interrupt inti;
 	struct kvm_s390_irq irq;
+	struct kvm_s390_interrupt_info *inti_mem = NULL;
 
 	if (start_token) {
 		irq.u.ext.ext_params2 = token;
 		irq.type = KVM_S390_INT_PFAULT_INIT;
 		WARN_ON_ONCE(kvm_s390_inject_vcpu(vcpu, &irq));
 	} else {
+		inti_mem = kzalloc_obj(*inti_mem, GFP_KERNEL_ACCOUNT);
+		if (!inti_mem)
+			return -ENOMEM;
+
 		inti.type = KVM_S390_INT_PFAULT_DONE;
 		inti.parm64 = token;
-		WARN_ON_ONCE(kvm_s390_inject_vm(vcpu->kvm, &inti));
+		WARN_ON_ONCE(kvm_s390_inject_vm(vcpu->kvm, &inti, inti_mem));
 	}
+	return 0;
 }
 
 bool kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu,
 				     struct kvm_async_pf *work)
 {
 	trace_kvm_s390_pfault_init(vcpu, work->arch.pfault_token);
-	__kvm_inject_pfault_token(vcpu, true, work->arch.pfault_token);
+	WARN_ON_ONCE(__kvm_inject_pfault_token(vcpu, true, work->arch.pfault_token));
 
 	return true;
 }
diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
index bf1d7798c1af..2f2da868a040 100644
--- a/arch/s390/kvm/kvm-s390.h
+++ b/arch/s390/kvm/kvm-s390.h
@@ -373,7 +373,8 @@ int __must_check kvm_s390_deliver_pending_interrupts(struct kvm_vcpu *vcpu);
 void kvm_s390_clear_local_irqs(struct kvm_vcpu *vcpu);
 void kvm_s390_clear_float_irqs(struct kvm *kvm);
 int __must_check kvm_s390_inject_vm(struct kvm *kvm,
-				    struct kvm_s390_interrupt *s390int);
+				    struct kvm_s390_interrupt *s390int,
+				    struct kvm_s390_interrupt_info *inti);
 int __must_check kvm_s390_inject_vcpu(struct kvm_vcpu *vcpu,
 				      struct kvm_s390_irq *irq);
 static inline int kvm_s390_inject_prog_irq(struct kvm_vcpu *vcpu,
-- 
2.52.0

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

[Matthew Rosato]

> +static struct page *get_map_page(struct kvm *kvm, u64 uaddr)
> +{
> +	struct mm_struct *mm = kvm->mm;
> +	struct page *page = NULL;
> +	int locked = 1;
> +
> +	if (mmget_not_zero(mm)) {
> +		mmap_read_lock(mm);
> +		get_user_pages_remote(mm, uaddr, 1, FOLL_WRITE,
> +				      &page, &locked);

I have wondered this before, and Sashiko mentions it now:  Would it make
sense to also FOLL_LONGTERM here?

I recognize that the old ioctl code that you are resurrecting here did
not use FOLL_LONGTERM, but I can't think of a reason why.

The mapping may indeed be held long-term (life of the guest or at least
the associated adapter in the guest), and it's effectively under
userspace control, waiting for a corresponding unmap ioctl or for the
guest to go away or enter pv mode.

Can you please test?

> +		if (locked)
> +			mmap_read_unlock(mm);
> +		mmput(mm);
> +	}
> +
> +	return page;
> +}
> +
> +static int kvm_s390_adapter_map(struct kvm *kvm, unsigned int id, __u64 addr)
> +{
> +	struct s390_io_adapter *adapter = get_io_adapter(kvm, id);
> +	struct s390_map_info *map;
> +	unsigned long flags;
> +	__u64 host_addr;
> +	int ret, idx;
> +
> +	if (!adapter || !addr)
> +		return -EINVAL;
> +
> +	map = kzalloc_obj(*map, GFP_KERNEL_ACCOUNT);
> +	if (!map)
> +		return -ENOMEM;
> +
> +	INIT_LIST_HEAD(&map->list);
> +	idx = srcu_read_lock(&kvm->srcu);
> +	host_addr = gpa_to_hva(kvm, addr);
> +	if (kvm_is_error_hva(host_addr)) {
> +		srcu_read_unlock(&kvm->srcu, idx);
> +		kfree(map);

Drop this kfree(), you already do this when you goto out

> +		ret = -EFAULT;
> +		goto out;
> +	}
> +	srcu_read_unlock(&kvm->srcu, idx);
> +	map->guest_addr = addr;
> +	map->addr = host_addr;
> +	map->page = get_map_page(kvm, host_addr);
> +	if (!map->page) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +	raw_spin_lock_irqsave(&adapter->maps_lock, flags);
> +	if (adapter->nr_maps < MAX_S390_ADAPTER_MAPS) {
> +		list_add_tail(&map->list, &adapter->maps);
> +		adapter->nr_maps++;
> +		ret = 0;
> +	} else {
> +		put_page(map->page);
> +		ret = -EINVAL;
> +	}
> +	raw_spin_unlock_irqrestore(&adapter->maps_lock, flags);

Sashiko is concerned about put_page() potentially sleeping under
PREEMPT_RT; drilling down to functions like free_one_page() indeed I see
regular spinlocks employed.

RT aside, it might be worth doing this anyway to reduce the critical
section you are holding this lock over, like so:

	raw_spin_lock_irqsave(&adapter->maps_lock, flags);
	if (adapter->nr_maps < MAX_S390_ADAPTER_MAPS) {
		list_add_tail(&map->list, &adapter->maps);
		adapter->nr_maps++;
		ret = 0;
	} else {
		ret = -EINVAL;
	}
	raw_spin_unlock_irqrestore(&adapter->maps_lock, flags);
	if (ret)
		put_page(map->page);


> +out:
> +	if (ret)
> +		kfree(map);
> +	return ret;
> +}
> +
> +static int kvm_s390_adapter_unmap(struct kvm *kvm, unsigned int id, __u64 addr)
> +{
> +	struct s390_io_adapter *adapter = get_io_adapter(kvm, id);
> +	struct s390_map_info *map, *tmp;
> +	struct page *map_page_to_put = NULL;
> +	u64 map_addr_to_mark = 0;
> +	unsigned long flags;
> +	int found = 0, idx;
> +
> +	if (!adapter || !addr)
> +		return -EINVAL;
> +
> +	raw_spin_lock_irqsave(&adapter->maps_lock, flags);
> +	list_for_each_entry_safe(map, tmp, &adapter->maps, list) {
> +		if (map->guest_addr == addr) {
> +			found = 1;
> +			adapter->nr_maps--;
> +			list_del(&map->list);
> +			map_page_to_put = map->page;
> +			map_addr_to_mark = map->guest_addr;
> +			kfree(map);

Move the kfree() outside of the raw spinlock and instead call it...

> +			break;
> +		}
> +	}
> +	raw_spin_unlock_irqrestore(&adapter->maps_lock, flags);
> +
> +	if (found) {

... right here.

> +		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);
> +		put_page(map_page_to_put);
> +	}
> +
> +	return found ? 0 : -ENOENT;
> +}
> +
>  void kvm_s390_destroy_adapters(struct kvm *kvm)
>  {
>  	int i;
> +	struct s390_map_info *map, *tmp;
> +	unsigned long flags;
>  
> -	for (i = 0; i < MAX_S390_IO_ADAPTERS; i++)
> +	for (i = 0; i < MAX_S390_IO_ADAPTERS; i++) {
> +		if (!kvm->arch.adapters[i])
> +			continue;
> +		raw_spin_lock_irqsave(&kvm->arch.adapters[i]->maps_lock, flags);
> +		list_for_each_entry_safe(map, tmp,
> +					 &kvm->arch.adapters[i]->maps, list) {
> +			list_del(&map->list);
> +			put_page(map->page);
> +			kfree(map);
> +		}
> +		raw_spin_unlock_irqrestore(&kvm->arch.adapters[i]->maps_lock, flags);

Moving put_page/kfree out of the spinlock is a bit more work here.
Handle this the same way you did in kvm_s390_unmap_all_adapters_pv()?

Actually wait -- besides the dirty page logic (which should be fine to
do here too) this is the same code as kvm_s390_unmap_all_adapters_pv().
Can you make the code in kvm_s390_unmap_all_adapters_pv() a single
routine with a different name (e.g. kvm_s390_unmap_all_adapters()?) that
is called both from here as well as from kvm_s390_handle_pv()?

Re: [PATCH v4 3/3] KVM: s390: Introducing kvm_arch_set_irq_inatomic fast inject

[Matthew Rosato]

> +static int adapter_indicators_set_fast(struct kvm *kvm,
> +				       struct s390_io_adapter *adapter,
> +				       struct kvm_s390_adapter_int *adapter_int,
> +				       int setbit)
> +{
> +	unsigned long bit;
> +	int summary_set;
> +	struct s390_map_info *ind_info, *summary_info;
> +	void *map;
> +
> +	raw_spin_lock(&adapter->maps_lock);
> +	ind_info = get_map_info(adapter, adapter_int->ind_addr);
> +	if (!ind_info) {
> +		raw_spin_unlock(&adapter->maps_lock);
> +		return -EWOULDBLOCK;
> +	}
> +	map = page_address(ind_info->page);
> +	bit = get_ind_bit(ind_info->addr, adapter_int->ind_offset, adapter->swap);
> +	if (setbit)
> +		set_bit(bit, map);
> +	else
> +		clear_bit(bit, map);

Hmm, I don't know about this.  In my comment on v2 I was only concerned
about undoing the setting of the summary indicator as that will be used
on the slow path to decide whether or not we need to inject an interrupt
in addition to setting the indicator bits.

I think we should drop the else clear_bit() here.  If _fast already set
it and we are now backing out to the slow path, then it will stay on all
the way through the slow path and that should be OK.

> +	summary_info = get_map_info(adapter, adapter_int->summary_addr);
> +	if (!summary_info) {
> +		raw_spin_unlock(&adapter->maps_lock);
> +		return -EWOULDBLOCK;
> +	}
> +	map = page_address(summary_info->page);
> +	bit = get_ind_bit(summary_info->addr, adapter_int->summary_offset,
> +			  adapter->swap);
> +	if (setbit)
> +		summary_set = test_and_set_bit(bit, map);
> +	else
> +		summary_set = test_and_clear_bit(bit, map);

I had to go back and refresh myself about WHY we needed to 'undo' our
prior setting of the summary bit specifically.

The reason is that, if we need to fall back to the slow path, that code
will see the summary bit already on and therefore not inject an
interrupt believing that the thread that initially set the summary bit
did that.  But, if we fell back from the _fast path via -EWOULDBLOCK
after setting the summary indicator, no interrupt was ever injected at
that time.

So my point: this _really_ deserves some comment blocks describing the
purpose of setbit + a specific statement that it's OK to clear this
summary bit now when setbit=0 but then the caller must re-drive this
summary indication again via adapter_indicators_set().

[...]

> +int kvm_arch_set_irq_inatomic(struct kvm_kernel_irq_routing_entry *e,
> +			      struct kvm *kvm, int irq_source_id, int level,
> +			      bool line_status)
> +{
> +	int ret, setbit;
> +	struct s390_io_adapter *adapter;
> +	struct kvm_s390_float_interrupt *fi = &kvm->arch.float_int;
> +	struct kvm_s390_interrupt_info *inti;
> +	struct kvm_s390_interrupt s390int = {
> +			.type = KVM_S390_INT_IO(1, 0, 0, 0),
> +			.parm = 0,
> +	};
> +
> +	kvm->stat.io_390_inatomic++;
> +
> +	/* We're only interested in the 0->1 transition. */
> +	if (!level)
> +		return -EWOULDBLOCK;
> +	if (e->type != KVM_IRQ_ROUTING_S390_ADAPTER)
> +		return -EWOULDBLOCK;
> +
> +	adapter = get_io_adapter(kvm, e->adapter.adapter_id);
> +	if (!adapter)
> +		return -EWOULDBLOCK;
> +
> +	s390int.parm64 = isc_to_int_word(adapter->isc);
> +	setbit = 1;
> +	ret = adapter_indicators_set_fast(kvm, adapter, &e->adapter, setbit);
> +	if (ret < 0)
> +		return -EWOULDBLOCK;
> +	if (!ret || adapter->masked) {
> +		kvm->stat.io_390_inatomic_adapter_masked++;
> +		return 0;
> +	}
> +
> +	inti = kzalloc_obj(*inti, GFP_ATOMIC);
> +	if (!inti)

You need to undo the summary bit indication on this path as well.

[...]

> -static void __kvm_inject_pfault_token(struct kvm_vcpu *vcpu, bool start_token,
> -				      unsigned long token)
> +static int __kvm_inject_pfault_token(struct kvm_vcpu *vcpu, bool start_token,
> +				     unsigned long token)
>  {
>  	struct kvm_s390_interrupt inti;
>  	struct kvm_s390_irq irq;
> +	struct kvm_s390_interrupt_info *inti_mem = NULL;
>  
>  	if (start_token) {
>  		irq.u.ext.ext_params2 = token;
>  		irq.type = KVM_S390_INT_PFAULT_INIT;
>  		WARN_ON_ONCE(kvm_s390_inject_vcpu(vcpu, &irq));
>  	} else {
> +		inti_mem = kzalloc_obj(*inti_mem, GFP_KERNEL_ACCOUNT);
> +		if (!inti_mem)
> +			return -ENOMEM;

To match the other nonzero cases here, rather than making
__kvm_inject_pfault_token() return a value can you leave it a void
return and then just do something like:

if (WARN_ON_ONCE(!inti_mem))
	return;

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

[Claudio Imbrenda]

On Wed, 29 Apr 2026 10:44:51 -0400
Matthew Rosato <mjrosato@linux.ibm.com> wrote:

[...]

> 
> > +		if (locked)
> > +			mmap_read_unlock(mm);
> > +		mmput(mm);
> > +	}
> > +
> > +	return page;
> > +}
> > +
> > +static int kvm_s390_adapter_map(struct kvm *kvm, unsigned int id, __u64 addr)
> > +{
> > +	struct s390_io_adapter *adapter = get_io_adapter(kvm, id);
> > +	struct s390_map_info *map;
> > +	unsigned long flags;
> > +	__u64 host_addr;
> > +	int ret, idx;
> > +
> > +	if (!adapter || !addr)
> > +		return -EINVAL;
> > +
> > +	map = kzalloc_obj(*map, GFP_KERNEL_ACCOUNT);
> > +	if (!map)
> > +		return -ENOMEM;
> > +
> > +	INIT_LIST_HEAD(&map->list);
> > +	idx = srcu_read_lock(&kvm->srcu);
> > +	host_addr = gpa_to_hva(kvm, addr);
> > +	if (kvm_is_error_hva(host_addr)) {
> > +		srcu_read_unlock(&kvm->srcu, idx);
> > +		kfree(map);  
> 
> Drop this kfree(), you already do this when you goto out

have you considered using __free(kvfree) and let the compiler free map
for you automatically? (or it doesn't work here?)

[...]

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

[Matthew Rosato]

On 4/29/26 10:44 AM, Matthew Rosato wrote:
> 
>> +static struct page *get_map_page(struct kvm *kvm, u64 uaddr)
>> +{
>> +	struct mm_struct *mm = kvm->mm;
>> +	struct page *page = NULL;
>> +	int locked = 1;
>> +
>> +	if (mmget_not_zero(mm)) {
>> +		mmap_read_lock(mm);
>> +		get_user_pages_remote(mm, uaddr, 1, FOLL_WRITE,
>> +				      &page, &locked);
> 
> I have wondered this before, and Sashiko mentions it now:  Would it make
> sense to also FOLL_LONGTERM here?
> 
> I recognize that the old ioctl code that you are resurrecting here did
> not use FOLL_LONGTERM, but I can't think of a reason why.
> 
> The mapping may indeed be held long-term (life of the guest or at least
> the associated adapter in the guest), and it's effectively under
> userspace control, waiting for a corresponding unmap ioctl or for the
> guest to go away or enter pv mode.
> 
> Can you please test?
> 

OK, I did some more looking into this.  FOLL_LONGTERM will require
FOLL_PIN, and you're using FOLL_GET here.  So that won't work as-is with
this code.

So let's step back and consider what these pages are intended to be used
for; we want to write to them prior to injecting an adapter interrupt
into a guest vm.  That seems to me to fall under case 5 in
Documentation/core-api/pin_user_pages.rst which says to use FOLL_PIN.

Whether we keep the page pinned for a long period of time or not depends
on whether or not it was mapped by userspace via the ioctl in this patch
-- but the intent is the same.

So I wonder if really we should be using pin_user_pages_remote here,
with (FOLL_WRITE | FOLL_LONGTERM) for the map/unmap ioctl case added by
this patch but also switch to pin_user_pages_remote() instead of
get_user_pages_remote() using only FOLL_WRITE for the existing 'slow'
path where we expect to pin, write some bits into the page and then
unpin the page right away

And of course unpin_user_page() instead of put_page() for both cases...

Christian / Janosch / Claudio, does this sound sane to you?

I did a quick test with the following diff on top of this series (ran
some I/O over virtio-ccw and virio-pci, saw io_390_inatomic numbers
going up) -- Doug can you give it a more thorough testing and if it
looks good / assuming no objections work it into your next version?


diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index ee3376b017cb..0960b6726f03 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -2453,7 +2453,8 @@ int kvm_s390_mask_adapter(struct kvm *kvm, unsigned int id, bool masked)
        return ret;
 }
 
-static struct page *get_map_page(struct kvm *kvm, u64 uaddr)
+static struct page *pin_map_page(struct kvm *kvm, u64 uaddr,
+                                unsigned int gup_flags)
 {
        struct mm_struct *mm = kvm->mm;
        struct page *page = NULL;
@@ -2461,7 +2462,7 @@ static struct page *get_map_page(struct kvm *kvm, u64 uaddr)
 
        if (mmget_not_zero(mm)) {
                mmap_read_lock(mm);
-               get_user_pages_remote(mm, uaddr, 1, FOLL_WRITE,
+               pin_user_pages_remote(mm, uaddr, 1, FOLL_WRITE | gup_flags,
                                      &page, &locked);
                if (locked)
                        mmap_read_unlock(mm);
@@ -2498,7 +2499,7 @@ static int kvm_s390_adapter_map(struct kvm *kvm, unsigned int id, __u64 addr)
        srcu_read_unlock(&kvm->srcu, idx);
        map->guest_addr = addr;
        map->addr = host_addr;
-       map->page = get_map_page(kvm, host_addr);
+       map->page = pin_map_page(kvm, host_addr, FOLL_LONGTERM);
        if (!map->page) {
                ret = -EINVAL;
                goto out;
@@ -2509,7 +2510,7 @@ static int kvm_s390_adapter_map(struct kvm *kvm, unsigned int id, __u64 addr)
                adapter->nr_maps++;
                ret = 0;
        } else {
-               put_page(map->page);
+               unpin_user_page(map->page);
                ret = -EINVAL;
        }
        raw_spin_unlock_irqrestore(&adapter->maps_lock, flags);
@@ -2550,7 +2551,7 @@ static int kvm_s390_adapter_unmap(struct kvm *kvm, unsigned int id, __u64 addr)
                mark_page_dirty(kvm, map_addr_to_mark >> PAGE_SHIFT);
                set_page_dirty_lock(map_page_to_put);
                srcu_read_unlock(&kvm->srcu, idx);
-               put_page(map_page_to_put);
+               unpin_user_page(map_page_to_put);
        }
 
        return found ? 0 : -ENOENT;
@@ -2569,7 +2570,7 @@ void kvm_s390_destroy_adapters(struct kvm *kvm)
                list_for_each_entry_safe(map, tmp,
                                         &kvm->arch.adapters[i]->maps, list) {
                        list_del(&map->list);
-                       put_page(map->page);
+                       unpin_user_page(map->page);
                        kfree(map);
                }
                raw_spin_unlock_irqrestore(&kvm->arch.adapters[i]->maps_lock, flags);
@@ -2899,7 +2900,7 @@ static int adapter_indicators_set(struct kvm *kvm,
        ind_info = get_map_info(adapter, adapter_int->ind_addr);
        if (!ind_info) {
                raw_spin_unlock_irqrestore(&adapter->maps_lock, flags);
-               ind_page = get_map_page(kvm, adapter_int->ind_addr);
+               ind_page = pin_map_page(kvm, adapter_int->ind_addr, 0);
                if (!ind_page)
                        return -1;
                idx = srcu_read_lock(&kvm->srcu);
@@ -2921,11 +2922,11 @@ static int adapter_indicators_set(struct kvm *kvm,
        summary_info = get_map_info(adapter, adapter_int->summary_addr);
        if (!summary_info) {
                raw_spin_unlock_irqrestore(&adapter->maps_lock, flags);
-               summary_page = get_map_page(kvm, adapter_int->summary_addr);
+               summary_page = pin_map_page(kvm, adapter_int->summary_addr, 0);
                if (!summary_page) {
                        if (!ind_info) {
                                WARN_ON_ONCE(!ind_page);
-                               put_page(ind_page);
+                               unpin_user_page(ind_page);
                        }
                        return -1;
                }
@@ -2946,9 +2947,9 @@ static int adapter_indicators_set(struct kvm *kvm,
        }
 
        if (!ind_info)
-               put_page(ind_page);
+               unpin_user_page(ind_page);
        if (!summary_info)
-               put_page(summary_page);
+               unpin_user_page(summary_page);
        return summary_set ? 0 : 1;
 }
 

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

[Douglas Freimuth]

On 4/29/26 10:44 AM, Matthew Rosato wrote:
> 
>> +static struct page *get_map_page(struct kvm *kvm, u64 uaddr)
>> +{
>> +	struct mm_struct *mm = kvm->mm;
>> +	struct page *page = NULL;
>> +	int locked = 1;
>> +
>> +	if (mmget_not_zero(mm)) {
>> +		mmap_read_lock(mm);
>> +		get_user_pages_remote(mm, uaddr, 1, FOLL_WRITE,
>> +				      &page, &locked);
> 
> I have wondered this before, and Sashiko mentions it now:  Would it make
> sense to also FOLL_LONGTERM here?
> 
> I recognize that the old ioctl code that you are resurrecting here did
> not use FOLL_LONGTERM, but I can't think of a reason why.
> 
> The mapping may indeed be held long-term (life of the guest or at least
> the associated adapter in the guest), and it's effectively under
> userspace control, waiting for a corresponding unmap ioctl or for the
> guest to go away or enter pv mode.
> 
> Can you please test?

I tested this with get_user_pages_remote() and
FOLL_WRITE | FOLL_LONGTERM. I get null pages back. Thus the exploration 
into whether pin_user_pages_remote() with those flags is desirous in 
this case.

> 
>> +		if (locked)
>> +			mmap_read_unlock(mm);
>> +		mmput(mm);
>> +	}
>> +
>> +	return page;
>> +}
>> +
>> +static int kvm_s390_adapter_map(struct kvm *kvm, unsigned int id, __u64 addr)
>> +{
>> +	struct s390_io_adapter *adapter = get_io_adapter(kvm, id);
>> +	struct s390_map_info *map;
>> +	unsigned long flags;
>> +	__u64 host_addr;
>> +	int ret, idx;
>> +
>> +	if (!adapter || !addr)
>> +		return -EINVAL;
>> +
>> +	map = kzalloc_obj(*map, GFP_KERNEL_ACCOUNT);
>> +	if (!map)
>> +		return -ENOMEM;
>> +
>> +	INIT_LIST_HEAD(&map->list);
>> +	idx = srcu_read_lock(&kvm->srcu);
>> +	host_addr = gpa_to_hva(kvm, addr);
>> +	if (kvm_is_error_hva(host_addr)) {
>> +		srcu_read_unlock(&kvm->srcu, idx);
>> +		kfree(map);
> 
> Drop this kfree(), you already do this when you goto out
> 
>> +		ret = -EFAULT;
>> +		goto out;
>> +	}
>> +	srcu_read_unlock(&kvm->srcu, idx);
>> +	map->guest_addr = addr;
>> +	map->addr = host_addr;
>> +	map->page = get_map_page(kvm, host_addr);
>> +	if (!map->page) {
>> +		ret = -EINVAL;
>> +		goto out;
>> +	}
>> +	raw_spin_lock_irqsave(&adapter->maps_lock, flags);
>> +	if (adapter->nr_maps < MAX_S390_ADAPTER_MAPS) {
>> +		list_add_tail(&map->list, &adapter->maps);
>> +		adapter->nr_maps++;
>> +		ret = 0;
>> +	} else {
>> +		put_page(map->page);
>> +		ret = -EINVAL;
>> +	}
>> +	raw_spin_unlock_irqrestore(&adapter->maps_lock, flags);
> 
> Sashiko is concerned about put_page() potentially sleeping under
> PREEMPT_RT; drilling down to functions like free_one_page() indeed I see
> regular spinlocks employed.
> 
> RT aside, it might be worth doing this anyway to reduce the critical
> section you are holding this lock over, like so:
> 
> 	raw_spin_lock_irqsave(&adapter->maps_lock, flags);
> 	if (adapter->nr_maps < MAX_S390_ADAPTER_MAPS) {
> 		list_add_tail(&map->list, &adapter->maps);
> 		adapter->nr_maps++;
> 		ret = 0;
> 	} else {
> 		ret = -EINVAL;
> 	}
> 	raw_spin_unlock_irqrestore(&adapter->maps_lock, flags);
> 	if (ret)
> 		put_page(map->page);
> 
> 
>> +out:
>> +	if (ret)
>> +		kfree(map);
>> +	return ret;
>> +}
>> +
>> +static int kvm_s390_adapter_unmap(struct kvm *kvm, unsigned int id, __u64 addr)
>> +{
>> +	struct s390_io_adapter *adapter = get_io_adapter(kvm, id);
>> +	struct s390_map_info *map, *tmp;
>> +	struct page *map_page_to_put = NULL;
>> +	u64 map_addr_to_mark = 0;
>> +	unsigned long flags;
>> +	int found = 0, idx;
>> +
>> +	if (!adapter || !addr)
>> +		return -EINVAL;
>> +
>> +	raw_spin_lock_irqsave(&adapter->maps_lock, flags);
>> +	list_for_each_entry_safe(map, tmp, &adapter->maps, list) {
>> +		if (map->guest_addr == addr) {
>> +			found = 1;
>> +			adapter->nr_maps--;
>> +			list_del(&map->list);
>> +			map_page_to_put = map->page;
>> +			map_addr_to_mark = map->guest_addr;
>> +			kfree(map);
> 
> Move the kfree() outside of the raw spinlock and instead call it...
> 
>> +			break;
>> +		}
>> +	}
>> +	raw_spin_unlock_irqrestore(&adapter->maps_lock, flags);
>> +
>> +	if (found) {
> 
> ... right here.
> 
>> +		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);
>> +		put_page(map_page_to_put);
>> +	}
>> +
>> +	return found ? 0 : -ENOENT;
>> +}
>> +
>>   void kvm_s390_destroy_adapters(struct kvm *kvm)
>>   {
>>   	int i;
>> +	struct s390_map_info *map, *tmp;
>> +	unsigned long flags;
>>   
>> -	for (i = 0; i < MAX_S390_IO_ADAPTERS; i++)
>> +	for (i = 0; i < MAX_S390_IO_ADAPTERS; i++) {
>> +		if (!kvm->arch.adapters[i])
>> +			continue;
>> +		raw_spin_lock_irqsave(&kvm->arch.adapters[i]->maps_lock, flags);
>> +		list_for_each_entry_safe(map, tmp,
>> +					 &kvm->arch.adapters[i]->maps, list) {
>> +			list_del(&map->list);
>> +			put_page(map->page);
>> +			kfree(map);
>> +		}
>> +		raw_spin_unlock_irqrestore(&kvm->arch.adapters[i]->maps_lock, flags);
> 
> Moving put_page/kfree out of the spinlock is a bit more work here.
> Handle this the same way you did in kvm_s390_unmap_all_adapters_pv()?
> 
> Actually wait -- besides the dirty page logic (which should be fine to
> do here too) this is the same code as kvm_s390_unmap_all_adapters_pv().
> Can you make the code in kvm_s390_unmap_all_adapters_pv() a single
> routine with a different name (e.g. kvm_s390_unmap_all_adapters()?) that
> is called both from here as well as from kvm_s390_handle_pv()?
> 
>