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