Details

Reviewers

markj
mhorne

Commits

rGfd3fcf71cced: riscv: Tidy panic messages for exceptions
rG92b8ce704a09: riscv: Tidy panic messages for exceptions
rGff79f35bdae5: riscv: Tidy panic messages for exceptions

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)

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

jhb created this revision.Sep 8 2023, 5:42 PM

Herald added subscribers: riscv, imp. · View Herald TranscriptSep 8 2023, 5:42 PM

jhb requested review of this revision.Sep 8 2023, 5:42 PM

Harbormaster completed remote builds in B53507: Diff 127127.Sep 8 2023, 5:42 PM

jrtc27 added a subscriber: jrtc27.Sep 8 2023, 5:56 PM

jrtc27 added inline comments.

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 added inline comments.Sep 8 2023, 11:43 PM

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 added inline comments.Sep 8 2023, 11:45 PM

sys/riscv/riscv/trap.c
337–338	Or maybe just `%#lx`

Use %#lx

Harbormaster completed remote builds in B53518: Diff 127190.Sep 9 2023, 6:14 PM

jhb added inline comments.Sep 9 2023, 6:18 PM

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 added inline comments.Sep 9 2023, 6:42 PM

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 added inline comments.Sep 10 2023, 3:58 AM

sys/riscv/riscv/trap.c
367–369	Oh, hmm, yes, they are in the low bits.

Parse stval to determine instruction width

Harbormaster completed remote builds in B53716: Diff 127792.Sep 25 2023, 4:29 PM

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);
}

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

Drop handling for > 32bit instructions

Harbormaster completed remote builds in B53738: Diff 127843.Sep 26 2023, 8:58 PM

jrtc27 added inline comments.Sep 26 2023, 9:14 PM

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 accepted this revision.Sep 27 2023, 11:13 AM

markj added inline comments.

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.

This revision is now accepted and ready to land.Sep 27 2023, 11:13 AM

jhb added inline comments.Oct 9 2023, 4:29 PM

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.