From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Glass Date: Fri, 26 Feb 2016 11:05:02 -0700 Subject: [U-Boot] [RFC PATCH v5 1/4] common: Convert ulong to phys_addr_t for image addresses In-Reply-To: References: <1456439779-4792-1-git-send-email-york.sun@nxp.com> <1456439779-4792-2-git-send-email-york.sun@nxp.com> <20160225230500.3FCAF3809F9@gemini.denx.de> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi York, On 26 February 2016 at 10:47, york sun wrote: > On 02/26/2016 09:31 AM, Simon Glass wrote: >> Hi York, >> >> On 26 February 2016 at 10:22, york sun wrote: >>> On 02/25/2016 03:05 PM, Wolfgang Denk wrote: >>>> Dear York Sun, >>>> >>>> In message <1456439779-4792-2-git-send-email-york.sun@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