Re: [PATCH v1 04/13] KVM: s390: move pv gmap functions into kvm

All of lore.kernel.org
 help / color / mirror / Atom feed

* Re: [PATCH v1 04/13] KVM: s390: move pv gmap functions into kvm
@ 2025-01-10 18:42 kernel test robot
  0 siblings, 0 replies; 6+ messages in thread
From: kernel test robot @ 2025-01-10 18:42 UTC (permalink / raw)
  To: oe-kbuild; +Cc: lkp, Dan Carpenter

BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20250108181451.74383-5-imbrenda@linux.ibm.com>
References: <20250108181451.74383-5-imbrenda@linux.ibm.com>
TO: Claudio Imbrenda <imbrenda@linux.ibm.com>
TO: kvm@vger.kernel.org
CC: linux-s390@vger.kernel.org
CC: frankja@linux.ibm.com
CC: borntraeger@de.ibm.com
CC: schlameuss@linux.ibm.com
CC: david@redhat.com
CC: willy@infradead.org
CC: hca@linux.ibm.com
CC: svens@linux.ibm.com
CC: agordeev@linux.ibm.com
CC: gor@linux.ibm.com
CC: nrb@linux.ibm.com
CC: nsg@linux.ibm.com

Hi Claudio,

kernel test robot noticed the following build warnings:

[auto build test WARNING on s390/features]
[also build test WARNING on kvm/queue kvm/next mst-vhost/linux-next linus/master v6.13-rc6 next-20250110]
[cannot apply to kvms390/next kvm/linux-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Claudio-Imbrenda/KVM-s390-wrapper-for-KVM_BUG/20250109-021808
base:   https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git features
patch link:    https://lore.kernel.org/r/20250108181451.74383-5-imbrenda%40linux.ibm.com
patch subject: [PATCH v1 04/13] KVM: s390: move pv gmap functions into kvm
:::::: branch date: 2 days ago
:::::: commit date: 2 days ago
config: s390-randconfig-r071-20250110 (https://download.01.org/0day-ci/archive/20250111/202501110214.sqdsgfNV-lkp@intel.com/config)
compiler: clang version 16.0.6 (https://github.com/llvm/llvm-project 7cbf1a2591520c2491aa35339f227775f4d3adf6)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Closes: https://lore.kernel.org/r/202501110214.sqdsgfNV-lkp@intel.com/

smatch warnings:
arch/s390/kernel/uv.c:302 uv_wiggle_folio() warn: missing error code? 'rc'

vim +/rc +302 arch/s390/kernel/uv.c

214d9bbcd3a672 Claudio Imbrenda  2020-01-21  280  
72b1daff2671ce Claudio Imbrenda  2022-06-28  281  /**
0fab98cab07bbe Claudio Imbrenda  2025-01-08  282   * uv_wiggle_folio() - try to drain extra references to a folio
0fab98cab07bbe Claudio Imbrenda  2025-01-08  283   * @folio: the folio
0fab98cab07bbe Claudio Imbrenda  2025-01-08  284   * @split: whether to split a large folio
72b1daff2671ce Claudio Imbrenda  2022-06-28  285   *
0fab98cab07bbe Claudio Imbrenda  2025-01-08  286   * Context: Must be called while holding an extra reference to the folio;
0fab98cab07bbe Claudio Imbrenda  2025-01-08  287   *          the mm lock should not be held.
68ad4743beaa71 David Hildenbrand 2024-05-08  288   */
0fab98cab07bbe Claudio Imbrenda  2025-01-08  289  int uv_wiggle_folio(struct folio *folio, bool split)
68ad4743beaa71 David Hildenbrand 2024-05-08  290  {
214d9bbcd3a672 Claudio Imbrenda  2020-01-21  291  	int rc;
214d9bbcd3a672 Claudio Imbrenda  2020-01-21  292  
0fab98cab07bbe Claudio Imbrenda  2025-01-08  293  	folio_wait_writeback(folio);
0fab98cab07bbe Claudio Imbrenda  2025-01-08  294  	if (split) {
eef88fe45ac978 David Hildenbrand 2024-05-08  295  		folio_lock(folio);
eef88fe45ac978 David Hildenbrand 2024-05-08  296  		rc = split_folio(folio);
eef88fe45ac978 David Hildenbrand 2024-05-08  297  		folio_unlock(folio);
eef88fe45ac978 David Hildenbrand 2024-05-08  298  
0fab98cab07bbe Claudio Imbrenda  2025-01-08  299  		if (rc == -EBUSY)
eef88fe45ac978 David Hildenbrand 2024-05-08  300  			return -EAGAIN;
0fab98cab07bbe Claudio Imbrenda  2025-01-08  301  		if (rc != -EAGAIN)
214d9bbcd3a672 Claudio Imbrenda  2020-01-21 @302  			return rc;
214d9bbcd3a672 Claudio Imbrenda  2020-01-21  303  	}
0fab98cab07bbe Claudio Imbrenda  2025-01-08  304  	lru_add_drain_all();
0fab98cab07bbe Claudio Imbrenda  2025-01-08  305  	return -EAGAIN;
a52c25848e3143 Claudio Imbrenda  2022-06-28  306  }
0fab98cab07bbe Claudio Imbrenda  2025-01-08  307  EXPORT_SYMBOL_GPL(uv_wiggle_folio);
a52c25848e3143 Claudio Imbrenda  2022-06-28  308  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v1 00/13] KVM: s390: Stop using page->index and other things
@ 2025-01-08 18:14 Claudio Imbrenda
  2025-01-08 18:14 ` [PATCH v1 04/13] KVM: s390: move pv gmap functions into kvm Claudio Imbrenda
  0 siblings, 1 reply; 6+ messages in thread
From: Claudio Imbrenda @ 2025-01-08 18:14 UTC (permalink / raw)
  To: kvm
  Cc: linux-s390, frankja, borntraeger, schlameuss, david, willy, hca,
	svens, agordeev, gor, nrb, nsg

This patchseries starts moving some of the gmap logic into KVM itself,
going towards the final goal of completely removing gmap from the
non-kvm memory management code. Aside from just moving some code from
mm/gmap into kvm, this series also starts using __kvm_faultin_pfn() to
fault-in pages as needed.

But more importantly, this series removes almost all uses of
page->index (and all uses of page->lru) from the s390 KVM code.
The only remaining use is for the vsie pages, but that has already been
taken care of by David in another series.

Unfortunately the mix of hastiness and holidays means that this series
is a little bit all over the place, and not as complete as I would have
liked to.

I'm posting it now so to try to speed up the removal of page->index,
hopefully I will be able to post another short series before the
upcoming merge window closes.

Claudio Imbrenda (13):
  KVM: s390: wrapper for KVM_BUG
  KVM: s390: fake memslots for ucontrol VMs
  KVM: s390: use __kvm_faultin_pfn()
  KVM: s390: move pv gmap functions into kvm
  KVM: s390: get rid of gmap_fault()
  KVM: s390: get rid of gmap_translate()
  KVM: s390: move some gmap shadowing functions away from mm/gmap.c
  KVM: s390: stop using page->index for non-shadow gmaps
  KVM: s390: stop using lists to keep track of used dat tables
  KVM: s390: move gmap_shadow_pgt_lookup() into kvm
  KVM: s390: remove useless page->index usage
  KVM: s390: move PGSTE softbits
  KVM: s390: remove the last user of page->index

 arch/s390/include/asm/gmap.h    |  16 +-
 arch/s390/include/asm/pgtable.h |  21 +-
 arch/s390/include/asm/uv.h      |   7 +-
 arch/s390/kernel/uv.c           | 293 +++------------
 arch/s390/kvm/Makefile          |   2 +-
 arch/s390/kvm/gaccess.c         |  42 +++
 arch/s390/kvm/gmap.c            | 183 ++++++++++
 arch/s390/kvm/gmap.h            |  19 +
 arch/s390/kvm/intercept.c       |   5 +-
 arch/s390/kvm/interrupt.c       |  19 +-
 arch/s390/kvm/kvm-s390.c        | 229 ++++++++++--
 arch/s390/kvm/kvm-s390.h        |  18 +
 arch/s390/kvm/pv.c              |   1 +
 arch/s390/kvm/vsie.c            | 137 +++++++
 arch/s390/mm/gmap.c             | 630 ++++++--------------------------
 15 files changed, 786 insertions(+), 836 deletions(-)
 create mode 100644 arch/s390/kvm/gmap.c
 create mode 100644 arch/s390/kvm/gmap.h

-- 
2.47.1

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v1 04/13] KVM: s390: move pv gmap functions into kvm
  2025-01-08 18:14 [PATCH v1 00/13] KVM: s390: Stop using page->index and other things Claudio Imbrenda
@ 2025-01-08 18:14 ` Claudio Imbrenda
  2025-01-09 17:42   ` kernel test robot
  2025-01-15 12:48   ` Janosch Frank
  0 siblings, 2 replies; 6+ messages in thread
From: Claudio Imbrenda @ 2025-01-08 18:14 UTC (permalink / raw)
  To: kvm
  Cc: linux-s390, frankja, borntraeger, schlameuss, david, willy, hca,
	svens, agordeev, gor, nrb, nsg

Move gmap related functions from kernel/uv into kvm.

Create a new file to collect gmap-related functions.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 arch/s390/include/asm/uv.h |   7 +-
 arch/s390/kernel/uv.c      | 293 ++++++-------------------------------
 arch/s390/kvm/Makefile     |   2 +-
 arch/s390/kvm/gmap.c       | 183 +++++++++++++++++++++++
 arch/s390/kvm/gmap.h       |  17 +++
 arch/s390/kvm/intercept.c  |   1 +
 arch/s390/kvm/kvm-s390.c   |   1 +
 arch/s390/kvm/pv.c         |   1 +
 8 files changed, 251 insertions(+), 254 deletions(-)
 create mode 100644 arch/s390/kvm/gmap.c
 create mode 100644 arch/s390/kvm/gmap.h

diff --git a/arch/s390/include/asm/uv.h b/arch/s390/include/asm/uv.h
index dc332609f2c3..22ec1a24c291 100644
--- a/arch/s390/include/asm/uv.h
+++ b/arch/s390/include/asm/uv.h
@@ -628,12 +628,13 @@ static inline int is_prot_virt_host(void)
 }
 
 int uv_pin_shared(unsigned long paddr);
-int gmap_make_secure(struct gmap *gmap, unsigned long gaddr, void *uvcb);
-int gmap_destroy_page(struct gmap *gmap, unsigned long gaddr);
 int uv_destroy_folio(struct folio *folio);
 int uv_destroy_pte(pte_t pte);
 int uv_convert_from_secure_pte(pte_t pte);
-int gmap_convert_to_secure(struct gmap *gmap, unsigned long gaddr);
+int uv_wiggle_folio(struct folio *folio, bool split);
+int make_folio_secure(struct folio *folio, struct uv_cb_header *uvcb);
+int uv_convert_from_secure(unsigned long paddr);
+int uv_convert_from_secure_folio(struct folio *folio);
 
 void setup_uv(void);
 
diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c
index 6f9654a191ad..832c39c9ccfa 100644
--- a/arch/s390/kernel/uv.c
+++ b/arch/s390/kernel/uv.c
@@ -19,19 +19,6 @@
 #include <asm/sections.h>
 #include <asm/uv.h>
 
-#if !IS_ENABLED(CONFIG_KVM)
-unsigned long __gmap_translate(struct gmap *gmap, unsigned long gaddr)
-{
-	return 0;
-}
-
-int gmap_fault(struct gmap *gmap, unsigned long gaddr,
-	       unsigned int fault_flags)
-{
-	return 0;
-}
-#endif
-
 /* the bootdata_preserved fields come from ones in arch/s390/boot/uv.c */
 int __bootdata_preserved(prot_virt_guest);
 EXPORT_SYMBOL(prot_virt_guest);
@@ -159,6 +146,7 @@ int uv_destroy_folio(struct folio *folio)
 	folio_put(folio);
 	return rc;
 }
