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

[Heiko Carstens <hca@linux.ibm.com>]

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
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 :)