Re: [RFC PATCH 19/34] x86: power: use proper set_pXd() for generic compile-time folded patable in resume_one_md_table_init()

["David Hildenbrand (Arm)" <david@kernel.org>]

On 7/13/26 16:38, Dave Hansen wrote:
> On 7/13/26 06:55, Yeoreum Yun wrote:
>> @@ -134,11 +129,12 @@ static inline void resume_init_first_level_page_table(pgd_t *pg_dir)
>>  {
>>  #ifdef CONFIG_X86_PAE
>>  	int i;
>> +	pud_t *pud = pud_offset(p4d_offset(pg_dir, 0), 0);
>>  
>>  	/* Init entries of the first-level page table to the zero page */
>>  	for (i = 0; i < PTRS_PER_PGD; i++)
>> -		set_pgd(pg_dir + i,
>> -			__pgd(__pa(empty_zero_page) | _PAGE_PRESENT));
>> +		set_pud(pud + i,
>> +			__pud(__pa(empty_zero_page) | _PAGE_PRESENT));
>>  #endif
>>  }
> 
> If this is the way forward, it's going to require the retraining of some
> awfully old brain cells.
> 
> It also doesn't really read correctly. Loop over each PGD entry:
> 
> 	for (i = 0; i < PTRS_PER_PGD; i++)
> 
> ... and set a pud?
> 
> 		set_pud(...)

The PTRS_PER_PGD is indeed a nasty aspect. Maybe we could clean that up to
exclusively work on PUDs here.

It's already nasty today if you take a look at e.g., patch #22 where we do:

	set_pgd(pgd, __pgd(__pa(pmd_table) | _PAGE_PRESENT));

Or patch #25:

	if (pgd_val(pgd) !=·0) {
		pmd_t *pmd = (pmd_t *)pgd_page_vaddr(pgd);


So yes, we know that we want to set a PMD already in these cases. And using a
PUD is the natural choice :)


> 
> I'm not sure how I'd ever learn the rules to write this code from
> scratch. Right now, the code says: "The pgd_t is the top level of the
> page tables. I know there are PTRS_PER_PGD of those top level entries
> entries I need to 'zero'."
> 
> You don't have to know what is folded, just that you're dealing with the
> top level.
> 
> But *this* code says:
> 
> 1. The pgd_t is the top level of the page tables. Start there.
> 2. "Walk" down *two* (folded) levels
> 3. Set the third-level (pud) entries to 'zero'
> 
> So the code now *has* to know how the folding occurs.
> 
> This seems really hard to work with to me.

It all goes back to the design choice people made to say for folded p4d that

	1) PGD entries are always treated as present page table entries.

static inline int pgd_none(pgd_t pgd)           { return 0; }
static inline int pgd_bad(pgd_t pgd)            { return 0; }
static inline int pgd_present(pgd_t pgd)        { return 1; }
static inline void pgd_clear(pgd_t *pgd)        { }
static inline bool pgd_leaf(pgd_t pgd)          { return false; }

	2) P4D entries are actually what we interpret later.


It kind-of makes sense, because then you have all of the folding happening "on
the top", and not somewhere down the chain.

But yes, ideally we wouldn't have

#define PTRS_PER_P4D            1

I wonder if we could clean that up ...


The good thing is, that the compiler will complain to you if you use the wrong
setter now.

Note the comments we remove in patch #29:

/*
 * (p4ds are folded into pgds so this doesn't get actually called,
 * but the define is needed for a generic inline function.)
 */
#define set_pgd(pgdptr, pgdval) set_p4d((p4d_t *)(pgdptr), (p4d_t) { pgdval })


Doesn't actually get called, right? ;)

-- 
Cheers,

David