Re: [U-Boot] [PATCH] ARM: Disable "DISCARD" for secure section if CONFIG_ARMV7_SECURE_BASE isn't defined

[Dongsheng Wang]

Hi Tom,

Thanks for your review.

I will update this patch.

Regards,
-Dongsheng

> On Mon, Jan 11, 2016 at 02:51:39AM +, Dongsheng Wang wrote:
> > Hi Tom,
> >
> > Sorry for my late reply, and thanks for your reply.
> >
> > How about the following comments, following your suggestion I remove
> some redundant comments?
> >
> > If my understanding is wrong, please correct me, thanks:
> > #if defined(CONFIG_ARMV7_SECURE_BASE) &&
> defined(CONFIG_ARMV7_NONSEC)
> > /*
> >  * If CONFIG_ARMV7_SECURE_BASE is true, secure code will not
> >  * bundle with u-boot, and codes offset are fixed. Secure zone
> 
> code offsets
> 
> >  * only needs to be copied from loading address to
> 
> from the loading address
> 
> >  * CONFIG_ARMV7_SECURE_BASE, which is the linking and running
> >  * address for secure code.
> >  *
> >  * If undefine CONFIG_ARMV7_SECURE_BASE secure zone will be
> 
> If CONFIG_ARMV7_SECURE_BASE is undefined, the secure zone will be
> 
> >  * included in u-boot space, and some absolute address were
> > used
> 
> u-boot address space
> 
> >  * in secure code. Accompanied by u-boot relocation secure code
> >  * also need to relocate the absolute address.
> 
> in secure code.  The absolute addresses of the secure code also needs to be
> relocated along with the accompanying u-boot code.
> 
> >  *
> >  * So DISCARD is only for CONFIG_ARMV7_SECURE_BASE.
> >  */
> > /DISCARD/ : { *(.rel._secure*) } #endif
> 
> Otherwise looks good, thanks!
> 
> --
> Tom
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] ARM: Disable "DISCARD" for secure section if CONFIG_ARMV7_SECURE_BASE isn't defined

[Tom Rini]

On Mon, Jan 11, 2016 at 02:51:39AM +, Dongsheng Wang wrote:
> Hi Tom,
> 
> Sorry for my late reply, and thanks for your reply.
> 
> How about the following comments, following your suggestion I remove some 
> redundant comments?
> 
> If my understanding is wrong, please correct me, thanks:
> #if defined(CONFIG_ARMV7_SECURE_BASE) && defined(CONFIG_ARMV7_NONSEC)
> /*
>  * If CONFIG_ARMV7_SECURE_BASE is true, secure code will not
>  * bundle with u-boot, and codes offset are fixed. Secure zone

code offsets

>  * only needs to be copied from loading address to

from the loading address

>  * CONFIG_ARMV7_SECURE_BASE, which is the linking and running
>  * address for secure code.
>  *
>  * If undefine CONFIG_ARMV7_SECURE_BASE secure zone will be

If CONFIG_ARMV7_SECURE_BASE is undefined, the secure zone will be

>  * included in u-boot space, and some absolute address were used

u-boot address space

>  * in secure code. Accompanied by u-boot relocation secure code
>  * also need to relocate the absolute address.

in secure code.  The absolute addresses of the secure code also needs to
be relocated along with the accompanying u-boot code.

>  *
>  * So DISCARD is only for CONFIG_ARMV7_SECURE_BASE.
>  */
> /DISCARD/ : { *(.rel._secure*) }
> #endif

Otherwise looks good, thanks!

-- 
Tom


signature.asc
Description: Digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] ARM: Disable "DISCARD" for secure section if CONFIG_ARMV7_SECURE_BASE isn't defined

[Dongsheng Wang]

Hi Tom,

Sorry for my late reply, and thanks for your reply.

How about the following comments, following your suggestion I remove some 
redundant comments?

If my understanding is wrong, please correct me, thanks:
#if defined(CONFIG_ARMV7_SECURE_BASE) && defined(CONFIG_ARMV7_NONSEC)
/*
 * If CONFIG_ARMV7_SECURE_BASE is true, secure code will not
 * bundle with u-boot, and codes offset are fixed. Secure zone
 * only needs to be copied from loading address to
 * CONFIG_ARMV7_SECURE_BASE, which is the linking and running
 * address for secure code.
 *
 * If undefine CONFIG_ARMV7_SECURE_BASE secure zone will be
 * included in u-boot space, and some absolute address were used
 * in secure code. Accompanied by u-boot relocation secure code
 * also need to relocate the absolute address.
 *
 * So DISCARD is only for CONFIG_ARMV7_SECURE_BASE.
 */
/DISCARD/ : { *(.rel._secure*) }
#endif

Regards,
-Dongsheng

