Re: [PATCH V2] mm: Standardize printing for pgtable entries

[Ryan Roberts]

On 08/07/2026 12:27, David Hildenbrand (Arm) wrote:
> On 7/8/26 13:22, Ryan Roberts wrote:
>> On 08/07/2026 04:28, Anshuman Khandual wrote:
>>> From: "David Hildenbrand (Arm)" <david@xxxxxxxxxx>
>>>
>>> Bad page map reporting currently stores page table entry values in an
>>> unsigned long long and prints them with fixed 64-bit-oriented format
>>> strings. This is inconsistent across call sites and does not work well for
>>> architectures where page table entry values are not naturally represented
>>> as 64-bit values, such as 32-bit or 128-bit entries.
>>>
>>> Introduce a common helper to convert raw page table entry values into a
>>> fixed-width hexadecimal string based on the actual entry size. Use it for
>>> bad page map reporting and for dumping the page table walk in
>>> __print_bad_page_map_pgtable().
>>>
>>> Pass page table entry values to the reporting path as raw bytes together
>>> with their size, instead of forcing them through an unsigned long long.
>>> It keeps the printed output consistent and avoids truncation or misleading
>>> formatting for non-64-bit page table entries.
>>>
>>> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
>>> Cc: linux-mm@xxxxxxxxx
>>> Cc: linux-kernel@xxxxxxxxxxxxxxx
>>> Signed-off-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>
>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@xxxxxxx>
>>> ---
>>> This patch applies on v7.2-rc2
>>>
>>> Changes in V2:
>>>
>>> - Dropped space after ":" during print per Matthew
>>> - Dropped CONFIG_CPU_BIG_ENDIAN per David
>>>
>>> Changes in V1:
>>>
>>> https://lore.kernel.org/all/20260707041703.658021-1-anshuman.khandual@xxxxxxx/
>>>
>>>  mm/memory.c | 98 ++++++++++++++++++++++++++++++++++++++++-------------
>>>  1 file changed, 75 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/mm/memory.c b/mm/memory.c
>>> index ff338c2abe92..a2b63af82792 100644
>>> --- a/mm/memory.c
>>> +++ b/mm/memory.c
>>> @@ -519,9 +519,48 @@ static bool is_bad_page_map_ratelimited(void)
>>>  	return false;
>>>  }
>>>  
>>> +#define PTVAL_STR_MAX	(32 + 1) /* Max 128-bit value in hex + NUL */
>>
>> Not sure if it's worth doing something like this?:
>>
>> #define PTVAL_STR_MAX						\
>> 	(MAX(MAX(MAX(MAX(sizeof(pteval_t), sizeof(pmdval_t)), 	\
>> 	    	     sizeof(pudval_t)), 			\
>> 		 sizeof(p4dval_t)), 				\
>>     	     sizeof(pgdval_t)) + 1)
>>
>> Would probably save stack space for 32bit arches?
> 
> Do we really care about that? This is about a corner cases error reporting right
> now.

Fair enough, it just seemed like an obvious and simple (very minor) improvement.

> 
>>
>>> +
>>> +static void ptval_bytes_to_hex_str(char *buf, size_t buf_size, const void *entry, size_t entry_size)
>>> +{
>>> +	if (WARN_ON_ONCE(buf_size < entry_size * 2 + 1)) {
>>> +		snprintf(buf, buf_size, "overflow");
>>> +		return;
>>> +	}
>>> +
>>> +	switch (entry_size) {
>>> +	case sizeof(u32):
>>> +		snprintf(buf, buf_size, "%08x", *(const u32 *)entry);
>>> +		break;
>>> +	case sizeof(u64):
>>> +		snprintf(buf, buf_size, "%016llx", *(const u64 *)entry);
>>> +		break;
>>> +#if defined(__SIZEOF_INT128__)
>>> +	case sizeof(u128):
>>> +		snprintf(buf, buf_size, "%016llx%016llx",
>>> +			 (unsigned long long)(*(const u128 *)entry >> 64),
>>> +			 (unsigned long long)*(const u128 *)entry);
>>> +		break;
>>> +#endif
>>> +	default:
>>> +		snprintf(buf, buf_size, "unsupported");
>>> +		break;
>>> +	}
>>> +}
>>> +
>>> +#define ptval_to_str(buf, val)								\
>>> +	do {										\
>>> +		__auto_type __val = (val);						\
>>> +											\
>>> +		ptval_bytes_to_hex_str((buf), sizeof(buf), &__val, sizeof(__val));	\
>>> +	} while (0)
>>
>> I think arm64 code also does pte printing, which you also need to fix up for
>> D128 support. Perhaps this could be moved to a header for reuse?
> 
> We could do that as a second step, right?

Sure, but why churn it twice? Anyway, no strong opinion, you're the boss :)

> 
>>
>> Also not sure if it's worth returning buf so that this pattern would be possible:
>>
>> char pmd_str[PTVAL_STR_MAX];
>> pr_err("pmd=%s\n, ptval_to_str(pmd_str, pmd_val(*pmdp));
>>
>> Although perhaps that's a bit busy and should be discouraged...
> 
> Yeah, let's not do that.
>