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

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

On Tue, 16 Sep 2025 18:22:03 +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(+)  
> 
> ...
> 
> > +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

they are defined to be the same for this reason, but I understand what
you mean, and I can use the TABLE_TYPE_* macros instead

> instead. Also using pgd_index() & friends here is semantically wrong.

how so? in the end all the various p?d_index() macros are just a
shift-and-mask. I think it's more readable than open coding
((address) >> PGDIR_SHIFT) & (PTRS_PER_PGD-1)) each time?

> 
> 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.

yes, and we do not use pte_t or any of the p?d_t types in KVM for that
reason

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