Re: [PATCH v2 10/20] KVM: s390: KVM page table management functions: walks

[Claudio Imbrenda <imbrenda@linux.ibm.com>]

On Wed, 17 Sep 2025 14:55:29 +0200
Heiko Carstens <hca@linux.ibm.com> wrote:

> On Wed, Sep 10, 2025 at 08:07:36PM +0200, Claudio Imbrenda wrote:
> > Add page table management functions to be used for KVM guest (gmap)
> > page tables.
> > 
> > This patch adds functions to walk to specific table entries, or to
> > perform actions on a range of entries.
> > 
> > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> > ---
> >  arch/s390/kvm/dat.c | 351 ++++++++++++++++++++++++++++++++++++++++++++
> >  arch/s390/kvm/dat.h |  38 +++++
> >  2 files changed, 389 insertions(+)  
> 
> ...
> 
> > +/*
> > + * dat_split_pmd is assumed to be called with mmap_lock held in read or write mode
> > + */
> > +static int dat_split_pmd(union pmd *pmdp, gfn_t gfn, union asce asce)
> > +{
> > +	struct page_table *pt;
> > +	union pmd new, old;
> > +	union pte init;
> > +	int i;
> > +
> > +	old = READ_ONCE(*pmdp);
> > +
> > +	/* Already split, nothing to do */
> > +	if (!old.h.i && !old.h.fc)
> > +		return 0;
> > +
> > +	pt = dat_alloc_pt_noinit();
> > +	if (!pt)
> > +		return -ENOMEM;
> > +	new.val = virt_to_phys(pt);
> > +
> > +	while (old.h.i || old.h.fc) {
> > +		init.val = pmd_origin_large(old);
> > +		init.h.p = old.h.p;
> > +		init.h.i = old.h.i;
> > +		init.s.d = old.s.fc1.d;
> > +		init.s.w = old.s.fc1.w;
> > +		init.s.y = old.s.fc1.y;
> > +		init.s.sd = old.s.fc1.sd;
> > +		init.s.pr = old.s.fc1.pr;
> > +		if (old.h.fc) {
> > +			for (i = 0; i < _PAGE_ENTRIES; i++)
> > +				pt->ptes[i].val = init.val | i * PAGE_SIZE;
> > +			/* no need to take locks as the page table is not installed yet */
> > +			dat_init_pgstes(pt, old.s.fc1.prefix_notif ? PGSTE_IN_BIT : 0);
> > +		} else {
> > +			dat_init_page_table(pt, init.val, 0);
> > +		}
> > +
> > +		if (dat_pmdp_xchg_atomic(pmdp, old, new, gfn, asce))
> > +			return 0;
> > +		old = READ_ONCE(*pmdp);
> > +	}
> > +
> > +	dat_free_pt(pt);
> > +	return 0;
> > +}  
> 
> Just to annoy you again: is there a particular reason why this function isn't
> called "dat_split_segment()" instead of "_pmd()"? I'm still trying to convince
> you to get rid of pmd(p) usage, whenever possible. IMHO this would make the

hmm I can rename it

> code easier to understand and maintain as it doesn't mix things, as I already
> stated previously.
> 
> > +	if (asce.dt >= ASCE_TYPE_REGION2) {
> > +		*last = table->crstes + p4d_index(gfn_to_gpa(gfn));
> > +		entry = READ_ONCE(**last);
> > +		if (WARN_ON_ONCE(unlikely(entry.h.tt != LEVEL_P4D)))
> > +			return -EINVAL;
> > +		if (crste_hole(entry) && !ign_holes)
> > +			return entry.tok.type == _DAT_TOKEN_PIC ? entry.tok.par : -EFAULT;
> > +		if (walk_level == LEVEL_P4D)  
> 
> Also this (even though also as stated earlier): IMHO it would be good to get
> rid of the LEVEL defines and use the corresponding TABLE_TYPEs instead.
> 
> > +	if (continue_anyway && !entry.pmd.h.fc && !entry.h.i) {
> > +		walk_level = LEVEL_PTE;  
> 
> Hm, ok, there is no TABLE_TYPE for this level. Invention required :)

yes, this is why I have the LEVEL_* macros :)

do you think I should instead  #define TABLE_TYPE_PAGE_TABLE -1  ?