Re: [PATCH] x86/boot: Expand __puthex() to print 64-bit value in i386-PAE

[Chao Fan]

On Thu, Jan 31, 2019 at 10:20:13PM +0100, Borislav Petkov wrote:
>On Thu, Jan 31, 2019 at 04:39:46PM +0800, Chao Fan wrote:
>> __puthex() is the only method to print value in compressed boot stage.
>> But in i386-PAE, sizeof(unsigned long) is 4, so that the value of
>> 64-bit is cut and print a wrong value. That will mislead developers
>> when debugging.
>> Expand the type to unsigned long long to print the right value.
>> 
>> Suggested-by: Ruan Shiyang <ruansy.fnst@xxxxxxxxxxxxxx>
>> Signed-off-by: Chao Fan <fanc.fnst@xxxxxxxxxxxxxx>
>> ---
>> Here is my PATCHSET:
>> https://lkml.org/lkml/2019/1/23/266
>> When debugging, I need to print the memory address and length then
>> check the value is right or not. They may exceed 4G in i386-PAE,
>> but the upper half was cut because unsigned long is not enough.
>> 
>> A same condition fix after start_kernel() in ACPI has been merged:
>> commit b9ced18acf68dffebe6888c7ec765a2b1db7a039
>> Author: Chao Fan <fanc.fnst@xxxxxxxxxxxxxx>
>> Date:   Wed Dec 26 11:34:50 2018 +0800
>> 
>>     ACPI: NUMA: Use correct type for printing addresses on i386-PAE
>> ---
>>  arch/x86/boot/compressed/misc.c | 2 +-
>>  arch/x86/boot/compressed/misc.h | 2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
>> index 8dd1d5ccae58..30fb8abdb154 100644
>> --- a/arch/x86/boot/compressed/misc.c
>> +++ b/arch/x86/boot/compressed/misc.c
>> @@ -153,7 +153,7 @@ void __putstr(const char *s)
>>  	outb(0xff & (pos >> 1), vidport+1);
>>  }
>>  
>> -void __puthex(unsigned long value)
>> +void __puthex(unsigned long long value)
>
>Not enough because debug_putaddr() still converts it to unsigned long.
>So you get (still truncated) values with 8 preceding zeroes:

Yes, so sometimes it's better to use debug_puthex().
>
>input_data: 0x00000000036bf100
>input_len: 0x00000000005b694b
>output: 0x0000000001000000
>output_len: 0x0000000000bfba30
>kernel_total_size: 0x0000000002c92000
>beefy: 0x00000000cafebabe
>
>beefy is a test variable I declared:
>
>u64 beefy = 0xdeadbeefcafebabe;
>
>If anything, there should be a separate set of helpers which take 64-bit
>quantities as arguments. But I don't see any need for them now.

It's not used for existing code, it's used to help developers debug.
Just in my RSDP PATCHSET, I need to print the memory regions from SRAT
and check the value, then I found the value of memory regions in last
2 nodes are always wrong, I though there are some problems in my code.
Later I found the value was cut and neither debug_putaddr() nor
debug_puthex() is enough to use, then I change them to unsigned
long long in my local code and continue to debug.
So I send the change as PATCH and to see if later developers need the
debug tool.

Thanks,
Chao Fan

>
>-- 
>Regards/Gruss,
>    Boris.
>
>Good mailing practices for 400: avoid top-posting and trim the reply.
>
>