simplify bhyve vlapic ESR logic

[pmooney_pfmooney.com]

Summary

The bhyve vLAPIC uses an instance-global flag to indicate when an error LVT is being delivered. This is to prevent infinite recursion if the error LVT itself is configured with an invalid vector. Such error-handling status could be passed as a function argument, rather than more complicated logic with "global" state.

This was inspired by the bhyve save/restore work, which called into question vLAPIC state outside the raw LAPIC page itself.

SmartOS ticket: OS-7777

[rgrimes]

Minor nits, and a major win, good work!

sys/amd64/vmm/io/vlapic.c

84

extra white space

696

Wow thats a win! 6 cases collapse to 1

[pmooney_pfmooney.com]

Fix excess whitespace and add attribution

[jhb]

I think it would actually be a smaller and simpler diff to add a 'firing_error' bool to vlapic_fire_lvt() that is normally false except in vlapic_set_error() and then have the vlapic_fire_lvt() pass that down rather through the various layers. OTOH, I'm not really sure that is a lot better than esr_firing and just ignoring the field during suspend and restore?

sys/amd64/vmm/io/vlapic.c

621

The APIC_LVT_DM_FIXED is lost here as well.

694

These forced FIXED delivery mode writes are getting lost now for TIMER and ERROR.

[pmooney_pfmooney.com]

sys/amd64/vmm/io/vlapic.c

621

See below

625

Will eliminate the ILLEGAL_VECTOR qualification here per @jhb's feedback.

694

APIC_LVT_DM_FIXED is defined to be zero, so those ORs were effectively no-ops. The code which minds the LVTs after writes ensures that the delivery modes are valid.

[pmooney_pfmooney.com]

Eliminate the ILLEGAL_VECTOR qualification in vlapic_set_error per @jhb's feedback and rebase.

[pmooney_pfmooney.com]

Merged downstream in SmartOS as aa2898c4.

[markj]

sys/amd64/vmm/io/vlapic.c

694

In particular, vlapic_lvt_write_handler() does not include the delivery mode in the saved LVT entry for timer and error interrupts, forcing it to fixed mode.

[jhb]

sys/amd64/vmm/io/vlapic.c

694

Ok. I found it kind of useful in a documentation sense that if you sit down with the SDM and read the requirements of the given LVT entries, these specific ones ignore the delivery mode and always use fixed, so even if the or with 0 was a no-op, it still matched the spec, as it were.

sys/amd64/vmm/io/vlapic.h

#ifdef KDB
         if (debugger_on_trap) {
                 kdb_why = KDB_WHY_TRAP;
                 handled = kdb_trap(frame->tf_scause & SCAUSE_CODE, 0, frame);
                 kdb_why = KDB_WHY_UNSET;
                 if (handled)
                         return;
         }
 #endif
