Hi,
On 22 February 2018 at 08:08, Alexander Graf agraf@suse.de wrote:
On 15.02.18 07:40, Andre Heider wrote:
The value at the end of the rom is not a pointer, it is an offset relative to the end of rom.
Do you have any documentation pointers to this? Even just pointing to a specific line of code in coreboot would be helpful in case anyone wants to read it up.
Signed-off-by: Andre Heider a.heider@gmail.com
fs/cbfs/cbfs.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/cbfs/cbfs.c b/fs/cbfs/cbfs.c index 6e1107d751..46da8f134f 100644 --- a/fs/cbfs/cbfs.c +++ b/fs/cbfs/cbfs.c @@ -168,9 +168,9 @@ static int file_cbfs_load_header(uintptr_t end_of_rom, struct cbfs_header *header) { struct cbfs_header *header_in_rom;
int32_t offset = *(u32 *)(end_of_rom - 3);Accessing a pointer that gets subtracted by 3 looks terribly wrong. Unfortunately it's correct, because the pointer itself is unaligned.
How about we change the logic so that we calculate an aligned pointer (after_rom?) which we sanity check for alignment and base all calculations on that instead?
That way the next time someone comes around he's not getting puzzled why we're subtracting 3 and adding 1 to the pointer ;).
Either that or a comment would be nice. But since this has been sitting around for a while and fixes a bug I think it is best to take it. Please feel free to send a follow-up patch..
We also have no tests for this code, so I'd really like to get some tests in there!
Reviewed-by: Simon Glass sjg@chromium.org
Regards, Simon