On Wed, Jan 22, 2025 at 08:49:54AM +0100, Heiko Carstens wrote:
static inline pgd_t *pgd_alloc(struct mm_struct *mm)
{
- return (pgd_t *) crst_table_alloc(mm);
+ unsigned long *table = crst_table_alloc(mm);
+
+ if (!table)
+ return NULL;
I do not know status of this series, but FWIW, this call is missed:
crst_table_init(table, _REGION1_ENTRY_EMPTY);
Why is that missing?
Because the follow-up pagetable_pgd_ctor() is called against uninitialized
page table, while other pagetable_pXd_ctor() variants are called against
initialized one. I could imagine complications as result of that.
Whether Region1 table is the right choice is a big question though, as you
noticed below.
A pgd table can be a Region1, Region2, or Region3 table. The only caller of
this function is mm_init() via mm_alloc_pgd(); and right after mm_alloc_pgd()
there is a call to init_new_context() which will initialize the pgd correctly.
init_new_context() is in a way a constructor as well, so whole thing looks odd
to me. But I do not immedeately see a better way :(
I guess what really gets odd, and might be broken (haven't checked yet) is
what happens on dynamic upgrade of page table levels (->crst_table_upgrade()).
Hmm, that is a good point.
With that a pgd may become a pud, and with that we get an imbalance with
the ctor/dtor calls for the various page table levels when they get freed
The ctor/dtor mismatch should not be a problem, as pagetable_pgd|p4d|pud_ctor()
are the same and there is one pagetable_dtor() for all top levels as of now.
But if it ever comes to separate implementations, then we are in the world
of pain.
again. Plus, at first glance, it looks also broken that we have open-coded
crst_alloc() calls instead of using the "proper" page table allocation API
within crst_table_upgrade(), which again would cause an imbalance.
This is a good point too.
Many thanks!
