[PATCH 0/3] KVM: selftests: arm64 exception handling and debug test

[PATCH 0/3] KVM: selftests: arm64 exception handling and debug test

[Ricardo Koller]

Hi,

These patches add a debug exception test in aarch64 KVM selftests while
also adding basic exception handling support.

The structure of the exception handling is based on its x86 counterpart.
Tests use the same calls to initialize exception handling.  And both
architectures use vm_handle_exception() to override the handler for a
particular vector, or (vector, ec) in the arm64 case.

The debug test is similar to x86_64/debug_regs, except that the x86 one
controls the debugging from outside the VM. This proposed arm64 test
controls and handles debug exceptions from the inside.

The final patch adapts the x86 unhandled-vector reporting to use the
same mechanism as the one introduced for arm64 (UCALL_UNHANDLED instead
of direct x86 port IO).

Thanks,
Ricardo

Ricardo Koller (3):
  KVM: selftests: Add exception handling support for aarch64
  KVM: selftests: Add aarch64/debug-exceptions test
  KVM: selftests: Use a ucall for x86 unhandled vector reporting

 tools/testing/selftests/kvm/.gitignore        |   1 +
 tools/testing/selftests/kvm/Makefile          |   3 +-
 .../selftests/kvm/aarch64/debug-exceptions.c  | 250 ++++++++++++++++++
 .../selftests/kvm/include/aarch64/processor.h |  86 ++++++
 .../testing/selftests/kvm/include/kvm_util.h  |   1 +
 .../selftests/kvm/include/x86_64/processor.h  |   2 -
 .../selftests/kvm/lib/aarch64/handlers.S      | 104 ++++++++
 .../selftests/kvm/lib/aarch64/processor.c     |  56 ++++
 .../selftests/kvm/lib/x86_64/processor.c      |  15 +-
 9 files changed, 506 insertions(+), 12 deletions(-)
 create mode 100644 tools/testing/selftests/kvm/aarch64/debug-exceptions.c
 create mode 100644 tools/testing/selftests/kvm/lib/aarch64/handlers.S

-- 
2.31.1.498.g6c1eba8ee3d-goog

[PATCH 1/3] KVM: selftests: Add exception handling support for aarch64

[Ricardo Koller]

Add the infrastructure needed to enable exception handling in aarch64
selftests. The exception handling defaults to an unhandled-exception
handler which aborts the test, just like x86. These handlers can be
overridden by calling vm_handle_exception with a (vector, error-code)
tuple. The unhandled exception reporting from the guest is done using
the new ucall UCALL_UNHANDLED.

Signed-off-by: Ricardo Koller <ricarkol@google.com>
---
 tools/testing/selftests/kvm/Makefile          |   2 +-
 .../selftests/kvm/include/aarch64/processor.h |  69 ++++++++++++
 .../testing/selftests/kvm/include/kvm_util.h  |   1 +
 .../selftests/kvm/lib/aarch64/handlers.S      | 104 ++++++++++++++++++
 .../selftests/kvm/lib/aarch64/processor.c     |  56 ++++++++++
 5 files changed, 231 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/kvm/lib/aarch64/handlers.S

diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index 4e548d7ab0ab..618c5903f478 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -35,7 +35,7 @@ endif
 
 LIBKVM = lib/assert.c lib/elf.c lib/io.c lib/kvm_util.c lib/sparsebit.c lib/test_util.c lib/guest_modes.c lib/perf_test_util.c
 LIBKVM_x86_64 = lib/x86_64/processor.c lib/x86_64/vmx.c lib/x86_64/svm.c lib/x86_64/ucall.c lib/x86_64/handlers.S
-LIBKVM_aarch64 = lib/aarch64/processor.c lib/aarch64/ucall.c
+LIBKVM_aarch64 = lib/aarch64/processor.c lib/aarch64/ucall.c lib/aarch64/handlers.S
 LIBKVM_s390x = lib/s390x/processor.c lib/s390x/ucall.c lib/s390x/diag318_test_handler.c
 
 TEST_GEN_PROGS_x86_64 = x86_64/cr4_cpuid_sync_test
diff --git a/tools/testing/selftests/kvm/include/aarch64/processor.h b/tools/testing/selftests/kvm/include/aarch64/processor.h
index b7fa0c8551db..5c902ad95c35 100644
--- a/tools/testing/selftests/kvm/include/aarch64/processor.h
+++ b/tools/testing/selftests/kvm/include/aarch64/processor.h
@@ -8,6 +8,7 @@
 #define SELFTEST_KVM_PROCESSOR_H
 
 #include "kvm_util.h"
+#include <linux/stringify.h>
 
 
 #define ARM64_CORE_REG(x) (KVM_REG_ARM64 | KVM_REG_SIZE_U64 | \
@@ -18,6 +19,7 @@
 #define MAIR_EL1	3, 0, 10, 2, 0
 #define TTBR0_EL1	3, 0,  2, 0, 0
 #define SCTLR_EL1	3, 0,  1, 0, 0
+#define VBAR_EL1	3, 0, 12, 0, 0
 
 /*
  * Default MAIR
@@ -56,4 +58,71 @@ void aarch64_vcpu_setup(struct kvm_vm *vm, int vcpuid, struct kvm_vcpu_init *ini
 void aarch64_vcpu_add_default(struct kvm_vm *vm, uint32_t vcpuid,
 			      struct kvm_vcpu_init *init, void *guest_code);
 
+struct ex_regs {
+	u64 pc;
+	u64 pstate;
+	u64 sp;
+	u64 lr;
+	u64 regs[31];
+};
+
+#define VECTOR_NUM	16
+
+enum {
+	VECTOR_SYNC_EL1_SP0,
+	VECTOR_IRQ_EL1_SP0,
+	VECTOR_FIQ_EL1_SP0,
+	VECTOR_ERROR_EL1_SP0,
+
+	VECTOR_SYNC_EL1,
+	VECTOR_IRQ_EL1,
+	VECTOR_FIQ_EL1,
+	VECTOR_ERROR_EL1,
+
+	VECTOR_SYNC_EL0_64,
+	VECTOR_IRQ_EL0_64,
+	VECTOR_FIQ_EL0_64,
+	VECTOR_ERROR_EL0_64,
+
+	VECTOR_SYNC_EL0_32,
+	VECTOR_IRQ_EL0_32,
+	VECTOR_FIQ_EL0_32,
+	VECTOR_ERROR_EL0_32,
+};
+
+/* Some common EC (Exception classes) */
+#define ESR_EC_ILLEGAL_INS	0x0e
+#define ESR_EC_SVC64		0x15
+#define ESR_EC_IABORT_EL1	0x21
+#define ESR_EC_DABORT_EL1	0x25
+#define ESR_EC_SERROR		0x2f
+#define ESR_EC_HW_BP_EL1	0x31
+#define ESR_EC_SSTEP_EL1	0x33
+#define ESR_EC_WP_EL1		0x35
+#define ESR_EC_BRK_INS		0x3C
+
+#define ESR_EC_NUM		64
+
+#define ESR_EC_SHIFT		26
+#define ESR_EC_MASK		0x3f
+
+void vm_init_descriptor_tables(struct kvm_vm *vm);
+void vcpu_init_descriptor_tables(struct kvm_vm *vm, uint32_t vcpuid);
+void vm_handle_exception(struct kvm_vm *vm, int vector, int ec,
+			void (*handler)(struct ex_regs *));
+
+#define SPSR_D          (1 << 9)
+#define SPSR_SS         (1 << 21)
+
+#define write_sysreg(reg, val)						  \
+({									  \
+	asm volatile("msr "__stringify(reg)", %0" : : "r"(val));	  \
+})
+
+#define read_sysreg(reg)						  \
+({	u64 val;							  \
+	asm volatile("mrs %0, "__stringify(reg) : "=r"(val) : : "memory");\
+	val;								  \
+})
+
 #endif /* SELFTEST_KVM_PROCESSOR_H */
diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
index bea4644d645d..7880929ea548 100644
--- a/tools/testing/selftests/kvm/include/kvm_util.h
+++ b/tools/testing/selftests/kvm/include/kvm_util.h
@@ -347,6 +347,7 @@ enum {
 	UCALL_SYNC,
 	UCALL_ABORT,
 	UCALL_DONE,
+	UCALL_UNHANDLED,
 };
 
 #define UCALL_MAX_ARGS 6
