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

Janosch Frank
2025-01-15T13:23:23Z

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