On Wed, 2025-05-21 at 17:19 +0200, Claudio Imbrenda wrote:
> On Wed, 21 May 2025 16:55:18 +0200
> Nina Schoetterl-Glausch <nsg@xxxxxxxxxxxxx> wrote:
>
> > On Wed, 2025-05-14 at 18:38 +0200, Claudio Imbrenda wrote:
> > > Refactor some gmap functions; move the implementation into a separate
> > > file with only helper functions. The new helper functions work on vm
> > > addresses, leaving all gmap logic in the gmap functions, which mostly
> > > become just wrappers.
> > >
> > > The whole gmap handling is going to be moved inside KVM soon, but the
> > > helper functions need to touch core mm functions, and thus need to
> > > stay in the core of kernel.
> > >
> > > Signed-off-by: Claudio Imbrenda <imbrenda@xxxxxxxxxxxxx>
> > > ---
> > > MAINTAINERS | 2 +
> > > arch/s390/include/asm/gmap_helpers.h | 18 ++
> > > arch/s390/kvm/diag.c | 11 +-
> > > arch/s390/kvm/kvm-s390.c | 3 +-
> > > arch/s390/mm/Makefile | 2 +
> > > arch/s390/mm/gmap.c | 46 ++---
> > > arch/s390/mm/gmap_helpers.c | 266 +++++++++++++++++++++++++++
> > > 7 files changed, 307 insertions(+), 41 deletions(-)
> > > create mode 100644 arch/s390/include/asm/gmap_helpers.h
> > > create mode 100644 arch/s390/mm/gmap_helpers.c
> > >
[...]
> > > +void __gmap_helper_zap_one(struct mm_struct *mm, unsigned long vmaddr)
> >
> > __gmap_helper_zap_mapping_pte ?
>
> but I'm not taking a pte as parameter
The pte being zapped is the one mapping vmaddr, right?
>
> >
> > > +{
> > > + struct vm_area_struct *vma;
> > > + spinlock_t *ptl;
> > > + pte_t *ptep;
> > > +
> > > + mmap_assert_locked(mm);
> > > +
> > > + /* Find the vm address for the guest address */
> > > + vma = vma_lookup(mm, vmaddr);
> > > + if (!vma || is_vm_hugetlb_page(vma))
> > > + return;
> > > +
> > > + /* Get pointer to the page table entry */
> > > + ptep = get_locked_pte(mm, vmaddr, &ptl);
> > > + if (!likely(ptep))
> >
> > if (unlikely(!ptep)) reads nicer to me.
>
> ok
>
> >
> > > + return;
> > > + if (pte_swap(*ptep))
> > > + ptep_zap_swap_entry(mm, pte_to_swp_entry(*ptep));
> > > + pte_unmap_unlock(ptep, ptl);
> > > +}
> > > +EXPORT_SYMBOL_GPL(__gmap_helper_zap_one);
> >
> > Looks reasonable, but I'm not well versed enough in mm code to evaluate
> > that with confidence.
> >
> > > +
> > > +void __gmap_helper_discard(struct mm_struct *mm, unsigned long vmaddr, unsigned long end)
> >
> > Maybe call this gmap_helper_discard_nolock or something.
>
> maybe __gmap_helper_discard_unlocked?
>
> the __ prefix often implies lack of locking
_nolock *definitely* implies it :P
[...]
> >
> > The stuff below is from arch/s390/mm/gmap.c right?
> > Are you going to delete it from there?
>
> not in this series, but the next series will remove mm/gmap.c altogether
Can't you do it with this one?
[...]
--
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Wolfgang Wendt
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294
