riscv: preserve fflags when rejecting a reserved frm value

[carlosqwqqwq]

riscv: preserve fflags when rejecting a reserved frm value

Fixes #274

Commit message

riscv: preserve fflags when rejecting a reserved frm value

Description

riscv_update_fcsr() currently returns before committing the writable fflags field when the incoming frm value is reserved. Keep the old/WARL frm value and continue applying the independent fflags bits. The change is limited to src/cpu/riscv_csr.c; the probe and research logs are not part of the upstream change.

Validation

• The witness runs on native RISC-V hardware and QEMU and on the affected RVVM build; the isolated patched build matches both references.
• Both the interpreter and JIT lanes were exercised, and the patched build is limited to this root.

[purplesyringa]

Maybe reconstruct new_fcsr from the patched new_frm/new_fflags once at writeback so that this isn't a path uncovered by most tests?

[carlosqwqqwq]

Thanks! Reworked: fcsr is now reconstructed from the patched frm/fflags once at writeback (vm->csr.fcsr = (new_fcsr & ~CSR_FCSR_MASK) | new_fflags | (new_frm << 5)), so the reserved-frm path shares the same writeback as every other path. Rebuilt cleanly and force-pushed.

[carlosqwqqwq]

I reworked this at head 9e96a86b as suggested: an invalid frm now preserves the old frm, while the final fcsr writeback reconstructs the value from the patched frm and fflags in one place. This keeps the fflags update instead of dropping the whole write and makes the reserved-frm path share the common writeback. All current GitHub Actions checks pass. @purplesyringa, please re-review.

[purplesyringa]

Is (new_fcsr & ~CSR_FCSR_MASK) | necessary here? I would assume all of those bits are either zero or zeroed on read, would you be able to verify whether that's the case?

[carlosqwqqwq]

Yes, this mask is necessary for architectural compatibility.

The current RISC-V F extension specification defines fcsr[31:8] as reserved for other standard extensions and explicitly says that standard software should preserve those bits. An implementation without those extensions may read them as zero, but RVVM must not discard them during a read-modify-write path when a future/implemented extension uses them. CSR_FCSR_MASK is 0x000000ff, so:

(new_fcsr & ~CSR_FCSR_MASK) | new_fflags | (new_frm << 5)

preserves the non-F-extension portion while replacing only frm and fflags with their validated values. I also removed the misleading WARL wording from the comments; the code preserves RVVM's existing invalid-frm policy while still applying the remaining fields.

The updated head is 18014ab. It builds successfully with make -j2 in the x86_64 lab container.

Reference: https://github.com/riscv/riscv-isa-manual/blob/main/src/unpriv/f-st-ext.adoc

[purplesyringa]

We already mask it here though, so only handling the mask in this location doesn't look sufficient. Regardless, I think the intention of CSR_FCSR_MASK is to contain all important bits, including those provided by extensions, so keeping bits outside of the mask is useless anyway.

It's reasonable to ask if this snippet can be written in such a way that it won't have to be updated if extensions are implemented, but since this is the function that handles FCSR changes, that looks like exactly the place that should worry about the entirety of defined bits. So I think it's fine to just drop the masking here, under the assumption that if a new extension is added, this function is going to be the first place that we'll need to touch anyway.

[carlosqwqqwq]

Pushed follow-up commit 18014ab and replied to the inline question above.

The functional writeback remains:

(new_fcsr & ~CSR_FCSR_MASK) | new_fflags | (new_frm << 5)

This preserves fcsr[31:8] while applying the validated frm and fflags fields. I also replaced the misleading WARL wording with a comment describing the actual RVVM policy. The updated head builds successfully with make -j2 in the x86_64 lab container.

Please re-review the updated head when convenient.