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

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

On Tue, 16 Sep 2025 19:24:33 +0200
Heiko Carstens <hca@linux.ibm.com> wrote:

> 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)};

hmm, that doesn't look ugly

maybe I can even add one more anonymous struct in vaddr, so that the
gfn can be assigned directly: union vaddress vaddr = { .gfn = 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)))

oh, right, I'll fix that