diff --git a/tools/testing/selftests/kvm/lib/aarch64/handlers.S b/tools/testing/selftests/kvm/lib/aarch64/handlers.S
new file mode 100644
index 000000000000..c920679b87c0
--- /dev/null
+++ b/tools/testing/selftests/kvm/lib/aarch64/handlers.S
@@ -0,0 +1,104 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+.macro save_registers, el
+	stp	x28, x29, [sp, #-16]!
+	stp	x26, x27, [sp, #-16]!
+	stp	x24, x25, [sp, #-16]!
+	stp	x22, x23, [sp, #-16]!
+	stp	x20, x21, [sp, #-16]!
+	stp	x18, x19, [sp, #-16]!
+	stp	x16, x17, [sp, #-16]!
+	stp	x14, x15, [sp, #-16]!
+	stp	x12, x13, [sp, #-16]!
+	stp	x10, x11, [sp, #-16]!
+	stp	x8, x9, [sp, #-16]!
+	stp	x6, x7, [sp, #-16]!
+	stp	x4, x5, [sp, #-16]!
+	stp	x2, x3, [sp, #-16]!
+	stp	x0, x1, [sp, #-16]!
+
+	.if \el == 0
+	mrs	x1, sp_el0
+	.else
+	mov	x1, sp
+	.endif
+	stp	x1, lr, [sp, #-16]! /* SP, LR */
+
+	mrs	x1, elr_el1
+	mrs	x2, spsr_el1
+	stp	x1, x2, [sp, #-16]! /* PC, PSTATE */
+.endm
+
+.macro restore_registers, el
+	ldp	x1, x2, [sp], #16 /* PC, PSTATE */
+	msr	elr_el1, x1
+	msr	spsr_el1, x2
+
+	ldp	x1, lr, [sp], #16 /* SP, LR */
+	.if \el == 0
+	msr	sp_el0, x1
+	.endif
+
+	ldp	x0, x1, [sp], #16
+	ldp	x2, x3, [sp], #16
+	ldp	x4, x5, [sp], #16
+	ldp	x6, x7, [sp], #16
+	ldp	x8, x9, [sp], #16
+	ldp	x10, x11, [sp], #16
+	ldp	x12, x13, [sp], #16
+	ldp	x14, x15, [sp], #16
+	ldp	x16, x17, [sp], #16
+	ldp	x18, x19, [sp], #16
+	ldp	x20, x21, [sp], #16
+	ldp	x22, x23, [sp], #16
+	ldp	x24, x25, [sp], #16
+	ldp	x26, x27, [sp], #16
+	ldp	x28, x29, [sp], #16
+
+	eret
+.endm
+
+.pushsection ".entry.text", "ax"
+.balign 0x800
+.global vectors
+vectors:
+.popsection
+
+/*
+ * Build an exception handler for vector and append a jump to it into
+ * vectors (while making sure that it's 0x80 aligned).
+ */
+.macro HANDLER, el, label, vector
+handler\()\vector:
+	save_registers \el
+	mov	x0, sp
+	mov	x1, \vector
+	bl	route_exception
+	restore_registers \el
+
+.pushsection ".entry.text", "ax"
+.balign 0x80
+	b	handler\()\vector
+.popsection
+.endm
+
+.global ex_handler_code
+ex_handler_code:
+	HANDLER	1, sync, 0			// Synchronous EL1t
+	HANDLER	1, irq, 1			// IRQ EL1t
+	HANDLER	1, fiq, 2			// FIQ EL1t
+	HANDLER	1, error, 3			// Error EL1t
+
+	HANDLER	1, sync, 4			// Synchronous EL1h
+	HANDLER	1, irq, 5			// IRQ EL1h
+	HANDLER	1, fiq, 6			// FIQ EL1h
+	HANDLER	1, error, 7			// Error EL1h
+
+	HANDLER	0, sync, 8			// Synchronous 64-bit EL0
+	HANDLER	0, irq, 9			// IRQ 64-bit EL0
+	HANDLER	0, fiq, 10			// FIQ 64-bit EL0
+	HANDLER	0, error, 11			// Error 64-bit EL0
+
+	HANDLER	0, sync, 12			// Synchronous 32-bit EL0
+	HANDLER	0, irq, 13			// IRQ 32-bit EL0
+	HANDLER	0, fiq, 14			// FIQ 32-bit EL0
+	HANDLER	0, error, 15			// Error 32-bit EL0
diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c
index cee92d477dc0..286305b561d8 100644
--- a/tools/testing/selftests/kvm/lib/aarch64/processor.c
+++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c
@@ -6,6 +6,7 @@
  */
 
 #include <linux/compiler.h>
+#include <assert.h>
 
 #include "kvm_util.h"
 #include "../kvm_util_internal.h"
@@ -14,6 +15,8 @@
 #define KVM_GUEST_PAGE_TABLE_MIN_PADDR		0x180000
 #define DEFAULT_ARM64_GUEST_STACK_VADDR_MIN	0xac0000
 
+vm_vaddr_t exception_handlers;
+
 static uint64_t page_align(struct kvm_vm *vm, uint64_t v)
 {
 	return (v + vm->page_size) & ~(vm->page_size - 1);
@@ -336,4 +339,57 @@ void vcpu_args_set(struct kvm_vm *vm, uint32_t vcpuid, unsigned int num, ...)
 
 void assert_on_unhandled_exception(struct kvm_vm *vm, uint32_t vcpuid)
 {
+	struct ucall uc;
+
+	if (get_ucall(vm, vcpuid, &uc) == UCALL_UNHANDLED) {
+		TEST_ASSERT(false,
+			"Unexpected exception guest (vector:0x%lx, ec:0x%lx)",
+			uc.args[0], uc.args[1]);
+	}
+}
+
+void kvm_exit_unexpected_vector(int vector, uint64_t ec)
+{
+	ucall(UCALL_UNHANDLED, 2, vector, ec);
+}
+
+#define HANDLERS_IDX(_vector, _ec)	((_vector * ESR_EC_NUM) + _ec)
+
+void vm_init_descriptor_tables(struct kvm_vm *vm)
+{
+	vm->handlers = vm_vaddr_alloc(vm,
+			VECTOR_NUM * ESR_EC_NUM * sizeof(void *),
+			vm->page_size, 0, 0);
+	*(vm_vaddr_t *)addr_gva2hva(vm, (vm_vaddr_t)(&exception_handlers)) = vm->handlers;
+}
+
+void vcpu_init_descriptor_tables(struct kvm_vm *vm, uint32_t vcpuid)
+{
+	extern char vectors;
+
+	set_reg(vm, vcpuid, ARM64_SYS_REG(VBAR_EL1), (uint64_t)&vectors);
+}
+
+void route_exception(struct ex_regs *regs, int vector)
+{
+	typedef void(*handler)(struct ex_regs *);
+	uint64_t esr = read_sysreg(esr_el1);
+	uint64_t ec = (esr >> ESR_EC_SHIFT) & ESR_EC_MASK;
+
+	handler *handlers = (handler *)exception_handlers;
+
+	if (handlers && handlers[HANDLERS_IDX(vector, ec)])
+		handlers[HANDLERS_IDX(vector, ec)](regs);
+	else
+		kvm_exit_unexpected_vector(vector, ec);
+}
+
+void vm_handle_exception(struct kvm_vm *vm, int vector, int ec,
+			 void (*handler)(struct ex_regs *))
+{
+	vm_vaddr_t *handlers = (vm_vaddr_t *)addr_gva2hva(vm, vm->handlers);
+
+	assert(vector < VECTOR_NUM);
+	assert(ec < ESR_EC_NUM);
+	handlers[HANDLERS_IDX(vector, ec)] = (vm_vaddr_t)handler;
 }
-- 
2.31.1.498.g6c1eba8ee3d-goog

Re: [PATCH 1/3] KVM: selftests: Add exception handling support for aarch64

[Marc Zyngier]

Hi Ricardo,

Thanks for starting this.

On Fri, 23 Apr 2021 05:03:49 +0100,
Ricardo Koller <ricarkol@google.com> wrote:
> 
> Add the infrastructure needed to enable exception handling in aarch64
> selftests. The exception handling defaults to an unhandled-exception
> handler which aborts the test, just like x86. These handlers can be
> overridden by calling vm_handle_exception with a (vector, error-code)
> tuple. The unhandled exception reporting from the guest is done using
> the new ucall UCALL_UNHANDLED.
> 
> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> ---
>  tools/testing/selftests/kvm/Makefile          |   2 +-
>  .../selftests/kvm/include/aarch64/processor.h |  69 ++++++++++++
>  .../testing/selftests/kvm/include/kvm_util.h  |   1 +
>  .../selftests/kvm/lib/aarch64/handlers.S      | 104 ++++++++++++++++++
>  .../selftests/kvm/lib/aarch64/processor.c     |  56 ++++++++++
>  5 files changed, 231 insertions(+), 1 deletion(-)
>  create mode 100644 tools/testing/selftests/kvm/lib/aarch64/handlers.S
> 
> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> index 4e548d7ab0ab..618c5903f478 100644
> --- a/tools/testing/selftests/kvm/Makefile
> +++ b/tools/testing/selftests/kvm/Makefile
> @@ -35,7 +35,7 @@ endif
>  
>  LIBKVM = lib/assert.c lib/elf.c lib/io.c lib/kvm_util.c lib/sparsebit.c lib/test_util.c lib/guest_modes.c lib/perf_test_util.c
>  LIBKVM_x86_64 = lib/x86_64/processor.c lib/x86_64/vmx.c lib/x86_64/svm.c lib/x86_64/ucall.c lib/x86_64/handlers.S
> -LIBKVM_aarch64 = lib/aarch64/processor.c lib/aarch64/ucall.c
> +LIBKVM_aarch64 = lib/aarch64/processor.c lib/aarch64/ucall.c lib/aarch64/handlers.S
>  LIBKVM_s390x = lib/s390x/processor.c lib/s390x/ucall.c lib/s390x/diag318_test_handler.c
>  
>  TEST_GEN_PROGS_x86_64 = x86_64/cr4_cpuid_sync_test
> diff --git a/tools/testing/selftests/kvm/include/aarch64/processor.h b/tools/testing/selftests/kvm/include/aarch64/processor.h
> index b7fa0c8551db..5c902ad95c35 100644
> --- a/tools/testing/selftests/kvm/include/aarch64/processor.h
> +++ b/tools/testing/selftests/kvm/include/aarch64/processor.h
> @@ -8,6 +8,7 @@
>  #define SELFTEST_KVM_PROCESSOR_H
>  
>  #include "kvm_util.h"
> +#include <linux/stringify.h>
>  
>  
>  #define ARM64_CORE_REG(x) (KVM_REG_ARM64 | KVM_REG_SIZE_U64 | \
> @@ -18,6 +19,7 @@
>  #define MAIR_EL1	3, 0, 10, 2, 0
>  #define TTBR0_EL1	3, 0,  2, 0, 0
>  #define SCTLR_EL1	3, 0,  1, 0, 0
> +#define VBAR_EL1	3, 0, 12, 0, 0
>  
>  /*
>   * Default MAIR
> @@ -56,4 +58,71 @@ void aarch64_vcpu_setup(struct kvm_vm *vm, int vcpuid, struct kvm_vcpu_init *ini
>  void aarch64_vcpu_add_default(struct kvm_vm *vm, uint32_t vcpuid,
>  			      struct kvm_vcpu_init *init, void *guest_code);
>  
> +struct ex_regs {
> +	u64 pc;
> +	u64 pstate;
> +	u64 sp;
> +	u64 lr;
> +	u64 regs[31];
> +};

Is there any reason why you are not using the layout of user_pt_regs?
I'm not specially attached to it, but it would at least avoid bugs
such as having both 31 GPRs *and* lr in this structure (LR really is
just another name for X30). It would also make the reviewing easier
for us kernel people ;-).

> +
> +#define VECTOR_NUM	16
> +
> +enum {
> +	VECTOR_SYNC_EL1_SP0,
> +	VECTOR_IRQ_EL1_SP0,
> +	VECTOR_FIQ_EL1_SP0,
> +	VECTOR_ERROR_EL1_SP0,
> +
> +	VECTOR_SYNC_EL1,
> +	VECTOR_IRQ_EL1,
> +	VECTOR_FIQ_EL1,
> +	VECTOR_ERROR_EL1,
> +
> +	VECTOR_SYNC_EL0_64,
> +	VECTOR_IRQ_EL0_64,
> +	VECTOR_FIQ_EL0_64,
> +	VECTOR_ERROR_EL0_64,
> +
> +	VECTOR_SYNC_EL0_32,
> +	VECTOR_IRQ_EL0_32,
> +	VECTOR_FIQ_EL0_32,
> +	VECTOR_ERROR_EL0_32,

nit: in order to be true to the architecture and to prepare for the
NV support, it'd be better to rename *_EL1_* to *_CURRENT_*, and
*_EL0_* to *_LOWER_*. This doesn't change the semantics, but instead
shows how the exceptions can nest across the various exception levels.

> +};
> +
> +/* Some common EC (Exception classes) */
> +#define ESR_EC_ILLEGAL_INS	0x0e
> +#define ESR_EC_SVC64		0x15
> +#define ESR_EC_IABORT_EL1	0x21
> +#define ESR_EC_DABORT_EL1	0x25
> +#define ESR_EC_SERROR		0x2f
> +#define ESR_EC_HW_BP_EL1	0x31
> +#define ESR_EC_SSTEP_EL1	0x33
> +#define ESR_EC_WP_EL1		0x35
> +#define ESR_EC_BRK_INS		0x3C
> +
> +#define ESR_EC_NUM		64

Isn't this redundant with the mask below?

> +
> +#define ESR_EC_SHIFT		26
> +#define ESR_EC_MASK		0x3f
> +
> +void vm_init_descriptor_tables(struct kvm_vm *vm);
> +void vcpu_init_descriptor_tables(struct kvm_vm *vm, uint32_t vcpuid);
> +void vm_handle_exception(struct kvm_vm *vm, int vector, int ec,
> +			void (*handler)(struct ex_regs *));
> +
> +#define SPSR_D          (1 << 9)
> +#define SPSR_SS         (1 << 21)
> +
> +#define write_sysreg(reg, val)						  \
> +({									  \
> +	asm volatile("msr "__stringify(reg)", %0" : : "r"(val));	  \
> +})
> +
> +#define read_sysreg(reg)						  \
> +({	u64 val;							  \
> +	asm volatile("mrs %0, "__stringify(reg) : "=r"(val) : : "memory");\
> +	val;								  \
> +})
> +
>  #endif /* SELFTEST_KVM_PROCESSOR_H */
> diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
> index bea4644d645d..7880929ea548 100644
> --- a/tools/testing/selftests/kvm/include/kvm_util.h
> +++ b/tools/testing/selftests/kvm/include/kvm_util.h
> @@ -347,6 +347,7 @@ enum {
>  	UCALL_SYNC,
>  	UCALL_ABORT,
>  	UCALL_DONE,
> +	UCALL_UNHANDLED,
>  };
>  
>  #define UCALL_MAX_ARGS 6
> diff --git a/tools/testing/selftests/kvm/lib/aarch64/handlers.S b/tools/testing/selftests/kvm/lib/aarch64/handlers.S
> new file mode 100644
> index 000000000000..c920679b87c0
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/lib/aarch64/handlers.S
> @@ -0,0 +1,104 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +.macro save_registers, el
> +	stp	x28, x29, [sp, #-16]!
> +	stp	x26, x27, [sp, #-16]!
> +	stp	x24, x25, [sp, #-16]!
> +	stp	x22, x23, [sp, #-16]!
> +	stp	x20, x21, [sp, #-16]!
> +	stp	x18, x19, [sp, #-16]!
> +	stp	x16, x17, [sp, #-16]!
> +	stp	x14, x15, [sp, #-16]!
> +	stp	x12, x13, [sp, #-16]!
> +	stp	x10, x11, [sp, #-16]!
> +	stp	x8, x9, [sp, #-16]!
> +	stp	x6, x7, [sp, #-16]!
> +	stp	x4, x5, [sp, #-16]!
> +	stp	x2, x3, [sp, #-16]!
> +	stp	x0, x1, [sp, #-16]!
> +
> +	.if \el == 0
> +	mrs	x1, sp_el0
> +	.else
> +	mov	x1, sp
> +	.endif

It there any point in saving SP_EL1, given that you already have
altered it significantly and will not be restoring it? I don't care
much, and maybe it is useful as debug information, but a comment would
certainly make the intent clearer.

> +	stp	x1, lr, [sp, #-16]! /* SP, LR */

See my comment above about the X30/LR duality.

> +
> +	mrs	x1, elr_el1
> +	mrs	x2, spsr_el1
> +	stp	x1, x2, [sp, #-16]! /* PC, PSTATE */
> +.endm
> +
> +.macro restore_registers, el
> +	ldp	x1, x2, [sp], #16 /* PC, PSTATE */
> +	msr	elr_el1, x1
> +	msr	spsr_el1, x2
> +
> +	ldp	x1, lr, [sp], #16 /* SP, LR */
> +	.if \el == 0
> +	msr	sp_el0, x1
> +	.endif
> +
> +	ldp	x0, x1, [sp], #16
> +	ldp	x2, x3, [sp], #16
> +	ldp	x4, x5, [sp], #16
> +	ldp	x6, x7, [sp], #16
> +	ldp	x8, x9, [sp], #16
> +	ldp	x10, x11, [sp], #16
> +	ldp	x12, x13, [sp], #16
> +	ldp	x14, x15, [sp], #16
> +	ldp	x16, x17, [sp], #16
> +	ldp	x18, x19, [sp], #16
> +	ldp	x20, x21, [sp], #16
> +	ldp	x22, x23, [sp], #16
> +	ldp	x24, x25, [sp], #16
> +	ldp	x26, x27, [sp], #16
> +	ldp	x28, x29, [sp], #16
> +
> +	eret
> +.endm
> +
> +.pushsection ".entry.text", "ax"
> +.balign 0x800
> +.global vectors
> +vectors:
> +.popsection
> +
> +/*
> + * Build an exception handler for vector and append a jump to it into
> + * vectors (while making sure that it's 0x80 aligned).
> + */
> +.macro HANDLER, el, label, vector
> +handler\()\vector:
> +	save_registers \el
> +	mov	x0, sp
> +	mov	x1, \vector
> +	bl	route_exception
> +	restore_registers \el
> +
> +.pushsection ".entry.text", "ax"
> +.balign 0x80
> +	b	handler\()\vector
> +.popsection
> +.endm

That's an interesting construct, wildly different from what we are
using elsewhere in the kernel, but hey, I like change ;-). It'd be
good to add a comment to spell out that anything that emits into
.entry.text between the declaration of 'vectors' and the end of this
file will break everything.

> +
> +.global ex_handler_code
> +ex_handler_code:
> +	HANDLER	1, sync, 0			// Synchronous EL1t
> +	HANDLER	1, irq, 1			// IRQ EL1t
> +	HANDLER	1, fiq, 2			// FIQ EL1t
> +	HANDLER	1, error, 3			// Error EL1t

Can any of these actually happen? As far as I can see, the whole
selftest environment seems to be designed around EL1h.

> +
> +	HANDLER	1, sync, 4			// Synchronous EL1h
> +	HANDLER	1, irq, 5			// IRQ EL1h
> +	HANDLER	1, fiq, 6			// FIQ EL1h
> +	HANDLER	1, error, 7			// Error EL1h
> +
> +	HANDLER	0, sync, 8			// Synchronous 64-bit EL0
> +	HANDLER	0, irq, 9			// IRQ 64-bit EL0
> +	HANDLER	0, fiq, 10			// FIQ 64-bit EL0
> +	HANDLER	0, error, 11			// Error 64-bit EL0
> +
> +	HANDLER	0, sync, 12			// Synchronous 32-bit EL0
> +	HANDLER	0, irq, 13			// IRQ 32-bit EL0
> +	HANDLER	0, fiq, 14			// FIQ 32-bit EL0
> +	HANDLER	0, error, 15			// Error 32-bit EL0

I find the whole numbering odd and pretty error prone. You could work
it out from the alignment of the vector if you really need it.

> diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> index cee92d477dc0..286305b561d8 100644
> --- a/tools/testing/selftests/kvm/lib/aarch64/processor.c
> +++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> @@ -6,6 +6,7 @@
>   */
>  
>  #include <linux/compiler.h>
> +#include <assert.h>
>  
>  #include "kvm_util.h"
>  #include "../kvm_util_internal.h"
> @@ -14,6 +15,8 @@
>  #define KVM_GUEST_PAGE_TABLE_MIN_PADDR		0x180000
>  #define DEFAULT_ARM64_GUEST_STACK_VADDR_MIN	0xac0000
>  
> +vm_vaddr_t exception_handlers;
> +
>  static uint64_t page_align(struct kvm_vm *vm, uint64_t v)
>  {
>  	return (v + vm->page_size) & ~(vm->page_size - 1);
> @@ -336,4 +339,57 @@ void vcpu_args_set(struct kvm_vm *vm, uint32_t vcpuid, unsigned int num, ...)
>  
>  void assert_on_unhandled_exception(struct kvm_vm *vm, uint32_t vcpuid)
>  {
> +	struct ucall uc;
> +
> +	if (get_ucall(vm, vcpuid, &uc) == UCALL_UNHANDLED) {
> +		TEST_ASSERT(false,
> +			"Unexpected exception guest (vector:0x%lx, ec:0x%lx)",
> +			uc.args[0], uc.args[1]);
> +	}
> +}
> +
> +void kvm_exit_unexpected_vector(int vector, uint64_t ec)
> +{
> +	ucall(UCALL_UNHANDLED, 2, vector, ec);
> +}
> +
> +#define HANDLERS_IDX(_vector, _ec)	((_vector * ESR_EC_NUM) + _ec)

This is definitely odd. Not all the ECs are valid for all vector entry
points. Actually, ECs only make sense for synchronous exceptions, and
asynchronous events (IRQ, FIQ, SError) cannot populate ESR_ELx.

> +
> +void vm_init_descriptor_tables(struct kvm_vm *vm)
> +{
> +	vm->handlers = vm_vaddr_alloc(vm,
> +			VECTOR_NUM * ESR_EC_NUM * sizeof(void *),
> +			vm->page_size, 0, 0);
> +	*(vm_vaddr_t *)addr_gva2hva(vm, (vm_vaddr_t)(&exception_handlers)) = vm->handlers;
> +}
> +
> +void vcpu_init_descriptor_tables(struct kvm_vm *vm, uint32_t vcpuid)
> +{
> +	extern char vectors;
> +
> +	set_reg(vm, vcpuid, ARM64_SYS_REG(VBAR_EL1), (uint64_t)&vectors);
> +}
> +
> +void route_exception(struct ex_regs *regs, int vector)
> +{
> +	typedef void(*handler)(struct ex_regs *);
> +	uint64_t esr = read_sysreg(esr_el1);
> +	uint64_t ec = (esr >> ESR_EC_SHIFT) & ESR_EC_MASK;
> +
> +	handler *handlers = (handler *)exception_handlers;
> +
> +	if (handlers && handlers[HANDLERS_IDX(vector, ec)])
> +		handlers[HANDLERS_IDX(vector, ec)](regs);

See my comment above. This will do the wrong thing in presence of an
interrupt.

> +	else
> +		kvm_exit_unexpected_vector(vector, ec);
> +}
> +
> +void vm_handle_exception(struct kvm_vm *vm, int vector, int ec,
> +			 void (*handler)(struct ex_regs *))

The name seems to be slightly ill defined. To me "handle exception" is
the action of handling the exception. Here, you are merely installing
an exception handler.

> +{
> +	vm_vaddr_t *handlers = (vm_vaddr_t *)addr_gva2hva(vm, vm->handlers);
> +
> +	assert(vector < VECTOR_NUM);
> +	assert(ec < ESR_EC_NUM);
> +	handlers[HANDLERS_IDX(vector, ec)] = (vm_vaddr_t)handler;
>  }
> -- 
> 2.31.1.498.g6c1eba8ee3d-goog
> 
> 

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 1/3] KVM: selftests: Add exception handling support for aarch64

[Andrew Jones]

On Fri, Apr 23, 2021 at 09:58:24AM +0100, Marc Zyngier wrote:
> Hi Ricardo,
> 
> Thanks for starting this.

Indeed! Thank you for contributing to AArch64 kvm selftests!

> > +void vm_handle_exception(struct kvm_vm *vm, int vector, int ec,
> > +			void (*handler)(struct ex_regs *));
> > +
> > +#define SPSR_D          (1 << 9)
> > +#define SPSR_SS         (1 << 21)
> > +
> > +#define write_sysreg(reg, val)						  \
> > +({									  \
> > +	asm volatile("msr "__stringify(reg)", %0" : : "r"(val));	  \
> > +})

Linux does fancy stuff with the Z constraint to allow xzr. We might as
well copy that.

> > diff --git a/tools/testing/selftests/kvm/lib/aarch64/handlers.S b/tools/testing/selftests/kvm/lib/aarch64/handlers.S
> > new file mode 100644
> > index 000000000000..c920679b87c0
> > --- /dev/null
> > +++ b/tools/testing/selftests/kvm/lib/aarch64/handlers.S
> > @@ -0,0 +1,104 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +.macro save_registers, el
> > +	stp	x28, x29, [sp, #-16]!
> > +	stp	x26, x27, [sp, #-16]!
> > +	stp	x24, x25, [sp, #-16]!
> > +	stp	x22, x23, [sp, #-16]!
> > +	stp	x20, x21, [sp, #-16]!
> > +	stp	x18, x19, [sp, #-16]!
> > +	stp	x16, x17, [sp, #-16]!
> > +	stp	x14, x15, [sp, #-16]!
> > +	stp	x12, x13, [sp, #-16]!
> > +	stp	x10, x11, [sp, #-16]!
> > +	stp	x8, x9, [sp, #-16]!
> > +	stp	x6, x7, [sp, #-16]!
> > +	stp	x4, x5, [sp, #-16]!
> > +	stp	x2, x3, [sp, #-16]!
> > +	stp	x0, x1, [sp, #-16]!
> > +
> > +	.if \el == 0
> > +	mrs	x1, sp_el0
> > +	.else
> > +	mov	x1, sp
> > +	.endif
> 
> It there any point in saving SP_EL1, given that you already have
> altered it significantly and will not be restoring it? I don't care
> much, and maybe it is useful as debug information, but a comment would
> certainly make the intent clearer.

kvm-unit-tests takes some pains to save the original sp. We may be able to
take some inspiration from there for this save and restore.

> > +void kvm_exit_unexpected_vector(int vector, uint64_t ec)
> > +{
> > +	ucall(UCALL_UNHANDLED, 2, vector, ec);
> > +}
> > +
> > +#define HANDLERS_IDX(_vector, _ec)	((_vector * ESR_EC_NUM) + _ec)
> 
> This is definitely odd. Not all the ECs are valid for all vector entry
> points. Actually, ECs only make sense for synchronous exceptions, and
> asynchronous events (IRQ, FIQ, SError) cannot populate ESR_ELx.

For this, kvm-unit-tests provides a separate API for interrupt handler
installation, which ensures ec is not used. Also, kvm-unit-tests uses
a 2-D array [vector][ec] for the synchronous exceptions. I think we
should be able to use a 2-D array here too, instead of the IDX macro.

> > +void vm_handle_exception(struct kvm_vm *vm, int vector, int ec,
> > +			 void (*handler)(struct ex_regs *))
> 
> The name seems to be slightly ill defined. To me "handle exception" is
> the action of handling the exception. Here, you are merely installing
> an exception handler.
>

I agree. Please rename this for all of kvm selftests to something with
'install' in the name with the first patch of this series.

Thanks,
drew

Re: [PATCH 1/3] KVM: selftests: Add exception handling support for aarch64

[Ricardo Koller]

On Fri, Apr 23, 2021 at 01:05:29PM +0200, Andrew Jones wrote:
> On Fri, Apr 23, 2021 at 09:58:24AM +0100, Marc Zyngier wrote:
> > Hi Ricardo,
> > 
> > Thanks for starting this.
> 
> Indeed! Thank you for contributing to AArch64 kvm selftests!
> 
> > > +void vm_handle_exception(struct kvm_vm *vm, int vector, int ec,
> > > +			void (*handler)(struct ex_regs *));
> > > +
> > > +#define SPSR_D          (1 << 9)
> > > +#define SPSR_SS         (1 << 21)
> > > +
> > > +#define write_sysreg(reg, val)						  \
> > > +({									  \
> > > +	asm volatile("msr "__stringify(reg)", %0" : : "r"(val));	  \
> > > +})
> 
> Linux does fancy stuff with the Z constraint to allow xzr. We might as
> well copy that.
> 
> > > diff --git a/tools/testing/selftests/kvm/lib/aarch64/handlers.S b/tools/testing/selftests/kvm/lib/aarch64/handlers.S
> > > new file mode 100644
> > > index 000000000000..c920679b87c0
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/kvm/lib/aarch64/handlers.S
> > > @@ -0,0 +1,104 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +.macro save_registers, el
> > > +	stp	x28, x29, [sp, #-16]!
> > > +	stp	x26, x27, [sp, #-16]!
> > > +	stp	x24, x25, [sp, #-16]!
> > > +	stp	x22, x23, [sp, #-16]!
> > > +	stp	x20, x21, [sp, #-16]!
> > > +	stp	x18, x19, [sp, #-16]!
> > > +	stp	x16, x17, [sp, #-16]!
> > > +	stp	x14, x15, [sp, #-16]!
> > > +	stp	x12, x13, [sp, #-16]!
> > > +	stp	x10, x11, [sp, #-16]!
> > > +	stp	x8, x9, [sp, #-16]!
> > > +	stp	x6, x7, [sp, #-16]!
> > > +	stp	x4, x5, [sp, #-16]!
> > > +	stp	x2, x3, [sp, #-16]!
> > > +	stp	x0, x1, [sp, #-16]!
> > > +
> > > +	.if \el == 0
> > > +	mrs	x1, sp_el0
> > > +	.else
> > > +	mov	x1, sp
> > > +	.endif
> > 
> > It there any point in saving SP_EL1, given that you already have
> > altered it significantly and will not be restoring it? I don't care
> > much, and maybe it is useful as debug information, but a comment would
> > certainly make the intent clearer.
> 
> kvm-unit-tests takes some pains to save the original sp. We may be able to
> take some inspiration from there for this save and restore.
> 
> > > +void kvm_exit_unexpected_vector(int vector, uint64_t ec)
> > > +{
> > > +	ucall(UCALL_UNHANDLED, 2, vector, ec);
> > > +}
> > > +
> > > +#define HANDLERS_IDX(_vector, _ec)	((_vector * ESR_EC_NUM) + _ec)
> > 
> > This is definitely odd. Not all the ECs are valid for all vector entry
> > points. Actually, ECs only make sense for synchronous exceptions, and
> > asynchronous events (IRQ, FIQ, SError) cannot populate ESR_ELx.
> 
> For this, kvm-unit-tests provides a separate API for interrupt handler
> installation, which ensures ec is not used. Also, kvm-unit-tests uses
> a 2-D array [vector][ec] for the synchronous exceptions. I think we
> should be able to use a 2-D array here too, instead of the IDX macro.
> 
> > > +void vm_handle_exception(struct kvm_vm *vm, int vector, int ec,
> > > +			 void (*handler)(struct ex_regs *))
> > 
> > The name seems to be slightly ill defined. To me "handle exception" is
> > the action of handling the exception. Here, you are merely installing
> > an exception handler.
> >
> 
> I agree. Please rename this for all of kvm selftests to something with
> 'install' in the name with the first patch of this series.
> 
> Thanks,
> drew
> 

Thank you Andrew and Marc for the reviews. Will send v2 with all the
feedback.

Re: [PATCH 1/3] KVM: selftests: Add exception handling support for aarch64

[Ricardo Koller]

On Fri, Apr 23, 2021 at 09:58:24AM +0100, Marc Zyngier wrote:
> Hi Ricardo,
> 
> Thanks for starting this.
> 
> On Fri, 23 Apr 2021 05:03:49 +0100,
> Ricardo Koller <ricarkol@google.com> wrote:
> > +.pushsection ".entry.text", "ax"
> > +.balign 0x800
> > +.global vectors
> > +vectors:
> > +.popsection
> > +
> > +/*
> > + * Build an exception handler for vector and append a jump to it into
> > + * vectors (while making sure that it's 0x80 aligned).
> > + */
> > +.macro HANDLER, el, label, vector
> > +handler\()\vector:
> > +	save_registers \el
> > +	mov	x0, sp
> > +	mov	x1, \vector
> > +	bl	route_exception
> > +	restore_registers \el
> > +
> > +.pushsection ".entry.text", "ax"
> > +.balign 0x80
> > +	b	handler\()\vector
> > +.popsection
> > +.endm
> 
> That's an interesting construct, wildly different from what we are
> using elsewhere in the kernel, but hey, I like change ;-). It'd be
> good to add a comment to spell out that anything that emits into
> .entry.text between the declaration of 'vectors' and the end of this
> file will break everything.
> 
> > +
> > +.global ex_handler_code
> > +ex_handler_code:
> > +	HANDLER	1, sync, 0			// Synchronous EL1t
> > +	HANDLER	1, irq, 1			// IRQ EL1t
> > +	HANDLER	1, fiq, 2			// FIQ EL1t
> > +	HANDLER	1, error, 3			// Error EL1t
> 
> Can any of these actually happen? As far as I can see, the whole
> selftest environment seems to be designed around EL1h.
>

They can happen. KVM defaults to use EL1h:

	#define VCPU_RESET_PSTATE_EL1   (PSR_MODE_EL1h | PSR_A_BIT | PSR_I_BIT | \

but then a guest can set the SPSel to 0:

	asm volatile("msr spsel, #0");

and this happens:

	  Unexpected exception guest (vector:0x0, ec:0x25)

I think it should still be a valid situation: some test might want to
try it.

Thanks,
Ricardo

Re: [PATCH 1/3] KVM: selftests: Add exception handling support for aarch64

[Marc Zyngier]

AOn Thu, 29 Apr 2021 18:51:59 +0100,
Ricardo Koller <ricarkol@google.com> wrote:
> 
> On Fri, Apr 23, 2021 at 09:58:24AM +0100, Marc Zyngier wrote:
> > Hi Ricardo,
> > 
> > Thanks for starting this.
> > 
> > On Fri, 23 Apr 2021 05:03:49 +0100,
> > Ricardo Koller <ricarkol@google.com> wrote:
> > > +.pushsection ".entry.text", "ax"
> > > +.balign 0x800
> > > +.global vectors
> > > +vectors:
> > > +.popsection
> > > +
> > > +/*
> > > + * Build an exception handler for vector and append a jump to it into
> > > + * vectors (while making sure that it's 0x80 aligned).
> > > + */
> > > +.macro HANDLER, el, label, vector
> > > +handler\()\vector:
> > > +	save_registers \el
> > > +	mov	x0, sp
> > > +	mov	x1, \vector
> > > +	bl	route_exception
> > > +	restore_registers \el
> > > +
> > > +.pushsection ".entry.text", "ax"
> > > +.balign 0x80
> > > +	b	handler\()\vector
> > > +.popsection
> > > +.endm
> > 
> > That's an interesting construct, wildly different from what we are
> > using elsewhere in the kernel, but hey, I like change ;-). It'd be
> > good to add a comment to spell out that anything that emits into
> > .entry.text between the declaration of 'vectors' and the end of this
> > file will break everything.
> > 
> > > +
> > > +.global ex_handler_code
> > > +ex_handler_code:
> > > +	HANDLER	1, sync, 0			// Synchronous EL1t
> > > +	HANDLER	1, irq, 1			// IRQ EL1t
> > > +	HANDLER	1, fiq, 2			// FIQ EL1t
> > > +	HANDLER	1, error, 3			// Error EL1t
> > 
> > Can any of these actually happen? As far as I can see, the whole
> > selftest environment seems to be designed around EL1h.
> >
> 
> They can happen. KVM defaults to use EL1h:

That's not a KVM decision. That's an architectural requirement. Reset
is an exception, exception use the handler mode.

> 
> 	#define VCPU_RESET_PSTATE_EL1   (PSR_MODE_EL1h | PSR_A_BIT | PSR_I_BIT | \
> 
> but then a guest can set the SPSel to 0:
> 
> 	asm volatile("msr spsel, #0");
> 
> and this happens:
> 
> 	  Unexpected exception guest (vector:0x0, ec:0x25)
> 
> I think it should still be a valid situation: some test might want to
> try it.

Sure, but that's not what this test (in patch #2) is doing, is it?
If, as I believe, this is an unexpected situation, why not handle it
separately? I'm not advocating one way or another, but it'd be good to
understand the actual scope of the exception handling in this
infrastructure.

If you plan to allow tests to run in the EL1t environment, where do
you decide to switch back to EL1t after taking the exception in EL1h?
Are the tests supposed to implement both stack layouts?

Overall, I'm worried that nobody is going to use this layout *unless*
it becomes mandated.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 1/3] KVM: selftests: Add exception handling support for aarch64

[Ricardo Koller]

On Thu, Apr 29, 2021 at 08:59:14PM +0100, Marc Zyngier wrote:
> AOn Thu, 29 Apr 2021 18:51:59 +0100,
> Ricardo Koller <ricarkol@google.com> wrote:
> > 
> > On Fri, Apr 23, 2021 at 09:58:24AM +0100, Marc Zyngier wrote:
> > > Hi Ricardo,
> > > 
> > > Thanks for starting this.
> > > 
> > > On Fri, 23 Apr 2021 05:03:49 +0100,
> > > Ricardo Koller <ricarkol@google.com> wrote:
> > > > +.pushsection ".entry.text", "ax"
> > > > +.balign 0x800
> > > > +.global vectors
> > > > +vectors:
> > > > +.popsection
> > > > +
> > > > +/*
> > > > + * Build an exception handler for vector and append a jump to it into
> > > > + * vectors (while making sure that it's 0x80 aligned).
> > > > + */
> > > > +.macro HANDLER, el, label, vector
> > > > +handler\()\vector:
> > > > +	save_registers \el
> > > > +	mov	x0, sp
> > > > +	mov	x1, \vector
> > > > +	bl	route_exception
> > > > +	restore_registers \el
> > > > +
> > > > +.pushsection ".entry.text", "ax"
> > > > +.balign 0x80
> > > > +	b	handler\()\vector
> > > > +.popsection
> > > > +.endm
> > > 
> > > That's an interesting construct, wildly different from what we are
> > > using elsewhere in the kernel, but hey, I like change ;-). It'd be
> > > good to add a comment to spell out that anything that emits into
> > > .entry.text between the declaration of 'vectors' and the end of this
> > > file will break everything.
> > > 
> > > > +
> > > > +.global ex_handler_code
> > > > +ex_handler_code:
> > > > +	HANDLER	1, sync, 0			// Synchronous EL1t
> > > > +	HANDLER	1, irq, 1			// IRQ EL1t
> > > > +	HANDLER	1, fiq, 2			// FIQ EL1t
> > > > +	HANDLER	1, error, 3			// Error EL1t
> > > 
> > > Can any of these actually happen? As far as I can see, the whole
> > > selftest environment seems to be designed around EL1h.
> > >
> > 
> > They can happen. KVM defaults to use EL1h:
> 
> That's not a KVM decision. That's an architectural requirement. Reset
> is an exception, exception use the handler mode.
> 

That makes sense, thanks for the clarification.

> > 
> > 	#define VCPU_RESET_PSTATE_EL1   (PSR_MODE_EL1h | PSR_A_BIT | PSR_I_BIT | \
> > 
> > but then a guest can set the SPSel to 0:
> > 
> > 	asm volatile("msr spsel, #0");
> > 
> > and this happens:
> > 
> > 	  Unexpected exception guest (vector:0x0, ec:0x25)
> > 
> > I think it should still be a valid situation: some test might want to
> > try it.
> 
> Sure, but that's not what this test (in patch #2) is doing, is it?
> If, as I believe, this is an unexpected situation, why not handle it
> separately? I'm not advocating one way or another, but it'd be good to
> understand the actual scope of the exception handling in this
> infrastructure.
> 
> If you plan to allow tests to run in the EL1t environment, where do
> you decide to switch back to EL1t after taking the exception in EL1h?
> Are the tests supposed to implement both stack layouts?
> 
> Overall, I'm worried that nobody is going to use this layout *unless*
> it becomes mandated.
> 
> Thanks,
> 
> 	M.
> 
> -- 
> Without deviation from the norm, progress is not possible.

Got it, I see your point. Yes, I'm definitely not planning to use it.
Will just treat those vectors as "invalid".

Thanks again,
Ricardo

[PATCH 2/3] KVM: selftests: Add aarch64/debug-exceptions test

[Ricardo Koller]

Covers fundamental tests for debug exceptions. The guest installs and
handle its debug exceptions itself, without KVM_SET_GUEST_DEBUG.

Signed-off-by: Ricardo Koller <ricarkol@google.com>
---
 tools/testing/selftests/kvm/.gitignore        |   1 +
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../selftests/kvm/aarch64/debug-exceptions.c  | 250 ++++++++++++++++++
 .../selftests/kvm/include/aarch64/processor.h |  17 ++
 4 files changed, 269 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/aarch64/debug-exceptions.c

diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
index e65d5572aefc..f09ed908422b 100644
--- a/tools/testing/selftests/kvm/.gitignore
+++ b/tools/testing/selftests/kvm/.gitignore
@@ -1,4 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0-only
+/aarch64/debug-exceptions
 /aarch64/get-reg-list
 /aarch64/get-reg-list-sve
 /aarch64/vgic_init
diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index 618c5903f478..2f92442c0cc9 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -73,6 +73,7 @@ TEST_GEN_PROGS_x86_64 += memslot_modification_stress_test
 TEST_GEN_PROGS_x86_64 += set_memory_region_test
 TEST_GEN_PROGS_x86_64 += steal_time
 
+TEST_GEN_PROGS_aarch64 += aarch64/debug-exceptions
 TEST_GEN_PROGS_aarch64 += aarch64/get-reg-list
 TEST_GEN_PROGS_aarch64 += aarch64/get-reg-list-sve
 TEST_GEN_PROGS_aarch64 += aarch64/vgic_init
diff --git a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
new file mode 100644
index 000000000000..18e8de2711d3
--- /dev/null
+++ b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
@@ -0,0 +1,250 @@
+// SPDX-License-Identifier: GPL-2.0
+#define _GNU_SOURCE /* for program_invocation_short_name */
+#include <fcntl.h>
+#include <pthread.h>
+#include <sched.h>
+#include <semaphore.h>
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/ioctl.h>
+#include <sys/mman.h>
+#include <asm/barrier.h>
+
+#include <linux/compiler.h>
+
+#include <test_util.h>
+#include <kvm_util.h>
+#include <processor.h>
+
+#define VCPU_ID 0
+
+extern unsigned char sw_bp, hw_bp, bp_svc, bp_brk, hw_wp, ss_start;
+static volatile uint64_t sw_bp_addr, hw_bp_addr;
+static volatile uint64_t wp_addr, wp_data_addr;
+static volatile uint64_t svc_addr;
+static volatile uint64_t ss_addr[4], ss_idx;
+#define  CAST_TO_PC(v)  ((uint64_t)&(v))
+
+static void reset_debug_state(void)
+{
+	asm volatile("msr daifset, #8");
+
+	write_sysreg(osdlr_el1, 0);
+	write_sysreg(oslar_el1, 0);
+	asm volatile("isb" : : : "memory");
+
+	write_sysreg(mdscr_el1, 0);
+	/* This test only uses the first bp and wp slot. */
+	write_sysreg(dbgbvr0_el1, 0);
+	write_sysreg(dbgbcr0_el1, 0);
+	write_sysreg(dbgwcr0_el1, 0);
+	write_sysreg(dbgwvr0_el1, 0);
+	asm volatile("isb" : : : "memory");
+}
+
+static void install_wp(uint64_t addr)
+{
+	uint32_t wcr;
+	uint32_t mdscr;
+
+	wcr = DBGWCR_LEN8 | DBGWCR_RD | DBGWCR_WR | DBGWCR_EL1 | DBGWCR_E;
+	write_sysreg(dbgwcr0_el1, wcr);
+	write_sysreg(dbgwvr0_el1, addr);
+	asm volatile("isb" : : : "memory");
+
+	asm volatile("msr daifclr, #8");
+
+	mdscr = read_sysreg(mdscr_el1) | MDSCR_KDE | MDSCR_MDE;
+	write_sysreg(mdscr_el1, mdscr);
+}
+
+static void install_hw_bp(uint64_t addr)
+{
+	uint32_t bcr;
+	uint32_t mdscr;
+
+	bcr = DBGBCR_LEN8 | DBGBCR_EXEC | DBGBCR_EL1 | DBGBCR_E;
+	write_sysreg(dbgbcr0_el1, bcr);
+	write_sysreg(dbgbvr0_el1, addr);
+	asm volatile("isb" : : : "memory");
+
+	asm volatile("msr daifclr, #8");
+
+	mdscr = read_sysreg(mdscr_el1) | MDSCR_KDE | MDSCR_MDE;
+	write_sysreg(mdscr_el1, mdscr);
+}
+
+static void install_ss(void)
+{
+	uint32_t mdscr;
+
+	asm volatile("msr daifclr, #8");
+
+	mdscr = read_sysreg(mdscr_el1) | MDSCR_KDE | MDSCR_SS;
+	write_sysreg(mdscr_el1, mdscr);
+}
+
+static volatile char write_data;
+
+#define GUEST_ASSERT_EQ(arg1, arg2) \
+	GUEST_ASSERT_2((arg1) == (arg2), (arg1), (arg2))
+
+static void guest_code(void)
+{
+	GUEST_SYNC(0);
+
+	/* Software-breakpoint */
+	asm volatile("sw_bp: brk #0");
+	GUEST_ASSERT_EQ(sw_bp_addr, CAST_TO_PC(sw_bp));
+
+	GUEST_SYNC(1);
+
+	/* Hardware-breakpoint */
+	reset_debug_state();
+	install_hw_bp(CAST_TO_PC(hw_bp));
+	asm volatile("hw_bp: nop");
+	GUEST_ASSERT_EQ(hw_bp_addr, CAST_TO_PC(hw_bp));
+
+	GUEST_SYNC(2);
+
+	/* Hardware-breakpoint + svc */
+	reset_debug_state();
+	install_hw_bp(CAST_TO_PC(bp_svc));
+	asm volatile("bp_svc: svc #0");
+	GUEST_ASSERT_EQ(hw_bp_addr, CAST_TO_PC(bp_svc));
+	GUEST_ASSERT_EQ(svc_addr, CAST_TO_PC(bp_svc) + 4);
+
+	GUEST_SYNC(3);
+
+	/* Hardware-breakpoint + software-breakpoint */
+	reset_debug_state();
+	install_hw_bp(CAST_TO_PC(bp_brk));
+	asm volatile("bp_brk: brk #0");
+	GUEST_ASSERT_EQ(sw_bp_addr, CAST_TO_PC(bp_brk));
+	GUEST_ASSERT_EQ(hw_bp_addr, CAST_TO_PC(bp_brk));
+
+	GUEST_SYNC(4);
+
+	/* Watchpoint */
+	reset_debug_state();
+	install_wp(CAST_TO_PC(write_data));
+	write_data = 'x';
+	GUEST_ASSERT_EQ(write_data, 'x');
+	GUEST_ASSERT_EQ(wp_data_addr, CAST_TO_PC(write_data));
+
+	GUEST_SYNC(5);
+
+	/* Single-step */
+	reset_debug_state();
+	install_ss();
+	ss_idx = 0;
+	asm volatile("ss_start:\n"
+		     "mrs x0, esr_el1\n"
+		     "add x0, x0, #1\n"
+		     "msr daifset, #8\n"
+		     : : : "x0");
+	GUEST_ASSERT_EQ(ss_addr[0], CAST_TO_PC(ss_start));
+	GUEST_ASSERT_EQ(ss_addr[1], CAST_TO_PC(ss_start) + 4);
+	GUEST_ASSERT_EQ(ss_addr[2], CAST_TO_PC(ss_start) + 8);
+
+	GUEST_DONE();
+}
+
+static void guest_sw_bp_handler(struct ex_regs *regs)
+{
+	sw_bp_addr = regs->pc;
+	regs->pc += 4;
+}
+
+static void guest_hw_bp_handler(struct ex_regs *regs)
+{
+	hw_bp_addr = regs->pc;
+	regs->pstate |= SPSR_D;
+}
+
+static void guest_wp_handler(struct ex_regs *regs)
+{
+	wp_data_addr = read_sysreg(far_el1);
+	wp_addr = regs->pc;
+	regs->pstate |= SPSR_D;
+}
+
+static void guest_ss_handler(struct ex_regs *regs)
+{
+	GUEST_ASSERT_1(ss_idx < 4, ss_idx);
+	ss_addr[ss_idx++] = regs->pc;
+	regs->pstate |= SPSR_SS;
+}
+
+static void guest_svc_handler(struct ex_regs *regs)
+{
+	svc_addr = regs->pc;
+}
+
+static int debug_version(struct kvm_vm *vm)
+{
+	uint64_t id_aa64dfr0;
+
+	get_reg(vm, VCPU_ID, ARM64_SYS_REG(ID_AA64DFR0_EL1), &id_aa64dfr0);
+	return id_aa64dfr0 & 0xf;
+}
+
+int main(int argc, char *argv[])
+{
+	struct kvm_vm *vm;
+	struct ucall uc;
+	int stage;
+	int ret;
+
+	vm = vm_create_default(VCPU_ID, 0, guest_code);
+	ucall_init(vm, NULL);
+
+	vm_init_descriptor_tables(vm);
+	vcpu_init_descriptor_tables(vm, VCPU_ID);
+
+	if (debug_version(vm) < 6) {
+		print_skip("Armv8 debug architecture not supported.");
+		kvm_vm_free(vm);
+		exit(KSFT_SKIP);
+	}
+
+	vm_handle_exception(vm, VECTOR_SYNC_EL1,
+			ESR_EC_BRK_INS, guest_sw_bp_handler);
+	vm_handle_exception(vm, VECTOR_SYNC_EL1,
+			ESR_EC_HW_BP_EL1, guest_hw_bp_handler);
+	vm_handle_exception(vm, VECTOR_SYNC_EL1,
+			ESR_EC_WP_EL1, guest_wp_handler);
+	vm_handle_exception(vm, VECTOR_SYNC_EL1,
+			ESR_EC_SSTEP_EL1, guest_ss_handler);
+	vm_handle_exception(vm, VECTOR_SYNC_EL1,
+			ESR_EC_SVC64, guest_svc_handler);
+
+	for (stage = 0; stage < 7; stage++) {
+		ret = _vcpu_run(vm, VCPU_ID);
+
+		TEST_ASSERT(ret == 0, "vcpu_run failed: %d\n", ret);
+		switch (get_ucall(vm, VCPU_ID, &uc)) {
+		case UCALL_SYNC:
+			TEST_ASSERT(uc.args[1] == stage,
+				"Stage %d: Unexpected sync ucall, got %lx",
+				stage, (ulong)uc.args[1]);
+
+			break;
+		case UCALL_ABORT:
+			TEST_FAIL("%s at %s:%ld\n\tvalues: %#lx, %#lx",
+				(const char *)uc.args[0],
+				__FILE__, uc.args[1], uc.args[2], uc.args[3]);
+			break;
+		case UCALL_DONE:
+			goto done;
+		default:
+			TEST_FAIL("Unknown ucall %lu", uc.cmd);
+		}
+	}
+
+done:
+	kvm_vm_free(vm);
+	return 0;
+}
diff --git a/tools/testing/selftests/kvm/include/aarch64/processor.h b/tools/testing/selftests/kvm/include/aarch64/processor.h
index 5c902ad95c35..eee69b92e01e 100644
--- a/tools/testing/selftests/kvm/include/aarch64/processor.h
+++ b/tools/testing/selftests/kvm/include/aarch64/processor.h
@@ -21,6 +21,8 @@
 #define SCTLR_EL1	3, 0,  1, 0, 0
 #define VBAR_EL1	3, 0, 12, 0, 0
 
+#define ID_AA64DFR0_EL1	3, 0,  0, 5, 0
+
 /*
  * Default MAIR
  *                  index   attribute
@@ -125,4 +127,19 @@ void vm_handle_exception(struct kvm_vm *vm, int vector, int ec,
 	val;								  \
 })
 
+#define MDSCR_KDE	(1 << 13)
+#define MDSCR_MDE	(1 << 15)
+#define MDSCR_SS	(1 << 0)
+
+#define DBGBCR_LEN8	(0xff << 5)
+#define DBGBCR_EXEC	(0x0 << 3)
+#define DBGBCR_EL1	(0x1 << 1)
+#define DBGBCR_E	(0x1 << 0)
+
+#define DBGWCR_LEN8	(0xff << 5)
+#define DBGWCR_RD	(0x1 << 3)
+#define DBGWCR_WR	(0x2 << 3)
+#define DBGWCR_EL1	(0x1 << 1)
+#define DBGWCR_E	(0x1 << 0)
+
 #endif /* SELFTEST_KVM_PROCESSOR_H */
-- 
2.31.1.498.g6c1eba8ee3d-goog

Re: [PATCH 2/3] KVM: selftests: Add aarch64/debug-exceptions test

[Andrew Jones]

On Thu, Apr 22, 2021 at 09:03:50PM -0700, Ricardo Koller wrote:
> Covers fundamental tests for debug exceptions. The guest installs and
> handle its debug exceptions itself, without KVM_SET_GUEST_DEBUG.
> 
> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> ---
>  tools/testing/selftests/kvm/.gitignore        |   1 +
>  tools/testing/selftests/kvm/Makefile          |   1 +
>  .../selftests/kvm/aarch64/debug-exceptions.c  | 250 ++++++++++++++++++
>  .../selftests/kvm/include/aarch64/processor.h |  17 ++
>  4 files changed, 269 insertions(+)
>  create mode 100644 tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> 
> diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
> index e65d5572aefc..f09ed908422b 100644
> --- a/tools/testing/selftests/kvm/.gitignore
> +++ b/tools/testing/selftests/kvm/.gitignore
> @@ -1,4 +1,5 @@
>  # SPDX-License-Identifier: GPL-2.0-only
> +/aarch64/debug-exceptions
>  /aarch64/get-reg-list
>  /aarch64/get-reg-list-sve
>  /aarch64/vgic_init
> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> index 618c5903f478..2f92442c0cc9 100644
> --- a/tools/testing/selftests/kvm/Makefile
> +++ b/tools/testing/selftests/kvm/Makefile
> @@ -73,6 +73,7 @@ TEST_GEN_PROGS_x86_64 += memslot_modification_stress_test
>  TEST_GEN_PROGS_x86_64 += set_memory_region_test
>  TEST_GEN_PROGS_x86_64 += steal_time
>  
> +TEST_GEN_PROGS_aarch64 += aarch64/debug-exceptions
>  TEST_GEN_PROGS_aarch64 += aarch64/get-reg-list
>  TEST_GEN_PROGS_aarch64 += aarch64/get-reg-list-sve
>  TEST_GEN_PROGS_aarch64 += aarch64/vgic_init
> diff --git a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> new file mode 100644
> index 000000000000..18e8de2711d3
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> @@ -0,0 +1,250 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#define _GNU_SOURCE /* for program_invocation_short_name */

There's no reference to program_invocation_short_name below.

> +#include <fcntl.h>
> +#include <pthread.h>
> +#include <sched.h>
> +#include <semaphore.h>
> +#include <signal.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/ioctl.h>
> +#include <sys/mman.h>
> +#include <asm/barrier.h>
> +
> +#include <linux/compiler.h>
> +
> +#include <test_util.h>
> +#include <kvm_util.h>
> +#include <processor.h>

This is a boatload of includes, but it looks to me like all we need are
the bottom three. Also, our pattern is to use "" for these local to kvm
selftests includes.

> +
> +#define VCPU_ID 0
> +
> +extern unsigned char sw_bp, hw_bp, bp_svc, bp_brk, hw_wp, ss_start;
> +static volatile uint64_t sw_bp_addr, hw_bp_addr;
> +static volatile uint64_t wp_addr, wp_data_addr;
> +static volatile uint64_t svc_addr;
> +static volatile uint64_t ss_addr[4], ss_idx;
> +#define  CAST_TO_PC(v)  ((uint64_t)&(v))

The 'CAST_TO_' prefix isn't necessary, please just use PC()

> +
> +static void reset_debug_state(void)
> +{
> +	asm volatile("msr daifset, #8");
> +
> +	write_sysreg(osdlr_el1, 0);
> +	write_sysreg(oslar_el1, 0);
> +	asm volatile("isb" : : : "memory");
> +
> +	write_sysreg(mdscr_el1, 0);
> +	/* This test only uses the first bp and wp slot. */
> +	write_sysreg(dbgbvr0_el1, 0);
> +	write_sysreg(dbgbcr0_el1, 0);
> +	write_sysreg(dbgwcr0_el1, 0);
> +	write_sysreg(dbgwvr0_el1, 0);
> +	asm volatile("isb" : : : "memory");
> +}
> +
> +static void install_wp(uint64_t addr)
> +{
> +	uint32_t wcr;
> +	uint32_t mdscr;
> +
> +	wcr = DBGWCR_LEN8 | DBGWCR_RD | DBGWCR_WR | DBGWCR_EL1 | DBGWCR_E;
> +	write_sysreg(dbgwcr0_el1, wcr);
> +	write_sysreg(dbgwvr0_el1, addr);
> +	asm volatile("isb" : : : "memory");
> +
> +	asm volatile("msr daifclr, #8");
> +
> +	mdscr = read_sysreg(mdscr_el1) | MDSCR_KDE | MDSCR_MDE;
> +	write_sysreg(mdscr_el1, mdscr);
> +}
> +
> +static void install_hw_bp(uint64_t addr)
> +{
> +	uint32_t bcr;
> +	uint32_t mdscr;
> +
> +	bcr = DBGBCR_LEN8 | DBGBCR_EXEC | DBGBCR_EL1 | DBGBCR_E;
> +	write_sysreg(dbgbcr0_el1, bcr);
> +	write_sysreg(dbgbvr0_el1, addr);
> +	asm volatile("isb" : : : "memory");
> +
> +	asm volatile("msr daifclr, #8");
> +
> +	mdscr = read_sysreg(mdscr_el1) | MDSCR_KDE | MDSCR_MDE;
> +	write_sysreg(mdscr_el1, mdscr);
> +}
> +
> +static void install_ss(void)
> +{
> +	uint32_t mdscr;
> +
> +	asm volatile("msr daifclr, #8");
> +
> +	mdscr = read_sysreg(mdscr_el1) | MDSCR_KDE | MDSCR_SS;
> +	write_sysreg(mdscr_el1, mdscr);
> +}
> +
> +static volatile char write_data;
> +
> +#define GUEST_ASSERT_EQ(arg1, arg2) \
> +	GUEST_ASSERT_2((arg1) == (arg2), (arg1), (arg2))

Please add this to kvm_util.h.

> +
> +static void guest_code(void)
> +{
> +	GUEST_SYNC(0);
> +
> +	/* Software-breakpoint */
> +	asm volatile("sw_bp: brk #0");
> +	GUEST_ASSERT_EQ(sw_bp_addr, CAST_TO_PC(sw_bp));
> +
> +	GUEST_SYNC(1);
> +
> +	/* Hardware-breakpoint */
> +	reset_debug_state();
> +	install_hw_bp(CAST_TO_PC(hw_bp));
> +	asm volatile("hw_bp: nop");
> +	GUEST_ASSERT_EQ(hw_bp_addr, CAST_TO_PC(hw_bp));
> +
> +	GUEST_SYNC(2);
> +
> +	/* Hardware-breakpoint + svc */
> +	reset_debug_state();
> +	install_hw_bp(CAST_TO_PC(bp_svc));
> +	asm volatile("bp_svc: svc #0");
> +	GUEST_ASSERT_EQ(hw_bp_addr, CAST_TO_PC(bp_svc));
> +	GUEST_ASSERT_EQ(svc_addr, CAST_TO_PC(bp_svc) + 4);
> +
> +	GUEST_SYNC(3);
> +
> +	/* Hardware-breakpoint + software-breakpoint */
> +	reset_debug_state();
> +	install_hw_bp(CAST_TO_PC(bp_brk));
> +	asm volatile("bp_brk: brk #0");
> +	GUEST_ASSERT_EQ(sw_bp_addr, CAST_TO_PC(bp_brk));
> +	GUEST_ASSERT_EQ(hw_bp_addr, CAST_TO_PC(bp_brk));
> +
> +	GUEST_SYNC(4);
> +
> +	/* Watchpoint */
> +	reset_debug_state();
> +	install_wp(CAST_TO_PC(write_data));
> +	write_data = 'x';
> +	GUEST_ASSERT_EQ(write_data, 'x');
> +	GUEST_ASSERT_EQ(wp_data_addr, CAST_TO_PC(write_data));
> +
> +	GUEST_SYNC(5);
> +
> +	/* Single-step */
> +	reset_debug_state();
> +	install_ss();
> +	ss_idx = 0;
> +	asm volatile("ss_start:\n"
> +		     "mrs x0, esr_el1\n"
> +		     "add x0, x0, #1\n"
> +		     "msr daifset, #8\n"
> +		     : : : "x0");
> +	GUEST_ASSERT_EQ(ss_addr[0], CAST_TO_PC(ss_start));
> +	GUEST_ASSERT_EQ(ss_addr[1], CAST_TO_PC(ss_start) + 4);
> +	GUEST_ASSERT_EQ(ss_addr[2], CAST_TO_PC(ss_start) + 8);
> +
> +	GUEST_DONE();
> +}
> +
> +static void guest_sw_bp_handler(struct ex_regs *regs)
> +{
> +	sw_bp_addr = regs->pc;
> +	regs->pc += 4;
> +}
> +
> +static void guest_hw_bp_handler(struct ex_regs *regs)
> +{
> +	hw_bp_addr = regs->pc;
> +	regs->pstate |= SPSR_D;
> +}
> +
> +static void guest_wp_handler(struct ex_regs *regs)
> +{
> +	wp_data_addr = read_sysreg(far_el1);
> +	wp_addr = regs->pc;
> +	regs->pstate |= SPSR_D;
> +}
> +
> +static void guest_ss_handler(struct ex_regs *regs)
> +{
> +	GUEST_ASSERT_1(ss_idx < 4, ss_idx);
> +	ss_addr[ss_idx++] = regs->pc;
> +	regs->pstate |= SPSR_SS;
> +}
> +
> +static void guest_svc_handler(struct ex_regs *regs)
> +{
> +	svc_addr = regs->pc;
> +}
> +
> +static int debug_version(struct kvm_vm *vm)
> +{
> +	uint64_t id_aa64dfr0;
> +
> +	get_reg(vm, VCPU_ID, ARM64_SYS_REG(ID_AA64DFR0_EL1), &id_aa64dfr0);
> +	return id_aa64dfr0 & 0xf;
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +	struct kvm_vm *vm;
> +	struct ucall uc;
> +	int stage;
> +	int ret;
> +
> +	vm = vm_create_default(VCPU_ID, 0, guest_code);
> +	ucall_init(vm, NULL);
> +
> +	vm_init_descriptor_tables(vm);
> +	vcpu_init_descriptor_tables(vm, VCPU_ID);
> +
> +	if (debug_version(vm) < 6) {
> +		print_skip("Armv8 debug architecture not supported.");
> +		kvm_vm_free(vm);
> +		exit(KSFT_SKIP);
> +	}
> +
> +	vm_handle_exception(vm, VECTOR_SYNC_EL1,
> +			ESR_EC_BRK_INS, guest_sw_bp_handler);
> +	vm_handle_exception(vm, VECTOR_SYNC_EL1,
> +			ESR_EC_HW_BP_EL1, guest_hw_bp_handler);
> +	vm_handle_exception(vm, VECTOR_SYNC_EL1,
> +			ESR_EC_WP_EL1, guest_wp_handler);
> +	vm_handle_exception(vm, VECTOR_SYNC_EL1,
> +			ESR_EC_SSTEP_EL1, guest_ss_handler);
> +	vm_handle_exception(vm, VECTOR_SYNC_EL1,
> +			ESR_EC_SVC64, guest_svc_handler);
> +
> +	for (stage = 0; stage < 7; stage++) {
> +		ret = _vcpu_run(vm, VCPU_ID);
> +
> +		TEST_ASSERT(ret == 0, "vcpu_run failed: %d\n", ret);

Instead of using the _ variant of vcpu_run and this assert on the ret==0,
you can just use the non _ variant (vcpu_run), which does the same thing.


> +		switch (get_ucall(vm, VCPU_ID, &uc)) {
> +		case UCALL_SYNC:
> +			TEST_ASSERT(uc.args[1] == stage,
> +				"Stage %d: Unexpected sync ucall, got %lx",
> +				stage, (ulong)uc.args[1]);
> +

Unnecessary blank line

> +			break;
> +		case UCALL_ABORT:
> +			TEST_FAIL("%s at %s:%ld\n\tvalues: %#lx, %#lx",
> +				(const char *)uc.args[0],
> +				__FILE__, uc.args[1], uc.args[2], uc.args[3]);
> +			break;
> +		case UCALL_DONE:
> +			goto done;
> +		default:
> +			TEST_FAIL("Unknown ucall %lu", uc.cmd);
> +		}
> +	}
> +
> +done:
> +	kvm_vm_free(vm);
> +	return 0;
> +}
> diff --git a/tools/testing/selftests/kvm/include/aarch64/processor.h b/tools/testing/selftests/kvm/include/aarch64/processor.h
> index 5c902ad95c35..eee69b92e01e 100644
> --- a/tools/testing/selftests/kvm/include/aarch64/processor.h
> +++ b/tools/testing/selftests/kvm/include/aarch64/processor.h
> @@ -21,6 +21,8 @@
>  #define SCTLR_EL1	3, 0,  1, 0, 0
>  #define VBAR_EL1	3, 0, 12, 0, 0
>  
> +#define ID_AA64DFR0_EL1	3, 0,  0, 5, 0

It'd be nice to add a couple more tabs to the register defines above to
get them aligned with this new one.

> +
>  /*
>   * Default MAIR
>   *                  index   attribute
> @@ -125,4 +127,19 @@ void vm_handle_exception(struct kvm_vm *vm, int vector, int ec,
>  	val;								  \
>  })
>  
> +#define MDSCR_KDE	(1 << 13)
> +#define MDSCR_MDE	(1 << 15)
> +#define MDSCR_SS	(1 << 0)
> +
> +#define DBGBCR_LEN8	(0xff << 5)
> +#define DBGBCR_EXEC	(0x0 << 3)
> +#define DBGBCR_EL1	(0x1 << 1)
> +#define DBGBCR_E	(0x1 << 0)
> +
> +#define DBGWCR_LEN8	(0xff << 5)
> +#define DBGWCR_RD	(0x1 << 3)
> +#define DBGWCR_WR	(0x2 << 3)
> +#define DBGWCR_EL1	(0x1 << 1)
> +#define DBGWCR_E	(0x1 << 0)

I think these debug specific defines should be put directly in the debug
test.

> +
>  #endif /* SELFTEST_KVM_PROCESSOR_H */
> -- 
> 2.31.1.498.g6c1eba8ee3d-goog
>

Thanks,
drew 

[PATCH 3/3] KVM: selftests: Use a ucall for x86 unhandled vector reporting

[Ricardo Koller]

x86 reports unhandled vectors using port IO at a specific port number,
which is replicating what ucall already does for x86.  Aarch64, on the
other hand, reports unhandled vector exceptions with a ucall using a
recently added UCALL_UNHANDLED ucall type.

Replace the x86 unhandled vector exception handling to use ucall
UCALL_UNHANDLED instead of port IO.

Tested: Forcing a page fault in the ./x86_64/xapic_ipi_test
	halter_guest_code() shows this:

	$ ./x86_64/xapic_ipi_test
	...
	  Unexpected vectored event in guest (vector:0xe)

Signed-off-by: Ricardo Koller <ricarkol@google.com>
---
 .../selftests/kvm/include/x86_64/processor.h      |  2 --
 .../testing/selftests/kvm/lib/x86_64/processor.c  | 15 ++++++---------
 2 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
index 0b30b4e15c38..379f12cbdc06 100644
--- a/tools/testing/selftests/kvm/include/x86_64/processor.h
+++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
@@ -53,8 +53,6 @@
 #define CPUID_PKU		(1ul << 3)
 #define CPUID_LA57		(1ul << 16)
 
-#define UNEXPECTED_VECTOR_PORT 0xfff0u
-
 /* General Registers in 64-Bit Mode */
 struct gpr64_regs {
 	u64 rax;
diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
index a8906e60a108..284d26a25cd3 100644
--- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
+++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
@@ -1207,7 +1207,7 @@ static void set_idt_entry(struct kvm_vm *vm, int vector, unsigned long addr,
 
 void kvm_exit_unexpected_vector(uint32_t value)
 {
-	outl(UNEXPECTED_VECTOR_PORT, value);
+	ucall(UCALL_UNHANDLED, 1, value);
 }
 
 void route_exception(struct ex_regs *regs)
@@ -1260,16 +1260,13 @@ void vm_handle_exception(struct kvm_vm *vm, int vector,
 
 void assert_on_unhandled_exception(struct kvm_vm *vm, uint32_t vcpuid)
 {
-	if (vcpu_state(vm, vcpuid)->exit_reason == KVM_EXIT_IO
-		&& vcpu_state(vm, vcpuid)->io.port == UNEXPECTED_VECTOR_PORT
-		&& vcpu_state(vm, vcpuid)->io.size == 4) {
-		/* Grab pointer to io data */
-		uint32_t *data = (void *)vcpu_state(vm, vcpuid)
-			+ vcpu_state(vm, vcpuid)->io.data_offset;
+	struct ucall uc;
 
+	if (get_ucall(vm, vcpuid, &uc) == UCALL_UNHANDLED) {
+		uint64_t vector = uc.args[0];
 		TEST_ASSERT(false,
-			    "Unexpected vectored event in guest (vector:0x%x)",
-			    *data);
+			    "Unexpected vectored event in guest (vector:0x%lx)",
+			    vector);
 	}
 }
 
-- 
2.31.1.498.g6c1eba8ee3d-goog

Re: [PATCH 3/3] KVM: selftests: Use a ucall for x86 unhandled vector reporting

[Andrew Jones]

Hi Ricardo,

It may be nicer to introduce UCALL_UNHANDLED with this patch and have it
come fist in the series.

Thanks,
drew


On Thu, Apr 22, 2021 at 09:03:51PM -0700, Ricardo Koller wrote:
> x86 reports unhandled vectors using port IO at a specific port number,
> which is replicating what ucall already does for x86.  Aarch64, on the
> other hand, reports unhandled vector exceptions with a ucall using a
> recently added UCALL_UNHANDLED ucall type.
> 
> Replace the x86 unhandled vector exception handling to use ucall
> UCALL_UNHANDLED instead of port IO.
> 
> Tested: Forcing a page fault in the ./x86_64/xapic_ipi_test
> 	halter_guest_code() shows this:
> 
> 	$ ./x86_64/xapic_ipi_test
> 	...
> 	  Unexpected vectored event in guest (vector:0xe)
> 
> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> ---
>  .../selftests/kvm/include/x86_64/processor.h      |  2 --
>  .../testing/selftests/kvm/lib/x86_64/processor.c  | 15 ++++++---------
>  2 files changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
> index 0b30b4e15c38..379f12cbdc06 100644
> --- a/tools/testing/selftests/kvm/include/x86_64/processor.h
> +++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
> @@ -53,8 +53,6 @@
>  #define CPUID_PKU		(1ul << 3)
>  #define CPUID_LA57		(1ul << 16)
>  
> -#define UNEXPECTED_VECTOR_PORT 0xfff0u
> -
>  /* General Registers in 64-Bit Mode */
>  struct gpr64_regs {
>  	u64 rax;
> diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> index a8906e60a108..284d26a25cd3 100644
> --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
> +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> @@ -1207,7 +1207,7 @@ static void set_idt_entry(struct kvm_vm *vm, int vector, unsigned long addr,
>  
>  void kvm_exit_unexpected_vector(uint32_t value)
>  {
> -	outl(UNEXPECTED_VECTOR_PORT, value);
> +	ucall(UCALL_UNHANDLED, 1, value);
>  }
>  
>  void route_exception(struct ex_regs *regs)
> @@ -1260,16 +1260,13 @@ void vm_handle_exception(struct kvm_vm *vm, int vector,
>  
>  void assert_on_unhandled_exception(struct kvm_vm *vm, uint32_t vcpuid)
>  {
> -	if (vcpu_state(vm, vcpuid)->exit_reason == KVM_EXIT_IO
> -		&& vcpu_state(vm, vcpuid)->io.port == UNEXPECTED_VECTOR_PORT
> -		&& vcpu_state(vm, vcpuid)->io.size == 4) {
> -		/* Grab pointer to io data */
> -		uint32_t *data = (void *)vcpu_state(vm, vcpuid)
> -			+ vcpu_state(vm, vcpuid)->io.data_offset;
> +	struct ucall uc;
>  
> +	if (get_ucall(vm, vcpuid, &uc) == UCALL_UNHANDLED) {
> +		uint64_t vector = uc.args[0];
>  		TEST_ASSERT(false,
> -			    "Unexpected vectored event in guest (vector:0x%x)",
> -			    *data);
> +			    "Unexpected vectored event in guest (vector:0x%lx)",
> +			    vector);
>  	}
>  }
>  
> -- 
> 2.31.1.498.g6c1eba8ee3d-goog
>