-        panic("Fatal page fault at %#lx: %#016lx", frame->tf_sepc, stval);
+        panic("Fatal page fault at %#lx: %#lx", frame->tf_sepc, stval);
 }
 
 void
 do_trap_supervisor(struct trapframe *frame)
 {
         uint64_t exception;
 
         /* Ensure we came from supervisor mode, interrupts disabled */
if ((frame->tf_scause & SCAUSE_INTR) != 0) {
                 return;
         }
 
 #ifdef KDTRACE_HOOKS
         if (dtrace_trap_func != NULL && (*dtrace_trap_func)(frame, exception))
                 return;
 #endif
 
-        CTR4(KTR_TRAP, "%s: exception=%lu, sepc=%lx, stval=%lx", __func__,
+        CTR4(KTR_TRAP, "%s: exception=%lu, sepc=%#lx, stval=%#lx", __func__,
             exception, frame->tf_sepc, frame->tf_stval);
 
         switch (exception) {
         case SCAUSE_LOAD_ACCESS_FAULT:
         case SCAUSE_STORE_ACCESS_FAULT:
         case SCAUSE_INST_ACCESS_FAULT:
                 dump_regs(frame);
-                panic("Memory access exception at 0x%016lx\n", frame->tf_sepc);
+                panic("Memory access exception at %#lx: %#lx",
+                    frame->tf_sepc, frame->tf_stval);
                 break;
         case SCAUSE_LOAD_MISALIGNED:
         case SCAUSE_STORE_MISALIGNED:
         case SCAUSE_INST_MISALIGNED:
                 dump_regs(frame);
-                panic("Misaligned address exception at %#016lx: %#016lx\n",
+                panic("Misaligned address exception at %#lx: %#lx",
                     frame->tf_sepc, frame->tf_stval);
                 break;
         case SCAUSE_STORE_PAGE_FAULT:
         case SCAUSE_LOAD_PAGE_FAULT:
         case SCAUSE_INST_PAGE_FAULT:
                 page_fault_handler(frame, 0);
                 break;
         case SCAUSE_BREAKPOINT:
 #ifdef KDTRACE_HOOKS
                 if (dtrace_invop_jump_addr != NULL &&
                     dtrace_invop_jump_addr(frame) == 0)
                                 break;
 #endif
 #ifdef KDB
                 kdb_trap(exception, 0, frame);
 #else
                 dump_regs(frame);
-                panic("No debugger in kernel.\n");
+                panic("No debugger in kernel.");
 #endif
                 break;
         case SCAUSE_ILLEGAL_INSTRUCTION:
                 dump_regs(frame);
-                panic("Illegal instruction at 0x%016lx\n", frame->tf_sepc);
+                panic("Illegal instruction %#lx at %#lx", frame->tf_stval,
+                    frame->tf_sepc);
                 break;
         default:
                 dump_regs(frame);
-                panic("Unknown kernel exception %lx trap value %lx\n",
+                panic("Unknown kernel exception %#lx trap value %#lx",
                     exception, frame->tf_stval);
         }
 }
 
 void
 do_trap_user(struct trapframe *frame)
 {
         uint64_t exception;
do_trap_user(struct trapframe *frame)
         exception = frame->tf_scause & SCAUSE_CODE;
         if ((frame->tf_scause & SCAUSE_INTR) != 0) {
                 /* Interrupt */
                 riscv_cpu_intr(frame);
                 return;
         }
         intr_enable();
 
-        CTR4(KTR_TRAP, "%s: exception=%lu, sepc=%lx, stval=%lx", __func__,
+        CTR4(KTR_TRAP, "%s: exception=%lu, sepc=%#lx, stval=%#lx", __func__,
             exception, frame->tf_sepc, frame->tf_stval);
 
         switch (exception) {
         case SCAUSE_LOAD_ACCESS_FAULT:
         case SCAUSE_STORE_ACCESS_FAULT:
         case SCAUSE_INST_ACCESS_FAULT:
                 call_trapsignal(td, SIGBUS, BUS_ADRERR, (void *)frame->tf_sepc,
                     exception);
case SCAUSE_ILLEGAL_INSTRUCTION:
                 break;
         case SCAUSE_BREAKPOINT:
                 call_trapsignal(td, SIGTRAP, TRAP_BRKPT, (void *)frame->tf_sepc,
                     exception);
                 userret(td, frame);
                 break;
         default:
                 dump_regs(frame);
-                panic("Unknown userland exception %lx, trap value %lx\n",
+                panic("Unknown userland exception %#lx, trap value %#lx",
                     exception, frame->tf_stval);
         }
 }

sys/amd64/vmm/io/vlapic.c

#ifdef KDB
         if (debugger_on_trap) {
                 kdb_why = KDB_WHY_TRAP;
                 handled = kdb_trap(frame->tf_scause & SCAUSE_CODE, 0, frame);
                 kdb_why = KDB_WHY_UNSET;
                 if (handled)
                         return;
         }
 #endif
-        panic("Fatal page fault at %#lx: %#016lx", frame->tf_sepc, stval);
+        panic("Fatal page fault at %#lx: %#lx", frame->tf_sepc, stval);
 }
 
 void
 do_trap_supervisor(struct trapframe *frame)
 {
         uint64_t exception;
 
         /* Ensure we came from supervisor mode, interrupts disabled */
if ((frame->tf_scause & SCAUSE_INTR) != 0) {
                 return;
         }
 
 #ifdef KDTRACE_HOOKS
         if (dtrace_trap_func != NULL && (*dtrace_trap_func)(frame, exception))
                 return;
 #endif
 
-        CTR4(KTR_TRAP, "%s: exception=%lu, sepc=%lx, stval=%lx", __func__,
+        CTR4(KTR_TRAP, "%s: exception=%lu, sepc=%#lx, stval=%#lx", __func__,
             exception, frame->tf_sepc, frame->tf_stval);
 
         switch (exception) {
         case SCAUSE_LOAD_ACCESS_FAULT:
         case SCAUSE_STORE_ACCESS_FAULT:
         case SCAUSE_INST_ACCESS_FAULT:
                 dump_regs(frame);
-                panic("Memory access exception at 0x%016lx\n", frame->tf_sepc);
+                panic("Memory access exception at %#lx: %#lx",
+                    frame->tf_sepc, frame->tf_stval);
                 break;
         case SCAUSE_LOAD_MISALIGNED:
         case SCAUSE_STORE_MISALIGNED:
         case SCAUSE_INST_MISALIGNED:
                 dump_regs(frame);
-                panic("Misaligned address exception at %#016lx: %#016lx\n",
+                panic("Misaligned address exception at %#lx: %#lx",
                     frame->tf_sepc, frame->tf_stval);
                 break;
         case SCAUSE_STORE_PAGE_FAULT:
         case SCAUSE_LOAD_PAGE_FAULT:
         case SCAUSE_INST_PAGE_FAULT:
                 page_fault_handler(frame, 0);
                 break;
         case SCAUSE_BREAKPOINT:
 #ifdef KDTRACE_HOOKS
                 if (dtrace_invop_jump_addr != NULL &&
                     dtrace_invop_jump_addr(frame) == 0)
                                 break;
 #endif
 #ifdef KDB
                 kdb_trap(exception, 0, frame);
 #else
                 dump_regs(frame);
-                panic("No debugger in kernel.\n");
+                panic("No debugger in kernel.");
 #endif
                 break;
         case SCAUSE_ILLEGAL_INSTRUCTION:
                 dump_regs(frame);
-                panic("Illegal instruction at 0x%016lx\n", frame->tf_sepc);
+                panic("Illegal instruction %#lx at %#lx", frame->tf_stval,
+                    frame->tf_sepc);
                 break;
         default:
                 dump_regs(frame);
-                panic("Unknown kernel exception %lx trap value %lx\n",
+                panic("Unknown kernel exception %#lx trap value %#lx",
                     exception, frame->tf_stval);
         }
 }
 
 void
 do_trap_user(struct trapframe *frame)
 {
         uint64_t exception;
do_trap_user(struct trapframe *frame)
         exception = frame->tf_scause & SCAUSE_CODE;
         if ((frame->tf_scause & SCAUSE_INTR) != 0) {
                 /* Interrupt */
                 riscv_cpu_intr(frame);
                 return;
         }
         intr_enable();
 
-        CTR4(KTR_TRAP, "%s: exception=%lu, sepc=%lx, stval=%lx", __func__,
+        CTR4(KTR_TRAP, "%s: exception=%lu, sepc=%#lx, stval=%#lx", __func__,
             exception, frame->tf_sepc, frame->tf_stval);
 
         switch (exception) {
         case SCAUSE_LOAD_ACCESS_FAULT:
         case SCAUSE_STORE_ACCESS_FAULT:
         case SCAUSE_INST_ACCESS_FAULT:
                 call_trapsignal(td, SIGBUS, BUS_ADRERR, (void *)frame->tf_sepc,
                     exception);
case SCAUSE_ILLEGAL_INSTRUCTION:
                 break;
         case SCAUSE_BREAKPOINT:
                 call_trapsignal(td, SIGTRAP, TRAP_BRKPT, (void *)frame->tf_sepc,
                     exception);
                 userret(td, frame);
                 break;
         default:
                 dump_regs(frame);
-                panic("Unknown userland exception %lx, trap value %lx\n",
+                panic("Unknown userland exception %#lx, trap value %#lx",
                     exception, frame->tf_stval);
         }
 }

sys/amd64/vmm/io/vlapic_priv.h

#ifdef KDB
         if (debugger_on_trap) {
                 kdb_why = KDB_WHY_TRAP;
                 handled = kdb_trap(frame->tf_scause & SCAUSE_CODE, 0, frame);
                 kdb_why = KDB_WHY_UNSET;
                 if (handled)
                         return;
         }
 #endif
-        panic("Fatal page fault at %#lx: %#016lx", frame->tf_sepc, stval);
+        panic("Fatal page fault at %#lx: %#lx", frame->tf_sepc, stval);
 }
 
 void
 do_trap_supervisor(struct trapframe *frame)
 {
         uint64_t exception;
 
         /* Ensure we came from supervisor mode, interrupts disabled */
if ((frame->tf_scause & SCAUSE_INTR) != 0) {
                 return;
         }
 
 #ifdef KDTRACE_HOOKS
         if (dtrace_trap_func != NULL && (*dtrace_trap_func)(frame, exception))
                 return;
 #endif
 
-        CTR4(KTR_TRAP, "%s: exception=%lu, sepc=%lx, stval=%lx", __func__,
+        CTR4(KTR_TRAP, "%s: exception=%lu, sepc=%#lx, stval=%#lx", __func__,
             exception, frame->tf_sepc, frame->tf_stval);
 
         switch (exception) {
         case SCAUSE_LOAD_ACCESS_FAULT:
         case SCAUSE_STORE_ACCESS_FAULT:
         case SCAUSE_INST_ACCESS_FAULT:
                 dump_regs(frame);
-                panic("Memory access exception at 0x%016lx\n", frame->tf_sepc);
+                panic("Memory access exception at %#lx: %#lx",
+                    frame->tf_sepc, frame->tf_stval);
                 break;
         case SCAUSE_LOAD_MISALIGNED:
         case SCAUSE_STORE_MISALIGNED:
         case SCAUSE_INST_MISALIGNED:
                 dump_regs(frame);
-                panic("Misaligned address exception at %#016lx: %#016lx\n",
+                panic("Misaligned address exception at %#lx: %#lx",
                     frame->tf_sepc, frame->tf_stval);
                 break;
         case SCAUSE_STORE_PAGE_FAULT:
         case SCAUSE_LOAD_PAGE_FAULT:
         case SCAUSE_INST_PAGE_FAULT:
                 page_fault_handler(frame, 0);
                 break;
         case SCAUSE_BREAKPOINT:
 #ifdef KDTRACE_HOOKS
                 if (dtrace_invop_jump_addr != NULL &&
                     dtrace_invop_jump_addr(frame) == 0)
                                 break;
 #endif
 #ifdef KDB
                 kdb_trap(exception, 0, frame);
 #else
                 dump_regs(frame);
-                panic("No debugger in kernel.\n");
+                panic("No debugger in kernel.");
 #endif
                 break;
         case SCAUSE_ILLEGAL_INSTRUCTION:
                 dump_regs(frame);
-                panic("Illegal instruction at 0x%016lx\n", frame->tf_sepc);
+                panic("Illegal instruction %#lx at %#lx", frame->tf_stval,
+                    frame->tf_sepc);
                 break;
         default:
                 dump_regs(frame);
-                panic("Unknown kernel exception %lx trap value %lx\n",
+                panic("Unknown kernel exception %#lx trap value %#lx",
                     exception, frame->tf_stval);
         }
 }
 
 void
 do_trap_user(struct trapframe *frame)
 {
         uint64_t exception;
do_trap_user(struct trapframe *frame)
         exception = frame->tf_scause & SCAUSE_CODE;
         if ((frame->tf_scause & SCAUSE_INTR) != 0) {
                 /* Interrupt */
                 riscv_cpu_intr(frame);
                 return;
         }
         intr_enable();
 
-        CTR4(KTR_TRAP, "%s: exception=%lu, sepc=%lx, stval=%lx", __func__,
+        CTR4(KTR_TRAP, "%s: exception=%lu, sepc=%#lx, stval=%#lx", __func__,
             exception, frame->tf_sepc, frame->tf_stval);
 
         switch (exception) {
         case SCAUSE_LOAD_ACCESS_FAULT:
         case SCAUSE_STORE_ACCESS_FAULT:
         case SCAUSE_INST_ACCESS_FAULT:
                 call_trapsignal(td, SIGBUS, BUS_ADRERR, (void *)frame->tf_sepc,
                     exception);
case SCAUSE_ILLEGAL_INSTRUCTION:
                 break;
         case SCAUSE_BREAKPOINT:
                 call_trapsignal(td, SIGTRAP, TRAP_BRKPT, (void *)frame->tf_sepc,
                     exception);
                 userret(td, frame);
                 break;
         default:
                 dump_regs(frame);
-                panic("Unknown userland exception %lx, trap value %lx\n",
+                panic("Unknown userland exception %#lx, trap value %#lx",
                     exception, frame->tf_stval);
         }
 }