Hi Albert,
On Saturday, March 2, 2013 7:45:06 AM, Albert ARIBAUD wrote:
Hi Benoît,
On Fri, 1 Mar 2013 23:54:26 +0100 (CET), Benoît Thébaudeau benoit.thebaudeau@advansee.com wrote:
Re: assembler error messages:
And in case you ask, with relocate_code() as a function in its own file instead of a macro called from start.S, it does not work because of the _start-relative word values that require relocate_code() to be in _start's section.
How does it "not work" exactly?
The assembler issues an error (I don't remember the exact message) for all lines like ".word __rel_dyn_start - _start", complaining that this is not a kind of expression that it can prepare for the linker to resolve.
Though, it would still be possible to find a more complicated way of doing the same thing at runtime.
(later)
This whole thing/way of "factorizing" start.S using macros feels wrong to me; this departs from what I have started with crt0.S, where code is actually really factorized in a single file which is actually a compilation unit, not a helper file.
Yes. Well, for this patch, I had first moved relocate_code() to crt0.S (could have been its own file), but I eventually switched to the macro solution because of the assembler errors.
I've had issue in the past similar to this when I implemented the ELF relocation, the crt0.S factorization and more recently the R_ARM_ABS32 relocation record removal. These must be fixed with much attention, for instance in order to not produce R_ARM_ABS32 relocations, the removal of which I have just submitted a patch for. I think there is a way to factorize relocate_code() (and other parts) out of start.S files, built on what I did for crt0.S, and which should not cause these issues.
Yes, I think so. In the worst case, it should be possible to access out-of-range symbol relatively using adr or adrl extensively at runtime instead of pre-computed _start-relative offsets.
Re: patch 31/31 generally:
This patch is just appended to the series because it depends on it, not because the series needs it.
This patch only aims at cleaning up code by removing copied/pasted code in order to simplify maintenance and to clarify things.
There have been many changes in this relocate_code(), and not always the same for all start.S, so from the outside, the purpose is unclear because one might wonder if those differences have been created on purpose or not.
(picked up from later in the reply)
Do you need patch 31/31 in your series?
As explained above, no. But I really think that something should be done to stop relocate_code() duplication, one way or another, by me or someone else. I just wanted to help here.
First, let me say that I appreciate the great help that you're giving us with this (30-patch!) series.
And I agree about the premise that ARM startup sequence, including but not limited to relocate_code(), is literally all over the place and that there is a need to fix this; I wrote so in the cover letter of my crt0.S patch series, which I consider a starting point and example of how I consider this should be done.
Also, we must keep in mind that part of the code in ARM should, and eventually will, be merged into a single U-Boot-wide version. ELF code relocation is not ARM specific except for the two (to be reduced soon to only one) ARM relocation record types. Thus, when we touch this code, we must keep it close, or make it closer, to the code in other U-Boot architectures; IIRC there are already patches out there to make relocate_code() a single project-wide true C function.
And I think that this newly added patch 31/31 in your series does not match either the way I want ARM startup simplification to go or the general goal of unifying relocate_code().
Thus, if you don't mind, I'd prefer patch 31/31 to move out of the series. And, since I want to avoid anyone the hassle of going through this again, I guess I will have to submit a patch for relocate_code() factorization -- quite probably above your series, since many fixes you make in it may be useful or even needed.
OK, let's do this. It will also help to stop postponing the application of this series because of more new versions.
Please just Cc me when you will post these patches so that I review them.
Best regards, Benoît