> 
> On Fri, Dec 11, 2015 at 03:30:24PM +, Dongsheng Wang wrote:
> > Hi Tom,
> >
> > > On Fri, Dec 11, 2015 at 10:15:03AM +, Dongsheng Wang wrote:
> > > > Hi Tom,
> > > >
> > > > Thanks for your review.
> > > >
> > > > > On Thu, Dec 10, 2015 at 10:49:01AM +0800, Dongsheng Wang wrote:
> > > > >
> > > > > > From: Wang Dongsheng 
> > > > > >
> > > > > > Fix PSCI hang up without CONFIG_ARMV7_SECURE_BASE define.
> > > > > > "DISCARD" will remove ._secure.text relocate, but PSCI
> > > > > > framework has already used some absolute address those need to
> relocate.
> > > > > >
> > > > > > Use readelf -t -r u-boot show us:
> > > > > > .__secure_start addr: 601408e4
> > > > > > .__secure_end   addr: 60141460
> > > > > >
> > > > > > 60141140  0017 R_ARM_RELATIVE
> > > > > > 46  _secure_monitor:
> > > > > > 47  #ifdef CONFIG_ARMV7_PSCI
> > > > > > 48  ldr r5, =_psci_vectors
> > > > > >
> > > > > > 60141194  0017 R_ARM_RELATIVE 6014119c  0017
> > > > > > R_ARM_RELATIVE
> > > > > > 601411a4  0017 R_ARM_RELATIVE 601411ac  0017
> > > > > > R_ARM_RELATIVE
> > > > > > 64  _psci_table:
> > > > > > 66  .word   psci_cpu_suspend
> > > > > > ...
> > > > > > 72  .word   psci_migrate
> > > > > >
> > > > > > 60141344  0017 R_ARM_RELATIVE 6014145c  0017
> > > > > > R_ARM_RELATIVE
> > > > > > 202 ldr r5, =psci_text_end
> > > > > >
> > > > > > Solutions:
> > > > > > 1. Change absolute address to RelAdr.
> > > > > >Based on LDR (immediate, ARM), we only have 4K offset to jump.
> > > > > > Now PSCI code size is close to 4K size that is LDR limit jump
> > > > > > size, so even if the LDR is based on the current instruction
> > > > > > address, there is also have a risk for RelAdr. If we use two
> > > > > > jump steps I think we can fix this issue, but looks too hack, so 
> > > > > > give up
> this way.
> > > > > >
> > > > > > 2. Enable "DISCARD" only for CONFIG_ARMV7_SECURE_BASE has
> defined.
> > > > > >If CONFIG_ARMV7_SECURE_BASE is defined in platform, all of
> > > > > > secure will in the BASE address that is absolute.
> > > > > > psci_update_dt will relocate the PSCI code into link stage
> > > > > > address(CONFIG_ARMV7_SECURE_BASE address), so using this
> method.
> > > > > >
> > > > > > Signed-off-by: Wang Dongsheng 
> > > > > > Reviewed-by: Peng Fan 
> > > > > > Cc: Albert Aribaud 
> > > > > > Cc: Tom Warren 
> > > > > > Cc: York Sun 
> > > > > > Cc: Hans De Goede 
> > > > > > Cc: Ian Campbell 
> > > > > > Cc: Tom Rini 
> > > > > > Cc: Jan Kiszka 
> > > > > > Cc: Stefano Babic 
> > > > > >
> > > > > > diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
> > > > > > index
> > > > > > d48a905..413d459 100644
> > > > > > --- a/arch/arm/cpu/u-boot.lds
> > > > > > +++ b/arch/arm/cpu/u-boot.lds
> > > > > > @@ -14,6 +14,7 @@ OUTPUT_ARCH(arm)
> > > > > >  ENTRY(_start)
> > > > > >  SECTIONS
> > > > > >  {
> > > > > > +#if defined(CONFIG_ARMV7_SECURE_BASE) &&
> > > > > defined(CONFIG_ARMV7_NONSEC)
> > > > > > /*
> > > > > >  * Discard the relocation entries for secure text.
> > > > > >  * The secure code is bundled with u-boot image, so there
> > > > > > will @@
> > > > > > -31,6 +32,7 @@ SECTIONS
> > > > > >  * avoid hole in the final image.
> > > > > >  */
> > > >
> > > > Update this comment, not my patch's comment, right?
> > >
> > > Correct.
> > >
> > Not sure we hope a detailed comment or concise comment.
> > Could you review my comment?
> >
> > If my understanding is wrong, please correct me, thanks:
> > /*
> >  * Based on the /DISCARD/ introduce by ARMv7 patch. And ARMv8 not
> >  * for sure has the same issue. Based on conservative this is only 
> > 

Re: [U-Boot] [PATCH] ARM: Disable "DISCARD" for secure section if CONFIG_ARMV7_SECURE_BASE isn't defined

[Peng Fan]

Hi Tom,
On Mon, Jan 04, 2016 at 10:50:16AM -0500, Tom Rini wrote:
>On Fri, Dec 11, 2015 at 03:30:24PM +, Dongsheng Wang wrote:
>> Hi Tom,
>> 
>> > On Fri, Dec 11, 2015 at 10:15:03AM +, Dongsheng Wang wrote:
>> > > Hi Tom,
>> > >
>> > > Thanks for your review.
>> > >
>> > > > On Thu, Dec 10, 2015 at 10:49:01AM +0800, Dongsheng Wang wrote:
>> > > >
>> > > > > From: Wang Dongsheng 
>> > > > >
>> > > > > Fix PSCI hang up without CONFIG_ARMV7_SECURE_BASE define.
>> > > > > "DISCARD" will remove ._secure.text relocate, but PSCI framework
>> > > > > has already used some absolute address those need to relocate.
>> > > > >
>> > > > > Use readelf -t -r u-boot show us:
>> > > > > .__secure_start  addr: 601408e4
>> > > > > .__secure_endaddr: 60141460
>> > > > >
>> > > > > 60141140  0017 R_ARM_RELATIVE
>> > > > > 46   _secure_monitor:
>> > > > > 47   #ifdef CONFIG_ARMV7_PSCI
>> > > > > 48  ldr r5, =_psci_vectors
>> > > > >
>> > > > > 60141194  0017 R_ARM_RELATIVE
>> > > > > 6014119c  0017 R_ARM_RELATIVE
>> > > > > 601411a4  0017 R_ARM_RELATIVE
>> > > > > 601411ac  0017 R_ARM_RELATIVE
>> > > > > 64   _psci_table:
>> > > > > 66   .word   psci_cpu_suspend
>> > > > > ...
>> > > > > 72   .word   psci_migrate
>> > > > >
>> > > > > 60141344  0017 R_ARM_RELATIVE
>> > > > > 6014145c  0017 R_ARM_RELATIVE
>> > > > > 202  ldr r5, =psci_text_end
>> > > > >
>> > > > > Solutions:
>> > > > > 1. Change absolute address to RelAdr.
>> > > > >Based on LDR (immediate, ARM), we only have 4K offset to jump.
>> > > > > Now PSCI code size is close to 4K size that is LDR limit jump
>> > > > > size, so even if the LDR is based on the current instruction
>> > > > > address, there is also have a risk for RelAdr. If we use two jump
>> > > > > steps I think we can fix this issue, but looks too hack, so give up 
>> > > > > this way.
>> > > > >
>> > > > > 2. Enable "DISCARD" only for CONFIG_ARMV7_SECURE_BASE has defined.
>> > > > >If CONFIG_ARMV7_SECURE_BASE is defined in platform, all of
>> > > > > secure will in the BASE address that is absolute. psci_update_dt
>> > > > > will relocate the PSCI code into link stage
>> > > > > address(CONFIG_ARMV7_SECURE_BASE address), so using this method.
>> > > > >
>> > > > > Signed-off-by: Wang Dongsheng 
>> > > > > Reviewed-by: Peng Fan 
>> > > > > Cc: Albert Aribaud 
>> > > > > Cc: Tom Warren 
>> > > > > Cc: York Sun 
>> > > > > Cc: Hans De Goede 
>> > > > > Cc: Ian Campbell 
>> > > > > Cc: Tom Rini 
>> > > > > Cc: Jan Kiszka 
>> > > > > Cc: Stefano Babic 
>> > > > >
>> > > > > diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
>> > > > > index
>> > > > > d48a905..413d459 100644
>> > > > > --- a/arch/arm/cpu/u-boot.lds
>> > > > > +++ b/arch/arm/cpu/u-boot.lds
>> > > > > @@ -14,6 +14,7 @@ OUTPUT_ARCH(arm)
>> > > > >  ENTRY(_start)
>> > > > >  SECTIONS
>> > > > >  {
>> > > > > +#if defined(CONFIG_ARMV7_SECURE_BASE) &&
>> > > > defined(CONFIG_ARMV7_NONSEC)
>> > > > >  /*
>> > > > >   * Discard the relocation entries for secure text.
>> > > > >   * The secure code is bundled with u-boot image, so there will
>> > > > > @@
>> > > > > -31,6 +32,7 @@ SECTIONS
>> > > > >   * avoid hole in the final image.
>> > > > >   */
>> > >
>> > > Update this comment, not my patch's comment, right?
>> > 
>> > Correct.
>> > 
>> Not sure we hope a detailed comment or concise comment.
>> Could you review my comment?
>> 
>> If my understanding is wrong, please correct me, thanks:
>> /*
>>  * Based on the /DISCARD/ introduce by ARMv7 patch. And ARMv8 not
>>  * for sure has the same issue. Based on conservative this is only 
>> for
>>  * ARMv7, another point the /DISCARD/ may isn't necessary in 
>> platform.
>>  * Please see below investigation:
>>  *
>>  * If undefine CONFIG_ARMV7_SECURE_BASE secure zone will be
>>  * included in u-boot space, and some absolute address were used
>>  * in secure code. Accompanied by u-boot relocation secure code
>>  * also need to relocate the absolute address.
>>  *
>>  * If CONFIG_ARMV7_SECURE_BASE is true, secure code will not
>>  * bundle with u-boot, and codes offset are fixed. Secure zone
>>  * only needs to be copied from loading address to
>>  * CONFIG_ARMV7_SECURE_BASE, which is the linking and running
>>  * address for secure code.
>>  *
>>  * About below may depend on toolchain:
>>  * 1. If keep the relocation entries in .rel.dyn section,
>>  * "relocation offset + linking address" may locates into an
>>  * address that is reserved by SoC, then will trigger data abort.
>>  

Re: [U-Boot] [PATCH] ARM: Disable "DISCARD" for secure section if CONFIG_ARMV7_SECURE_BASE isn't defined

[Tom Rini]

On Fri, Dec 11, 2015 at 03:30:24PM +, Dongsheng Wang wrote:
> Hi Tom,
> 
> > On Fri, Dec 11, 2015 at 10:15:03AM +, Dongsheng Wang wrote:
> > > Hi Tom,
> > >
> > > Thanks for your review.
> > >
> > > > On Thu, Dec 10, 2015 at 10:49:01AM +0800, Dongsheng Wang wrote:
> > > >
> > > > > From: Wang Dongsheng 
> > > > >
> > > > > Fix PSCI hang up without CONFIG_ARMV7_SECURE_BASE define.
> > > > > "DISCARD" will remove ._secure.text relocate, but PSCI framework
> > > > > has already used some absolute address those need to relocate.
> > > > >
> > > > > Use readelf -t -r u-boot show us:
> > > > > .__secure_start   addr: 601408e4
> > > > > .__secure_end addr: 60141460
> > > > >
> > > > > 60141140  0017 R_ARM_RELATIVE
> > > > > 46_secure_monitor:
> > > > > 47#ifdef CONFIG_ARMV7_PSCI
> > > > > 48  ldr r5, =_psci_vectors
> > > > >
> > > > > 60141194  0017 R_ARM_RELATIVE
> > > > > 6014119c  0017 R_ARM_RELATIVE
> > > > > 601411a4  0017 R_ARM_RELATIVE
> > > > > 601411ac  0017 R_ARM_RELATIVE
> > > > > 64_psci_table:
> > > > > 66.word   psci_cpu_suspend
> > > > > ...
> > > > > 72.word   psci_migrate
> > > > >
> > > > > 60141344  0017 R_ARM_RELATIVE
> > > > > 6014145c  0017 R_ARM_RELATIVE
> > > > > 202   ldr r5, =psci_text_end
> > > > >
> > > > > Solutions:
> > > > > 1. Change absolute address to RelAdr.
> > > > >Based on LDR (immediate, ARM), we only have 4K offset to jump.
> > > > > Now PSCI code size is close to 4K size that is LDR limit jump
> > > > > size, so even if the LDR is based on the current instruction
> > > > > address, there is also have a risk for RelAdr. If we use two jump
> > > > > steps I think we can fix this issue, but looks too hack, so give up 
> > > > > this way.
> > > > >
> > > > > 2. Enable "DISCARD" only for CONFIG_ARMV7_SECURE_BASE has defined.
> > > > >If CONFIG_ARMV7_SECURE_BASE is defined in platform, all of
> > > > > secure will in the BASE address that is absolute. psci_update_dt
> > > > > will relocate the PSCI code into link stage
> > > > > address(CONFIG_ARMV7_SECURE_BASE address), so using this method.
> > > > >
> > > > > Signed-off-by: Wang Dongsheng 
> > > > > Reviewed-by: Peng Fan 
> > > > > Cc: Albert Aribaud 
> > > > > Cc: Tom Warren 
> > > > > Cc: York Sun 
> > > > > Cc: Hans De Goede 
> > > > > Cc: Ian Campbell 
> > > > > Cc: Tom Rini 
> > > > > Cc: Jan Kiszka 
> > > > > Cc: Stefano Babic 
> > > > >
> > > > > diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
> > > > > index
> > > > > d48a905..413d459 100644
> > > > > --- a/arch/arm/cpu/u-boot.lds
> > > > > +++ b/arch/arm/cpu/u-boot.lds
> > > > > @@ -14,6 +14,7 @@ OUTPUT_ARCH(arm)
> > > > >  ENTRY(_start)
> > > > >  SECTIONS
> > > > >  {
> > > > > +#if defined(CONFIG_ARMV7_SECURE_BASE) &&
> > > > defined(CONFIG_ARMV7_NONSEC)
> > > > >   /*
> > > > >* Discard the relocation entries for secure text.
> > > > >* The secure code is bundled with u-boot image, so there will
> > > > > @@
> > > > > -31,6 +32,7 @@ SECTIONS
> > > > >* avoid hole in the final image.
> > > > >*/
> > >
> > > Update this comment, not my patch's comment, right?
> > 
> > Correct.
> > 
> Not sure we hope a detailed comment or concise comment.
> Could you review my comment?
> 
> If my understanding is wrong, please correct me, thanks:
> /*
>  * Based on the /DISCARD/ introduce by ARMv7 patch. And ARMv8 not
>  * for sure has the same issue. Based on conservative this is only for
>  * ARMv7, another point the /DISCARD/ may isn't necessary in platform.
>  * Please see below investigation:
>  *
>  * If undefine CONFIG_ARMV7_SECURE_BASE secure zone will be
>  * included in u-boot space, and some absolute address were used
>  * in secure code. Accompanied by u-boot relocation secure code
>  * also need to relocate the absolute address.
>  *
>  * If CONFIG_ARMV7_SECURE_BASE is true, secure code will not
>  * bundle with u-boot, and codes offset are fixed. Secure zone
>  * only needs to be copied from loading address to
>  * CONFIG_ARMV7_SECURE_BASE, which is the linking and running
>  * address for secure code.
>  *
>  * About below may depend on toolchain:
>  * 1. If keep the relocation entries in .rel.dyn section,
>  * "relocation offset + linking address" may locates into an
>  * address that is reserved by SoC, then will trigger data abort.
>  * The reason that move .rel._secure at the beginning, is to
>  * avoid hole in the final image.
>  *
>  * 2. .rel.dyn will not include secure code, becuase
> 

Re: [U-Boot] [PATCH] ARM: Disable "DISCARD" for secure section if CONFIG_ARMV7_SECURE_BASE isn't defined

[Tom Rini]

On Fri, Dec 11, 2015 at 10:15:03AM +, Dongsheng Wang wrote:
> Hi Tom,
> 
> Thanks for your review.
> 
> > On Thu, Dec 10, 2015 at 10:49:01AM +0800, Dongsheng Wang wrote:
> > 
> > > From: Wang Dongsheng 
> > >
> > > Fix PSCI hang up without CONFIG_ARMV7_SECURE_BASE define.
> > > "DISCARD" will remove ._secure.text relocate, but PSCI framework has
> > > already used some absolute address those need to relocate.
> > >
> > > Use readelf -t -r u-boot show us:
> > > .__secure_start   addr: 601408e4
> > > .__secure_end addr: 60141460
> > >
> > > 60141140  0017 R_ARM_RELATIVE
> > > 46_secure_monitor:
> > > 47#ifdef CONFIG_ARMV7_PSCI
> > > 48  ldr r5, =_psci_vectors
> > >
> > > 60141194  0017 R_ARM_RELATIVE
> > > 6014119c  0017 R_ARM_RELATIVE
> > > 601411a4  0017 R_ARM_RELATIVE
> > > 601411ac  0017 R_ARM_RELATIVE
> > > 64_psci_table:
> > > 66.word   psci_cpu_suspend
> > > ...
> > > 72.word   psci_migrate
> > >
> > > 60141344  0017 R_ARM_RELATIVE
> > > 6014145c  0017 R_ARM_RELATIVE
> > > 202   ldr r5, =psci_text_end
> > >
> > > Solutions:
> > > 1. Change absolute address to RelAdr.
> > >Based on LDR (immediate, ARM), we only have 4K offset to jump.
> > > Now PSCI code size is close to 4K size that is LDR limit jump size, so
> > > even if the LDR is based on the current instruction address, there is
> > > also have a risk for RelAdr. If we use two jump steps I think we can
> > > fix this issue, but looks too hack, so give up this way.
> > >
> > > 2. Enable "DISCARD" only for CONFIG_ARMV7_SECURE_BASE has defined.
> > >If CONFIG_ARMV7_SECURE_BASE is defined in platform, all of secure
> > > will in the BASE address that is absolute. psci_update_dt will
> > > relocate the PSCI code into link stage
> > > address(CONFIG_ARMV7_SECURE_BASE address), so using this method.
> > >
> > > Signed-off-by: Wang Dongsheng 
> > > Reviewed-by: Peng Fan 
> > > Cc: Albert Aribaud 
> > > Cc: Tom Warren 
> > > Cc: York Sun 
> > > Cc: Hans De Goede 
> > > Cc: Ian Campbell 
> > > Cc: Tom Rini 
> > > Cc: Jan Kiszka 
> > > Cc: Stefano Babic 
> > >
> > > diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds index
> > > d48a905..413d459 100644
> > > --- a/arch/arm/cpu/u-boot.lds
> > > +++ b/arch/arm/cpu/u-boot.lds
> > > @@ -14,6 +14,7 @@ OUTPUT_ARCH(arm)
> > >  ENTRY(_start)
> > >  SECTIONS
> > >  {
> > > +#if defined(CONFIG_ARMV7_SECURE_BASE) &&
> > defined(CONFIG_ARMV7_NONSEC)
> > >   /*
> > >* Discard the relocation entries for secure text.
> > >* The secure code is bundled with u-boot image, so there will @@
> > > -31,6 +32,7 @@ SECTIONS
> > >* avoid hole in the final image.
> > >*/
> 
> Update this comment, not my patch's comment, right?

Correct.

-- 
Tom


signature.asc
Description: Digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] ARM: Disable "DISCARD" for secure section if CONFIG_ARMV7_SECURE_BASE isn't defined

