[htif,elfloader] Add an option to skip target memory loading

[ss2783]

Adds an option to htif which allows emulators to skip target memory loading. Useful for implementations that can load memory image snapshots.

[ss2783]

This patch supports reading a test binary which embeds the from_host and to_host addresses but skips loading the test memory for an implementation that magically loads a test memory image to speedup the simulation. Is there a better way to achieve the same?

[aswaterman]

You should be able to do that without modifying this repository, by overriding read_chunk, write_chunk, and reset to make read_chunk and write_chunk do nothing before reset is called.

[ccelio]

But isn't "overriding" read_chunk/write_chunk/reset mean forking the repo to add a new htif subclass? That seems a lot less attractive than adding an extra switch that's fairly contained. I would think that this capability would be useful to many other users.

[aswaterman]

It doesn't necessitate maintaining a fork. Rocket-chip used to have its own htif subclass that overrode some of these methods, without forking this repo.

[aswaterman]

(In other words, there's no reason that your subclass must live in this repo.)

[ccelio]

I'm not sure I follow your suggestion.

Are you recommending we subclass dtm_t or htif_t and maintain a private copy of the new subclass?

If we try to extend dtm_t, the read_chunk/write_chunk/reset functions are private, so we can't extend dtm_t (and friends) and then gate off read_chunk/write_chunk/reset as desired. Instead, it looks like we would have to copy dtm_t and make the desired change. If we instead extend htif_t we effectively end up doing the same copy/paste. Did you have something else in mind, because this seems very fragile.

[aswaterman]

Ah, I didn't realize you were using dtm_t. We didn't envision subclassing dtm_t, so didn't make those methods protected. This is a good reason to make them visible, though, so I'll PR that change.

[ccelio]

Thanks, making those functions protected would significantly reduce the maintenance headache, but is there a larger reason why this specific PR is not acceptable? This still requires subclassing/extending each subclass of htif_t (dtm, tsi, etc.) to patch read_chunk\write_chunk\reset and also forking emulator.cc for the htif argument parsing that otherwise comes for free with this PR.

To me this PR seems to hit a functionality that would be quite useful to a larger group of people. There are a handful of techniques to "magically" write target memory (readmemh, FPGA toolings, service cores in ASIC land...) that are very useful while still wanting to maintain the proxying syscall/tether nature of riscv-fesvr.

Also, patching read_chunk and write_chunk to nop before reset is called is super wonky, lol.

[aswaterman]

Changing the ELF loader doesn't seem to me to be the right way to do it. The HTIF should be the thing that decides which writes it can safely skip.

For instance, you might have multiple memories, only some of them support preloading. (Say, main memory lives in the test harness can be initialized, but not scratchpads within the DUT.) Depending on the addresses in the ELF, only some of the writes can be elided.

[ccelio]

I see, thanks. (Option 1:) We patched load_elf because we still needed to get access to the htif tohost and fromhost addresses so we needed to run load_elf but not actually have it load the elf to target memory.

(Option 2:) We considered doing the reset hack dance but using implicit/global class state to gate off reads and writes felt more opaque and less maintainable.

(Option 3:) Would you accept a patch where we pass in tohost and fromhost addresses via the CLI args, and then set binary to none, so then htif avoids invoking load_elf? That seems the least invasive (and only requires us changing the build system to inject tohost/fromhost).

[aswaterman]

Yeah, I understand the rationale behind option 1, but it seems to lack the generality of option 2. Consider the rocket-chip case: some designs use preloadable test harness memory, whereas some only have memory inside the DUT, and the Makefiles invoking the simulators certainly don't know which is which. However, the HTIF has more context (including access to the Device Tree) so can make these decisions.

I actually prefer option 1 over option 3, because option 3 is just foisting the work onto the user. This is an important enough feature that we should decide what the right way to support it is, then support it.

It's possible we can do option 2 in a way that avoids coupling read/write with reset. Perhaps it's just a matter of adding a function to htif_t that its subclass can call to query whether it is currently doing the initial program load; then write_chunk can be overridden to do nothing if the function returns true and the address lies within a known-preloaded range. It's the same thing as I proposed before, but it's now an explicitly supported API.

[aswaterman]

I have a variant on option 2 that might be superior. Instead of overriding write_chunk to skip the write during program load, there could be virtual methods on htif_t that indicate whether an address has been preloaded. The default implementation would unconditionally return false, indicating the write must be performed. Subclasses would simply provide different implementations that check the bounds on the address. The subclass doesn't need to care about whether it's in the initial-program-loading phase or not; that is htif_t's job.

In the degenerate case that you always preload, you'd just override the function to unconditionally return true.

Sound OK?

[ccelio]

I see, so the proposal is that read_chunk/write_chunk are performed under the following conditions:

1) Are we past reset?

2) Are we still in reset AND the address was not preloaded?

That seems clean, extendable, and easy to maintain.

[aswaterman]

Yup, that's what I'm thinking.

[aswaterman]

I'll try to mock this up, then pass it over to y'all to see if it works for you.

[ccelio]

Thank you!

[ccelio]

Closed; superseded by #55. The solution is to extend one of htif's subclasses and override the is_address_preloaded function.

[aswaterman]

Closing in favor of #55 — thanks for pushing for this feature.

[aswaterman]

Jinx

[ccelio]

Thanks for working with us on a resolution!

[aswaterman]

Now to add support to Rocket-chip :P