Re: [PATCH v2 03/15] KVM: s390: move pv gmap functions into kvm

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

On 1/16/25 12:33 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       | 196 +++++++++++++++++++++++++
>   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, 264 insertions(+), 254 deletions(-)
>   create mode 100644 arch/s390/kvm/gmap.c
>   create mode 100644 arch/s390/kvm/gmap.h

[...]

>   
> -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;

I'd expect busy for writeback and too many references since someone is 
referencing or working with the page.

But EBUSY for large mappings?
Also, the large mapping doesn't just vanish by waiting, does it?
You're actively splitting the mapping.

> + *         -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

How about adding the split into the name?
uv_wiggle_split

> + * @folio: the folio
> + * @split: whether to split a large folio

[...]

> +/**
> + * 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 plays dirty tricks and decides to map huge pages at a
> +	 * later point in time, it will receive a segmentation fault or
> +	 * KVM_RUN will return -EFAULT.
> +	 */
> +	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);
> +	}
> +
Maybe add something like:

/* 
  
  
  

  * We can race with someone making the folio large again until
  * we have locked the folio below.
  *
  * If we did, we will do the SIE entry/exit dance and end up
  * here again.
  */

> +	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;
> +}