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

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

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.