[Dongsheng Wang]

Hi Tom,

Thanks for your review.

> On Thu, Dec 10, 2015 at 10:49:01AM +0800, Dongsheng Wang wrote:
> 
> > From: Wang Dongsheng 
> >
> > Fix PSCI hang up without CONFIG_ARMV7_SECURE_BASE define.
> > "DISCARD" will remove ._secure.text relocate, but PSCI framework has
> > already used some absolute address those need to relocate.
> >
> > Use readelf -t -r u-boot show us:
> > .__secure_start addr: 601408e4
> > .__secure_end   addr: 60141460
> >
> > 60141140  0017 R_ARM_RELATIVE
> > 46  _secure_monitor:
> > 47  #ifdef CONFIG_ARMV7_PSCI
> > 48  ldr r5, =_psci_vectors
> >
> > 60141194  0017 R_ARM_RELATIVE
> > 6014119c  0017 R_ARM_RELATIVE
> > 601411a4  0017 R_ARM_RELATIVE
> > 601411ac  0017 R_ARM_RELATIVE
> > 64  _psci_table:
> > 66  .word   psci_cpu_suspend
> > ...
> > 72  .word   psci_migrate
> >
> > 60141344  0017 R_ARM_RELATIVE
> > 6014145c  0017 R_ARM_RELATIVE
> > 202 ldr r5, =psci_text_end
> >
> > Solutions:
> > 1. Change absolute address to RelAdr.
> >Based on LDR (immediate, ARM), we only have 4K offset to jump.
> > Now PSCI code size is close to 4K size that is LDR limit jump size, so
> > even if the LDR is based on the current instruction address, there is
> > also have a risk for RelAdr. If we use two jump steps I think we can
> > fix this issue, but looks too hack, so give up this way.
> >
> > 2. Enable "DISCARD" only for CONFIG_ARMV7_SECURE_BASE has defined.
> >If CONFIG_ARMV7_SECURE_BASE is defined in platform, all of secure
> > will in the BASE address that is absolute. psci_update_dt will
> > relocate the PSCI code into link stage
> > address(CONFIG_ARMV7_SECURE_BASE address), so using this method.
> >
> > Signed-off-by: Wang Dongsheng 
> > Reviewed-by: Peng Fan 
> > Cc: Albert Aribaud 
> > Cc: Tom Warren 
> > Cc: York Sun 
> > Cc: Hans De Goede 
> > Cc: Ian Campbell 
> > Cc: Tom Rini 
> > Cc: Jan Kiszka 
> > Cc: Stefano Babic 
> >
> > diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds index
> > d48a905..413d459 100644
> > --- a/arch/arm/cpu/u-boot.lds
> > +++ b/arch/arm/cpu/u-boot.lds
> > @@ -14,6 +14,7 @@ OUTPUT_ARCH(arm)
> >  ENTRY(_start)
> >  SECTIONS
> >  {
> > +#if defined(CONFIG_ARMV7_SECURE_BASE) &&
> defined(CONFIG_ARMV7_NONSEC)
> > /*
> >  * Discard the relocation entries for secure text.
> >  * The secure code is bundled with u-boot image, so there will @@
> > -31,6 +32,7 @@ SECTIONS
> >  * avoid hole in the final image.
> >  */

Update this comment, not my patch's comment, right?

Regards,
-Dongsheng

> > /DISCARD/ : { *(.rel._secure*) }
> > +#endif
> > . = 0x;
> >
> > . = ALIGN(4);
> 
> Please update the big comment that you're now #if'ing around to explain why
> the statement isn't true in some cases and hence the test, thanks!
> 
> --
> Tom
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] ARM: Disable "DISCARD" for secure section if CONFIG_ARMV7_SECURE_BASE isn't defined

