[U-Boot] [PATCH] cmd: booti: fix the image runtime location

[Tom Rini]

On Tue, Sep 13, 2016 at 02:02:53PM +0800, Peng Fan wrote:
> Hi Tom,
> 
> On Mon, Sep 12, 2016 at 08:04:01AM -0400, Tom Rini wrote:
> >On Mon, Sep 12, 2016 at 05:18:53PM +0800, Peng Fan wrote:
> >> On Mon, Sep 12, 2016 at 05:07:58PM +0800, Peng Fan wrote:
> >> >We should not use "bi_dram[0].start + text_offset" as the image dst.
> >> >The text_offset maybe 0 for some images, such as XEN. Then the dst
> >> >is actually bi_dram[0].start, which maybe the location of spin table.
> >> >
> >> >Let's use "images->ep & ~(ih->text_offset)" as the dst address.
> >> 
> >> This patch maybe not that correct according to the doc from Linux kernel.
> >> "
> >> The Image must be placed text_offset bytes from a 2MB aligned base
> >> address anywhere in usable system RAM and called there. The region
> >> between the 2 MB aligned base address and the start of the image has no
> >> special significance to the kernel, and may be used for other purposes.
> >> "
> >> 
> >> Now I do not have a good idea that we may have a spin table in the start
> >> of DRAM or even a small firmware.
> >> 
> >> Is it better to change gd->bd->bi_dram[0].start?
> >> For example physical dram starts from 0x80000000, but 4K is reserved at the beginning.
> >> So is it ok to change gd->bd->bi_dram[0].start to 0x80001000?
> >
> >So, to be more precise, with v4.5 the memory location restrictions were
> >relaxed.  So we first need to update cmd/booti.c to know that res1 is
> >now 'flags' and that we must check bit3 to determine where / how to
> >align the image.  In the case of bit3 == 0 we must continue to do what
> >we do today.  In the case of bit3 == 1, we have to decide what exactly
> >to do.  My first thought is to see if images->ep is 2MB aligned and if
> >so move to images->ep + ih->text_offset.  If not 2MB aligned, align that
> >and then add ih->text_offset.
> >
> >Then any areas that need to be reserved in memory, wherever they are,
> >can still be marked off as reserved in the DT.
> 
> 
> How about the following patch?
> If it is ok, I'll send out a formal one.

Pretty close:

> 
> diff --git a/cmd/booti.c b/cmd/booti.c
> index 6c1c998..efa8bc8 100644
> --- a/cmd/booti.c
> +++ b/cmd/booti.c
> @@ -20,7 +20,7 @@ struct Image_header {
>  	uint32_t	code1;		/* Executable code */
>  	uint64_t	text_offset;	/* Image load offset, LE */
>  	uint64_t	image_size;	/* Effective Image size, LE */
> -	uint64_t	res1;		/* reserved */
> +	uint64_t	flags;		/* Information flags, LE */
>  	uint64_t	res2;		/* reserved */
>  	uint64_t	res3;		/* reserved */
>  	uint64_t	res4;		/* reserved */
> @@ -29,6 +29,7 @@ struct Image_header {
>  };
>  
>  #define LINUX_ARM64_IMAGE_MAGIC	0x644d5241
> +#define LINUX_PHYSICAL_PLACEMENT (0x1 << 3)

Just '1' not '0x1'

>  
>  static int booti_setup(bootm_headers_t *images)
>  {
> @@ -54,7 +55,11 @@ static int booti_setup(bootm_headers_t *images)
>  	 * If we are not at the correct run-time location, set the new
>  	 * correct location and then move the image there.
>  	 */
> -	dst = gd->bd->bi_dram[0].start + le64_to_cpu(ih->text_offset);
> +	dst = gd->bd->bi_dram[0].start;
> +	if (le64_to_cpu(ih->flags) & LINUX_PHYSICAL_PLACEMENT)
> +		dst = round_up(images->ep, SZ_2M);

Can you do this as if/else and expand the comment so that it explains
that earlier it was recommended to be as close to the start of memory
as possible but later a flag was introduced to allow a more flexible
placement?  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160913/d95dab95/attachment.sig>