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, apply the independent fflags bits, and reconstruct the final CSR from the validated fields. The change is limited to src/cpu/riscv_csr.c and applies independently to staging; validation artifacts stay out of the source diff.

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]

Reworked the reserved-frm path: an invalid frm keeps the old value, while the final fcsr writeback applies fflags and the validated frm together. CI passes; 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]

Applied the follow-up from your inline review: the final writeback now uses the validated fflags and frm fields directly, with no duplicate CSR_FCSR_MASK preservation at this point. The branch is now a single commit based on current staging; local build and patch checks pass. Please re-review.