[Dongsheng Wang]

Hi Tom,

> On Fri, Dec 11, 2015 at 10:15:03AM +, Dongsheng Wang wrote:
> > Hi Tom,
> >
> > Thanks for your review.
> >
> > > On Thu, Dec 10, 2015 at 10:49:01AM +0800, Dongsheng Wang wrote:
> > >
> > > > From: Wang Dongsheng 
> > > >
> > > > Fix PSCI hang up without CONFIG_ARMV7_SECURE_BASE define.
> > > > "DISCARD" will remove ._secure.text relocate, but PSCI framework
> > > > has already used some absolute address those need to relocate.
> > > >
> > > > Use readelf -t -r u-boot show us:
> > > > .__secure_start addr: 601408e4
> > > > .__secure_end   addr: 60141460
> > > >
> > > > 60141140  0017 R_ARM_RELATIVE
> > > > 46  _secure_monitor:
> > > > 47  #ifdef CONFIG_ARMV7_PSCI
> > > > 48  ldr r5, =_psci_vectors
> > > >
> > > > 60141194  0017 R_ARM_RELATIVE
> > > > 6014119c  0017 R_ARM_RELATIVE
> > > > 601411a4  0017 R_ARM_RELATIVE
> > > > 601411ac  0017 R_ARM_RELATIVE
> > > > 64  _psci_table:
> > > > 66  .word   psci_cpu_suspend
> > > > ...
> > > > 72  .word   psci_migrate
> > > >
> > > > 60141344  0017 R_ARM_RELATIVE
> > > > 6014145c  0017 R_ARM_RELATIVE
> > > > 202 ldr r5, =psci_text_end
> > > >
> > > > Solutions:
> > > > 1. Change absolute address to RelAdr.
> > > >Based on LDR (immediate, ARM), we only have 4K offset to jump.
> > > > Now PSCI code size is close to 4K size that is LDR limit jump
> > > > size, so even if the LDR is based on the current instruction
> > > > address, there is also have a risk for RelAdr. If we use two jump
> > > > steps I think we can fix this issue, but looks too hack, so give up 
> > > > this way.
> > > >
> > > > 2. Enable "DISCARD" only for CONFIG_ARMV7_SECURE_BASE has defined.
> > > >If CONFIG_ARMV7_SECURE_BASE is defined in platform, all of
> > > > secure will in the BASE address that is absolute. psci_update_dt
> > > > will relocate the PSCI code into link stage
> > > > address(CONFIG_ARMV7_SECURE_BASE address), so using this method.
> > > >
> > > > Signed-off-by: Wang Dongsheng 
> > > > Reviewed-by: Peng Fan 
> > > > Cc: Albert Aribaud 
> > > > Cc: Tom Warren 
> > > > Cc: York Sun 
> > > > Cc: Hans De Goede 
> > > > Cc: Ian Campbell 
> > > > Cc: Tom Rini 
> > > > Cc: Jan Kiszka 
> > > > Cc: Stefano Babic 
> > > >
> > > > diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
> > > > index
> > > > d48a905..413d459 100644
> > > > --- a/arch/arm/cpu/u-boot.lds
> > > > +++ b/arch/arm/cpu/u-boot.lds
> > > > @@ -14,6 +14,7 @@ OUTPUT_ARCH(arm)
> > > >  ENTRY(_start)
> > > >  SECTIONS
> > > >  {
> > > > +#if defined(CONFIG_ARMV7_SECURE_BASE) &&
> > > defined(CONFIG_ARMV7_NONSEC)
> > > > /*
> > > >  * Discard the relocation entries for secure text.
> > > >  * The secure code is bundled with u-boot image, so there will
> > > > @@
> > > > -31,6 +32,7 @@ SECTIONS
> > > >  * avoid hole in the final image.
> > > >  */
> >
> > Update this comment, not my patch's comment, right?
> 
> Correct.
> 
Not sure we hope a detailed comment or concise comment.
Could you review my comment?

If my understanding is wrong, please correct me, thanks:
/*
 * Based on the /DISCARD/ introduce by ARMv7 patch. And ARMv8 not
 * for sure has the same issue. Based on conservative this is only for
 * ARMv7, another point the /DISCARD/ may isn't necessary in platform.
 * Please see below investigation:
 *
 * If undefine CONFIG_ARMV7_SECURE_BASE secure zone will be
 * included in u-boot space, and some absolute address were used
 * in secure code. Accompanied by u-boot relocation secure code
 * also need to relocate the absolute address.
 *
 * If CONFIG_ARMV7_SECURE_BASE is true, secure code will not
 * bundle with u-boot, and codes offset are fixed. Secure zone
 * only needs to be copied from loading address to
 * CONFIG_ARMV7_SECURE_BASE, which is the linking and running
 * address for secure code.
 *
 * About below may depend on toolchain:
 * 1. If keep the relocation entries in .rel.dyn section,
 * "relocation offset + linking address" may locates into an
 * address that is reserved by SoC, then will trigger data abort.
 * The reason that move .rel._secure at the beginning, is to
 * avoid hole in the final image.
 *
 * 2. .rel.dyn will not include secure code, becuase
 * CONFIG_ARMV7_SECURE_BASE give us an real absolute address, all
 * of codes offset has fixed on build and link stage, and the same
 * to runtime address.
 * e.g:
 * NXP Layerscape platform, gcc version:
 * crosstool-NG 

Re: [U-Boot] [PATCH] ARM: Disable "DISCARD" for secure section if CONFIG_ARMV7_SECURE_BASE isn't defined

[Tom Rini]

On Thu, Dec 10, 2015 at 10:49:01AM +0800, Dongsheng Wang wrote:

> From: Wang Dongsheng 
> 
> Fix PSCI hang up without CONFIG_ARMV7_SECURE_BASE define.
> "DISCARD" will remove ._secure.text relocate, but PSCI framework
> has already used some absolute address those need to relocate.
> 
> Use readelf -t -r u-boot show us:
> .__secure_start   addr: 601408e4
> .__secure_end addr: 60141460
> 
> 60141140  0017 R_ARM_RELATIVE
> 46_secure_monitor:
> 47#ifdef CONFIG_ARMV7_PSCI
> 48  ldr r5, =_psci_vectors
> 
> 60141194  0017 R_ARM_RELATIVE
> 6014119c  0017 R_ARM_RELATIVE
> 601411a4  0017 R_ARM_RELATIVE
> 601411ac  0017 R_ARM_RELATIVE
> 64_psci_table:
> 66.word   psci_cpu_suspend
> ...
> 72.word   psci_migrate
> 
> 60141344  0017 R_ARM_RELATIVE
> 6014145c  0017 R_ARM_RELATIVE
> 202   ldr r5, =psci_text_end
> 
> Solutions:
> 1. Change absolute address to RelAdr.
>Based on LDR (immediate, ARM), we only have 4K offset to jump.
> Now PSCI code size is close to 4K size that is LDR limit jump size,
> so even if the LDR is based on the current instruction address,
> there is also have a risk for RelAdr. If we use two jump steps I
> think we can fix this issue, but looks too hack, so give up this way.
> 
> 2. Enable "DISCARD" only for CONFIG_ARMV7_SECURE_BASE has defined.
>If CONFIG_ARMV7_SECURE_BASE is defined in platform, all of secure
> will in the BASE address that is absolute. psci_update_dt will relocate
> the PSCI code into link stage address(CONFIG_ARMV7_SECURE_BASE address),
> so using this method.
> 
> Signed-off-by: Wang Dongsheng 
> Reviewed-by: Peng Fan 
> Cc: Albert Aribaud 
> Cc: Tom Warren 
> Cc: York Sun 
> Cc: Hans De Goede 
> Cc: Ian Campbell 
> Cc: Tom Rini 
> Cc: Jan Kiszka 
> Cc: Stefano Babic 
> 
> diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
> index d48a905..413d459 100644
> --- a/arch/arm/cpu/u-boot.lds
> +++ b/arch/arm/cpu/u-boot.lds
> @@ -14,6 +14,7 @@ OUTPUT_ARCH(arm)
>  ENTRY(_start)
>  SECTIONS
>  {
> +#if defined(CONFIG_ARMV7_SECURE_BASE) && defined(CONFIG_ARMV7_NONSEC)
>   /*
>* Discard the relocation entries for secure text.
>* The secure code is bundled with u-boot image, so there will
> @@ -31,6 +32,7 @@ SECTIONS
>* avoid hole in the final image.
>*/
>   /DISCARD/ : { *(.rel._secure*) }
> +#endif
>   . = 0x;
>  
>   . = ALIGN(4);

Please update the big comment that you're now #if'ing around to explain
why the statement isn't true in some cases and hence the test, thanks!

-- 
Tom


signature.asc
Description: Digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH] ARM: Disable "DISCARD" for secure section if CONFIG_ARMV7_SECURE_BASE isn't defined

