[U-Boot] [PATCH] Gracefully handle 64-bit Load Address and Entry Point Address mkimage parameters

[William Cohen]

On 02/15/2016 06:34 AM, Wolfgang Denk wrote:
> Dear William,
> 
> In message <1455506732-22307-1-git-send-email-wcohen at redhat.com> you wrote:
>>
>> Recent MIPS Linux kernels are using a 64-bit value for the load
>> address (0xffffffff80010000) for the Creator CI20 board kernel.  When
>> this argument was passed to the mkimage program running on a 32-bit
>> machine such as the Creator CI20 board the load address was
>> incorrectly obtained from the first half of the argument, 0xffffffff
>> by the strtoul.  The mkimage should be able to tolerate the longer,
>> 64-bit signed version of the arguments with the use of strtoull.
> 
> Hm...  I think we have multiple problems here.

Hi,

The write up in the patch wasn't as clear as it should have been. I
have revised the patch to try to make the reason for this patch
clearer.  The MIP Linux kernel is using sign-extending 32-bit values
to 64-bit values, so the truncation of the strtoll values to 32-bits
is actually desired. The revised patch now passes the linux kernel
checkpatch.pl checks.  I will be resending the patch in a moment.

-Will
> 
> First, the old legacy uImage header usersd 32 bit data types for the
> addresses.  This is a binary data structure, and there is no way to
> extend it for 64 bit addresses without breaking compatibility.
> 
> 
>> -				params.addr = strtoul (*++argv, &ptr, 16);
>> +				params.addr = strtoull (*++argv, &ptr, 16);
> 
> I don't see what you win here...  strtoull() returns unsigned long long,
> but params.addr is an unsigned int, so the value will be truncated to
> 32 bit.  Are you sure this is what you want?
> 
>>  					fprintf (stderr,
>>  						"%s: invalid load address %s\n",
>> @@ -146,7 +146,7 @@ int main(int argc, char **argv)
>>  			case 'e':
>>  				if (--argc <= 0)
>>  					usage ();
>> -				params.ep = strtoul (*++argv, &ptr, 16);
>> +				params.ep = strtoull (*++argv, &ptr, 16);
> 
> Ditto...
> 
> 
> Also please note that your patch triggers a number of checkpatch
> warnings and an error, especially:
> 
> WARNING: space prohibited between function name and open parenthesis '('
> #108: FILE: tools/mkimage.c:132:
> +                               params.addr = strtoull (*++argv, &ptr, 16);
> 
> WARNING: space prohibited between function name and open parenthesis '('
> #117: FILE: tools/mkimage.c:149:
> +                               params.ep = strtoull (*++argv, &ptr, 16);
> 
> ERROR: Missing Signed-off-by: line(s)
> 
> 
> Best regards,
> 
> Wolfgang Denk
>