> -----Original Message----- > From: Peng Fan [mailto:[email protected]] > Sent: Monday, October 19, 2015 7:47 AM > To: Albert ARIBAUD <[email protected]> > Cc: Li Frank-B20596 <[email protected]>; [email protected]; u- > [email protected]; [email protected]; Estevam Fabio-R49496 > <[email protected]>; [email protected]; > [email protected]; [email protected]; Fan Peng-B51431 > <[email protected]> > 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. > > -- _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
