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

Claudio Imbrenda
2025-09-17T11:26:11Z

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