Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #550: OpenAMP elf loader loads ELF sections to their load addresses #551

Merged
merged 1 commit into from
Feb 16, 2024

Conversation

UmairKhanUET
Copy link
Contributor

@UmairKhanUET UmairKhanUET commented Feb 6, 2024

Unlike the Linux rproc elf loader, the OpenAMP elf loader loads the remote elf program segments to their load addresses instead of their link addresses. This results in memory corruption when the remote firmware attempts to relocate segments from their link addresses to load addresses at runtime such as the data section.

Signed-off-by: Umair Khan [email protected]

Unlike the Linux rproc elf loader, the OpenAMP elf loader loads
the remote elf program segments to their load addresses instead
of their link addresses. This results in memory corruption when
the remote firmware attempts to relocate segments from their link
addresses to load addresses at runtime such as the data section.

Signed-off-by: Umair Khan <[email protected]>
Copy link
Collaborator

@arnopo arnopo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM,
@tnmysh , @edmooring could you verify that this does not introduce regression on yopur platforms?

@tnmysh
Copy link
Collaborator

tnmysh commented Feb 7, 2024

Yes, I will get back on this. I was wondering how its working without this patch.

@bentheredonethat FYI.

@arnopo
Copy link
Collaborator

arnopo commented Feb 7, 2024

Yes, I will get back on this. I was wondering how its working without this patch.

@bentheredonethat FYI.

Probably because you have same memory mapping regarding the main and remote processor.

@UmairKhanUET
Copy link
Contributor Author

Yes, I will get back on this. I was wondering how its working without this patch.

@bentheredonethat FYI.

May be because generally when firmware runs out of RAM, the ELF sections are placed at same link and load addresses. But it doesn't stop one from having different addresses either. May be this is the first time we are seeing such a case.

@arnopo
Copy link
Collaborator

arnopo commented Feb 15, 2024

LGTM, @tnmysh , @edmooring could you verify that this does not introduce regression on yopur platforms?

@tnmysh , @edmooring, @bentheredonethat : Gentle reminder

@tnmysh
Copy link
Collaborator

tnmysh commented Feb 15, 2024

@arnopo I have been trying to look for history on when this code was used on Xilinx's platforms. I can't find it being used currently or anytime in near past. It's very hard to prepare setup for this atleast with 2023 petalinux.

The code looks okay for me, and we don't have any use case currently that is practicing this code. I am okay with code, and sure no regression will happen on xlnx platforms as it's not being used.

Copy link
Contributor

@edmooring edmooring left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change looks correct to me.
With the default CMake options, this code can only be built if the target is zynqmp_r5. It also has to be specifically enabled with WITH_LOAD_FW=on. The default Petalinux/Yocto build for the R5 forces this option to "off".
To my knowledge, this code has not been exercised, much less tested, in either the official OpenAMP repository or in the Xilinx/AMD downstream in at least 5 years.

@arnopo
Copy link
Collaborator

arnopo commented Feb 16, 2024

Thanks @edmooring and @tnmysh for the feedback.

We have to find a way to better test it.
Look to me that the load code could be optimized to make this more simple with less footprint.
let's add this in agenda of next open-amp meeting call.

@arnopo arnopo merged commit b48965a into OpenAMP:main Feb 16, 2024
3 checks passed
@arnopo arnopo added this to the Release V2024.04 milestone Feb 19, 2024
@UmairKhanUET UmairKhanUET deleted the openamp-550 branch February 28, 2024 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OpenAMP ELF loader loads ELF sections to their load addresses.
4 participants