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

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

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 :)