* Re: [PATCH] KVM: selftests: Rename vm_handle_exception in evmcs test
@ 2021-06-04 21:26 ` Sean Christopherson
0 siblings, 0 replies; 20+ messages in thread
From: Sean Christopherson @ 2021-06-04 21:26 UTC (permalink / raw)
To: Ricardo Koller; +Cc: kvm, maz, kernel test robot, pbonzini, kvmarm
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.
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):
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@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH] KVM: selftests: Rename vm_handle_exception in evmcs test
2021-06-04 21:26 ` Sean Christopherson
@ 2021-06-04 23:11 ` Ricardo Koller
-1 siblings, 0 replies; 20+ messages in thread
From: Ricardo Koller @ 2021-06-04 23:11 UTC (permalink / raw)
To: Sean Christopherson
Cc: kvmarm, kvm, maz, pbonzini, drjones, eric.auger, kernel test robot
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...
Thanks for the 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.
Got it. What about this renaming:
vm_handle_exception(vec) -> vm_install_exception_handler(vec)
vm_install_exception_handler(vec, ec) -> vm_install_sync_handler(vec, ec)
>
> 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.
>
> 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):
I think that works and it does remove a bunch of code. Just need to play
with the idea and check that it can cover all cases.
For now, given that the build is broken, what about this series of
patches:
1. keep this patch to fix x86 kvm selftests
2. rename both arm and x86 to vm_install_exception_handler and vm_install_sync_handler
3. restructure the internals of exception handling in arm
Alternatively, I can send 1+2 together and then 3. What do you think?
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);
> }
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH] KVM: selftests: Rename vm_handle_exception in evmcs test
@ 2021-06-04 23:11 ` Ricardo Koller
0 siblings, 0 replies; 20+ messages in thread
From: Ricardo Koller @ 2021-06-04 23:11 UTC (permalink / raw)
To: Sean Christopherson; +Cc: kvm, maz, kernel test robot, pbonzini, kvmarm
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...
Thanks for the 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.
Got it. What about this renaming:
vm_handle_exception(vec) -> vm_install_exception_handler(vec)
vm_install_exception_handler(vec, ec) -> vm_install_sync_handler(vec, ec)
>
> 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.
>
> 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):
I think that works and it does remove a bunch of code. Just need to play
with the idea and check that it can cover all cases.
For now, given that the build is broken, what about this series of
patches:
1. keep this patch to fix x86 kvm selftests
2. rename both arm and x86 to vm_install_exception_handler and vm_install_sync_handler
3. restructure the internals of exception handling in arm
Alternatively, I can send 1+2 together and then 3. What do you think?
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@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH] KVM: selftests: Rename vm_handle_exception in evmcs test
2021-06-04 23:11 ` Ricardo Koller
@ 2021-06-06 10:10 ` Marc Zyngier
-1 siblings, 0 replies; 20+ messages in thread
From: Marc Zyngier @ 2021-06-06 10:10 UTC (permalink / raw)
To: Ricardo Koller
Cc: Sean Christopherson, kvmarm, kvm, pbonzini, drjones, eric.auger,
kernel test robot
On 2021-06-05 00:11, Ricardo Koller wrote:
> 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...
>
> Thanks for the 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.
>
> Got it. What about this renaming:
>
> vm_handle_exception(vec) -> vm_install_exception_handler(vec)
> vm_install_exception_handler(vec, ec) -> vm_install_sync_handler(vec,
> ec)
>
>>
>> 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.
>>
>> 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):
>
> I think that works and it does remove a bunch of code. Just need to
> play
> with the idea and check that it can cover all cases.
>
> For now, given that the build is broken, what about this series of
> patches:
>
> 1. keep this patch to fix x86 kvm selftests
> 2. rename both arm and x86 to vm_install_exception_handler and
> vm_install_sync_handler
> 3. restructure the internals of exception handling in arm
>
> Alternatively, I can send 1+2 together and then 3. What do you think?
This is becoming a bit messy. I'd rather drop the whole series from
-next, and get something that doesn't break in the middle. Please
resend the series tested on top of -rc4.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH] KVM: selftests: Rename vm_handle_exception in evmcs test
@ 2021-06-06 10:10 ` Marc Zyngier
0 siblings, 0 replies; 20+ messages in thread
From: Marc Zyngier @ 2021-06-06 10:10 UTC (permalink / raw)
To: Ricardo Koller
Cc: kvm, Sean Christopherson, kernel test robot, pbonzini, kvmarm
On 2021-06-05 00:11, Ricardo Koller wrote:
> 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...
>
> Thanks for the 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.
>
> Got it. What about this renaming:
>
> vm_handle_exception(vec) -> vm_install_exception_handler(vec)
> vm_install_exception_handler(vec, ec) -> vm_install_sync_handler(vec,
> ec)
>
>>
>> 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.
>>
>> 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):
>
> I think that works and it does remove a bunch of code. Just need to
> play
> with the idea and check that it can cover all cases.
>
> For now, given that the build is broken, what about this series of
> patches:
>
> 1. keep this patch to fix x86 kvm selftests
> 2. rename both arm and x86 to vm_install_exception_handler and
> vm_install_sync_handler
> 3. restructure the internals of exception handling in arm
>
> Alternatively, I can send 1+2 together and then 3. What do you think?
This is becoming a bit messy. I'd rather drop the whole series from
-next, and get something that doesn't break in the middle. Please
resend the series tested on top of -rc4.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH] KVM: selftests: Rename vm_handle_exception in evmcs test
2021-06-06 10:10 ` Marc Zyngier
@ 2021-06-07 16:07 ` Sean Christopherson
-1 siblings, 0 replies; 20+ messages in thread
From: Sean Christopherson @ 2021-06-07 16:07 UTC (permalink / raw)
To: Marc Zyngier
Cc: Ricardo Koller, kvmarm, kvm, pbonzini, drjones, eric.auger,
kernel test robot
On Sun, Jun 06, 2021, Marc Zyngier wrote:
> This is becoming a bit messy. I'd rather drop the whole series from
> -next, and get something that doesn't break in the middle. Please
> resend the series tested on top of -rc4.
That'd be my preference too. I almost asked if it could be (temporarily)
dropped, but I assumed the hashes in -next are intended to be stable.
Thanks!
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] KVM: selftests: Rename vm_handle_exception in evmcs test
@ 2021-06-07 16:07 ` Sean Christopherson
0 siblings, 0 replies; 20+ messages in thread
From: Sean Christopherson @ 2021-06-07 16:07 UTC (permalink / raw)
To: Marc Zyngier; +Cc: kvm, kernel test robot, pbonzini, kvmarm
On Sun, Jun 06, 2021, Marc Zyngier wrote:
> This is becoming a bit messy. I'd rather drop the whole series from
> -next, and get something that doesn't break in the middle. Please
> resend the series tested on top of -rc4.
That'd be my preference too. I almost asked if it could be (temporarily)
dropped, but I assumed the hashes in -next are intended to be stable.
Thanks!
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] KVM: selftests: Rename vm_handle_exception in evmcs test
2021-06-07 16:07 ` Sean Christopherson
@ 2021-06-07 16:19 ` Marc Zyngier
-1 siblings, 0 replies; 20+ messages in thread
From: Marc Zyngier @ 2021-06-07 16:19 UTC (permalink / raw)
To: Sean Christopherson
Cc: Ricardo Koller, kvmarm, kvm, pbonzini, drjones, eric.auger,
kernel test robot
On Mon, 07 Jun 2021 17:07:40 +0100,
Sean Christopherson <seanjc@google.com> wrote:
>
> On Sun, Jun 06, 2021, Marc Zyngier wrote:
> > This is becoming a bit messy. I'd rather drop the whole series from
> > -next, and get something that doesn't break in the middle. Please
> > resend the series tested on top of -rc4.
>
> That'd be my preference too. I almost asked if it could be (temporarily)
> dropped, but I assumed the hashes in -next are intended to be stable.
I usually try and keep these commits stable.
But in this case, we end-up with a broken build at some point in the
series. For such cases, I'm more than happy to drop the series and
merge a clean version again (I keep each series on a separate branch
for this exact purpose).
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] KVM: selftests: Rename vm_handle_exception in evmcs test
@ 2021-06-07 16:19 ` Marc Zyngier
0 siblings, 0 replies; 20+ messages in thread
From: Marc Zyngier @ 2021-06-07 16:19 UTC (permalink / raw)
To: Sean Christopherson; +Cc: kvm, kernel test robot, pbonzini, kvmarm
On Mon, 07 Jun 2021 17:07:40 +0100,
Sean Christopherson <seanjc@google.com> wrote:
>
> On Sun, Jun 06, 2021, Marc Zyngier wrote:
> > This is becoming a bit messy. I'd rather drop the whole series from
> > -next, and get something that doesn't break in the middle. Please
> > resend the series tested on top of -rc4.
>
> That'd be my preference too. I almost asked if it could be (temporarily)
> dropped, but I assumed the hashes in -next are intended to be stable.
I usually try and keep these commits stable.
But in this case, we end-up with a broken build at some point in the
series. For such cases, I'm more than happy to drop the series and
merge a clean version again (I keep each series on a separate branch
for this exact purpose).
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] KVM: selftests: Rename vm_handle_exception in evmcs test
2021-06-07 16:19 ` Marc Zyngier
@ 2021-06-07 16:56 ` Ricardo Koller
-1 siblings, 0 replies; 20+ messages in thread
From: Ricardo Koller @ 2021-06-07 16:56 UTC (permalink / raw)
To: Marc Zyngier
Cc: Sean Christopherson, kvmarm, kvm, pbonzini, drjones, eric.auger,
kernel test robot
On Mon, Jun 07, 2021 at 05:19:03PM +0100, Marc Zyngier wrote:
> On Mon, 07 Jun 2021 17:07:40 +0100,
> Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Sun, Jun 06, 2021, Marc Zyngier wrote:
> > > This is becoming a bit messy. I'd rather drop the whole series from
> > > -next, and get something that doesn't break in the middle. Please
> > > resend the series tested on top of -rc4.
> >
> > That'd be my preference too. I almost asked if it could be (temporarily)
> > dropped, but I assumed the hashes in -next are intended to be stable.
>
> I usually try and keep these commits stable.
>
> But in this case, we end-up with a broken build at some point in the
> series. For such cases, I'm more than happy to drop the series and
> merge a clean version again (I keep each series on a separate branch
> for this exact purpose).
Thank you both. I will send a new series with the fix and Seans
suggestions (tested on the latest rc).
Apologies again for the mess.
Ricardo
>
> Thanks,
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] KVM: selftests: Rename vm_handle_exception in evmcs test
@ 2021-06-07 16:56 ` Ricardo Koller
0 siblings, 0 replies; 20+ messages in thread
From: Ricardo Koller @ 2021-06-07 16:56 UTC (permalink / raw)
To: Marc Zyngier
Cc: kvm, Sean Christopherson, kernel test robot, pbonzini, kvmarm
On Mon, Jun 07, 2021 at 05:19:03PM +0100, Marc Zyngier wrote:
> On Mon, 07 Jun 2021 17:07:40 +0100,
> Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Sun, Jun 06, 2021, Marc Zyngier wrote:
> > > This is becoming a bit messy. I'd rather drop the whole series from
> > > -next, and get something that doesn't break in the middle. Please
> > > resend the series tested on top of -rc4.
> >
> > That'd be my preference too. I almost asked if it could be (temporarily)
> > dropped, but I assumed the hashes in -next are intended to be stable.
>
> I usually try and keep these commits stable.
>
> But in this case, we end-up with a broken build at some point in the
> series. For such cases, I'm more than happy to drop the series and
> merge a clean version again (I keep each series on a separate branch
> for this exact purpose).
Thank you both. I will send a new series with the fix and Seans
suggestions (tested on the latest rc).
Apologies again for the mess.
Ricardo
>
> Thanks,
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] KVM: selftests: Rename vm_handle_exception in evmcs test
2021-06-07 16:56 ` Ricardo Koller
@ 2021-06-07 17:18 ` Marc Zyngier
-1 siblings, 0 replies; 20+ messages in thread
From: Marc Zyngier @ 2021-06-07 17:18 UTC (permalink / raw)
To: Ricardo Koller
Cc: Sean Christopherson, kvmarm, kvm, pbonzini, drjones, eric.auger,
kernel test robot
On Mon, 07 Jun 2021 17:56:59 +0100,
Ricardo Koller <ricarkol@google.com> wrote:
>
> On Mon, Jun 07, 2021 at 05:19:03PM +0100, Marc Zyngier wrote:
> > On Mon, 07 Jun 2021 17:07:40 +0100,
> > Sean Christopherson <seanjc@google.com> wrote:
> > >
> > > On Sun, Jun 06, 2021, Marc Zyngier wrote:
> > > > This is becoming a bit messy. I'd rather drop the whole series from
> > > > -next, and get something that doesn't break in the middle. Please
> > > > resend the series tested on top of -rc4.
> > >
> > > That'd be my preference too. I almost asked if it could be (temporarily)
> > > dropped, but I assumed the hashes in -next are intended to be stable.
> >
> > I usually try and keep these commits stable.
> >
> > But in this case, we end-up with a broken build at some point in the
> > series. For such cases, I'm more than happy to drop the series and
> > merge a clean version again (I keep each series on a separate branch
> > for this exact purpose).
>
> Thank you both. I will send a new series with the fix and Seans
> suggestions (tested on the latest rc).
>
> Apologies again for the mess.
No worries, that's a pretty common situation.
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] KVM: selftests: Rename vm_handle_exception in evmcs test
@ 2021-06-07 17:18 ` Marc Zyngier
0 siblings, 0 replies; 20+ messages in thread
From: Marc Zyngier @ 2021-06-07 17:18 UTC (permalink / raw)
To: Ricardo Koller
Cc: kvm, Sean Christopherson, kernel test robot, pbonzini, kvmarm
On Mon, 07 Jun 2021 17:56:59 +0100,
Ricardo Koller <ricarkol@google.com> wrote:
>
> On Mon, Jun 07, 2021 at 05:19:03PM +0100, Marc Zyngier wrote:
> > On Mon, 07 Jun 2021 17:07:40 +0100,
> > Sean Christopherson <seanjc@google.com> wrote:
> > >
> > > On Sun, Jun 06, 2021, Marc Zyngier wrote:
> > > > This is becoming a bit messy. I'd rather drop the whole series from
> > > > -next, and get something that doesn't break in the middle. Please
> > > > resend the series tested on top of -rc4.
> > >
> > > That'd be my preference too. I almost asked if it could be (temporarily)
> > > dropped, but I assumed the hashes in -next are intended to be stable.
> >
> > I usually try and keep these commits stable.
> >
> > But in this case, we end-up with a broken build at some point in the
> > series. For such cases, I'm more than happy to drop the series and
> > merge a clean version again (I keep each series on a separate branch
> > for this exact purpose).
>
> Thank you both. I will send a new series with the fix and Seans
> suggestions (tested on the latest rc).
>
> Apologies again for the mess.
No worries, that's a pretty common situation.
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] KVM: selftests: Rename vm_handle_exception in evmcs test
2021-06-04 21:26 ` Sean Christopherson
@ 2021-06-08 22:11 ` Ricardo Koller
-1 siblings, 0 replies; 20+ messages in thread
From: Ricardo Koller @ 2021-06-08 22:11 UTC (permalink / raw)
To: Sean Christopherson
Cc: kvmarm, kvm, maz, pbonzini, drjones, eric.auger, kernel test robot
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);
> }
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH] KVM: selftests: Rename vm_handle_exception in evmcs test
@ 2021-06-08 22:11 ` Ricardo Koller
0 siblings, 0 replies; 20+ messages in thread
From: Ricardo Koller @ 2021-06-08 22:11 UTC (permalink / raw)
To: Sean Christopherson; +Cc: kvm, maz, kernel test robot, pbonzini, kvmarm
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@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH] KVM: selftests: Rename vm_handle_exception in evmcs test
2021-06-08 22:11 ` Ricardo Koller
@ 2021-06-09 16:34 ` Sean Christopherson
-1 siblings, 0 replies; 20+ messages in thread
From: Sean Christopherson @ 2021-06-09 16:34 UTC (permalink / raw)
To: Ricardo Koller
Cc: kvmarm, kvm, maz, pbonzini, drjones, eric.auger, kernel test robot
On Tue, Jun 08, 2021, Ricardo Koller wrote:
> On Fri, Jun 04, 2021 at 09:26:54PM +0000, Sean Christopherson wrote:
> > 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.
My objection to layering handlers is that it introduces ambiguity regarding
ordering and override functionality, e.g. if a test overrides both the "default"
handler and a specific exception handler, which handler will be invoked? My
expectation would be that the more specific override would win, but someone else
might expect that overriding the default would win.
It should be relatively easy to provide helpers to override the handler for
multiple/all exceptions if we do end up with tests that want that functionality.
But yeah, definitely a future problem :-)
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] KVM: selftests: Rename vm_handle_exception in evmcs test
@ 2021-06-09 16:34 ` Sean Christopherson
0 siblings, 0 replies; 20+ messages in thread
From: Sean Christopherson @ 2021-06-09 16:34 UTC (permalink / raw)
To: Ricardo Koller; +Cc: kvm, maz, kernel test robot, pbonzini, kvmarm
On Tue, Jun 08, 2021, Ricardo Koller wrote:
> On Fri, Jun 04, 2021 at 09:26:54PM +0000, Sean Christopherson wrote:
> > 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.
My objection to layering handlers is that it introduces ambiguity regarding
ordering and override functionality, e.g. if a test overrides both the "default"
handler and a specific exception handler, which handler will be invoked? My
expectation would be that the more specific override would win, but someone else
might expect that overriding the default would win.
It should be relatively easy to provide helpers to override the handler for
multiple/all exceptions if we do end up with tests that want that functionality.
But yeah, definitely a future problem :-)
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 20+ messages in thread