[Dongsheng Wang]

From: Wang Dongsheng 

Fix PSCI hang up without CONFIG_ARMV7_SECURE_BASE define.
"DISCARD" will remove ._secure.text relocate, but PSCI framework
has already used some absolute address those need to relocate.

Use readelf -t -r u-boot show us:
.__secure_start addr: 601408e4
.__secure_end   addr: 60141460

60141140  0017 R_ARM_RELATIVE
46  _secure_monitor:
47  #ifdef CONFIG_ARMV7_PSCI
48  ldr r5, =_psci_vectors

60141194  0017 R_ARM_RELATIVE
6014119c  0017 R_ARM_RELATIVE
601411a4  0017 R_ARM_RELATIVE
601411ac  0017 R_ARM_RELATIVE
64  _psci_table:
66  .word   psci_cpu_suspend
...
72  .word   psci_migrate

60141344  0017 R_ARM_RELATIVE
6014145c  0017 R_ARM_RELATIVE
202 ldr r5, =psci_text_end

Solutions:
1. Change absolute address to RelAdr.
   Based on LDR (immediate, ARM), we only have 4K offset to jump.
Now PSCI code size is close to 4K size that is LDR limit jump size,
so even if the LDR is based on the current instruction address,
there is also have a risk for RelAdr. If we use two jump steps I
think we can fix this issue, but looks too hack, so give up this way.

