How about to provide an option like spike -m<addr:size> here

[soberl]

https://github.com/rems-project/sail-riscv/blob/ae6cb1de092e9ea727e2318d76e9b88999bbee59/c_emulator/riscv_platform_impl.c#L15

E.g., for m mode, the physical address is not start from 0x80000000, or even not continued. Spike provides option -m to assign a list of memory areas.

Regards.

[pmundkur]

Would it suffice for your purpose if the memory regions needed to be specified in an FDT file such as those typically used for OS boots?

[soberl]

Thanks for the reply.
For the start address, currently is hard coded in sail-riscv:
uint64_t rv_ram_base = UINT64_C(0x80000000); mach_bits plat_ram_base(unit u) { return rv_ram_base; }
The FDT (dtc for spike) can define another base address for sure, how could that base address applied in riscv-sail? Maybe this is a silly question since I am new to sail. :)

[allenjbaum]

Before we dive into this: why do we need to specify a base other than zero in the Sail model? Sail is supposed to be an archtectural simulator, and zero is the architectural base. What effect does setting this value have on running code that tries to access something below 0x80000000?

[allenjbaum]

I haven't heard a response to my question, so let me rephrase this:
what would break if we hardcoded rv_ram_base to 0x0?
I don't like unresolved issues hanging around, so I'll assume it doesn't matter and close this issue at the end of the year if I get no response, or will ask for a PR to set it to zero if it doesn't break anything.

[allenjbaum]

I haven't seen any response. I'm going to authorize a Sail change to default this to 0. If someone can point out how that breaks something, we'll add a command line option that resembles what spike provides

[jrtc27]

0x80000000 is the de-facto standard. If you use BBL, OSes rely on BBL having been loaded in the first superpage of physical memory, and BBL by default is linked at 0x80000000, so your physical memory must start at 0x80000000 if you want to be able to use BBL.

[jrtc27]

(if it's lower, OSes will infer BBL to be at an earlier location than it actually is and trample over the top of it, trapping if you have a PMP and totally corrupting it if not)

[jrtc27]

So, adding an option is a great idea, but changing the hard-coded value is not as that breaks real-world cases

[allenjbaum]

Thanks - I've been waiting for an answer like that. I'm a bit uncomfortable with having it be a default value ((which I am assuming is just a default, and could be changed) when it not an architectural requirement But, like many other things, it's now legacy and we have to live with it. So, the hardcoded default value should remain, and the option to change it should (and shall) be added. Is there a specific methodology ofr this, or should it by like all the other options we'v been discussing, and be part of the YAML?

…

On Fri, Feb 4, 2022 at 3:47 PM Jessica Clarke ***@***.***> wrote: So, adding an option is a great idea, but changing the hard-coded value is not as that breaks real-world cases — Reply to this email directly, view it on GitHub <#73 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHPXVJWSCPLTXPHH7AYRW63UZRQQXANCNFSM4TD2OBYA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you commented.Message ID: ***@***.***>

[jrtc27]

I think having it as a command line option would be justifiable, it's the kind of thing a user might want to set even without caring about matching a specific configuration

[martinberger]

If you use BBL,

BBL is short for Berkeley boot loader, right?

[jrtc27]

If you use BBL,

BBL is short for Berkeley boot loader, right?

Yes, which, unhelpfully, is a component of the riscv-pk repository.

[allenjbaum]

I figured it was Boot Loader, but had no idea what the first "B" was for.