[PATCH v1 0/2] KVM: s390: fix two newly introduced bugs

[PATCH v1 0/2] KVM: s390: fix two newly introduced bugs

[Claudio Imbrenda]

* fix race when making a page secure (hold pte lock again)
* fix issues when splitting folios (use split_huge_page_to_list_to_order())

This should fix the issues I have seen, which I think/hope are also the same
issues that David found.

Claudio Imbrenda (2):
  KVM: s390: fix issues when splitting folios
  KVM: s390: pv: fix race when making a page secure

 arch/s390/include/asm/gmap.h |  2 +-
 arch/s390/include/asm/uv.h   |  2 +-
 arch/s390/kernel/uv.c        | 19 +++++++++++++++++--
 arch/s390/kvm/gmap.c         | 16 ++++++++++------
 arch/s390/mm/gmap.c          | 11 ++++++++---
 5 files changed, 37 insertions(+), 13 deletions(-)

-- 
2.48.1

[PATCH v1 1/2] KVM: s390: fix issues when splitting folios

[Claudio Imbrenda]

When splitting a folio with split_folio(), the extra reference on the
folio gets assigned to the first page of the old folio. Use
split_huge_page_to_list_to_order() instead, which transfers the extra
reference to a specified page.

Fixes: 5cbe24350b7d ("KVM: s390: move pv gmap functions into kvm")
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 arch/s390/include/asm/gmap.h |  2 +-
 arch/s390/kvm/gmap.c         |  4 ++--
 arch/s390/mm/gmap.c          | 11 ++++++++---
 3 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/arch/s390/include/asm/gmap.h b/arch/s390/include/asm/gmap.h
index 4e73ef46d4b2..563df4d8ba90 100644
--- a/arch/s390/include/asm/gmap.h
+++ b/arch/s390/include/asm/gmap.h
@@ -139,7 +139,7 @@ int s390_replace_asce(struct gmap *gmap);
 void s390_uv_destroy_pfns(unsigned long count, unsigned long *pfns);
 int __s390_uv_destroy_range(struct mm_struct *mm, unsigned long start,
 			    unsigned long end, bool interruptible);
-int kvm_s390_wiggle_split_folio(struct mm_struct *mm, struct folio *folio, bool split);
+int kvm_s390_wiggle_split_folio(struct mm_struct *mm, struct page *page, bool split);
 unsigned long *gmap_table_walk(struct gmap *gmap, unsigned long gaddr, int level);
 
 /**
diff --git a/arch/s390/kvm/gmap.c b/arch/s390/kvm/gmap.c
index 02adf151d4de..fc4d490d25a2 100644
--- a/arch/s390/kvm/gmap.c
+++ b/arch/s390/kvm/gmap.c
@@ -72,7 +72,7 @@ static int __gmap_make_secure(struct gmap *gmap, struct page *page, void *uvcb)
 		return -EFAULT;
 	if (folio_test_large(folio)) {
 		mmap_read_unlock(gmap->mm);
-		rc = kvm_s390_wiggle_split_folio(gmap->mm, folio, true);
+		rc = kvm_s390_wiggle_split_folio(gmap->mm, page, true);
 		mmap_read_lock(gmap->mm);
 		if (rc)
 			return rc;
@@ -100,7 +100,7 @@ static int __gmap_make_secure(struct gmap *gmap, struct page *page, void *uvcb)
 	/* The folio has too many references, try to shake some off */
 	if (rc == -EBUSY) {
 		mmap_read_unlock(gmap->mm);
-		kvm_s390_wiggle_split_folio(gmap->mm, folio, false);
+		kvm_s390_wiggle_split_folio(gmap->mm, page, false);
 		mmap_read_lock(gmap->mm);
 		return -EAGAIN;
 	}
diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
index 94d927785800..8117597419d3 100644
--- a/arch/s390/mm/gmap.c
+++ b/arch/s390/mm/gmap.c
@@ -2630,14 +2630,15 @@ EXPORT_SYMBOL_GPL(s390_replace_asce);
 /**
  * kvm_s390_wiggle_split_folio() - try to drain extra references to a folio and optionally split
  * @mm:    the mm containing the folio to work on
- * @folio: the folio
+ * @page:  one of the pages of the folio that needs to be split
  * @split: whether to split a large folio
  *
  * Context: Must be called while holding an extra reference to the folio;
  *          the mm lock should not be held.
  */
-int kvm_s390_wiggle_split_folio(struct mm_struct *mm, struct folio *folio, bool split)
+int kvm_s390_wiggle_split_folio(struct mm_struct *mm, struct page *page, bool split)
 {
+	struct folio *folio = page_folio(page);
 	int rc;
 
 	lockdep_assert_not_held(&mm->mmap_lock);
@@ -2645,7 +2646,11 @@ int kvm_s390_wiggle_split_folio(struct mm_struct *mm, struct folio *folio, bool
 	lru_add_drain_all();
 	if (split) {
 		folio_lock(folio);
-		rc = split_folio(folio);
+		rc = min_order_for_split(folio);
+		if (rc > 0)
+			rc = -EINVAL;
+		if (!rc)
+			rc = split_huge_page_to_list_to_order(page, NULL, 0);
 		folio_unlock(folio);
 
 		if (rc != -EBUSY)
-- 
2.48.1

[PATCH v1 2/2] KVM: s390: pv: fix race when making a page secure

[Claudio Imbrenda]

Holding the pte lock for the page that is being converted to secure is
needed to avoid races. A previous commit removed the locking, which
caused issues. Fix by locking the pte again.

Fixes: 5cbe24350b7d ("KVM: s390: move pv gmap functions into kvm")
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 arch/s390/include/asm/uv.h |  2 +-
 arch/s390/kernel/uv.c      | 19 +++++++++++++++++--
 arch/s390/kvm/gmap.c       | 12 ++++++++----
 3 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/arch/s390/include/asm/uv.h b/arch/s390/include/asm/uv.h
index b11f5b6d0bd1..46fb0ef6f984 100644
--- a/arch/s390/include/asm/uv.h
+++ b/arch/s390/include/asm/uv.h
@@ -631,7 +631,7 @@ int uv_pin_shared(unsigned long paddr);
 int uv_destroy_folio(struct folio *folio);
 int uv_destroy_pte(pte_t pte);
 int uv_convert_from_secure_pte(pte_t pte);
-int make_folio_secure(struct folio *folio, struct uv_cb_header *uvcb);
+int make_hva_secure(struct mm_struct *mm, unsigned long hva, struct uv_cb_header *uvcb);
 int uv_convert_from_secure(unsigned long paddr);
 int uv_convert_from_secure_folio(struct folio *folio);
 
diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c
index 9f05df2da2f7..de3c092da7b9 100644
--- a/arch/s390/kernel/uv.c
+++ b/arch/s390/kernel/uv.c
@@ -245,7 +245,7 @@ static int expected_folio_refs(struct folio *folio)
  * 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)
+static int __make_folio_secure(struct folio *folio, unsigned long hva, struct uv_cb_header *uvcb)
 {
 	int expected, cc = 0;
 
@@ -277,7 +277,22 @@ 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);
+
+int make_hva_secure(struct mm_struct *mm, unsigned long hva, struct uv_cb_header *uvcb)
+{
+	spinlock_t *ptelock;
+	pte_t *ptep;
+	int rc;
+
+	ptep = get_locked_pte(mm, hva, &ptelock);
+	if (!ptep)
+		return -ENXIO;
+	rc = __make_folio_secure(page_folio(pte_page(*ptep)), hva, uvcb);
+	pte_unmap_unlock(ptep, ptelock);
+
+	return rc;
+}
+EXPORT_SYMBOL_GPL(make_hva_secure);
 
 /*
  * To be called with the folio locked or with an extra reference! This will
diff --git a/arch/s390/kvm/gmap.c b/arch/s390/kvm/gmap.c
index fc4d490d25a2..e56c0ab5fec7 100644
--- a/arch/s390/kvm/gmap.c
+++ b/arch/s390/kvm/gmap.c
@@ -55,7 +55,7 @@ static bool should_export_before_import(struct uv_cb_header *uvcb, struct mm_str
 	return atomic_read(&mm->context.protected_count) > 1;
 }
 
-static int __gmap_make_secure(struct gmap *gmap, struct page *page, void *uvcb)
+static int __gmap_make_secure(struct gmap *gmap, struct page *page, unsigned long hva, void *uvcb)
 {
 	struct folio *folio = page_folio(page);
 	int rc;
@@ -83,7 +83,7 @@ static int __gmap_make_secure(struct gmap *gmap, struct page *page, void *uvcb)
 		return -EAGAIN;
 	if (should_export_before_import(uvcb, gmap->mm))
 		uv_convert_from_secure(folio_to_phys(folio));
-	rc = make_folio_secure(folio, uvcb);
+	rc = make_hva_secure(gmap->mm, hva, uvcb);
 	folio_unlock(folio);
 
 	/*
@@ -120,6 +120,7 @@ static int __gmap_make_secure(struct gmap *gmap, struct page *page, void *uvcb)
 int gmap_make_secure(struct gmap *gmap, unsigned long gaddr, void *uvcb)
 {
 	struct kvm *kvm = gmap->private;
+	unsigned long vmaddr;
 	struct page *page;
 	int rc = 0;
 
@@ -127,8 +128,11 @@ int gmap_make_secure(struct gmap *gmap, unsigned long gaddr, void *uvcb)
 
 	page = gfn_to_page(kvm, gpa_to_gfn(gaddr));
 	mmap_read_lock(gmap->mm);
-	if (page)
-		rc = __gmap_make_secure(gmap, page, uvcb);
+	vmaddr = gfn_to_hva(gmap->private, gpa_to_gfn(gaddr));
+	if (kvm_is_error_hva(vmaddr))
+		rc = -ENXIO;
+	if (!rc && page)
+		rc = __gmap_make_secure(gmap, page, vmaddr, uvcb);
 	kvm_release_page_clean(page);
 	mmap_read_unlock(gmap->mm);
 
-- 
2.48.1

Re: [PATCH v1 2/2] KVM: s390: pv: fix race when making a page secure

[David Hildenbrand]

On 13.02.25 21:07, Claudio Imbrenda wrote:
> Holding the pte lock for the page that is being converted to secure is
> needed to avoid races. A previous commit removed the locking, which
> caused issues. Fix by locking the pte again.
> 
> Fixes: 5cbe24350b7d ("KVM: s390: move pv gmap functions into kvm")

If you found this because of my report about the changed locking, 
consider adding a Suggested-by / Reported-y.

> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
>   arch/s390/include/asm/uv.h |  2 +-
>   arch/s390/kernel/uv.c      | 19 +++++++++++++++++--
>   arch/s390/kvm/gmap.c       | 12 ++++++++----
>   3 files changed, 26 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/s390/include/asm/uv.h b/arch/s390/include/asm/uv.h
> index b11f5b6d0bd1..46fb0ef6f984 100644
> --- a/arch/s390/include/asm/uv.h
> +++ b/arch/s390/include/asm/uv.h
> @@ -631,7 +631,7 @@ int uv_pin_shared(unsigned long paddr);
>   int uv_destroy_folio(struct folio *folio);
>   int uv_destroy_pte(pte_t pte);
>   int uv_convert_from_secure_pte(pte_t pte);
> -int make_folio_secure(struct folio *folio, struct uv_cb_header *uvcb);
> +int make_hva_secure(struct mm_struct *mm, unsigned long hva, struct uv_cb_header *uvcb);
>   int uv_convert_from_secure(unsigned long paddr);
>   int uv_convert_from_secure_folio(struct folio *folio);
>   
> diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c
> index 9f05df2da2f7..de3c092da7b9 100644
> --- a/arch/s390/kernel/uv.c
> +++ b/arch/s390/kernel/uv.c
> @@ -245,7 +245,7 @@ static int expected_folio_refs(struct folio *folio)
>    * 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)
> +static int __make_folio_secure(struct folio *folio, unsigned long hva, struct uv_cb_header *uvcb)
>   {
>   	int expected, cc = 0;
>   
> @@ -277,7 +277,22 @@ 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);
> +
> +int make_hva_secure(struct mm_struct *mm, unsigned long hva, struct uv_cb_header *uvcb)
> +{
> +	spinlock_t *ptelock;
> +	pte_t *ptep;
> +	int rc;
> +
> +	ptep = get_locked_pte(mm, hva, &ptelock);
> +	if (!ptep)
> +		return -ENXIO;
> +	rc = __make_folio_secure(page_folio(pte_page(*ptep)), hva, uvcb);
> +	pte_unmap_unlock(ptep, ptelock);
> +
> +	return rc;
> +}
> +EXPORT_SYMBOL_GPL(make_hva_secure);
>   
>   /*
>    * To be called with the folio locked or with an extra reference! This will
> diff --git a/arch/s390/kvm/gmap.c b/arch/s390/kvm/gmap.c
> index fc4d490d25a2..e56c0ab5fec7 100644
> --- a/arch/s390/kvm/gmap.c
> +++ b/arch/s390/kvm/gmap.c
> @@ -55,7 +55,7 @@ static bool should_export_before_import(struct uv_cb_header *uvcb, struct mm_str
>   	return atomic_read(&mm->context.protected_count) > 1;
>   }
>   
> -static int __gmap_make_secure(struct gmap *gmap, struct page *page, void *uvcb)
> +static int __gmap_make_secure(struct gmap *gmap, struct page *page, unsigned long hva, void *uvcb)
>   {
>   	struct folio *folio = page_folio(page);
>   	int rc;
> @@ -83,7 +83,7 @@ static int __gmap_make_secure(struct gmap *gmap, struct page *page, void *uvcb)
>   		return -EAGAIN;
>   	if (should_export_before_import(uvcb, gmap->mm))
>   		uv_convert_from_secure(folio_to_phys(folio));
> -	rc = make_folio_secure(folio, uvcb);
> +	rc = make_hva_secure(gmap->mm, hva, uvcb);
>   	folio_unlock(folio);
>   
>   	/*
> @@ -120,6 +120,7 @@ static int __gmap_make_secure(struct gmap *gmap, struct page *page, void *uvcb)
>   int gmap_make_secure(struct gmap *gmap, unsigned long gaddr, void *uvcb)
>   {
>   	struct kvm *kvm = gmap->private;
> +	unsigned long vmaddr;
>   	struct page *page;
>   	int rc = 0;
>   
> @@ -127,8 +128,11 @@ int gmap_make_secure(struct gmap *gmap, unsigned long gaddr, void *uvcb)
>   
>   	page = gfn_to_page(kvm, gpa_to_gfn(gaddr));
>   	mmap_read_lock(gmap->mm);
> -	if (page)
> -		rc = __gmap_make_secure(gmap, page, uvcb);
> +	vmaddr = gfn_to_hva(gmap->private, gpa_to_gfn(gaddr));
> +	if (kvm_is_error_hva(vmaddr))
> +		rc = -ENXIO;
> +	if (!rc && page)
> +		rc = __gmap_make_secure(gmap, page, vmaddr, uvcb);
>   	kvm_release_page_clean(page);
>   	mmap_read_unlock(gmap->mm);
>   

You effectively make the code more complicated and inefficient than 
before. Now you effectively walk the page table twice in the common 
small-folio case ...

Can we just go back to the old handling that we had before here?

-- 
Cheers,

David / dhildenb

Re: [PATCH v1 1/2] KVM: s390: fix issues when splitting folios

[David Hildenbrand]

> +	struct folio *folio = page_folio(page);
>   	int rc;
>   
>   	lockdep_assert_not_held(&mm->mmap_lock);
> @@ -2645,7 +2646,11 @@ int kvm_s390_wiggle_split_folio(struct mm_struct *mm, struct folio *folio, bool
>   	lru_add_drain_all();
>   	if (split) {
>   		folio_lock(folio);
> -		rc = split_folio(folio);
> +		rc = min_order_for_split(folio);
> +		if (rc > 0)
> +			rc = -EINVAL;
> +		if (!rc)
> +			rc = split_huge_page_to_list_to_order(page, NULL, 0);

split_huge_page() ?

But see my reply to #2. Likely we should just undo the refactorings you 
added while moving the code.

-- 
Cheers,

David / dhildenb

Re: [PATCH v1 2/2] KVM: s390: pv: fix race when making a page secure

[David Hildenbrand]

On 13.02.25 21:16, David Hildenbrand wrote:
> On 13.02.25 21:07, Claudio Imbrenda wrote:
>> Holding the pte lock for the page that is being converted to secure is
>> needed to avoid races. A previous commit removed the locking, which
>> caused issues. Fix by locking the pte again.
>>
>> Fixes: 5cbe24350b7d ("KVM: s390: move pv gmap functions into kvm")
> 
> If you found this because of my report about the changed locking,
> consider adding a Suggested-by / Reported-y.
> 
>> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
>> ---
>>    arch/s390/include/asm/uv.h |  2 +-
>>    arch/s390/kernel/uv.c      | 19 +++++++++++++++++--
>>    arch/s390/kvm/gmap.c       | 12 ++++++++----
>>    3 files changed, 26 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/s390/include/asm/uv.h b/arch/s390/include/asm/uv.h
>> index b11f5b6d0bd1..46fb0ef6f984 100644
>> --- a/arch/s390/include/asm/uv.h
>> +++ b/arch/s390/include/asm/uv.h
>> @@ -631,7 +631,7 @@ int uv_pin_shared(unsigned long paddr);
>>    int uv_destroy_folio(struct folio *folio);
>>    int uv_destroy_pte(pte_t pte);
>>    int uv_convert_from_secure_pte(pte_t pte);
>> -int make_folio_secure(struct folio *folio, struct uv_cb_header *uvcb);
>> +int make_hva_secure(struct mm_struct *mm, unsigned long hva, struct uv_cb_header *uvcb);
>>    int uv_convert_from_secure(unsigned long paddr);
>>    int uv_convert_from_secure_folio(struct folio *folio);
>>    
>> diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c
>> index 9f05df2da2f7..de3c092da7b9 100644
>> --- a/arch/s390/kernel/uv.c
>> +++ b/arch/s390/kernel/uv.c
>> @@ -245,7 +245,7 @@ static int expected_folio_refs(struct folio *folio)
>>     * 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)
>> +static int __make_folio_secure(struct folio *folio, unsigned long hva, struct uv_cb_header *uvcb)
>>    {
>>    	int expected, cc = 0;
>>    
>> @@ -277,7 +277,22 @@ 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);
>> +
>> +int make_hva_secure(struct mm_struct *mm, unsigned long hva, struct uv_cb_header *uvcb)
>> +{
>> +	spinlock_t *ptelock;
>> +	pte_t *ptep;
>> +	int rc;
>> +
>> +	ptep = get_locked_pte(mm, hva, &ptelock);
>> +	if (!ptep)
>> +		return -ENXIO;
>> +	rc = __make_folio_secure(page_folio(pte_page(*ptep)), hva, uvcb);
>> +	pte_unmap_unlock(ptep, ptelock);
>> +
>> +	return rc;
>> +}
>> +EXPORT_SYMBOL_GPL(make_hva_secure);
>>    
>>    /*
>>     * To be called with the folio locked or with an extra reference! This will
>> diff --git a/arch/s390/kvm/gmap.c b/arch/s390/kvm/gmap.c
>> index fc4d490d25a2..e56c0ab5fec7 100644
>> --- a/arch/s390/kvm/gmap.c
>> +++ b/arch/s390/kvm/gmap.c
>> @@ -55,7 +55,7 @@ static bool should_export_before_import(struct uv_cb_header *uvcb, struct mm_str
>>    	return atomic_read(&mm->context.protected_count) > 1;
>>    }
>>    
>> -static int __gmap_make_secure(struct gmap *gmap, struct page *page, void *uvcb)
>> +static int __gmap_make_secure(struct gmap *gmap, struct page *page, unsigned long hva, void *uvcb)
>>    {
>>    	struct folio *folio = page_folio(page);
>>    	int rc;
>> @@ -83,7 +83,7 @@ static int __gmap_make_secure(struct gmap *gmap, struct page *page, void *uvcb)
>>    		return -EAGAIN;
>>    	if (should_export_before_import(uvcb, gmap->mm))
>>    		uv_convert_from_secure(folio_to_phys(folio));
>> -	rc = make_folio_secure(folio, uvcb);
>> +	rc = make_hva_secure(gmap->mm, hva, uvcb);
>>    	folio_unlock(folio);
>>    
>>    	/*
>> @@ -120,6 +120,7 @@ static int __gmap_make_secure(struct gmap *gmap, struct page *page, void *uvcb)
>>    int gmap_make_secure(struct gmap *gmap, unsigned long gaddr, void *uvcb)
>>    {
>>    	struct kvm *kvm = gmap->private;
>> +	unsigned long vmaddr;
>>    	struct page *page;
>>    	int rc = 0;
>>    
>> @@ -127,8 +128,11 @@ int gmap_make_secure(struct gmap *gmap, unsigned long gaddr, void *uvcb)
>>    
>>    	page = gfn_to_page(kvm, gpa_to_gfn(gaddr));
>>    	mmap_read_lock(gmap->mm);
>> -	if (page)
>> -		rc = __gmap_make_secure(gmap, page, uvcb);
>> +	vmaddr = gfn_to_hva(gmap->private, gpa_to_gfn(gaddr));
>> +	if (kvm_is_error_hva(vmaddr))
>> +		rc = -ENXIO;
>> +	if (!rc && page)
>> +		rc = __gmap_make_secure(gmap, page, vmaddr, uvcb);
>>    	kvm_release_page_clean(page);
>>    	mmap_read_unlock(gmap->mm);
>>    
> 
> You effectively make the code more complicated and inefficient than
> before. Now you effectively walk the page table twice in the common
> small-folio case ...
> 
> Can we just go back to the old handling that we had before here?

I'll note that there is still the possibility for a different race: 
nothing guarantees that the page you looked up using gfn_to_hva() will 
still be mapped when you perform the get_locked_pte(). Not sure what 
would happen if we would have a different page mapped.

You could re-verify it is still there, but then, doing two page table 
walks is still more than required in the common case where we can just 
perform the conversion.

-- 
Cheers,

David / dhildenb

Re: [PATCH v1 1/2] KVM: s390: fix issues when splitting folios

[Claudio Imbrenda]

On Thu, 13 Feb 2025 21:17:10 +0100
David Hildenbrand <david@redhat.com> wrote:

> > +	struct folio *folio = page_folio(page);
> >   	int rc;
> >   
> >   	lockdep_assert_not_held(&mm->mmap_lock);
> > @@ -2645,7 +2646,11 @@ int kvm_s390_wiggle_split_folio(struct mm_struct *mm, struct folio *folio, bool
> >   	lru_add_drain_all();
> >   	if (split) {
> >   		folio_lock(folio);
> > -		rc = split_folio(folio);
> > +		rc = min_order_for_split(folio);
> > +		if (rc > 0)
> > +			rc = -EINVAL;
> > +		if (!rc)
> > +			rc = split_huge_page_to_list_to_order(page, NULL, 0);  
> 
> split_huge_page() ?

ah, yes

> 
> But see my reply to #2. Likely we should just undo the refactorings you 
> added while moving the code.
> 

Re: [PATCH v1 2/2] KVM: s390: pv: fix race when making a page secure

[Claudio Imbrenda]

On Thu, 13 Feb 2025 21:16:03 +0100
David Hildenbrand <david@redhat.com> wrote:

> On 13.02.25 21:07, Claudio Imbrenda wrote:
> > Holding the pte lock for the page that is being converted to secure is
> > needed to avoid races. A previous commit removed the locking, which
> > caused issues. Fix by locking the pte again.
> > 
> > Fixes: 5cbe24350b7d ("KVM: s390: move pv gmap functions into kvm")  
> 
> If you found this because of my report about the changed locking, 
> consider adding a Suggested-by / Reported-y.

yes, sorry; I sent the patch in haste and forgot. Which one would you
prefer (or both?)

[...]

> > @@ -127,8 +128,11 @@ int gmap_make_secure(struct gmap *gmap, unsigned long gaddr, void *uvcb)
> >   
> >   	page = gfn_to_page(kvm, gpa_to_gfn(gaddr));
> >   	mmap_read_lock(gmap->mm);
> > -	if (page)
> > -		rc = __gmap_make_secure(gmap, page, uvcb);
> > +	vmaddr = gfn_to_hva(gmap->private, gpa_to_gfn(gaddr));
> > +	if (kvm_is_error_hva(vmaddr))
> > +		rc = -ENXIO;
> > +	if (!rc && page)
> > +		rc = __gmap_make_secure(gmap, page, vmaddr, uvcb);
> >   	kvm_release_page_clean(page);
> >   	mmap_read_unlock(gmap->mm);
> >     
> 
> You effectively make the code more complicated and inefficient than 
> before. Now you effectively walk the page table twice in the common 
> small-folio case ...

I think in every case, but see below

> 
> Can we just go back to the old handling that we had before here?
> 

I'd rather not, this is needed to prepare for the next series (for
6.15) in a couple of weeks, where gmap gets completely removed from
s390/mm, and gmap dat tables will not share ptes with userspace anymore
(i.e. we will use mmu_notifiers, like all other archs)

I will remove the double walk, though, since there is no reason to keep
it in there

Re: [PATCH v1 2/2] KVM: s390: pv: fix race when making a page secure

[David Hildenbrand]

On 14.02.25 11:17, Claudio Imbrenda wrote:
> On Thu, 13 Feb 2025 21:16:03 +0100
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 13.02.25 21:07, Claudio Imbrenda wrote:
>>> Holding the pte lock for the page that is being converted to secure is
>>> needed to avoid races. A previous commit removed the locking, which
>>> caused issues. Fix by locking the pte again.
>>>
>>> Fixes: 5cbe24350b7d ("KVM: s390: move pv gmap functions into kvm")
>>
>> If you found this because of my report about the changed locking,
>> consider adding a Suggested-by / Reported-y.
> 
> yes, sorry; I sent the patch in haste and forgot. Which one would you
> prefer (or both?)
> 

Maybe Reported-by.

> [...]
> 
>>> @@ -127,8 +128,11 @@ int gmap_make_secure(struct gmap *gmap, unsigned long gaddr, void *uvcb)
>>>    
>>>    	page = gfn_to_page(kvm, gpa_to_gfn(gaddr));
>>>    	mmap_read_lock(gmap->mm);
>>> -	if (page)
>>> -		rc = __gmap_make_secure(gmap, page, uvcb);
>>> +	vmaddr = gfn_to_hva(gmap->private, gpa_to_gfn(gaddr));
>>> +	if (kvm_is_error_hva(vmaddr))
>>> +		rc = -ENXIO;
>>> +	if (!rc && page)
>>> +		rc = __gmap_make_secure(gmap, page, vmaddr, uvcb);
>>>    	kvm_release_page_clean(page);
>>>    	mmap_read_unlock(gmap->mm);
>>>      
>>
>> You effectively make the code more complicated and inefficient than
>> before. Now you effectively walk the page table twice in the common
>> small-folio case ...
> 
> I think in every case, but see below
> 
>>
>> Can we just go back to the old handling that we had before here?
>>
> 
> I'd rather not, this is needed to prepare for the next series (for
> 6.15) in a couple of weeks, where gmap gets completely removed from
> s390/mm, and gmap dat tables will not share ptes with userspace anymore
> (i.e. we will use mmu_notifiers, like all other archs)

I think for the conversion we would still:

GFN -> HVA

Walk to the folio mapped at HVA, lock the PTE and perform the conversion.

So even with memory notifiers, that should be fine, no?

So not necessarily "the old handling that we had before" but rather "the 
old way of looking up what's mapped and performing the conversion under 
the PTL".

For me to fix the refcount freezing properly on top of your work, we'll 
need the PTL (esp. to exclude concurrent GUP-slow) etc.

-- 
Cheers,

David / dhildenb

Re: [PATCH v1 2/2] KVM: s390: pv: fix race when making a page secure

[Claudio Imbrenda]

On Fri, 14 Feb 2025 11:27:15 +0100
David Hildenbrand <david@redhat.com> wrote:

> On 14.02.25 11:17, Claudio Imbrenda wrote:
> > On Thu, 13 Feb 2025 21:16:03 +0100
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> On 13.02.25 21:07, Claudio Imbrenda wrote:  
> >>> Holding the pte lock for the page that is being converted to secure is
> >>> needed to avoid races. A previous commit removed the locking, which
> >>> caused issues. Fix by locking the pte again.
> >>>
> >>> Fixes: 5cbe24350b7d ("KVM: s390: move pv gmap functions into kvm")  
> >>
> >> If you found this because of my report about the changed locking,
> >> consider adding a Suggested-by / Reported-y.  
> > 
> > yes, sorry; I sent the patch in haste and forgot. Which one would you
> > prefer (or both?)
> >   
> 
> Maybe Reported-by.
> 
> > [...]
> >   
> >>> @@ -127,8 +128,11 @@ int gmap_make_secure(struct gmap *gmap, unsigned long gaddr, void *uvcb)
> >>>    
> >>>    	page = gfn_to_page(kvm, gpa_to_gfn(gaddr));
> >>>    	mmap_read_lock(gmap->mm);
> >>> -	if (page)
> >>> -		rc = __gmap_make_secure(gmap, page, uvcb);
> >>> +	vmaddr = gfn_to_hva(gmap->private, gpa_to_gfn(gaddr));
> >>> +	if (kvm_is_error_hva(vmaddr))
> >>> +		rc = -ENXIO;
> >>> +	if (!rc && page)
> >>> +		rc = __gmap_make_secure(gmap, page, vmaddr, uvcb);
> >>>    	kvm_release_page_clean(page);
> >>>    	mmap_read_unlock(gmap->mm);
> >>>        
> >>
> >> You effectively make the code more complicated and inefficient than
> >> before. Now you effectively walk the page table twice in the common
> >> small-folio case ...  
> > 
> > I think in every case, but see below
> >   
> >>
> >> Can we just go back to the old handling that we had before here?
> >>  
> > 
> > I'd rather not, this is needed to prepare for the next series (for
> > 6.15) in a couple of weeks, where gmap gets completely removed from
> > s390/mm, and gmap dat tables will not share ptes with userspace anymore
> > (i.e. we will use mmu_notifiers, like all other archs)  
> 
> I think for the conversion we would still:
> 
> GFN -> HVA
> 
> Walk to the folio mapped at HVA, lock the PTE and perform the conversion.
> 
> So even with memory notifiers, that should be fine, no?

yes

> 
> So not necessarily "the old handling that we had before" but rather "the 
> old way of looking up what's mapped and performing the conversion under 
> the PTL".

ahhh, yes

> 
> For me to fix the refcount freezing properly on top of your work, we'll 
> need the PTL (esp. to exclude concurrent GUP-slow) etc.

let's discuss this off-list