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

Claudio Imbrenda
2025-09-17T13:13:53Z

On Wed, 17 Sep 2025 14:55:29 +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(+)
>
> ...
>
> > +/*
> > + * dat_split_pmd is assumed to be called with mmap_lock held in read or write mode
> > + */
> > +static int dat_split_pmd(union pmd *pmdp, gfn_t gfn, union asce asce)
> > +{
> > + struct page_table *pt;
> > + union pmd new, old;
> > + union pte init;
> > + int i;
> > +
> > + old = READ_ONCE(*pmdp);
> > +
> > + /* Already split, nothing to do */
> > + if (!old.h.i && !old.h.fc)
> > + return 0;
> > +
> > + pt = dat_alloc_pt_noinit();
> > + if (!pt)
> > + return -ENOMEM;
> > + new.val = virt_to_phys(pt);
> > +
> > + while (old.h.i || old.h.fc) {
> > + init.val = pmd_origin_large(old);
> > + init.h.p = old.h.p;
> > + init.h.i = old.h.i;
> > + init.s.d = old.s.fc1.d;
> > + init.s.w = old.s.fc1.w;
> > + init.s.y = old.s.fc1.y;
> > + init.s.sd = old.s.fc1.sd;
> > + init.s.pr = old.s.fc1.pr;
> > + if (old.h.fc) {
> > + for (i = 0; i < _PAGE_ENTRIES; i++)
> > + pt->ptes[i].val = init.val | i * PAGE_SIZE;
> > + /* no need to take locks as the page table is not installed yet */
> > + dat_init_pgstes(pt, old.s.fc1.prefix_notif ? PGSTE_IN_BIT : 0);
> > + } else {
> > + dat_init_page_table(pt, init.val, 0);
> > + }
> > +
> > + if (dat_pmdp_xchg_atomic(pmdp, old, new, gfn, asce))
> > + return 0;
> > + old = READ_ONCE(*pmdp);
> > + }
> > +
> > + dat_free_pt(pt);
> > + return 0;
> > +}
>
> Just to annoy you again: is there a particular reason why this function isn't
> called "dat_split_segment()" instead of "_pmd()"? I'm still trying to convince
> you to get rid of pmd(p) usage, whenever possible. IMHO this would make the

hmm I can rename it

> code easier to understand and maintain as it doesn't mix things, as I already
> stated previously.
>
> > + if (asce.dt >= ASCE_TYPE_REGION2) {
> > + *last = table->crstes + p4d_index(gfn_to_gpa(gfn));
> > + entry = READ_ONCE(**last);
> > + if (WARN_ON_ONCE(unlikely(entry.h.tt != LEVEL_P4D)))
> > + return -EINVAL;
> > + if (crste_hole(entry) && !ign_holes)
> > + return entry.tok.type == _DAT_TOKEN_PIC ? entry.tok.par : -EFAULT;
> > + if (walk_level == LEVEL_P4D)
>
> Also this (even though also as stated earlier): IMHO it would be good to get
> rid of the LEVEL defines and use the corresponding TABLE_TYPEs instead.
>
> > + if (continue_anyway && !entry.pmd.h.fc && !entry.h.i) {
> > + walk_level = LEVEL_PTE;
>
> Hm, ok, there is no TABLE_TYPE for this level. Invention required :)

yes, this is why I have the LEVEL_* macros :)

do you think I should instead #define TABLE_TYPE_PAGE_TABLE -1 ?