sail-riscv icon indicating copy to clipboard operation
sail-riscv copied to clipboard

Exit condition for SAIL model simulating elf code is not intuitive

Open ssecatch-w opened this issue 2 years ago • 12 comments

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.

ssecatch-w avatar Feb 13 '23 22:02 ssecatch-w

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 avatar Feb 13 '23 23:02 Alasdair

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

Alasdair avatar Feb 13 '23 23:02 Alasdair

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

abukharmeh avatar Feb 14 '23 10:02 abukharmeh

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

trdthg avatar Sep 24 '24 10:09 trdthg

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.

Alasdair avatar Sep 24 '24 11:09 Alasdair

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.

Timmmm avatar Sep 24 '24 12:09 Timmmm

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?

Alasdair avatar Sep 24 '24 18:09 Alasdair

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.

jrtc27 avatar Sep 24 '24 18:09 jrtc27

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!

billmcspadden-riscv avatar Sep 24 '24 19:09 billmcspadden-riscv

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.

Alasdair avatar Sep 24 '24 19:09 Alasdair

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.

jrtc27 avatar Sep 24 '24 19:09 jrtc27

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.

trdthg avatar Sep 24 '24 20:09 trdthg