riscv / sail-riscv

Sail RISC-V model
https://lists.riscv.org/g/tech-golden-model
Other
461 stars 167 forks source link

Exit condition for SAIL model simulating elf code is not intuitive #218

Open ssecatch-w opened 1 year ago

ssecatch-w commented 1 year ago

I'm running pre-compiled elf files within the SAIL environment running under the C emulator. There is a variable declared in c_emulator/riscv_platform_impl.c called rv_htif_tohost, which appears to be unused in favor of the tohost label in my load file. I don't know if that's a default that's overridden because that tag exists or if it's unused.

However, it seems that a sw to the tohost location is needed three times before the simulation terminates (with a successful exit code). I don't understand why a single write to that location is not sufficient. (I see three htif updates, but htif-syscall-proxy only happens on the third)

I didn't see any requirements listed within the documentation for conditions to end the simulation, but I think that these need to be documented, and it seems incorrect to me that three writes are needed to a tohost address in order to terminate.

Alasdair commented 1 year ago

I didn't write the code, but it seems like a workaround for tests that don't write the control bytes? There's this comment:

/ Applications sometimes write the lower 32-bit payload bytes without writing the control bytes; this is seen in the riscv-tests suite. However, processing the payload bytes too early could miss a subsequent write to the control bytes. As a workaround, if the payload is written a few times with the same value, without an intervening write to the control bytes, we process the whole htif command anyway. /

Alasdair commented 1 year ago

Is there any documentation for this HTIF thing? Some googling just lead to me to github issues about the lack of documentation which seems like a poor sign.

Edit: To clarify - a poor sign for being confident that we are implementing it correctly

abukharmeh commented 1 year ago

Hi, I think HTIF does not support blocking character device for 32-bit but the model have some work around to allow it to support that https://github.com/riscv/sail-riscv/issues/147 I think if we want to correctly support HTIF as in Spike, then BCD should be dropped for 32-bit in favor of syscalls

trdthg commented 1 month ago

Hi, I previously tried to run ACT using qemu, but encountered similar issues. The handling of tohost by qemu is roughly the same as that of sail, But this part is missing:

and it seems incorrect to me that three writes are needed to a tohost address in order to terminate.

So the result is that the test program can never terminate, stuck in an infinite loop

Adding the same thing to qemu doesn't seem like a good idea

Alasdair commented 1 month ago

My understanding is that the HTIF thing isn't a standard, so there really isn't much to do other than whatever makes the tests work.

Timmmm commented 1 month ago

Yeah I think it was just some ad hoc thing added to QEMU and Spike. It's apparently deprecated.

In our fork I added support for fairly arbitrary exit conditions that are specified on the command line, instead of via an actual MMIO device implemented in Sail. E.g. this is the command we use for the tests in this repo:

emulator --arch ${arch} --stop 'written($tohost) || steps > 100000'  --success 'written($tohost) && mem($tohost) >> 1 == 0' ${elf}

$tohost means it finds the address of the tohost symbol in the ELF. It also supports UART, like --uart 'expression that evaluates to the address of a byte to monitor', though I think eventually I will switch to implementing AXI UART Lite so you can do input too. I believe it means HTIF isn't needed.

I would be happy to donate this, but it's lots of C++ and CMake, and I haven't had a lot of success convincing people that they are better than C & Make.

Alasdair commented 1 month ago

I think if it is significantly better than what we have currently (and it sounds like it is) I would be open to switching out our current C wrapper. Maybe you could demonstrate it at one of our meetings?

jrtc27 commented 1 month ago

What does Spike do? We should just follow that rather than invent something new. Whether that means making the HTIF implementation less janky or some other magic MMIO device.

billmcspadden-riscv commented 1 month ago

Spike uses HTIF.

Bill Mc.

On Tue, Sep 24, 2024 at 11:54 AM Jessica Clarke @.***> wrote:

What does Spike do? We should just follow that rather than invent something new. Whether that means making the HTIF implementation less janky or some other magic MMIO device.

— Reply to this email directly, view it on GitHub https://github.com/riscv/sail-riscv/issues/218#issuecomment-2372064210, or unsubscribe https://github.com/notifications/unsubscribe-auth/AXROLOHOGXYLZ2W3PUJEIBDZYGYO7AVCNFSM6AAAAABOYAPX6OVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNZSGA3DIMRRGA . You are receiving this because you are subscribed to this thread.Message ID: @.***>

-- Bill McSpadden Formal Verification Engineer RISC-V International mobile: 503-807-9309

Join me at RISC-V Summit North America http://www.riscvsummit.com/ in October!

Alasdair commented 1 month ago

After some digging, I think what spike is doing is if you write the payload only in RV32, it zero-extends it and writes the upper 32 command bits for you. It's undocumented so I might be misunderstanding what it's doing.

jrtc27 commented 1 month ago

QEMU says:

 * For RV32, the tohost register is zero-extended, so only device=0 and
 * command=0 (i.e. HTIF syscalls/exit codes) are supported.

(https://github.com/qemu/qemu/blob/01dc65a3bc262ab1bec8fe89775e9bbfa627becb/hw/char/riscv_htif.c#L153-L154)

So yes, we should follow that, problem solved.

trdthg commented 1 month ago

Additionally, here is aswaterman's description of HTIF(include RV32) if someone doesn't know:

You might notice the 64-bit orientation. For RV32, the tohost register is zero-extended, so only device=0 and command=0 (i.e. HTIF syscalls/exit codes) are supported.