On Wed, 21 May 2025 17:30:00 +0200
Nina Schoetterl-Glausch <nsg@xxxxxxxxxxxxx> wrote:
> 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?
I don't know, _pte kinda sounds to me as the function would be taking a
pte as parameter
> >
> > >
> > > > +{
> > > > + 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?
if you mean removing mm/gmap.c, no. I would need to push the whole gmap
rewrite series, which is not ready yet.
if you mean removing the redundant functions... I guess I could
>
>
> [...]
