On Thu, 13 Feb 2025 21:16:03 +0100
David Hildenbrand <david@xxxxxxxxxx> wrote:
> On 13.02.25 21:07, Claudio Imbrenda wrote:
> > Holding the pte lock for the page that is being converted to secure is
> > needed to avoid races. A previous commit removed the locking, which
> > caused issues. Fix by locking the pte again.
> >
> > Fixes: 5cbe24350b7d ("KVM: s390: move pv gmap functions into kvm")
>
> If you found this because of my report about the changed locking,
> consider adding a Suggested-by / Reported-y.
yes, sorry; I sent the patch in haste and forgot. Which one would you
prefer (or both?)
[...]
> > @@ -127,8 +128,11 @@ int gmap_make_secure(struct gmap *gmap, unsigned long gaddr, void *uvcb)
> >
> > page = gfn_to_page(kvm, gpa_to_gfn(gaddr));
> > mmap_read_lock(gmap->mm);
> > - if (page)
> > - rc = __gmap_make_secure(gmap, page, uvcb);
> > + vmaddr = gfn_to_hva(gmap->private, gpa_to_gfn(gaddr));
> > + if (kvm_is_error_hva(vmaddr))
> > + rc = -ENXIO;
> > + if (!rc && page)
> > + rc = __gmap_make_secure(gmap, page, vmaddr, uvcb);
> > kvm_release_page_clean(page);
> > mmap_read_unlock(gmap->mm);
> >
>
> You effectively make the code more complicated and inefficient than
> before. Now you effectively walk the page table twice in the common
> small-folio case ...
I think in every case, but see below
>
> Can we just go back to the old handling that we had before here?
>
I'd rather not, this is needed to prepare for the next series (for
6.15) in a couple of weeks, where gmap gets completely removed from
s390/mm, and gmap dat tables will not share ptes with userspace anymore
(i.e. we will use mmu_notifiers, like all other archs)
I will remove the double walk, though, since there is no reason to keep
it in there
