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

[Albert ARIBAUD <albert.u.boot@aribaud.net>]

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.

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.

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.

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].

> 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?")

> >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.