2. Enable "DISCARD" only for CONFIG_ARMV7_SECURE_BASE has defined.
   If CONFIG_ARMV7_SECURE_BASE is defined in platform, all of secure
will in the BASE address that is absolute. psci_update_dt will relocate
the PSCI code into link stage address(CONFIG_ARMV7_SECURE_BASE address),
so using this method.

Signed-off-by: Wang Dongsheng 
Reviewed-by: Peng Fan 
Cc: Albert Aribaud 
Cc: Tom Warren 
Cc: York Sun 
Cc: Hans De Goede 
Cc: Ian Campbell 
Cc: Tom Rini 
Cc: Jan Kiszka 
Cc: Stefano Babic 

diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
index d48a905..413d459 100644
--- a/arch/arm/cpu/u-boot.lds
+++ b/arch/arm/cpu/u-boot.lds
@@ -14,6 +14,7 @@ OUTPUT_ARCH(arm)
 ENTRY(_start)
 SECTIONS
 {
+#if defined(CONFIG_ARMV7_SECURE_BASE) && defined(CONFIG_ARMV7_NONSEC)
/*
 * Discard the relocation entries for secure text.
 * The secure code is bundled with u-boot image, so there will
@@ -31,6 +32,7 @@ SECTIONS
 * avoid hole in the final image.
 */
/DISCARD/ : { *(.rel._secure*) }
+#endif
. = 0x;
 
. = ALIGN(4);
-- 
2.1.0.27.g96db324

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot