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

Fix relocation algorithm description #12

Merged
merged 1 commit into from
Aug 3, 2023

Conversation

jwerner-chromium
Copy link
Contributor

This patch fixes a mistake that crept in during review of PR #6 (patch 062d542). The alignment_offset calculated from tl_base_addr was not superfluous in that calculation -- in fact, this is the only way in which tl_base_addr entered the calculation at all. If we don't have this, relocation for base addresses that do not already happen to match the highest alignment requirement won't work.

Also fix another small mistake in the same algorithm regarding calculating the new tl.max_size.

This patch fixes a mistake that crept in during review of PR FirmwareHandoff#6 (patch
062d542). The `alignment_offset` calculated from `tl_base_addr` was not
superfluous in that calculation -- in fact, this is the only way in
which `tl_base_addr` entered the calculation at all. If we don't have
this, relocation for base addresses that do not already happen to match
the highest alignment requirement won't work.

Also fix another small mistake in the same algorithm regarding
calculating the new `tl.max_size`.

Signed-off-by: Julius Werner <jwerner@chromium.org>
Copy link
Contributor

@sjg20 sjg20 left a comment

Choose a reason for hiding this comment

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

This is really confusing.

Can we use alignx() to align to x bytes?

Then I think some of these expressions might become more intuitive

@jwerner-chromium
Copy link
Contributor Author

Can we use alignx() to align to x bytes?

Well, we do use align8() where applicable, but this case is not so simple. It's not just aligning a value, it's masking off the offset from the previous alignment boundary, and then adding that to the down-aligned new address. Since those are a bunch of unique operations that only appear here in this document I think it's probably clearer to write them out directly rather than define new shorthands that would only be used in one place (especially since the alignment operation here is aligning down, whereas the align8() we use elsewhere means aligning up).

Copy link
Contributor

@sjg20 sjg20 left a comment

Choose a reason for hiding this comment

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

As I've never quite been a fan of the idea of maintaining an alignment offset, I would prefer not to comment on this one. I'm approving and will let Dan decide

Copy link
Contributor

@danh-arm danh-arm left a comment

Choose a reason for hiding this comment

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

I'm good with this and Julius's rationale for not introducing new alignment macros

@jwerner-chromium jwerner-chromium temporarily deployed to pr_ci August 3, 2023 13:06 — with GitHub Actions Inactive
@danh-arm danh-arm merged commit 3dd0d1c into FirmwareHandoff:main Aug 3, 2023
2 checks passed
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.

None yet

3 participants