[U-Boot] [PATCH 1/3] ARM: relocate: fix hang when CONFIG_ARMV7_SECURE_BASE

[Peng Fan]

Hi Albert,
On Mon, Oct 19, 2015 at 01:48:25PM +0200, Albert ARIBAUD wrote:
>Hello Peng,
>
>(cutting a bit through the previous mails quoting)
>
>> >This, in turn, leads to new questions:
>> >
>> >1. How is this PSCI code put in place? Is it bundled with the image,
>> >   with a specificy copy routine which puts it in place then locks the
>> Yeah.
>> >   memory range against writing? Or is it loaded in place by some other
>> >   agent than U-Boot, and how does this agent know what to put where?
>> 
>> In arch/arm/cpu/armv7/virt-v7.c, relocate_secure_section() does this job.
>> 
>> CONFIG_ARMV7_SECURE_BASE is the location that secure code will be copied
>> to. To ARMV7, PSCI code is bundled with the uboot image.
>
>Hmm, the 'relocate' part of the function name is a somewhat non-optimal
>choice, since the routine just copies data without modifying it.
>
>Looking at relocate_secure_section(), I see that it it is called long
>after the U-Boot code was relocated -- actually, it is called during
>bootm.

oh. You remind me, thanks. There maybe something wrong with my previous analysis.

I am not the one who finds the issue, just my understanding.
The data abort may be triggered by line 104 or line 106.
96 fixloop:
97         ldmia   r2!, {r0-r1}            /* (r0,r1) <- (SRC location,fixup) */
98         and     r1, r1, #0xff   
99         cmp     r1, #23                 /* relative fixup? */
100         bne     fixnext
101
102         /* relative fix: increase location by offset */
103         add     r0, r0, r4
104         ldr     r1, [r0]
105         add     r1, r1, r4
106         str     r1, [r0]

At line 104 or 106, the value of r0 may be an address that can not be accessed
which trigger data abort.

Frank, am I right?

I'll set up one board to test this.

>
>This means that for any one of the other boards around that seem to use
>PSCI, either "their" PSCI code does not have any relocations (which I
>doubt), or it has but they don't cause any data abort when done by the
>relocate_code() routine.

There maybe something wrong from me. See above.

>
>So what's the difference between those and your board? My bet is that
>their CONFIG_ARMV7_SECURE_BASE is not write-protected at the time the
>relocate_code() routine executes (whereas on your board it is), and
>will be unlocked in some way by the time relocate_secure_section()
>executes.
>
>I'm not saying that the correct solution would be to enable write
>access to CONFIG_ARMV7_SECURE_BASE before running relocate_code(),
>because doing that would be obviously wrong: relocate_code() would try
>to fix code which is not there yet.

Yeah. This is true, relocate_code may fix code which is not there.

>
>I'm just saying that's what actually happens, and if I am right, then
>it confirms that the correct fix is to not let relocations to the
>secure stay in the U-Boot ELF, or better yet, to not let them happen to
>boot [pun half-intended].

They do not happen to boot. My bad. See above.

>
>> CONFIG_ARMV7_SECURE_BASE is the location that secure code will be copied
>> to. To ARMV7, PSCI code is bundled with the uboot image.
>> 
>> >
>> >2. Does this code and the relocatable image refer to symbols of one
>> >   another,
>> Yeah. There is asm function reference. You can see arch/arm/cpu/armv7/psci.S
>
>Looking at this file, I suspect that actually, PSCI is called through a
>software interrupt / exception vector, not through a function name, and
>the only symbolic relationship is via __secure_start and __secure_end
>-- and those must (and will) be correctly relocated along with the
>U-Boot image.
>
>> >or do they interact through an IPC independent of their
>> >   respective location (in which case, one wonders why they are built
>> >   into a single ELF file)?
>> 
>> This comes a question, why PSCI framework intergated into U-Boot? I have no idea.
>
>There could have been alternative designs, indeed. Here is the design
>now, so let's handle it.
>
>> >More generally... which target do you see this problem on? Reproducing
>> >the build myself would save both you and me some time, I guess. :)
>> 
>> Build target: mx7dsabresd
>> 
>> Needs to apply the three patches:
>> https://patchwork.ozlabs.org/patch/527040/
>> https://patchwork.ozlabs.org/patch/527038/
>> https://patchwork.ozlabs.org/patch/527039/
>> 
>> There is a small difference from my previous log, since my previous log use
>> 0x180000 as the secure address, but the patches use 0x900000 as the secure
>> address. But sure there will be relocation entry there.
>
>Thanks. I'll try and play with compiler / linker options, but from
>my own experience, this can be tricky. Meanwhile, I suggest you for for
>the not-too-quick-and-not-too-dirty linker script solution.
>
>> >Do you need these relocation records? If not, then just append the
>> >'*(.rel._secure*) input section spec (with a suitable comment) to
>> >the already existing DISCARD output section. By the way, doesn't this
>> >linker script change alone fix the data abort?
>> 
>> Yeah. Since the relocation entry for secure section will be stored
>> in rel.secure section. And this section will not be touched in relocate.S
>
>Ok, so that's our first clean, linker-script-based, solution confirmed.
>
>> >Still, if there *are* relocation records emitted, that's because the
>> >toolchain considered them necessary -- IOW, it was (wrongly) told the
>> >PSCI code must be relocatable [or the code really is relocatable and we
>> >just haven't not hit a bug about this yet].
>> 
>> The PSCI code is bundled with the u-boot image. But it's running address
>> is not same with u-boot. In my example, u-boot is compiled with starting
>> address 0x87800000, but to PSCI, it's running address is 0x180000.
>> relocate_secure_section is just copy the psci code from uboot to 0x180000.
>> 
>> compliation address is same with running address, to PSCI part.
>
>(I understand that. My question was not "why is PSCI built for its
>run-time address?" but "Since PSCI is built for its run-time address,
>and since there is little dependency on how come it is built as
>relocatable?")

uboot will not effect after switch to kernel, and its image will be overriden
by kernel. But PSCI will always effect during kernel lifecyle.
Maybe built PSCI part non-relocatable is an alternative choice.

>
>> >Testing every single relocation record target against __image_copy_start
>> >and __image_copy_end would be rather costly in the tight loop of the
>> >relocation routine, which should run as fast as possible.
>> >
>> >There is at least one fix to your problem which removes the out-of-
>> >bounds relocation records, and quite probably one better fix which
>> >prevents generating them in the first place.
>> >
>> >Therefore, instead of the currently proposed patch which increases the
>> >size (very slightly) and boot time (statistically in O(n) proportion)
>> >of the image, I prefer one of the two alternative solutions which
>> >decrease the image size (by removing useless relocation records) and
>> >leave the boot time unchanged (since the fix is at build time only).
>> 
>> Agree.
>
>OK. So, for now, I suggest that you:
>
>- resubmit v2 of this series with patch 1/3 reworked into a linker
>  script change rather than a relocate routine change (and collect the
>  secure relocation records input sections into the DISCARD output
>  section rather than into a purposeless non-discarded section);
>
>- make sure that you CC: the maintainers for the other boards which use
>  PSCI and explicitly ask them to test the change on their boards and
>  give their "Tested-by". It seems like the list of candidates for
>  testing includes jetson-tk1, ls1021aqds, ls1021atwr and all sun[678]i
>  boards, but I am by no means a PSCI specialist, so anyone feel free
>  to correct me about this list.
>
>> Regards,
>> Peng.
>
>Amicalement,
>-- 
>Albert.

--