Skip to content

Conversation

@Arielfoever
Copy link
Contributor

add a -d parameter for DYN files. Then give a offset, mostly 0x80000000 like openSBI.

@github-actions
Copy link

github-actions bot commented Jul 26, 2025

Test Results

2 101 tests  ±0   2 101 ✅ ±0   18m 3s ⏱️ +5s
    1 suites ±0       0 💤 ±0 
    1 files   ±0       0 ❌ ±0 

Results for commit 1560d17. ± Comparison against base commit 87eb6fb.

♻️ This comment has been updated with latest results.

.PHONY: sail
sail: build/sail.dtb $(elf) $(SAIL_SIM)
$(SAIL_SIM) --no-trace -p -l $(LIMIT_INSTRUCTIONS) --device-tree-blob build/sail.dtb $(elf)
$(SAIL_SIM) --no-trace -p -l $(LIMIT_INSTRUCTIONS) --device-tree-blob build/sail.dtb $(elf) -d 0x80000000
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be nice to make this the default behaviour when passed a PIE executable

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah if you don't specify it it should use a sane default like the start of DRAM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be nice to make this the default behaviour when passed a PIE executable

No, you need to specify the offset or the loader will reject to load DYN. for openSBI it can be 0x80000000 but different value may be work too.

sail didn't provide the API for detecting whether it is a DYN or EXEC.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok so Sail's load_elf_at_offset is really load_elf_with_dyn(/pie)_offset; that is it will ignore the offset if the provided binary is not ET_DYN. This means you should just use load_elf_at_offset unconditionally with a sensible offset, and if it's an ET_EXEC it will ignore the offset and load at the object's fixed address.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Slightly unfortunate naming. We should definitely add a comment to clarify that behaviour.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The other option is that we just use our own ELF loading code. That's what I ended up doing (using ELFIO).

uint64_t begin_sig, end_sig;
load_elf(f, &is32bit, &entry);
if (load_offset) {
// dynamic load
Copy link
Collaborator

@jrtc27 jrtc27 Jul 26, 2025

Choose a reason for hiding this comment

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

Uh ET_EXEC should ignore it (or give an error, though that's annoying if loading multiple ELF files, e.g. an ET_DYN OpenSBI and an ET_EXEC kernel)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do you mean we load an fw_jump.elf then with a kernel?

Copy link
Collaborator

Choose a reason for hiding this comment

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

fw_jump vs fw_dynamic is an orthogonal concern (referring to whether OpenSBI hard-codes the location at which the payload is loaded) to whether the kernel being loaded requests a fixed address (that may or may not match OpenSBI's expectation).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

currently the sail gives no api on detect whether it is DYN or EXEC.

fprintf(stderr, "Unable to locate tohost symbol; disabling HTIF.\n");
rv_enable_htif = false;
} else {
rv_htif_tohost += load_offset;
Copy link
Collaborator

Choose a reason for hiding this comment

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

These need a way to know whether load_elf_at_offset used load_offset or not; at first glance I don't see a way to tell. I suspect Sail needs a lookup_sym_with_(dyn/pie_)offset that is like load_elf_at_offset in conditionally including the PIE (dynamic) offset.

@nadime15
Copy link
Collaborator

I prefer and suggest we keep the -d option reserved for debug mode, and use a different one for this PR.

@Arielfoever
Copy link
Contributor Author

I prefer and suggest we keep the -d option reserved for debug mode, and use a different one for this PR.

which char do you like?

@Timmmm
Copy link
Collaborator

Timmmm commented Jul 29, 2025

In the other PR to update the CLI we've removed short options so when you rebase on that I'd just remove it.

Also I don't think having a single flag actually works because we support loading multiple elfs. We might need some syntax like foo.elf@0x100 bar.elf@0x400.

@Arielfoever Arielfoever marked this pull request as draft August 4, 2025 04:28
@Arielfoever
Copy link
Contributor Author

Convert to draft since it needs #1177 merging.

@Arielfoever
Copy link
Contributor Author

Do we still need this pr? @Timmmm

@Timmmm
Copy link
Collaborator

Timmmm commented Nov 6, 2025

Yes, if we want to support loading executables at arbitrary offsets. I don't know how much demand there is for that though. What was the original motivation for this?

@pmundkur
Copy link
Collaborator

pmundkur commented Nov 6, 2025

I think the original motivation was loading the default OpenSBI payload.

@allenjbaum
Copy link
Collaborator

If I'm understanding this correctly (not a sure thing), the dynamic part lets the Sail model load ELFs at addresses that are in .json files, as opposed to some fixed address. The .S files should themselves be position independent (because they need to be run.
We want that capability so that we can configure the Sail model to match what a DUT is actually executing
(so exceptions report exactly the same EPC and TVAL, etc)

@Arielfoever
Copy link
Contributor Author

I think the original motivation was loading the default OpenSBI payload.

That's correct.

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.

7 participants