[U-Boot] [RFC PATCH v5 1/4] common: Convert ulong to phys_addr_t for image addresses

[Simon Glass]

Hi York,

On 26 February 2016 at 10:47, york sun <york.sun at nxp.com> wrote:
> On 02/26/2016 09:31 AM, Simon Glass wrote:
>> Hi York,
>>
>> On 26 February 2016 at 10:22, york sun <york.sun at nxp.com> wrote:
>>> On 02/25/2016 03:05 PM, Wolfgang Denk wrote:
>>>> Dear York Sun,
>>>>
>>>> In message <1456439779-4792-2-git-send-email-york.sun at nxp.com> you wrote:
>>>>> When dealing with image addresses, ulong has been used. Some files
>>>>> are used by both host and target. It is OK for the target, but not
>>>>> always enough for host tools including mkimage. This patch replaces
>>>>> "ulong" with "phys_addr_t" to make sure addresses are correct for
>>>>> both the target and the host.
>>>>
>>>> You talk here about using "phys_addr_t"...
>>>>
>>>>> -                       ulong, ulong, ulong))images->ep;
>>>>> +                       ulong, ulong, ulong))(uintptr_t)images->ep;
>>>>
>>>> ...but here you use  uintptr_t  , hich is something different?
>>>>
>>>>> -            ulong, ulong, ulong))images->ep)(images->ft_addr,
>>>>> +            ulong, ulong, ulong))(uintptr_t)images->ep)(images->ft_addr,
>>>>
>>>> Ditto.
>>>>
>>>>> +    phys_addr_t os_data;
>>>>> +    ulong os_len;
>>>>>      void *data = NULL;
>>>>>      size_t len;
>>>>>      int ret;
>>>>> @@ -87,11 +89,10 @@ static int boot_prep_linux(bootm_headers_t *images)
>>>>>      if (images->legacy_hdr_valid) {
>>>>>              hdr = images->legacy_hdr_os;
>>>>>              if (image_check_type(hdr, IH_TYPE_MULTI)) {
>>>>> -                    ulong os_data, os_len;
>>>>
>>>> Why do you moe the declarations out of this block?  The variables are
>>>> only used within this block so there is no need for a wider scope?
>>>>
>>>>> -                    data = (void *)os_data;
>>>>> +                    data = (void *)(uintptr_t)os_data;
>>>>
>>>> This double cast looks scary to me, and you don;t explain it in the
>>>> commit message.  Why exactly is this needed?
>>>>
>>>>> -            cmd_line_dest = (void *)images->ep + COMMAND_LINE_OFFSET;
>>>>> +            cmd_line_dest = (void *)(uintptr_t)images->ep +
>>>>> +                            COMMAND_LINE_OFFSET;
>>>>
>>>> Ditto.
>>>>
>>>>> -    printf("Setup at %#08lx\n", images->ep);
>>>>> -    ret = setup_zimage((void *)images->ep, cmd_line_dest,
>>>>> +    printf("Setup at %#08" PRIpa "\n", images->ep);
>>>>
>>>> This is really ugly...
>>>>
>>>>> +    ret = setup_zimage((void *)(uintptr_t)images->ep, cmd_line_dest,
>>>>
>>>> See before.
>>>>
>>>>> -    debug("## Transferring control to Linux (at address %08lx, kernel %08lx) ...\n",
>>>>> +    debug("## Transferring control to Linux (at address %#08" PRIpa
>>>>> +          ", kernel %#08" PRIpa ") ...\n",
>>>>
>>>> See before...
>>>>
>>>>> -            debug("*  kernel: cmdline image address = 0x%08lx\n",
>>>>> -                    images->ep);
>>>>> +            debug("*  kernel: cmdline image address = %#08" PRIpa "\n",
>>>>> +                  images->ep);
>>>>
>>>> Ditto.  etc. etc.
>>>>
>>>>> +    /*
>>>>> +     * In this function, data is decalred as phys_addr_t type.
>>>>
>>>> s/decalred/declared/
>>>>
>>>>> +     * On some systems (eg. ARM, PowerPC) phys_addr_t can be
>>>>> +     * "unsigned long", or "unsigned long long", depending on
>>>>> +     * CONFIG_PHYS_64BIT.  It is safe to cast 64-bit phys_addr_t
>>>>> +     * to 32-bit pointer for image handling because the actual
>>>>> +     * address the image is loaded is within 32-bit space.
>>>>
>>>> Who guarantees that?
>>>>
>>>>> -            data = (ulong)fit_data;
>>>>> +            data = (phys_addr_t)(uintptr_t)fit_data;
>>>>
>>>> This double cast looks strange to me.  Why is it needed?
>>>>
>>>>> -                            void *from = (void *)data;
>>>>> +                            void *from = (void *)(uintptr_t)data;
>>>>
>>>> Ditto.
>>>>
>>>>> -                    memmove((char *) dest, (char *)data, len);
>>>>> +                    memmove((char *)dest, (char *)(uintptr_t)data, len);
>>>>
>>>> Ditto. etc. etc.
>>>>
>>>>
>>>> All these double casts look somewhat wrong to me.  Why are they
>>>> needed?
>>>
>>> Dear Wolfgang,
>>>
>>> I can use some serious help here. What I am really trying to achieve is the last
>>> two patches in this set. I didn't want to use replace ulong with phys_addr_t. I
>>> am not proud with the change I proposed, but I didn't come up with a smarter
>>> solution. My specific trouble is to build ARMv8 targets on 32-bit Ubuntu host.
>>> Some code is shared between the target and host tool (mkimage). I started from
>>> small changes, but it gets wider and wider when I tried to get rid of the
>>> compiling warnings.
>>>
>>> York
>>>
>>
>> I suggest just documenting it better with comments and in the commit
>> message. It's mostly the same comment I made.
>>
>> One concern is that if you cast to uintptr_t on a 32-bit host machine,
>> won't you end up dropping the top 32 bits?
>>
>
> Simon,
>
> Some code is shared between targets and hosts, but not all. The ugly casting is
> used by targets only. Maybe I should take another look at the issue and back
> away from converting ulong to phys_addr_t totally. It is only broken on 32-bit
> host tool and seems nobody really cares. Sandbox is also broken on 32-bit host
> (compiling warnings), and yet no one complains. Am I the only one living in old
> age? I can upgrade to 64-bit Ubuntu and we all can forget about this mess I
> created. (Getting exhausted here...)

Well maybe. You could instead just print an error and quit when trying
to use 64-bit images on a 32-bit host machine. I think that would be
acceptable these days.

Regards,
Simon