+EXPORT_SYMBOL(uv_destroy_folio);
 
 /*
  * The present PTE still indirectly holds a folio reference through the mapping.
@@ -175,7 +163,7 @@ int uv_destroy_pte(pte_t pte)
  *
  * @paddr: Absolute host address of page to be exported
  */
-static int uv_convert_from_secure(unsigned long paddr)
+int uv_convert_from_secure(unsigned long paddr)
 {
 	struct uv_cb_cfs uvcb = {
 		.header.cmd = UVC_CMD_CONV_FROM_SEC_STOR,
@@ -187,11 +175,12 @@ static int uv_convert_from_secure(unsigned long paddr)
 		return -EINVAL;
 	return 0;
 }
+EXPORT_SYMBOL_GPL(uv_convert_from_secure);
 
 /*
  * The caller must already hold a reference to the folio.
  */
-static int uv_convert_from_secure_folio(struct folio *folio)
+int uv_convert_from_secure_folio(struct folio *folio)
 {
 	int rc;
 
@@ -206,6 +195,7 @@ static int uv_convert_from_secure_folio(struct folio *folio)
 	folio_put(folio);
 	return rc;
 }
+EXPORT_SYMBOL_GPL(uv_convert_from_secure_folio);
 
 /*
  * The present PTE still indirectly holds a folio reference through the mapping.
@@ -237,13 +227,32 @@ static int expected_folio_refs(struct folio *folio)
 	return res;
 }
 
-static int make_folio_secure(struct folio *folio, struct uv_cb_header *uvcb)
+/**
+ * make_folio_secure() - make a folio secure
+ * @folio: the folio to make secure
+ * @uvcb: the uvcb that describes the UVC to be used
+ *
+ * The folio @folio will be made secure if possible, @uvcb will be passed
+ * as-is to the UVC.
+ *
+ * Return: 0 on success;
+ *         -EBUSY if the folio is in writeback, has too many references, or is large;
+ *         -EAGAIN if the UVC needs to be attempted again;
+ *         -ENXIO if the address is not mapped;
+ *         -EINVAL if the UVC failed for other reasons.
+ *
+ * Context: The caller must hold exactly one extra reference on the folio
+ *          (it's the same logic as split_folio())
+ */
+int make_folio_secure(struct folio *folio, struct uv_cb_header *uvcb)
 {
 	int expected, cc = 0;
 
+	if (folio_test_large(folio))
+		return -EBUSY;
 	if (folio_test_writeback(folio))
-		return -EAGAIN;
-	expected = expected_folio_refs(folio);
+		return -EBUSY;
+	expected = expected_folio_refs(folio) + 1;
 	if (!folio_ref_freeze(folio, expected))
 		return -EBUSY;
 	set_bit(PG_arch_1, &folio->flags);
@@ -267,251 +276,35 @@ static int make_folio_secure(struct folio *folio, struct uv_cb_header *uvcb)
 		return -EAGAIN;
 	return uvcb->rc == 0x10a ? -ENXIO : -EINVAL;
 }
+EXPORT_SYMBOL_GPL(make_folio_secure);
 
 /**
- * should_export_before_import - Determine whether an export is needed
- * before an import-like operation
- * @uvcb: the Ultravisor control block of the UVC to be performed
- * @mm: the mm of the process
- *
- * Returns whether an export is needed before every import-like operation.
- * This is needed for shared pages, which don't trigger a secure storage
- * exception when accessed from a different guest.
- *
- * Although considered as one, the Unpin Page UVC is not an actual import,
- * so it is not affected.
+ * uv_wiggle_folio() - try to drain extra references to a folio
+ * @folio: the folio
+ * @split: whether to split a large folio
  *
- * No export is needed also when there is only one protected VM, because the
- * page cannot belong to the wrong VM in that case (there is no "other VM"
- * it can belong to).
- *
- * Return: true if an export is needed before every import, otherwise false.
+ * Context: Must be called while holding an extra reference to the folio;
+ *          the mm lock should not be held.
  */
-static bool should_export_before_import(struct uv_cb_header *uvcb, struct mm_struct *mm)
+int uv_wiggle_folio(struct folio *folio, bool split)
 {
-	/*
-	 * The misc feature indicates, among other things, that importing a
-	 * shared page from a different protected VM will automatically also
-	 * transfer its ownership.
-	 */
-	if (uv_has_feature(BIT_UV_FEAT_MISC))
-		return false;
-	if (uvcb->cmd == UVC_CMD_UNPIN_PAGE_SHARED)
-		return false;
-	return atomic_read(&mm->context.protected_count) > 1;
-}
-
-/*
- * Drain LRU caches: the local one on first invocation and the ones of all
- * CPUs on successive invocations. Returns "true" on the first invocation.
- */
-static bool drain_lru(bool *drain_lru_called)
-{
-	/*
-	 * If we have tried a local drain and the folio refcount
-	 * still does not match our expected safe value, try with a
-	 * system wide drain. This is needed if the pagevecs holding
-	 * the page are on a different CPU.
-	 */
-	if (*drain_lru_called) {
-		lru_add_drain_all();
-		/* We give up here, don't retry immediately. */
-		return false;
-	}
-	/*
-	 * We are here if the folio refcount does not match the
-	 * expected safe value. The main culprits are usually
-	 * pagevecs. With lru_add_drain() we drain the pagevecs
-	 * on the local CPU so that hopefully the refcount will
-	 * reach the expected safe value.
-	 */
-	lru_add_drain();
-	*drain_lru_called = true;
-	/* The caller should try again immediately */
-	return true;
-}
-
-/*
- * Requests the Ultravisor to make a page accessible to a guest.
- * If it's brought in the first time, it will be cleared. If
- * it has been exported before, it will be decrypted and integrity
- * checked.
- */
-int gmap_make_secure(struct gmap *gmap, unsigned long gaddr, void *uvcb)
-{
-	struct vm_area_struct *vma;
-	bool drain_lru_called = false;
-	spinlock_t *ptelock;
-	unsigned long uaddr;
-	struct folio *folio;
-	pte_t *ptep;
 	int rc;
 
-again:
-	rc = -EFAULT;
-	mmap_read_lock(gmap->mm);
-
-	uaddr = __gmap_translate(gmap, gaddr);
-	if (IS_ERR_VALUE(uaddr))
-		goto out;
-	vma = vma_lookup(gmap->mm, uaddr);
-	if (!vma)
-		goto out;
-	/*
-	 * Secure pages cannot be huge and userspace should not combine both.
-	 * In case userspace does it anyway this will result in an -EFAULT for
-	 * the unpack. The guest is thus never reaching secure mode. If
-	 * userspace is playing dirty tricky with mapping huge pages later
-	 * on this will result in a segmentation fault.
-	 */
-	if (is_vm_hugetlb_page(vma))
-		goto out;
-
-	rc = -ENXIO;
-	ptep = get_locked_pte(gmap->mm, uaddr, &ptelock);
-	if (!ptep)
-		goto out;
-	if (pte_present(*ptep) && !(pte_val(*ptep) & _PAGE_INVALID) && pte_write(*ptep)) {
-		folio = page_folio(pte_page(*ptep));
-		rc = -EAGAIN;
-		if (folio_test_large(folio)) {
-			rc = -E2BIG;
-		} else if (folio_trylock(folio)) {
-			if (should_export_before_import(uvcb, gmap->mm))
-				uv_convert_from_secure(PFN_PHYS(folio_pfn(folio)));
-			rc = make_folio_secure(folio, uvcb);
-			folio_unlock(folio);
-		}
-
-		/*
-		 * Once we drop the PTL, the folio may get unmapped and
-		 * freed immediately. We need a temporary reference.
-		 */
-		if (rc == -EAGAIN || rc == -E2BIG)
-			folio_get(folio);
-	}
-	pte_unmap_unlock(ptep, ptelock);
-out:
-	mmap_read_unlock(gmap->mm);
-
-	switch (rc) {
-	case -E2BIG:
+	folio_wait_writeback(folio);
+	if (split) {
 		folio_lock(folio);
 		rc = split_folio(folio);
 		folio_unlock(folio);
-		folio_put(folio);
-
-		switch (rc) {
-		case 0:
-			/* Splitting succeeded, try again immediately. */
-			goto again;
-		case -EAGAIN:
-			/* Additional folio references. */
-			if (drain_lru(&drain_lru_called))
-				goto again;
-			return -EAGAIN;
-		case -EBUSY:
-			/* Unexpected race. */
+
+		if (rc == -EBUSY)
 			return -EAGAIN;
-		}
-		WARN_ON_ONCE(1);
-		return -ENXIO;
-	case -EAGAIN:
-		/*
-		 * If we are here because the UVC returned busy or partial
-		 * completion, this is just a useless check, but it is safe.
-		 */
-		folio_wait_writeback(folio);
-		folio_put(folio);
-		return -EAGAIN;
-	case -EBUSY:
-		/* Additional folio references. */
-		if (drain_lru(&drain_lru_called))
-			goto again;
-		return -EAGAIN;
-	case -ENXIO:
-		if (gmap_fault(gmap, gaddr, FAULT_FLAG_WRITE))
-			return -EFAULT;
-		return -EAGAIN;
+		if (rc != -EAGAIN)
+			return rc;
 	}
-	return rc;
-}
-EXPORT_SYMBOL_GPL(gmap_make_secure);
-
-int gmap_convert_to_secure(struct gmap *gmap, unsigned long gaddr)
-{
-	struct uv_cb_cts uvcb = {
-		.header.cmd = UVC_CMD_CONV_TO_SEC_STOR,
-		.header.len = sizeof(uvcb),
-		.guest_handle = gmap->guest_handle,
-		.gaddr = gaddr,
-	};
-
-	return gmap_make_secure(gmap, gaddr, &uvcb);
-}
-EXPORT_SYMBOL_GPL(gmap_convert_to_secure);
-
-/**
- * gmap_destroy_page - Destroy a guest page.
- * @gmap: the gmap of the guest
- * @gaddr: the guest address to destroy
- *
- * An attempt will be made to destroy the given guest page. If the attempt
- * fails, an attempt is made to export the page. If both attempts fail, an
- * appropriate error is returned.
- */
-int gmap_destroy_page(struct gmap *gmap, unsigned long gaddr)
-{
-	struct vm_area_struct *vma;
-	struct folio_walk fw;
-	unsigned long uaddr;
-	struct folio *folio;
-	int rc;
-
-	rc = -EFAULT;
-	mmap_read_lock(gmap->mm);
-
-	uaddr = __gmap_translate(gmap, gaddr);
-	if (IS_ERR_VALUE(uaddr))
-		goto out;
-	vma = vma_lookup(gmap->mm, uaddr);
-	if (!vma)
-		goto out;
-	/*
-	 * Huge pages should not be able to become secure
-	 */
-	if (is_vm_hugetlb_page(vma))
-		goto out;
-
-	rc = 0;
-	folio = folio_walk_start(&fw, vma, uaddr, 0);
-	if (!folio)
-		goto out;
-	/*
-	 * See gmap_make_secure(): large folios cannot be secure. Small
-	 * folio implies FW_LEVEL_PTE.
-	 */
-	if (folio_test_large(folio) || !pte_write(fw.pte))
-		goto out_walk_end;
-	rc = uv_destroy_folio(folio);
-	/*
-	 * Fault handlers can race; it is possible that two CPUs will fault
-	 * on the same secure page. One CPU can destroy the page, reboot,
-	 * re-enter secure mode and import it, while the second CPU was
-	 * stuck at the beginning of the handler. At some point the second
-	 * CPU will be able to progress, and it will not be able to destroy
-	 * the page. In that case we do not want to terminate the process,
-	 * we instead try to export the page.
-	 */
-	if (rc)
-		rc = uv_convert_from_secure_folio(folio);
-out_walk_end:
-	folio_walk_end(&fw, vma);
-out:
-	mmap_read_unlock(gmap->mm);
-	return rc;
+	lru_add_drain_all();
+	return -EAGAIN;
 }
-EXPORT_SYMBOL_GPL(gmap_destroy_page);
+EXPORT_SYMBOL_GPL(uv_wiggle_folio);
 
 /*
  * To be called with the folio locked or with an extra reference! This will
diff --git a/arch/s390/kvm/Makefile b/arch/s390/kvm/Makefile
index 02217fb4ae10..d972dea657fd 100644
--- a/arch/s390/kvm/Makefile
+++ b/arch/s390/kvm/Makefile
@@ -8,7 +8,7 @@ include $(srctree)/virt/kvm/Makefile.kvm
 ccflags-y := -Ivirt/kvm -Iarch/s390/kvm
 
 kvm-y += kvm-s390.o intercept.o interrupt.o priv.o sigp.o
-kvm-y += diag.o gaccess.o guestdbg.o vsie.o pv.o
+kvm-y += diag.o gaccess.o guestdbg.o vsie.o pv.o gmap.o
 
 kvm-$(CONFIG_VFIO_PCI_ZDEV_KVM) += pci.o
 obj-$(CONFIG_KVM) += kvm.o
diff --git a/arch/s390/kvm/gmap.c b/arch/s390/kvm/gmap.c
new file mode 100644
index 000000000000..a142bbbddc25
--- /dev/null
+++ b/arch/s390/kvm/gmap.c
@@ -0,0 +1,183 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Guest memory management for KVM/s390
+ *
+ * Copyright IBM Corp. 2008, 2020, 2024
+ *
+ *    Author(s): Claudio Imbrenda <imbrenda@linux.ibm.com>
+ *               Martin Schwidefsky <schwidefsky@de.ibm.com>
+ *               David Hildenbrand <david@redhat.com>
+ *               Janosch Frank <frankja@linux.vnet.ibm.com>
+ */
+
+#include <linux/compiler.h>
+#include <linux/kvm.h>
+#include <linux/kvm_host.h>
+#include <linux/pgtable.h>
+#include <linux/pagemap.h>
+
+#include <asm/lowcore.h>
+#include <asm/gmap.h>
+#include <asm/uv.h>
+
+#include "gmap.h"
+
+/**
+ * should_export_before_import - Determine whether an export is needed
+ * before an import-like operation
+ * @uvcb: the Ultravisor control block of the UVC to be performed
+ * @mm: the mm of the process
+ *
+ * Returns whether an export is needed before every import-like operation.
+ * This is needed for shared pages, which don't trigger a secure storage
+ * exception when accessed from a different guest.
+ *
+ * Although considered as one, the Unpin Page UVC is not an actual import,
+ * so it is not affected.
+ *
+ * No export is needed also when there is only one protected VM, because the
+ * page cannot belong to the wrong VM in that case (there is no "other VM"
+ * it can belong to).
+ *
+ * Return: true if an export is needed before every import, otherwise false.
+ */
+static bool should_export_before_import(struct uv_cb_header *uvcb, struct mm_struct *mm)
+{
+	/*
+	 * The misc feature indicates, among other things, that importing a
+	 * shared page from a different protected VM will automatically also
+	 * transfer its ownership.
+	 */
+	if (uv_has_feature(BIT_UV_FEAT_MISC))
+		return false;
+	if (uvcb->cmd == UVC_CMD_UNPIN_PAGE_SHARED)
+		return false;
+	return atomic_read(&mm->context.protected_count) > 1;
+}
+
+static int __gmap_make_secure(struct gmap *gmap, struct page *page, void *uvcb)
+{
+	struct folio *folio = page_folio(page);
+	int rc;
+
+	/*
+	 * Secure pages cannot be huge and userspace should not combine both.
+	 * In case userspace does it anyway this will result in an -EFAULT for
+	 * the unpack. The guest is thus never reaching secure mode. If
+	 * userspace is playing dirty tricky with mapping huge pages later
+	 * on this will result in a segmentation fault or in a -EFAULT return
+	 * code from the KVM_RUN ioctl.
+	 */
+	if (folio_test_hugetlb(folio))
+		return -EFAULT;
+	if (folio_test_large(folio)) {
+		mmap_read_unlock(gmap->mm);
+		rc = uv_wiggle_folio(folio, true);
+		mmap_read_lock(gmap->mm);
+		if (rc)
+			return rc;
+		folio = page_folio(page);
+	}
+
+	rc = -EAGAIN;
+	if (folio_trylock(folio)) {
+		if (should_export_before_import(uvcb, gmap->mm))
+			uv_convert_from_secure(folio_to_phys(folio));
+		rc = make_folio_secure(folio, uvcb);
+		folio_unlock(folio);
+	}
+
+	/*
+	 * Unlikely case: the page is not mapped anymore. Return success
+	 * and let the proper fault handler fault in the page again.
+	 */
+	if (rc == -ENXIO)
+		return 0;
+	/* The folio has too many references, try to shake some off */
+	if (rc == -EBUSY) {
+		mmap_read_unlock(gmap->mm);
+		uv_wiggle_folio(folio, false);
+		mmap_read_lock(gmap->mm);
+		return -EAGAIN;
+	}
+
+	return rc;
+}
+
+int gmap_make_secure(struct gmap *gmap, unsigned long gaddr, void *uvcb)
+{
+	struct page *page;
+	int rc = 0;
+
+	mmap_read_lock(gmap->mm);
+	page = gfn_to_page(gmap->private, gpa_to_gfn(gaddr));
+	if (page)
+		rc = __gmap_make_secure(gmap, page, uvcb);
+	kvm_release_page_clean(page);
+	mmap_read_unlock(gmap->mm);
+
+	return rc;
+}
+
+int gmap_convert_to_secure(struct gmap *gmap, unsigned long gaddr)
+{
+	struct uv_cb_cts uvcb = {
+		.header.cmd = UVC_CMD_CONV_TO_SEC_STOR,
+		.header.len = sizeof(uvcb),
+		.guest_handle = gmap->guest_handle,
+		.gaddr = gaddr,
+	};
+
+	return gmap_make_secure(gmap, gaddr, &uvcb);
+}
+
+/**
+ * gmap_destroy_page - Destroy a guest page.
+ * @gmap: the gmap of the guest
+ * @gaddr: the guest address to destroy
+ *
+ * An attempt will be made to destroy the given guest page. If the attempt
+ * fails, an attempt is made to export the page. If both attempts fail, an
+ * appropriate error is returned.
+ */
+static int __gmap_destroy_page(struct gmap *gmap, struct page *page)
+{
+	struct folio *folio = page_folio(page);
+	int rc;
+
+	/*
+	 * See gmap_make_secure(): large folios cannot be secure. Small
+	 * folio implies FW_LEVEL_PTE.
+	 */
+	if (folio_test_large(folio))
+		return -EFAULT;
+
+	rc = uv_destroy_folio(folio);
+	/*
+	 * Fault handlers can race; it is possible that two CPUs will fault
+	 * on the same secure page. One CPU can destroy the page, reboot,
+	 * re-enter secure mode and import it, while the second CPU was
+	 * stuck at the beginning of the handler. At some point the second
+	 * CPU will be able to progress, and it will not be able to destroy
+	 * the page. In that case we do not want to terminate the process,
+	 * we instead try to export the page.
+	 */
+	if (rc)
+		rc = uv_convert_from_secure_folio(folio);
+
+	return rc;
+}
+
+int gmap_destroy_page(struct gmap *gmap, unsigned long gaddr)
+{
+	struct page *page;
+	int rc = 0;
+
+	mmap_read_lock(gmap->mm);
+	page = gfn_to_page(gmap->private, gpa_to_gfn(gaddr));
+	if (page)
+		rc = __gmap_destroy_page(gmap, page);
+	kvm_release_page_clean(page);
+	mmap_read_unlock(gmap->mm);
+	return rc;
+}
diff --git a/arch/s390/kvm/gmap.h b/arch/s390/kvm/gmap.h
new file mode 100644
index 000000000000..f2b52ce29be3
--- /dev/null
+++ b/arch/s390/kvm/gmap.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ *  KVM guest address space mapping code
+ *
+ *    Copyright IBM Corp. 2007, 2016, 2025
+ *    Author(s): Martin Schwidefsky <schwidefsky@de.ibm.com>
+ *               Claudio Imbrenda <imbrenda@linux.ibm.com>
+ */
+
+#ifndef ARCH_KVM_S390_GMAP_H
+#define ARCH_KVM_S390_GMAP_H
+
+int gmap_make_secure(struct gmap *gmap, unsigned long gaddr, void *uvcb);
+int gmap_convert_to_secure(struct gmap *gmap, unsigned long gaddr);
+int gmap_destroy_page(struct gmap *gmap, unsigned long gaddr);
+
+#endif
diff --git a/arch/s390/kvm/intercept.c b/arch/s390/kvm/intercept.c
index 5bbaadf75dc6..92ae003cd215 100644
--- a/arch/s390/kvm/intercept.c
+++ b/arch/s390/kvm/intercept.c
@@ -21,6 +21,7 @@
 #include "gaccess.h"
 #include "trace.h"
 #include "trace-s390.h"
+#include "gmap.h"
 
 u8 kvm_s390_get_ilen(struct kvm_vcpu *vcpu)
 {
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 8e4e7e45238b..bdbb143a75c9 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -50,6 +50,7 @@
 #include "kvm-s390.h"
 #include "gaccess.h"
 #include "pci.h"
+#include "gmap.h"
 
 #define CREATE_TRACE_POINTS
 #include "trace.h"
diff --git a/arch/s390/kvm/pv.c b/arch/s390/kvm/pv.c
index 75e81ba26d04..f0301e673810 100644
--- a/arch/s390/kvm/pv.c
+++ b/arch/s390/kvm/pv.c
@@ -17,6 +17,7 @@
 #include <linux/sched/mm.h>
 #include <linux/mmu_notifier.h>
 #include "kvm-s390.h"
+#include "gmap.h"
 
 bool kvm_s390_pv_is_protected(struct kvm *kvm)
 {
-- 
2.47.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v1 04/13] KVM: s390: move pv gmap functions into kvm
  2025-01-08 18:14 ` [PATCH v1 04/13] KVM: s390: move pv gmap functions into kvm Claudio Imbrenda
@ 2025-01-09 17:42   ` kernel test robot
  2025-01-15 12:48   ` Janosch Frank
  1 sibling, 0 replies; 6+ messages in thread
From: kernel test robot @ 2025-01-09 17:42 UTC (permalink / raw)
  To: Claudio Imbrenda, kvm
  Cc: oe-kbuild-all, linux-s390, frankja, borntraeger, schlameuss,
	david, willy, hca, svens, agordeev, gor, nrb, nsg

Hi Claudio,

kernel test robot noticed the following build warnings:

[auto build test WARNING on s390/features]
[also build test WARNING on kvm/queue kvm/next mst-vhost/linux-next linus/master v6.13-rc6 next-20250109]
[cannot apply to kvms390/next kvm/linux-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Claudio-Imbrenda/KVM-s390-wrapper-for-KVM_BUG/20250109-021808
base:   https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git features
patch link:    https://lore.kernel.org/r/20250108181451.74383-5-imbrenda%40linux.ibm.com
patch subject: [PATCH v1 04/13] KVM: s390: move pv gmap functions into kvm
config: s390-randconfig-001-20250109 (https://download.01.org/0day-ci/archive/20250110/202501100045.U1NGK9qJ-lkp@intel.com/config)
compiler: s390-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250110/202501100045.U1NGK9qJ-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202501100045.U1NGK9qJ-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> arch/s390/kvm/gmap.c:144: warning: Function parameter or struct member 'page' not described in '__gmap_destroy_page'
>> arch/s390/kvm/gmap.c:144: warning: expecting prototype for gmap_destroy_page(). Prototype was for __gmap_destroy_page() instead


vim +144 arch/s390/kvm/gmap.c

   133	
   134	/**
   135	 * gmap_destroy_page - Destroy a guest page.
   136	 * @gmap: the gmap of the guest
   137	 * @gaddr: the guest address to destroy
   138	 *
   139	 * An attempt will be made to destroy the given guest page. If the attempt
   140	 * fails, an attempt is made to export the page. If both attempts fail, an
   141	 * appropriate error is returned.
   142	 */
   143	static int __gmap_destroy_page(struct gmap *gmap, struct page *page)
 > 144	{
   145		struct folio *folio = page_folio(page);
   146		int rc;
   147	
   148		/*
   149		 * See gmap_make_secure(): large folios cannot be secure. Small
   150		 * folio implies FW_LEVEL_PTE.
   151		 */
   152		if (folio_test_large(folio))
   153			return -EFAULT;
   154	
   155		rc = uv_destroy_folio(folio);
   156		/*
   157		 * Fault handlers can race; it is possible that two CPUs will fault
   158		 * on the same secure page. One CPU can destroy the page, reboot,
   159		 * re-enter secure mode and import it, while the second CPU was
   160		 * stuck at the beginning of the handler. At some point the second
   161		 * CPU will be able to progress, and it will not be able to destroy
   162		 * the page. In that case we do not want to terminate the process,
   163		 * we instead try to export the page.
   164		 */
   165		if (rc)
   166			rc = uv_convert_from_secure_folio(folio);
   167	
   168		return rc;
   169	}
   170	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v1 04/13] KVM: s390: move pv gmap functions into kvm
  2025-01-08 18:14 ` [PATCH v1 04/13] KVM: s390: move pv gmap functions into kvm Claudio Imbrenda
  2025-01-09 17:42   ` kernel test robot
@ 2025-01-15 12:48   ` Janosch Frank
  2025-01-15 12:59     ` Claudio Imbrenda
  1 sibling, 1 reply; 6+ messages in thread
From: Janosch Frank @ 2025-01-15 12:48 UTC (permalink / raw)
  To: Claudio Imbrenda, kvm
  Cc: linux-s390, borntraeger, schlameuss, david, willy, hca, svens,
	agordeev, gor, nrb, nsg

On 1/8/25 7:14 PM, Claudio Imbrenda wrote:
> Move gmap related functions from kernel/uv into kvm.
> 
> Create a new file to collect gmap-related functions.
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
>   arch/s390/include/asm/uv.h |   7 +-
>   arch/s390/kernel/uv.c      | 293 ++++++-------------------------------
>   arch/s390/kvm/Makefile     |   2 +-
>   arch/s390/kvm/gmap.c       | 183 +++++++++++++++++++++++
>   arch/s390/kvm/gmap.h       |  17 +++
>   arch/s390/kvm/intercept.c  |   1 +
>   arch/s390/kvm/kvm-s390.c   |   1 +
>   arch/s390/kvm/pv.c         |   1 +
>   8 files changed, 251 insertions(+), 254 deletions(-)
>   create mode 100644 arch/s390/kvm/gmap.c
>   create mode 100644 arch/s390/kvm/gmap.h
> 
> diff --git a/arch/s390/include/asm/uv.h b/arch/s390/include/asm/uv.h
> index dc332609f2c3..22ec1a24c291 100644
> --- a/arch/s390/include/asm/uv.h
> +++ b/arch/s390/include/asm/uv.h
> @@ -628,12 +628,13 @@ static inline int is_prot_virt_host(void)
>   }
>   
>   int uv_pin_shared(unsigned long paddr);
> -int gmap_make_secure(struct gmap *gmap, unsigned long gaddr, void *uvcb);
> -int gmap_destroy_page(struct gmap *gmap, unsigned long gaddr);
>   int uv_destroy_folio(struct folio *folio);
>   int uv_destroy_pte(pte_t pte);
>   int uv_convert_from_secure_pte(pte_t pte);
> -int gmap_convert_to_secure(struct gmap *gmap, unsigned long gaddr);
> +int uv_wiggle_folio(struct folio *folio, bool split);
> +int make_folio_secure(struct folio *folio, struct uv_cb_header *uvcb);
> +int uv_convert_from_secure(unsigned long paddr);
> +int uv_convert_from_secure_folio(struct folio *folio);
>   
>   void setup_uv(void);
>   
> diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c
> index 6f9654a191ad..832c39c9ccfa 100644
> --- a/arch/s390/kernel/uv.c
> +++ b/arch/s390/kernel/uv.c
> @@ -19,19 +19,6 @@
>   #include <asm/sections.h>
>   #include <asm/uv.h>
>   
> -#if !IS_ENABLED(CONFIG_KVM)
> -unsigned long __gmap_translate(struct gmap *gmap, unsigned long gaddr)
> -{
> -	return 0;
> -}
> -
> -int gmap_fault(struct gmap *gmap, unsigned long gaddr,
> -	       unsigned int fault_flags)
> -{
> -	return 0;
> -}
> -#endif
> -
>   /* the bootdata_preserved fields come from ones in arch/s390/boot/uv.c */
>   int __bootdata_preserved(prot_virt_guest);
>   EXPORT_SYMBOL(prot_virt_guest);
> @@ -159,6 +146,7 @@ int uv_destroy_folio(struct folio *folio)
>   	folio_put(folio);
>   	return rc;
>   }
> +EXPORT_SYMBOL(uv_destroy_folio);
>   
>   /*
>    * The present PTE still indirectly holds a folio reference through the mapping.
> @@ -175,7 +163,7 @@ int uv_destroy_pte(pte_t pte)
>    *
>    * @paddr: Absolute host address of page to be exported
>    */
> -static int uv_convert_from_secure(unsigned long paddr)
> +int uv_convert_from_secure(unsigned long paddr)
>   {
>   	struct uv_cb_cfs uvcb = {
>   		.header.cmd = UVC_CMD_CONV_FROM_SEC_STOR,
> @@ -187,11 +175,12 @@ static int uv_convert_from_secure(unsigned long paddr)
>   		return -EINVAL;
>   	return 0;
>   }
> +EXPORT_SYMBOL_GPL(uv_convert_from_secure);
>   
>   /*
>    * The caller must already hold a reference to the folio.
>    */
> -static int uv_convert_from_secure_folio(struct folio *folio)
> +int uv_convert_from_secure_folio(struct folio *folio)
>   {
>   	int rc;
>   
> @@ -206,6 +195,7 @@ static int uv_convert_from_secure_folio(struct folio *folio)
>   	folio_put(folio);
>   	return rc;
>   }
> +EXPORT_SYMBOL_GPL(uv_convert_from_secure_folio);
>   
>   /*
>    * The present PTE still indirectly holds a folio reference through the mapping.
> @@ -237,13 +227,32 @@ static int expected_folio_refs(struct folio *folio)
>   	return res;
>   }
>   
> -static int make_folio_secure(struct folio *folio, struct uv_cb_header *uvcb)
> +/**
> + * make_folio_secure() - make a folio secure
> + * @folio: the folio to make secure
> + * @uvcb: the uvcb that describes the UVC to be used
> + *
> + * The folio @folio will be made secure if possible, @uvcb will be passed
> + * as-is to the UVC.
> + *
> + * Return: 0 on success;
> + *         -EBUSY if the folio is in writeback, has too many references, or is large;
> + *         -EAGAIN if the UVC needs to be attempted again;
> + *         -ENXIO if the address is not mapped;
> + *         -EINVAL if the UVC failed for other reasons.
> + *
> + * Context: The caller must hold exactly one extra reference on the folio
> + *          (it's the same logic as split_folio())
> + */
> +int make_folio_secure(struct folio *folio, struct uv_cb_header *uvcb)
>   {
>   	int expected, cc = 0;
>   
> +	if (folio_test_large(folio))
> +		return -EBUSY;
>   	if (folio_test_writeback(folio))
> -		return -EAGAIN;
> -	expected = expected_folio_refs(folio);
> +		return -EBUSY;
> +	expected = expected_folio_refs(folio) + 1;
>   	if (!folio_ref_freeze(folio, expected))
>   		return -EBUSY;
>   	set_bit(PG_arch_1, &folio->flags);
> @@ -267,251 +276,35 @@ static int make_folio_secure(struct folio *folio, struct uv_cb_header *uvcb)
>   		return -EAGAIN;
>   	return uvcb->rc == 0x10a ? -ENXIO : -EINVAL;
>   }
> +EXPORT_SYMBOL_GPL(make_folio_secure);
>   
>   /**
> - * should_export_before_import - Determine whether an export is needed
> - * before an import-like operation
> - * @uvcb: the Ultravisor control block of the UVC to be performed
> - * @mm: the mm of the process
> - *
> - * Returns whether an export is needed before every import-like operation.
> - * This is needed for shared pages, which don't trigger a secure storage
> - * exception when accessed from a different guest.
> - *
> - * Although considered as one, the Unpin Page UVC is not an actual import,
> - * so it is not affected.
> + * uv_wiggle_folio() - try to drain extra references to a folio
> + * @folio: the folio
> + * @split: whether to split a large folio
>    *
> - * No export is needed also when there is only one protected VM, because the
> - * page cannot belong to the wrong VM in that case (there is no "other VM"
> - * it can belong to).
> - *
> - * Return: true if an export is needed before every import, otherwise false.
> + * Context: Must be called while holding an extra reference to the folio;
> + *          the mm lock should not be held.
>    */
> -static bool should_export_before_import(struct uv_cb_header *uvcb, struct mm_struct *mm)
> +int uv_wiggle_folio(struct folio *folio, bool split)
>   {
> -	/*
> -	 * The misc feature indicates, among other things, that importing a
> -	 * shared page from a different protected VM will automatically also
> -	 * transfer its ownership.
> -	 */
> -	if (uv_has_feature(BIT_UV_FEAT_MISC))
> -		return false;
> -	if (uvcb->cmd == UVC_CMD_UNPIN_PAGE_SHARED)
> -		return false;
> -	return atomic_read(&mm->context.protected_count) > 1;
> -}
> -
> -/*
> - * Drain LRU caches: the local one on first invocation and the ones of all
> - * CPUs on successive invocations. Returns "true" on the first invocation.
> - */
> -static bool drain_lru(bool *drain_lru_called)
> -{
> -	/*
> -	 * If we have tried a local drain and the folio refcount
> -	 * still does not match our expected safe value, try with a
> -	 * system wide drain. This is needed if the pagevecs holding
> -	 * the page are on a different CPU.
> -	 */
> -	if (*drain_lru_called) {
> -		lru_add_drain_all();
> -		/* We give up here, don't retry immediately. */
> -		return false;
> -	}
> -	/*
> -	 * We are here if the folio refcount does not match the
> -	 * expected safe value. The main culprits are usually
> -	 * pagevecs. With lru_add_drain() we drain the pagevecs
> -	 * on the local CPU so that hopefully the refcount will
> -	 * reach the expected safe value.
> -	 */
> -	lru_add_drain();
> -	*drain_lru_called = true;
> -	/* The caller should try again immediately */
> -	return true;
> -}
> -
> -/*
> - * Requests the Ultravisor to make a page accessible to a guest.
> - * If it's brought in the first time, it will be cleared. If
> - * it has been exported before, it will be decrypted and integrity
> - * checked.
> - */
> -int gmap_make_secure(struct gmap *gmap, unsigned long gaddr, void *uvcb)
> -{
> -	struct vm_area_struct *vma;
> -	bool drain_lru_called = false;
> -	spinlock_t *ptelock;
> -	unsigned long uaddr;
> -	struct folio *folio;
> -	pte_t *ptep;
>   	int rc;
>   
> -again:
> -	rc = -EFAULT;
> -	mmap_read_lock(gmap->mm);
> -
> -	uaddr = __gmap_translate(gmap, gaddr);
> -	if (IS_ERR_VALUE(uaddr))
> -		goto out;
> -	vma = vma_lookup(gmap->mm, uaddr);
> -	if (!vma)
> -		goto out;
> -	/*
> -	 * Secure pages cannot be huge and userspace should not combine both.
> -	 * In case userspace does it anyway this will result in an -EFAULT for
> -	 * the unpack. The guest is thus never reaching secure mode. If
> -	 * userspace is playing dirty tricky with mapping huge pages later
> -	 * on this will result in a segmentation fault.
> -	 */
> -	if (is_vm_hugetlb_page(vma))
> -		goto out;
> -
> -	rc = -ENXIO;
> -	ptep = get_locked_pte(gmap->mm, uaddr, &ptelock);
> -	if (!ptep)
> -		goto out;
> -	if (pte_present(*ptep) && !(pte_val(*ptep) & _PAGE_INVALID) && pte_write(*ptep)) {
> -		folio = page_folio(pte_page(*ptep));
> -		rc = -EAGAIN;
> -		if (folio_test_large(folio)) {
> -			rc = -E2BIG;
> -		} else if (folio_trylock(folio)) {
> -			if (should_export_before_import(uvcb, gmap->mm))
> -				uv_convert_from_secure(PFN_PHYS(folio_pfn(folio)));
> -			rc = make_folio_secure(folio, uvcb);
> -			folio_unlock(folio);
> -		}
> -
> -		/*
> -		 * Once we drop the PTL, the folio may get unmapped and
> -		 * freed immediately. We need a temporary reference.
> -		 */
> -		if (rc == -EAGAIN || rc == -E2BIG)
> -			folio_get(folio);
> -	}
> -	pte_unmap_unlock(ptep, ptelock);
> -out:
> -	mmap_read_unlock(gmap->mm);
> -
> -	switch (rc) {
> -	case -E2BIG:
> +	folio_wait_writeback(folio);
> +	if (split) {
>   		folio_lock(folio);
>   		rc = split_folio(folio);
>   		folio_unlock(folio);
> -		folio_put(folio);
> -
> -		switch (rc) {
> -		case 0:
> -			/* Splitting succeeded, try again immediately. */
> -			goto again;
> -		case -EAGAIN:
> -			/* Additional folio references. */
> -			if (drain_lru(&drain_lru_called))
> -				goto again;
> -			return -EAGAIN;
> -		case -EBUSY:
> -			/* Unexpected race. */
> +
> +		if (rc == -EBUSY)
>   			return -EAGAIN;
> -		}
> -		WARN_ON_ONCE(1);
> -		return -ENXIO;
> -	case -EAGAIN:
> -		/*
> -		 * If we are here because the UVC returned busy or partial
> -		 * completion, this is just a useless check, but it is safe.
> -		 */
> -		folio_wait_writeback(folio);
> -		folio_put(folio);
> -		return -EAGAIN;
> -	case -EBUSY:
> -		/* Additional folio references. */
> -		if (drain_lru(&drain_lru_called))
> -			goto again;
> -		return -EAGAIN;
> -	case -ENXIO:
> -		if (gmap_fault(gmap, gaddr, FAULT_FLAG_WRITE))
> -			return -EFAULT;
> -		return -EAGAIN;
> +		if (rc != -EAGAIN)
> +			return rc;
>   	}
> -	return rc;
> -}
> -EXPORT_SYMBOL_GPL(gmap_make_secure);
> -
> -int gmap_convert_to_secure(struct gmap *gmap, unsigned long gaddr)
> -{
> -	struct uv_cb_cts uvcb = {
> -		.header.cmd = UVC_CMD_CONV_TO_SEC_STOR,
> -		.header.len = sizeof(uvcb),
> -		.guest_handle = gmap->guest_handle,
> -		.gaddr = gaddr,
> -	};
> -
> -	return gmap_make_secure(gmap, gaddr, &uvcb);
> -}
> -EXPORT_SYMBOL_GPL(gmap_convert_to_secure);
> -
> -/**
> - * gmap_destroy_page - Destroy a guest page.
> - * @gmap: the gmap of the guest
> - * @gaddr: the guest address to destroy
> - *
> - * An attempt will be made to destroy the given guest page. If the attempt
> - * fails, an attempt is made to export the page. If both attempts fail, an
> - * appropriate error is returned.
> - */
> -int gmap_destroy_page(struct gmap *gmap, unsigned long gaddr)
> -{
> -	struct vm_area_struct *vma;
> -	struct folio_walk fw;
> -	unsigned long uaddr;
> -	struct folio *folio;
> -	int rc;
> -
> -	rc = -EFAULT;
> -	mmap_read_lock(gmap->mm);
> -
> -	uaddr = __gmap_translate(gmap, gaddr);
> -	if (IS_ERR_VALUE(uaddr))
> -		goto out;
> -	vma = vma_lookup(gmap->mm, uaddr);
> -	if (!vma)
> -		goto out;
> -	/*
> -	 * Huge pages should not be able to become secure
> -	 */
> -	if (is_vm_hugetlb_page(vma))
> -		goto out;
> -
> -	rc = 0;
> -	folio = folio_walk_start(&fw, vma, uaddr, 0);
> -	if (!folio)
> -		goto out;
> -	/*
> -	 * See gmap_make_secure(): large folios cannot be secure. Small
> -	 * folio implies FW_LEVEL_PTE.
> -	 */
> -	if (folio_test_large(folio) || !pte_write(fw.pte))
> -		goto out_walk_end;
> -	rc = uv_destroy_folio(folio);
> -	/*
> -	 * Fault handlers can race; it is possible that two CPUs will fault
> -	 * on the same secure page. One CPU can destroy the page, reboot,
> -	 * re-enter secure mode and import it, while the second CPU was
> -	 * stuck at the beginning of the handler. At some point the second
> -	 * CPU will be able to progress, and it will not be able to destroy
> -	 * the page. In that case we do not want to terminate the process,
> -	 * we instead try to export the page.
> -	 */
> -	if (rc)
> -		rc = uv_convert_from_secure_folio(folio);
> -out_walk_end:
> -	folio_walk_end(&fw, vma);
> -out:
> -	mmap_read_unlock(gmap->mm);
> -	return rc;
> +	lru_add_drain_all();
> +	return -EAGAIN;
>   }
> -EXPORT_SYMBOL_GPL(gmap_destroy_page);
> +EXPORT_SYMBOL_GPL(uv_wiggle_folio);
>   
>   /*
>    * To be called with the folio locked or with an extra reference! This will
> diff --git a/arch/s390/kvm/Makefile b/arch/s390/kvm/Makefile
> index 02217fb4ae10..d972dea657fd 100644
> --- a/arch/s390/kvm/Makefile
> +++ b/arch/s390/kvm/Makefile
> @@ -8,7 +8,7 @@ include $(srctree)/virt/kvm/Makefile.kvm
>   ccflags-y := -Ivirt/kvm -Iarch/s390/kvm
>   
>   kvm-y += kvm-s390.o intercept.o interrupt.o priv.o sigp.o
> -kvm-y += diag.o gaccess.o guestdbg.o vsie.o pv.o
> +kvm-y += diag.o gaccess.o guestdbg.o vsie.o pv.o gmap.o
>   
>   kvm-$(CONFIG_VFIO_PCI_ZDEV_KVM) += pci.o
>   obj-$(CONFIG_KVM) += kvm.o
> diff --git a/arch/s390/kvm/gmap.c b/arch/s390/kvm/gmap.c
> new file mode 100644
> index 000000000000..a142bbbddc25
> --- /dev/null
> +++ b/arch/s390/kvm/gmap.c
> @@ -0,0 +1,183 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Guest memory management for KVM/s390
> + *
> + * Copyright IBM Corp. 2008, 2020, 2024
> + *
> + *    Author(s): Claudio Imbrenda <imbrenda@linux.ibm.com>
> + *               Martin Schwidefsky <schwidefsky@de.ibm.com>
> + *               David Hildenbrand <david@redhat.com>
> + *               Janosch Frank <frankja@linux.vnet.ibm.com>
> + */
> +
> +#include <linux/compiler.h>
> +#include <linux/kvm.h>
> +#include <linux/kvm_host.h>
> +#include <linux/pgtable.h>
> +#include <linux/pagemap.h>
> +
> +#include <asm/lowcore.h>
> +#include <asm/gmap.h>
> +#include <asm/uv.h>
> +
> +#include "gmap.h"
> +
> +/**
> + * should_export_before_import - Determine whether an export is needed
> + * before an import-like operation
> + * @uvcb: the Ultravisor control block of the UVC to be performed
> + * @mm: the mm of the process
> + *
> + * Returns whether an export is needed before every import-like operation.
> + * This is needed for shared pages, which don't trigger a secure storage
> + * exception when accessed from a different guest.
> + *
> + * Although considered as one, the Unpin Page UVC is not an actual import,
> + * so it is not affected.
> + *
> + * No export is needed also when there is only one protected VM, because the
> + * page cannot belong to the wrong VM in that case (there is no "other VM"
> + * it can belong to).
> + *
> + * Return: true if an export is needed before every import, otherwise false.
> + */
> +static bool should_export_before_import(struct uv_cb_header *uvcb, struct mm_struct *mm)
> +{
> +	/*
> +	 * The misc feature indicates, among other things, that importing a
> +	 * shared page from a different protected VM will automatically also
> +	 * transfer its ownership.
> +	 */
> +	if (uv_has_feature(BIT_UV_FEAT_MISC))
> +		return false;
> +	if (uvcb->cmd == UVC_CMD_UNPIN_PAGE_SHARED)
> +		return false;
> +	return atomic_read(&mm->context.protected_count) > 1;
> +}
> +
> +static int __gmap_make_secure(struct gmap *gmap, struct page *page, void *uvcb)
> +{
> +	struct folio *folio = page_folio(page);
> +	int rc;
> +
> +	/*
> +	 * Secure pages cannot be huge and userspace should not combine both.
> +	 * In case userspace does it anyway this will result in an -EFAULT for
> +	 * the unpack. The guest is thus never reaching secure mode. If
> +	 * userspace is playing dirty tricky with mapping huge pages later

s/tricky/tricks/

But the whole last sentence is a bit iffy.

> +	 * on this will result in a segmentation fault or in a -EFAULT return
> +	 * code from the KVM_RUN ioctl.
> +	 */
> +	if (folio_test_hugetlb(folio))
> +		return -EFAULT;
> +	if (folio_test_large(folio)) {
> +		mmap_read_unlock(gmap->mm);
> +		rc = uv_wiggle_folio(folio, true);
> +		mmap_read_lock(gmap->mm);

You could move the unlock to uv_wiggle_folio() and add a 
mmap_assert_locked() in front.

At least if you have no other users in upcoming series which don't need 
the unlock.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v1 04/13] KVM: s390: move pv gmap functions into kvm
  2025-01-15 12:48   ` Janosch Frank
@ 2025-01-15 12:59     ` Claudio Imbrenda
  2025-01-15 13:23       ` Janosch Frank
  0 siblings, 1 reply; 6+ messages in thread
From: Claudio Imbrenda @ 2025-01-15 12:59 UTC (permalink / raw)
  To: Janosch Frank
  Cc: kvm, linux-s390, borntraeger, schlameuss, david, willy, hca,
	svens, agordeev, gor, nrb, nsg

On Wed, 15 Jan 2025 13:48:47 +0100
Janosch Frank <frankja@linux.ibm.com> wrote:

[...]

> > +static int __gmap_make_secure(struct gmap *gmap, struct page *page, void *uvcb)
> > +{
> > +	struct folio *folio = page_folio(page);
> > +	int rc;
> > +
> > +	/*
> > +	 * Secure pages cannot be huge and userspace should not combine both.
> > +	 * In case userspace does it anyway this will result in an -EFAULT for
> > +	 * the unpack. The guest is thus never reaching secure mode. If
> > +	 * userspace is playing dirty tricky with mapping huge pages later  
> 
> s/tricky/tricks/
> 
> But the whole last sentence is a bit iffy.

hmm yes I'll reword it

> 
> > +	 * on this will result in a segmentation fault or in a -EFAULT return
> > +	 * code from the KVM_RUN ioctl.
> > +	 */
> > +	if (folio_test_hugetlb(folio))
> > +		return -EFAULT;
> > +	if (folio_test_large(folio)) {
> > +		mmap_read_unlock(gmap->mm);
> > +		rc = uv_wiggle_folio(folio, true);
> > +		mmap_read_lock(gmap->mm);  
> 
> You could move the unlock to uv_wiggle_folio() and add a 
> mmap_assert_locked() in front.

oh no, I don't want a function that drops a lock that has been acquired
outside of it.

by explicitly dropping and acquiring it, it's obvious what's going on,
and you can easily see that the lock is being dropped and re-acquired.

__gmap_destroy_page() does it, but it's called exactly in one spot,
namely gmap_destroy_page(), which is literally below it.

> 
> At least if you have no other users in upcoming series which don't need 
> the unlock.


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v1 04/13] KVM: s390: move pv gmap functions into kvm
  2025-01-15 12:59     ` Claudio Imbrenda
@ 2025-01-15 13:23       ` Janosch Frank
  0 siblings, 0 replies; 6+ messages in thread
From: Janosch Frank @ 2025-01-15 13:23 UTC (permalink / raw)
  To: Claudio Imbrenda
  Cc: kvm, linux-s390, borntraeger, schlameuss, david, willy, hca,
	svens, agordeev, gor, nrb, nsg

On 1/15/25 1:59 PM, Claudio Imbrenda wrote:
> On Wed, 15 Jan 2025 13:48:47 +0100
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
> [...]
> 
>>> +static int __gmap_make_secure(struct gmap *gmap, struct page *page, void *uvcb)
>>> +{
>>> +	struct folio *folio = page_folio(page);
>>> +	int rc;
>>> +
>>> +	/*
>>> +	 * Secure pages cannot be huge and userspace should not combine both.
>>> +	 * In case userspace does it anyway this will result in an -EFAULT for
>>> +	 * the unpack. The guest is thus never reaching secure mode. If
>>> +	 * userspace is playing dirty tricky with mapping huge pages later
>>
>> s/tricky/tricks/
>>
>> But the whole last sentence is a bit iffy.
> 
> hmm yes I'll reword it
> 
>>
>>> +	 * on this will result in a segmentation fault or in a -EFAULT return
>>> +	 * code from the KVM_RUN ioctl.
>>> +	 */
>>> +	if (folio_test_hugetlb(folio))
>>> +		return -EFAULT;
>>> +	if (folio_test_large(folio)) {
>>> +		mmap_read_unlock(gmap->mm);
>>> +		rc = uv_wiggle_folio(folio, true);
>>> +		mmap_read_lock(gmap->mm);
>>
>> You could move the unlock to uv_wiggle_folio() and add a
>> mmap_assert_locked() in front.
> 
> oh no, I don't want a function that drops a lock that has been acquired
> outside of it.
> 
> by explicitly dropping and acquiring it, it's obvious what's going on,
> and you can easily see that the lock is being dropped and re-acquired.
> 
> __gmap_destroy_page() does it, but it's called exactly in one spot,
> namely gmap_destroy_page(), which is literally below it.
> 

It's just very weird to to me to have the (un)lock dance the wrong way 
around when reading it. At first I assumed you made a mistake when 
remembering the context description in wiggle because I misread the 
unlock() as lock().

But maybe that's just me and I don't mind it too much :)

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2025-01-15 13:23 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-01-10 18:42 [PATCH v1 04/13] KVM: s390: move pv gmap functions into kvm kernel test robot
  -- strict thread matches above, loose matches on Subject: below --
2025-01-08 18:14 [PATCH v1 00/13] KVM: s390: Stop using page->index and other things Claudio Imbrenda
2025-01-08 18:14 ` [PATCH v1 04/13] KVM: s390: move pv gmap functions into kvm Claudio Imbrenda
2025-01-09 17:42   ` kernel test robot
2025-01-15 12:48   ` Janosch Frank
2025-01-15 12:59     ` Claudio Imbrenda
2025-01-15 13:23       ` Janosch Frank

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.