Re: [PATCH v1 4/5] KVM: s390: refactor and split some gmap helpers

[Nina Schoetterl-Glausch <nsg@linux.ibm.com>]

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@linux.ibm.com> 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@linux.ibm.com>
> > > ---
> > >  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