riscv: Tidy panic messages for exceptions

[jhb]

Summary

• Remove trailing newlines

• Be consistent about the format used to print pointer values

• Print the trap value for access faults (it is the faulting address if non-zero) and illegal instructions (it is the first N bytes of the decoded instruction if non-zero)

Sponsored by: DARPA

[jrtc27]

sys/riscv/riscv/trap.c

337–338

The 0x is included in the width, so these need to be %#018lx (yes, this is somewhat stupid for zero padding, but makes some sense for space padding)

367–369

Can we make this %04lx if the low 2 bits are 11 and %02lx otherwise? (I imagine %*0lx with (stval & 3) == 3 ? 4 : 2 is the "nice" way to implement it)

[jhb]

sys/riscv/riscv/trap.c

337–338

Hmm, I wonder if it makes sense to use # at al here then. Normally the goal with that is to get a simple "0" for zero values, but this still ends up zero padded:

> printf "%#04x" 0
0000

So probably these should instead all be 0x%016lx instead.

[jhb]

sys/riscv/riscv/trap.c

337–338

Or maybe just %#lx

[jhb]

Use %#lx

[jhb]

sys/riscv/riscv/trap.c

367–369

I could do this, but it will be pretty close with %#lx. The spec says implementations can either set this to 0, or they will only set it to the first N valid bytes (one of the constraints is ILEN which I believe means that for a 16-bit instruction the value would only have the 16 bits and the upper bits would all be zero, similarly for 32-bit instructions which by definition would have 0xc0 set in the most significant byte). With %#lx it is only 16-bit instructions for which the first byte is zero that would be printed as a 0xXX instead of 0x00XX. I'm not sure it's worth adding a special case just for those?

[jrtc27]

sys/riscv/riscv/trap.c

367–369

You seem confused about bytes vs halfwords, and the order of the bits? The length indicator is in the low 2 bits (high bits are often immediates for 32-bit instructions).

[jhb]

sys/riscv/riscv/trap.c

367–369

Oh, hmm, yes, they are in the low bits.

[jhb]

Parse stval to determine instruction width

[jhb]

To test the illegal instruction bits I wrote the following kernel module:

#include <sys/param.h>
#include <sys/kassert.h>
#include <sys/sysctl.h>

static SYSCTL_NODE(_debug, OID_AUTO, riscv, CTLFLAG_RD, NULL, "");

static int
riscv_handler(SYSCTL_HANDLER_ARGS)
{
	void (*f)(void) = arg1;
	int error, val;

	val = 0;
	error = sysctl_handle_int(oidp, &val, 0, req);
	if (error != 0 || req->newptr == NULL)
		return (error);

	f();
	return (0);
}

static void
riscv_bad0(void)
{
	__asm __volatile(".byte 0,0");
}
SYSCTL_PROC(_debug_riscv, OID_AUTO, bad0, CTLTYPE_INT | CTLFLAG_MPSAFE |
    CTLFLAG_RW, riscv_bad0, 0, riscv_handler, "I", "");

static void
riscv_bad2(void)
{
	/* 100xxxxx xxxxxxaa (aa != 11) */
	__asm __volatile(".short 0x8010");
}
SYSCTL_PROC(_debug_riscv, OID_AUTO, bad2, CTLTYPE_INT | CTLFLAG_MPSAFE |
    CTLFLAG_RW, riscv_bad2, 0, riscv_handler, "I", "");

static void
riscv_bad4(void)
{
	/* xxxxxxxx xxxxxxxx xxxxxxxx x1101011 */
	__asm __volatile(".byte 0x6b,0x12,0x34,0x56");
}
SYSCTL_PROC(_debug_riscv, OID_AUTO, bad4, CTLTYPE_INT | CTLFLAG_MPSAFE |
    CTLFLAG_RW, riscv_bad4, 0, riscv_handler, "I", "");

static void
riscv_bad6(void)
{
	/* xxxxxxxx xxxxxxxx xxxxxxxx xxxxxxxx xxxxxxxx xx011111 */
	__asm __volatile(".byte 0x1f,0x12,0x34,0x56,0x78,0x9a");
}
SYSCTL_PROC(_debug_riscv, OID_AUTO, bad6, CTLTYPE_INT | CTLFLAG_MPSAFE |
    CTLFLAG_RW, riscv_bad6, 0, riscv_handler, "I", "");

static void
riscv_bad8(void)
{
	/*
	 * xxxxxxxx xxxxxxxx xxxxxxxx xxxxxxxx
	 * xxxxxxxx xxxxxxxx xxxxxxxx x0111111
	 */
	__asm __volatile(".byte 0x3f,0x12,0x34,0x56,0x78,0x9a,0xbc,0xde\n"
			 ".byte 0xf0,0x12,0x34,0x56,0x78,0x9a,0xbc,0xde");
}
SYSCTL_PROC(_debug_riscv, OID_AUTO, bad8, CTLTYPE_INT | CTLFLAG_MPSAFE |
    CTLFLAG_RW, riscv_bad8, 0, riscv_handler, "I", "");

and then tested the various cases under QEMU with the following results:

panic: Illegal instruction 0x0000 at 0xffffffc06c001714
panic: Illegal instruction 0x8010 at 0xffffffc06c001766
panic: Illegal instruction 0x5634126b at 0xffffffc06c001778
panic: Illegal instruction 0x00005634121f at 0xffffffc06c00178c
panic: Illegal instruction 0x000000005634123f at 0xffffffc06c0017a2

It seems QEMU doesn't understand >32bit instructions on RISC-V yet. In fact, it looks like it always loads 32-bits into tval (at least in CHERI QEMU):

/* Exceptions processing helpers */
void QEMU_NORETURN riscv_raise_exception(CPURISCVState *env,
                                          uint32_t exception, uintptr_t pc)
{
...
    if (exception == RISCV_EXCP_ILLEGAL_INST) {
        // Try to fetch the faulting instruction and store it in badaddr
        uint32_t opcode = 0;
        int ret = cpu_memory_rw_debug(env_cpu(env), PC_ADDR(env),
                                      (uint8_t *)&opcode, sizeof(opcode),
                                      /*is_write=*/false);
        opcode = tswap32(opcode); // FIXME is this needed?
        if (ret != 0 && PC_ADDR(env) != 0) {
            warn_report("RISCV_EXCP_ILLEGAL_INST: Could not read %zu bytes at "
                        "vaddr 0x" TARGET_FMT_lx "\r\n",
                        sizeof(opcode), PC_ADDR(env));
        } else {
            env->badaddr = opcode;
        }
    }
    cpu_loop_exit(cs);
}

[jrtc27]

\>32-bit is just tentative encodings, it’s not ratified, just handle 16 vs 32 and add >32 if/when it’s ratified.

[jhb]

Drop handling for > 32bit instructions

[jrtc27]

sys/riscv/riscv/trap.c

370

Dunno what aa is meant to be, but do we really need the variable here? This is just frame->tf_stval & 3) == 3 ? 8 : 4, it's simpler to just inline it when you're not handling all the crazy unratified lengths.

[markj]

sys/riscv/riscv/trap.c

370

"aa" is some notation from the ISA spec ("Expanded Instruction-Length Encoding") but it looks a bit confusing here.

[jhb]

sys/riscv/riscv/trap.c

370

I was kind of assuming at some point we might have to add the 48-bit stuff back in (and in that context the various annotated bit fields made a bit more sense) so keeping the structure makes that future diff simpler. I can reduce it down to a ternary though.