[U-Boot] [PATCH 1/3] ARM: relocate: fix hang when CONFIG_ARMV7_SECURE_BASE
Li Frank
Frank.Li at freescale.com
Mon Oct 19 16:13:13 CEST 2015
> -----Original Message-----
> From: Peng Fan [mailto:b51431 at freescale.com]
> Sent: Monday, October 19, 2015 7:47 AM
> To: Albert ARIBAUD <albert.u.boot at aribaud.net>
> Cc: Li Frank-B20596 <Frank.Li at freescale.com>; lznuaa at gmail.com; u-
> boot at lists.denx.de; sbabic at denx.de; Estevam Fabio-R49496
> <Fabio.Estevam at freescale.com>; otavio at ossystems.com.br;
> trini at konsulko.com; hdegoede at redhat.com; Fan Peng-B51431
> <Peng.Fan at freescale.com>
> Subject: Re: [U-Boot] [PATCH 1/3] ARM: relocate: fix hang when
> CONFIG_ARMV7_SECURE_BASE
>
> 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?
Yes.
Secure_text + relocate_offset will be invalidate address.
So generate data abort.
Best regards
Frank Li
>
> 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.
Their secure_text + relocate_offset is possible in unused validate memory region.
If not data abort, this don't hurt anything.
Best regards
Frank Li
>
> 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.
>
> --
More information about the U-Boot
mailing list