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

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

On Tue, Sep 16, 2025 at 06:48:52PM +0200, Claudio Imbrenda wrote:
> On Tue, 16 Sep 2025 18:22:03 +0200
> Heiko Carstens <hca@linux.ibm.com> wrote:
> > > +	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

Yes, please.

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

I'm not saying that this code is incorrect. What I'm complaining about
is that this is mixing hardware and software definitions all over the
place, which at least to me is very confusing. But ok, maybe I'm the
only one.

So e.g. for the above, which operates with a region 1 table entry, I
would expect to see similar code like in gaccess.c. E.g. _something_
like (and most likely incorrect):

	table = dereference_asce(asce);
	if (asce.dt >= ASCE_TYPE_REGION1) {
		union vaddress vaddr = {.addr = gfn_to_gpa(gfn)};

		*last = table->crstes + vaddr.rfx;
		entry = READ_ONCE(**last);
		if (WARN_ON_ONCE(entry.h.tt != TABLE_TYPE_REGION1))
			return -EINVAL;  

Or in other words, this doesn't mix hw vs sw functions. Anyway, just
my 0.02 cents. If you disagree feel free to keep it.
You have to maintain this :)

Btw.: WARN_ON_ONCE() has a builtin unlikely(), please don't add
unlikely again like above:

		if (WARN_ON_ONCE(unlikely(entry.h.tt != LEVEL_PGD)))