On Fri, 14 Feb 2025 11:27:15 +0100
David Hildenbrand <david@xxxxxxxxxx> wrote:
> On 14.02.25 11:17, Claudio Imbrenda wrote:
> > 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?)
> >
>
> Maybe Reported-by.
>
> > [...]
> >
> >>> @@ -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 think for the conversion we would still:
>
> GFN -> HVA
>
> Walk to the folio mapped at HVA, lock the PTE and perform the conversion.
>
> So even with memory notifiers, that should be fine, no?
yes
>
> So not necessarily "the old handling that we had before" but rather "the
> old way of looking up what's mapped and performing the conversion under
> the PTL".
ahhh, yes
>
> For me to fix the refcount freezing properly on top of your work, we'll
> need the PTL (esp. to exclude concurrent GUP-slow) etc.
let's discuss this off-list
