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

[David Hildenbrand (Arm)]

On 7/7/26 10:58, Anshuman Khandual wrote:
> 
> 
> 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 :)

Well, you did some :)

> 
>>> ---
>>
>> [...]
>>
>>> +
>>> +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.

I think any removed endianess dependency is a thing worth considering :)

-- 
Cheers,

David