On Fri, Jun 04, 2021 at 09:26:54PM +0000, Sean Christopherson wrote: > On Fri, Jun 04, 2021, Ricardo Koller wrote: > > Kernel test robot reports this: > > > > > /usr/bin/ld: tools/testing/selftests/kvm/x86_64/evmcs_test.c:157: undefined reference to `vm_handle_exception' > > > /usr/bin/ld: tools/testing/selftests/kvm/x86_64/evmcs_test.c:158: undefined reference to `vm_handle_exception' > > > collect2: error: ld returned 1 exit status > > > > Fix it by renaming vm_handle_exception to vm_install_vector_handler in > > evmcs_test.c. > > > > Fixes: a2bad6a990a4 ("KVM: selftests: Rename vm_handle_exception") > > Belated code review... Can we rename the helper to vm_install_exception_handler()? > > In x86, "vector" is the number of the exception and "vectoring" is the process > of determining the resulting vector that gets delivered to software (e.g. when > dealing with contributory faults like #GP->#PF->#DF), but the thing that's being > handled is an exception. > > arm appears to have similar terminology. And looking at the arm code, it's very > confusing to have a helper vm_install_vector_handler() install into > exception_handlers, _not_ into vector_handlers. Calling the vector_handlers > "default" handlers is also confusing, as "default" usually implies the thing can > be overwritten. But in this case, the "default" handler is just another layer > in the routing. FWIW, this terminology makes sense in kvm-unit-tests (KUT) because KUT has a library function to update the default entries in vector_handlers. I based my patch on it (with names and everything) but didn't add this function to update the default entries, hence the confusion. > > The multiple layers of routing is also confusing and a bit hard to wade through > for the uninitiated. The whole thing can be made more straightfoward by doing > away with the intermediate routing, whacking ~50 lines of code in the process. > E.g. (definitely not functional code): This works but it would remove the ability to replace the default sync handler with something else, like a handler that can cover all possible ec values. In this case we would have to call vm_install_exception_handler_ec 64 times. On the other hand, the tests that we are planning don't seem to need it, so I will move on with the suggestion. Thanks, Ricardo > > diff --git a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c > index 51c42ac24dca..c784e4b770cf 100644 > --- a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c > +++ b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c > @@ -212,15 +212,15 @@ int main(int argc, char *argv[]) > exit(KSFT_SKIP); > } > > - vm_install_exception_handler(vm, VECTOR_SYNC_CURRENT, > + vm_install_exception_handler_ec(vm, VECTOR_SYNC_CURRENT, > ESR_EC_BRK_INS, guest_sw_bp_handler); > - vm_install_exception_handler(vm, VECTOR_SYNC_CURRENT, > + vm_install_exception_handler_ec(vm, VECTOR_SYNC_CURRENT, > ESR_EC_HW_BP_CURRENT, guest_hw_bp_handler); > - vm_install_exception_handler(vm, VECTOR_SYNC_CURRENT, > + vm_install_exception_handler_ec(vm, VECTOR_SYNC_CURRENT, > ESR_EC_WP_CURRENT, guest_wp_handler); > - vm_install_exception_handler(vm, VECTOR_SYNC_CURRENT, > + vm_install_exception_handler_ec(vm, VECTOR_SYNC_CURRENT, > ESR_EC_SSTEP_CURRENT, guest_ss_handler); > - vm_install_exception_handler(vm, VECTOR_SYNC_CURRENT, > + vm_install_exception_handler_ec(vm, VECTOR_SYNC_CURRENT, > ESR_EC_SVC64, guest_svc_handler); > > for (stage = 0; stage < 7; stage++) { > diff --git a/tools/testing/selftests/kvm/include/aarch64/processor.h b/tools/testing/selftests/kvm/include/aarch64/processor.h > index 1a3abe1037b0..211cb684577a 100644 > --- a/tools/testing/selftests/kvm/include/aarch64/processor.h > +++ b/tools/testing/selftests/kvm/include/aarch64/processor.h > @@ -110,10 +110,10 @@ void vm_init_descriptor_tables(struct kvm_vm *vm); > void vcpu_init_descriptor_tables(struct kvm_vm *vm, uint32_t vcpuid); > > typedef void(*handler_fn)(struct ex_regs *); > -void vm_install_exception_handler(struct kvm_vm *vm, > - int vector, int ec, handler_fn handler); > -void vm_install_vector_handler(struct kvm_vm *vm, > - int vector, handler_fn handler); > +void vm_install_exception_handler_ec(struct kvm_vm *vm, int vector, int ec, > + handler_fn handler); > +void vm_install_exception_handler(struct kvm_vm *vm, int vector, > + handler_fn handler); > > #define write_sysreg(reg, val) \ > ({ \ > diff --git a/tools/testing/selftests/kvm/lib/aarch64/handlers.S b/tools/testing/selftests/kvm/lib/aarch64/handlers.S > index 49bf8827c6ab..fee0c3155ec7 100644 > --- a/tools/testing/selftests/kvm/lib/aarch64/handlers.S > +++ b/tools/testing/selftests/kvm/lib/aarch64/handlers.S > @@ -93,7 +93,8 @@ handler_\label: > .balign 0x80 > /* This will abort so no need to save and restore registers. */ > mov x0, #vector > - b kvm_exit_unexpected_vector > + <sean doesn't know what goes here> > + b kvm_exit_unexpected_exception > .popsection > > .set vector, vector + 1 > diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c > index 03ce507d49d2..ff63e66e2c5d 100644 > --- a/tools/testing/selftests/kvm/lib/aarch64/processor.c > +++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c > @@ -337,16 +337,9 @@ void vcpu_args_set(struct kvm_vm *vm, uint32_t vcpuid, unsigned int num, ...) > va_end(ap); > } > > -void kvm_exit_unexpected_vector(int vector) > +void kvm_exit_unexpected_exception(int vector, uint64_t ec, bool valid_ec) > { > - ucall(UCALL_UNHANDLED, 3, vector, 0, false /* !valid_ec */); > - while (1) > - ; > -} > - > -static void kvm_exit_unexpected_exception(int vector, uint64_t ec) > -{ > - ucall(UCALL_UNHANDLED, 3, vector, ec, true /* valid_ec */); > + ucall(UCALL_UNHANDLED, 3, vector, ec, valid_ec); > while (1) > ; > } > @@ -369,18 +362,7 @@ void assert_on_unhandled_exception(struct kvm_vm *vm, uint32_t vcpuid) > } > } > > -/* > - * This exception handling code was heavily inspired on kvm-unit-tests. There > - * is a set of default vector handlers stored in vector_handlers. These default > - * vector handlers call user-installed handlers stored in exception_handlers. > - * Synchronous handlers are indexed by (vector, ec), and irq handlers by > - * (vector, ec=0). > - */ > - > -typedef void(*vector_fn)(struct ex_regs *, int vector); > - > struct handlers { > - vector_fn vector_handlers[VECTOR_NUM]; > handler_fn exception_handlers[VECTOR_NUM][ESR_EC_NUM]; > }; > > @@ -391,80 +373,56 @@ void vcpu_init_descriptor_tables(struct kvm_vm *vm, uint32_t vcpuid) > set_reg(vm, vcpuid, ARM64_SYS_REG(VBAR_EL1), (uint64_t)&vectors); > } > > -static void default_sync_handler(struct ex_regs *regs, int vector) > -{ > - struct handlers *handlers = (struct handlers *)exception_handlers; > - uint64_t esr = read_sysreg(esr_el1); > - uint64_t ec = (esr >> ESR_EC_SHIFT) & ESR_EC_MASK; > - > - GUEST_ASSERT(VECTOR_IS_SYNC(vector)); > - > - if (handlers && handlers->exception_handlers[vector][ec]) > - handlers->exception_handlers[vector][ec](regs); > - else > - kvm_exit_unexpected_exception(vector, ec); > -} > - > -static void default_handler(struct ex_regs *regs, int vector) > -{ > - struct handlers *handlers = (struct handlers *)exception_handlers; > - > - GUEST_ASSERT(!VECTOR_IS_SYNC(vector)); > - > - if (handlers && handlers->exception_handlers[vector][0]) > - handlers->exception_handlers[vector][0](regs); > - else > - kvm_exit_unexpected_vector(vector); > -} > - > void route_exception(struct ex_regs *regs, int vector) > { > - struct handlers *handlers = (struct handlers *)exception_handlers; > + struct handler_fn *handlers = exception_handlers; > + bool valid_ec; > + int ec; > > - if (handlers && handlers->vector_handlers[vector]) > - handlers->vector_handlers[vector](regs, vector); > - else > - kvm_exit_unexpected_vector(vector); > + switch (vector) { > + case VECTOR_SYNC_CURRENT: > + case VECTOR_SYNC_LOWER_64: > + ec = (read_sysreg(esr_el1) >> ESR_EC_SHIFT) & ESR_EC_MASK; > + valid_ec = true; > + break; > + case VECTOR_IRQ_CURRENT: > + case VECTOR_IRQ_LOWER_64: > + case VECTOR_FIQ_CURRENT: > + case VECTOR_FIQ_LOWER_64: > + case VECTOR_ERROR_CURRENT: > + case VECTOR_ERROR_LOWER_64: > + ec = 0; > + valid_ec = false; > + break; > + default: > + goto unexpected_exception; > + } > + > + if (handlers && handlers[vector][ec]) > + return handlers[vector][ec](regs); > + > +unexpected_exception: > + kvm_exit_unexpected_exception(vector, ec, valid_ec); > } > > void vm_init_descriptor_tables(struct kvm_vm *vm) > { > - struct handlers *handlers; > - > - vm->handlers = vm_vaddr_alloc(vm, sizeof(struct handlers), > - vm->page_size, 0, 0); > - > - handlers = (struct handlers *)addr_gva2hva(vm, vm->handlers); > - handlers->vector_handlers[VECTOR_SYNC_CURRENT] = default_sync_handler; > - handlers->vector_handlers[VECTOR_IRQ_CURRENT] = default_handler; > - handlers->vector_handlers[VECTOR_FIQ_CURRENT] = default_handler; > - handlers->vector_handlers[VECTOR_ERROR_CURRENT] = default_handler; > - > - handlers->vector_handlers[VECTOR_SYNC_LOWER_64] = default_sync_handler; > - handlers->vector_handlers[VECTOR_IRQ_LOWER_64] = default_handler; > - handlers->vector_handlers[VECTOR_FIQ_LOWER_64] = default_handler; > - handlers->vector_handlers[VECTOR_ERROR_LOWER_64] = default_handler; > - > - *(vm_vaddr_t *)addr_gva2hva(vm, (vm_vaddr_t)(&exception_handlers)) = vm->handlers; > + *(vm_vaddr_t *)addr_gva2hva(vm, (vm_vaddr_t)(&exception_handlers)) = __exception_handlers; > } > > -void vm_install_exception_handler(struct kvm_vm *vm, int vector, int ec, > - void (*handler)(struct ex_regs *)) > +void vm_install_exception_handler_ec(struct kvm_vm *vm, int vector, int ec, > + void (*handler)(struct ex_regs *)) > { > - struct handlers *handlers = (struct handlers *)addr_gva2hva(vm, vm->handlers); > + struct handlers *handlers = addr_gva2hva(vm, vm->handlers); > > - assert(VECTOR_IS_SYNC(vector)); > + assert(!ec == !VECTOR_IS_SYNC(vector)); > assert(vector < VECTOR_NUM); > assert(ec < ESR_EC_NUM); > - handlers->exception_handlers[vector][ec] = handler; > + exception_handlers[vector][ec] = handler; > } > > -void vm_install_vector_handler(struct kvm_vm *vm, int vector, > - void (*handler)(struct ex_regs *)) > +void vm_install_exception_handler(struct kvm_vm *vm, int vector, > + void (*handler)(struct ex_regs *)) > { > - struct handlers *handlers = (struct handlers *)addr_gva2hva(vm, vm->handlers); > - > - assert(!VECTOR_IS_SYNC(vector)); > - assert(vector < VECTOR_NUM); > - handlers->exception_handlers[vector][0] = handler; > + vm_install_exception_handler_ec(vm, vector, 0, handler); > } _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/mailman/listinfo/kvmarm