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

[Claudio Imbrenda <imbrenda@linux.ibm.com>]

On Fri, 17 Jan 2025 14:38:08 +0100
Janosch Frank <frankja@linux.ibm.com> wrote:

> 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>
> > ---  
> 
> Thanks git, this is close to unreadable
> 
> >   /**
> > - * 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)  
> 
> Should I expect a drop of references to also split THPs?

no, that's why we have the @split parameter

> Seems a bit odd to me but I do not know a lot about folios.
> 
> >   {
> > -	/*
> > -	 * 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;
> > -}
> > -  
> 
> [...]
> 
> > -/*
> > - * 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);  
> 
> Add an assert_not_held for the mm mutex above 
> "folio_wait_writeback(folio);"?

will do

[...]

> > +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);
> > +	}
> > +
> > +	rc = -EAGAIN;
> > +	if (folio_trylock(folio)) {  
> 
> If you test for !folio_trylock() you could do an early return, no?

true

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