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(+)

...

> +int dat_entry_walk(gfn_t gfn, union asce asce, int flags, int walk_level,
> +		   union crste **last, union pte **ptepp)
> +{

...

> +	table = dereference_asce(asce);
> +	if (asce.dt >= ASCE_TYPE_REGION1) {
> +		*last = table->crstes + pgd_index(gfn_to_gpa(gfn));
> +		entry = READ_ONCE(**last);
> +		if (WARN_ON_ONCE(unlikely(entry.h.tt != LEVEL_PGD)))
> +			return -EINVAL;

Since I've seen this all over the place: this looks just wrong to me.

This is mixing some random software definition "LEVEL_PGD" with hardware
bits. A "correct" table type compare would compare with TABLE_TYPE_REGION1
instead. Also using pgd_index() & friends here is semantically wrong.

For normal processes the pgd is always the top page table level (present and
used), and lower levels like p4d may be folded. This is not the case with this
code, where pgd, p4d, etc. seem to have a completely different meaning and are
mapped to specific hardware page table levels / types, where the top levels
(pgd, ...) are optional.

Why is this mixed? To me this looks like the potential source for a _lot_ of
confusion and potential bugs.