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

[Anshuman Khandual]

On 07/07/26 12:38 PM, David Hildenbrand (Arm) wrote:
> On 7/7/26 06:17, 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>
> 
> You should add a Co-developed-by here :)

Have not done much changes from your original patch :)

> >> Signed-off-by: Anshuman Khandual <anshuman.khandual@xxxxxxx>
>> ---
> 
> [...]
> 
>> +
>> +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): {
>> +		const u64 *val = entry;
>> +
>> +		if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
>> +			snprintf(buf, buf_size, "%016llx%016llx", val[0], val[1]);
>> +		else
>> +			snprintf(buf, buf_size, "%016llx%016llx", val[1], val[0]);
>> +		break;
> 
> As discussed, can we simplify that by casting to (u128) and then shifting the
> values into place?

which would be something like the following - had similar
construct in D128 RFC V2 series as well.

snprintf(buf, buf_size, "%016llx%016llx",
	(unsigned long long)*(const u128 *)entry >> 64),
	(unsigned long long)*(const u128 *)entry;

I did think about it but is not the proposed chunk bit cleaner
instead and easier to follow. Do you have concern regarding u64
pointers into u128 then accessed in chunks ? Regardless don't
have a strong opinion either way.