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

[David Hildenbrand (Arm)]

On 7/9/26 09:40, Andy Shevchenko wrote:
> +Petr, wondering your opinion about code dedup (see below).
> 
> On Thu, Jul 09, 2026 at 10:13:34AM +0530, 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
> 
> FWIW, you may move these to be after...

We generally don't handle it like that in MM. Maybe one day we'll switch over
and document it accordingly.

For now, having selected patches doing this differently is not any helpful.

[...]

>> +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 __val = (val);							\
>> +											\
>> +		ptval_bytes_to_hex_str((buf), sizeof(buf), &__val, sizeof(__val));	\
>> +	} while (0)
>> +
>> +#if defined(__SIZEOF_INT128__)
>> +#define PTVAL_STR_MAX	(32 + 1) /* Max 128-bit value in hex + NUL */
>> +#else
>> +#define PTVAL_STR_MAX	(16 + 1) /* Max 64-bit value in hex + NUL */
>> +#endif
> 
> The above is quite duplicative with what we have in lib/vsprintf.c. Have you considered
> using something from there instead? (Yes, it might require some functions to be wrapped
> or dropped from static.)

Which part in particular do you have in mind?

num_to_str() does not apply due to the u128.

We could have u128 variant that we would only provide with __SIZEOF_INT128__

int num128_to_str(char *buf, int size, u128 num, unsigned int width)

And then have the code pass the value instead of a pointer to the value. A bit
tricky to handle this based on conditional __SIZEOF_INT128__ support, but could
be done.

Not sure if that is really what we want here, though. ptval_bytes_to_hex_str()
is pretty ... simple :)

I didn't immediately spot a replacement for PTVAL_STR_MAX. E.g.,
of_unittest_printf what calls num_to_str() just hard-codes "16".

So it would be good if you could clarify what you had in mind, thanks!

-- 
Cheers,

David