Re: [PATCH 2/2] target/riscv: fixes a bug against `ssamoswap` behavior in M-mode

[Deepak Gupta <debug@rivosinc.com>]

On Thu, Mar 06, 2025 at 04:22:52PM +1000, Alistair Francis wrote:
>On Thu, Mar 6, 2025 at 4:13 PM Deepak Gupta <debug@rivosinc.com> wrote:
>>
>> On Thu, Mar 06, 2025 at 03:29:00PM +1000, Alistair Francis wrote:
>> >On Tue, Feb 18, 2025 at 12:57 PM Deepak Gupta <debug@rivosinc.com> wrote:
>> >>
>> >> Commit f06bfe3dc38c ("target/riscv: implement zicfiss instructions") adds
>> >> `ssamoswap` instruction. `ssamoswap` takes the code-point from existing
>> >> reserved encoding (and not a zimop like other shadow stack instructions).
>> >> If shadow stack is not enabled (via xenvcfg.SSE), then `ssamoswap` must
>> >> result in an illegal instruction exception. However there is a slightly
>> >> modified behavior for M-mode.
>> >>
>> >> Shadow stack are not available in M-mode and all shadow stack instructions
>> >> in M-mode exhibit zimop behavior. However, `ssamoswap` can still succeed
>> >> if MPRV=1 and MPP is non-zero (see section 2.7 of zicfiss specification).
>> >> This patch corrects that behavior for `ssamoswap`.
>> >
>> >Section "22.2.3. Shadow Stack Memory Protection " of the latest priv
>> >spec [1] seems to say: "When the effective privilege mode is M, any
>> >memory access by an SSAMOSWAP.W/D
>> >instruction will result in a store/AMO access-fault exception."
>>
>> Hmm I didn't look at priv spec. Let me fix this one.
>
>I hope the two don't conflict, that will be a nightmare

No they don't conflict. Last "else" block below basically means that
it should be store/AMO access fault because there is no shadow stack page.

"""
if privilege_mode != M && menvcfg.SSE == 0
	raise illegal-instruction exception
if S-mode not implemented
	raise illegal-instruction exception
else if privilege_mode == U && senvcfg.SSE == 0
	raise illegal-instruction exception
else if privilege_mode == VS && henvcfg.SSE == 0
	raise virtual instruction exception
else if privilege_mode == VU && senvcfg.SSE == 0
	raise virtual instruction exception
else
	temp[31:0] = mem[X(rs1)]
	X(rd) = SignExtend(temp[31:0])
	mem[X(rs1)] = X(rs2)[31:0]
endif
"""
>
>Alistair
>
>>
>> >
>> >1: https://github.com/riscv/riscv-isa-manual/releases/tag/riscv-isa-release-9cfaf37-2025-03-06
>> >
>> >Alistair
>> >
>> >>
>> >> Fixes: f06bfe3dc38c ("target/riscv: implement zicfiss instructions")
>> >>
>> >> Reported-by: Ved Shanbhogue <ved@rivosinc.com>
>> >> Signed-off-by: Deepak Gupta <debug@rivosinc.com>
>> >> ---
>> >>  target/riscv/insn_trans/trans_rvzicfiss.c.inc | 13 +++++++++++--
>> >>  1 file changed, 11 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/target/riscv/insn_trans/trans_rvzicfiss.c.inc b/target/riscv/insn_trans/trans_rvzicfiss.c.inc
>> >> index e3ebc4977c..ec016cd70f 100644
>> >> --- a/target/riscv/insn_trans/trans_rvzicfiss.c.inc
>> >> +++ b/target/riscv/insn_trans/trans_rvzicfiss.c.inc
>> >> @@ -15,6 +15,13 @@
>> >>   * You should have received a copy of the GNU General Public License along with
>> >>   * this program.  If not, see <http://www.gnu.org/licenses/>.
>> >>   */
>> >> +
>> >> + #define REQUIRE_ZICFISS(ctx) do {       \
>> >> +    if (!ctx->cfg_ptr->ext_zicfiss) {    \
>> >> +        return false;                    \
>> >> +    }                                    \
>> >> +} while (0)
>> >> +
>> >>  static bool trans_sspopchk(DisasContext *ctx, arg_sspopchk *a)
>> >>  {
>> >>      if (!ctx->bcfi_enabled) {
>> >> @@ -77,7 +84,8 @@ static bool trans_ssrdp(DisasContext *ctx, arg_ssrdp *a)
>> >>  static bool trans_ssamoswap_w(DisasContext *ctx, arg_amoswap_w *a)
>> >>  {
>> >>      REQUIRE_A_OR_ZAAMO(ctx);
>> >> -    if (!ctx->bcfi_enabled) {
>> >> +    REQUIRE_ZICFISS(ctx);
>> >> +    if ((ctx->priv != PRV_M) && !ctx->bcfi_enabled) {
>> >>          return false;
>> >>      }
>> >>
>> >> @@ -97,7 +105,8 @@ static bool trans_ssamoswap_d(DisasContext *ctx, arg_amoswap_w *a)
>> >>  {
>> >>      REQUIRE_64BIT(ctx);
>> >>      REQUIRE_A_OR_ZAAMO(ctx);
>> >> -    if (!ctx->bcfi_enabled) {
>> >> +    REQUIRE_ZICFISS(ctx);
>> >> +    if ((ctx->priv != PRV_M) && !ctx->bcfi_enabled) {
>> >>          return false;
>> >>      }
>> >>
>> >> --
>> >> 2.34.1
>> >>
>> >>