[PATCH 0/9] KVM: arm64: selftests: Test linked {break,watch}points

[PATCH 0/9] KVM: arm64: selftests: Test linked {break,watch}points

[Reiji Watanabe]

This series adds test cases for linked {break,watch}points to the
debug-exceptions test, and expands {break,watch}point tests to
use non-zero {break,watch}points (the current test always uses
{break,watch}point#0).

Patches 1-6 add some helpers or do minor refactoring for
preparation of adding test cases in subsequent patches.
Patches 7-8 add test cases for a linked {break,watch}point.
Patch 9 expands {break,watch}point test cases to use non-zero
{break,watch}points.

Reiji Watanabe (9):
  KVM: arm64: selftests: Add helpers to extract a field of an ID
    register
  KVM: arm64: selftests: Add write_dbg{b,w}{c,v}r helpers in
    debug-exceptions
  KVM: arm64: selftests: Remove the hard-coded {b,w}pn#0 from
    debug-exceptions
  KVM: arm64: selftests: Add helpers to enable debug exceptions
  KVM: arm64: selftests: Have debug_version() use cpuid_get_ufield()
    helper
  KVM: arm64: selftests: Change debug_version() to take ID_AA64DFR0_EL1
  KVM: arm64: selftests: Add a test case for a linked breakpoint
  KVM: arm64: selftests: Add a test case for a linked watchpoint
  KVM: arm64: selftests: Test with every breakpoint/watchpoint

 .../selftests/kvm/aarch64/debug-exceptions.c  | 281 +++++++++++++++---
 .../selftests/kvm/include/aarch64/processor.h |   2 +
 .../selftests/kvm/lib/aarch64/processor.c     |  15 +
 3 files changed, 262 insertions(+), 36 deletions(-)


base-commit: 1c23f9e627a7b412978b4e852793c5e3c3efc555
-- 
2.37.1.595.g718a3a8f04-goog

[PATCH 1/9] KVM: arm64: selftests: Add helpers to extract a field of an ID register

[Reiji Watanabe]

Introduce helpers to extract a field of an ID register.
Subsequent patches will use those helpers.

Signed-off-by: Reiji Watanabe <reijiw@google.com>
---
 .../selftests/kvm/include/aarch64/processor.h     |  2 ++
 .../testing/selftests/kvm/lib/aarch64/processor.c | 15 +++++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/tools/testing/selftests/kvm/include/aarch64/processor.h b/tools/testing/selftests/kvm/include/aarch64/processor.h
index a8124f9dd68a..a9b4b4e0e592 100644
--- a/tools/testing/selftests/kvm/include/aarch64/processor.h
+++ b/tools/testing/selftests/kvm/include/aarch64/processor.h
@@ -193,4 +193,6 @@ void smccc_hvc(uint32_t function_id, uint64_t arg0, uint64_t arg1,
 
 uint32_t guest_get_vcpuid(void);
 
+int cpuid_get_sfield(uint64_t val, int field_shift);
+unsigned int cpuid_get_ufield(uint64_t val, int field_shift);
 #endif /* SELFTEST_KVM_PROCESSOR_H */
diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c
index 6f5551368944..0b2ad46e7ff5 100644
--- a/tools/testing/selftests/kvm/lib/aarch64/processor.c
+++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c
@@ -528,3 +528,18 @@ void smccc_hvc(uint32_t function_id, uint64_t arg0, uint64_t arg1,
 		       [arg4] "r"(arg4), [arg5] "r"(arg5), [arg6] "r"(arg6)
 		     : "x0", "x1", "x2", "x3", "x4", "x5", "x6", "x7");
 }
+
+/* Helpers to get a signed/unsigned feature field from ID register value */
+int cpuid_get_sfield(uint64_t val, int field_shift)
+{
+	int width = 4;
+
+	return (int64_t)(val << (64 - width - field_shift)) >> (64 - width);
+}
+
+unsigned int cpuid_get_ufield(uint64_t val, int field_shift)
+{
+	int width = 4;
+
+	return (uint64_t)(val << (64 - width - field_shift)) >> (64 - width);
+}
-- 
2.37.1.595.g718a3a8f04-goog

[PATCH 2/9] KVM: arm64: selftests: Add write_dbg{b,w}{c,v}r helpers in debug-exceptions

[Reiji Watanabe]

Introduce helpers in the debug-exceptions test to write to
dbg{b,w}{c,v}r registers. Those helpers will be useful for
test cases that will be added to the test in subsequent patches.

Signed-off-by: Reiji Watanabe <reijiw@google.com>
---
 .../selftests/kvm/aarch64/debug-exceptions.c  | 72 +++++++++++++++++--
 1 file changed, 68 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
index 2ee35cf9801e..51047e6b8db3 100644
--- a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
+++ b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
@@ -28,6 +28,69 @@ static volatile uint64_t svc_addr;
 static volatile uint64_t ss_addr[4], ss_idx;
 #define  PC(v)  ((uint64_t)&(v))
 
+#define GEN_DEBUG_WRITE_REG(reg_name)			\
+static void write_##reg_name(int num, uint64_t val)	\
+{							\
+	switch (num) {					\
+	case 0:						\
+		write_sysreg(val, reg_name##0_el1);	\
+		break;					\
+	case 1:						\
+		write_sysreg(val, reg_name##1_el1);	\
+		break;					\
+	case 2:						\
+		write_sysreg(val, reg_name##2_el1);	\
+		break;					\
+	case 3:						\
+		write_sysreg(val, reg_name##3_el1);	\
+		break;					\
+	case 4:						\
+		write_sysreg(val, reg_name##4_el1);	\
+		break;					\
+	case 5:						\
+		write_sysreg(val, reg_name##5_el1);	\
+		break;					\
+	case 6:						\
+		write_sysreg(val, reg_name##6_el1);	\
+		break;					\
+	case 7:						\
+		write_sysreg(val, reg_name##7_el1);	\
+		break;					\
+	case 8:						\
+		write_sysreg(val, reg_name##8_el1);	\
+		break;					\
+	case 9:						\
+		write_sysreg(val, reg_name##9_el1);	\
+		break;					\
+	case 10:					\
+		write_sysreg(val, reg_name##10_el1);	\
+		break;					\
+	case 11:					\
+		write_sysreg(val, reg_name##11_el1);	\
+		break;					\
+	case 12:					\
+		write_sysreg(val, reg_name##12_el1);	\
+		break;					\
+	case 13:					\
+		write_sysreg(val, reg_name##13_el1);	\
+		break;					\
+	case 14:					\
+		write_sysreg(val, reg_name##14_el1);	\
+		break;					\
+	case 15:					\
+		write_sysreg(val, reg_name##15_el1);	\
+		break;					\
+	default:					\
+		GUEST_ASSERT(0);			\
+	}						\
+}
+
+/* Define write_dbgbcr()/write_dbgbvr()/write_dbgwcr()/write_dbgwvr() */
+GEN_DEBUG_WRITE_REG(dbgbcr)
+GEN_DEBUG_WRITE_REG(dbgbvr)
+GEN_DEBUG_WRITE_REG(dbgwcr)
+GEN_DEBUG_WRITE_REG(dbgwvr)
+
 static void reset_debug_state(void)
 {
 	asm volatile("msr daifset, #8");
@@ -59,8 +122,9 @@ static void install_wp(uint64_t addr)
 	uint32_t mdscr;
 
 	wcr = DBGWCR_LEN8 | DBGWCR_RD | DBGWCR_WR | DBGWCR_EL1 | DBGWCR_E;
-	write_sysreg(wcr, dbgwcr0_el1);
-	write_sysreg(addr, dbgwvr0_el1);
+	write_dbgwcr(0, wcr);
+	write_dbgwvr(0, addr);
+
 	isb();
 
 	asm volatile("msr daifclr, #8");
@@ -76,8 +140,8 @@ static void install_hw_bp(uint64_t addr)
 	uint32_t mdscr;
 
 	bcr = DBGBCR_LEN8 | DBGBCR_EXEC | DBGBCR_EL1 | DBGBCR_E;
-	write_sysreg(bcr, dbgbcr0_el1);
-	write_sysreg(addr, dbgbvr0_el1);
+	write_dbgbcr(0, bcr);
+	write_dbgbvr(0, addr);
 	isb();
 
 	asm volatile("msr daifclr, #8");
-- 
2.37.1.595.g718a3a8f04-goog

[PATCH 3/9] KVM: arm64: selftests: Remove the hard-coded {b,w}pn#0 from debug-exceptions

[Reiji Watanabe]

Remove the hard-coded {break,watch}point #0 from the guest_code()
in debug-exceptions to allow {break,watch}point number to be
specified.  Subsequent patches will add test cases for non-zero
{break,watch}points.

Signed-off-by: Reiji Watanabe <reijiw@google.com>
---
 .../selftests/kvm/aarch64/debug-exceptions.c  | 50 ++++++++++++-------
 1 file changed, 32 insertions(+), 18 deletions(-)

diff --git a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
index 51047e6b8db3..183ee16acb7d 100644
--- a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
+++ b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
@@ -93,6 +93,9 @@ GEN_DEBUG_WRITE_REG(dbgwvr)
 
 static void reset_debug_state(void)
 {
+	uint8_t brps, wrps, i;
+	uint64_t dfr0;
+
 	asm volatile("msr daifset, #8");
 
 	write_sysreg(0, osdlr_el1);
@@ -100,11 +103,20 @@ static void reset_debug_state(void)
 	isb();
 
 	write_sysreg(0, mdscr_el1);
-	/* This test only uses the first bp and wp slot. */
-	write_sysreg(0, dbgbvr0_el1);
-	write_sysreg(0, dbgbcr0_el1);
-	write_sysreg(0, dbgwcr0_el1);
-	write_sysreg(0, dbgwvr0_el1);
+
+	/* Reset all bcr/bvr/wcr/wvr registers */
+	dfr0 = read_sysreg(id_aa64dfr0_el1);
+	brps = cpuid_get_ufield(dfr0, ID_AA64DFR0_BRPS_SHIFT);
+	for (i = 0; i <= brps; i++) {
+		write_dbgbcr(i, 0);
+		write_dbgbvr(i, 0);
+	}
+	wrps = cpuid_get_ufield(dfr0, ID_AA64DFR0_WRPS_SHIFT);
+	for (i = 0; i <= wrps; i++) {
+		write_dbgwcr(i, 0);
+		write_dbgwvr(i, 0);
+	}
+
 	isb();
 }
 
@@ -116,14 +128,14 @@ static void enable_os_lock(void)
 	GUEST_ASSERT(read_sysreg(oslsr_el1) & 2);
 }
 
-static void install_wp(uint64_t addr)
+static void install_wp(uint8_t wpn, uint64_t addr)
 {
 	uint32_t wcr;
 	uint32_t mdscr;
 
 	wcr = DBGWCR_LEN8 | DBGWCR_RD | DBGWCR_WR | DBGWCR_EL1 | DBGWCR_E;
-	write_dbgwcr(0, wcr);
-	write_dbgwvr(0, addr);
+	write_dbgwcr(wpn, wcr);
+	write_dbgwvr(wpn, addr);
 
 	isb();
 
@@ -134,14 +146,14 @@ static void install_wp(uint64_t addr)
 	isb();
 }
 
-static void install_hw_bp(uint64_t addr)
+static void install_hw_bp(uint8_t bpn, uint64_t addr)
 {
 	uint32_t bcr;
 	uint32_t mdscr;
 
 	bcr = DBGBCR_LEN8 | DBGBCR_EXEC | DBGBCR_EL1 | DBGBCR_E;
-	write_dbgbcr(0, bcr);
-	write_dbgbvr(0, addr);
+	write_dbgbcr(bpn, bcr);
+	write_dbgbvr(bpn, addr);
 	isb();
 
 	asm volatile("msr daifclr, #8");
@@ -164,7 +176,7 @@ static void install_ss(void)
 
 static volatile char write_data;
 
-static void guest_code(void)
+static void guest_code(uint8_t bpn, uint8_t wpn)
 {
 	GUEST_SYNC(0);
 
@@ -177,7 +189,7 @@ static void guest_code(void)
 
 	/* Hardware-breakpoint */
 	reset_debug_state();
-	install_hw_bp(PC(hw_bp));
+	install_hw_bp(bpn, PC(hw_bp));
 	asm volatile("hw_bp: nop");
 	GUEST_ASSERT_EQ(hw_bp_addr, PC(hw_bp));
 
@@ -185,7 +197,7 @@ static void guest_code(void)
 
 	/* Hardware-breakpoint + svc */
 	reset_debug_state();
-	install_hw_bp(PC(bp_svc));
+	install_hw_bp(bpn, PC(bp_svc));
 	asm volatile("bp_svc: svc #0");
 	GUEST_ASSERT_EQ(hw_bp_addr, PC(bp_svc));
 	GUEST_ASSERT_EQ(svc_addr, PC(bp_svc) + 4);
@@ -194,7 +206,7 @@ static void guest_code(void)
 
 	/* Hardware-breakpoint + software-breakpoint */
 	reset_debug_state();
-	install_hw_bp(PC(bp_brk));
+	install_hw_bp(bpn, PC(bp_brk));
 	asm volatile("bp_brk: brk #0");
 	GUEST_ASSERT_EQ(sw_bp_addr, PC(bp_brk));
 	GUEST_ASSERT_EQ(hw_bp_addr, PC(bp_brk));
@@ -203,7 +215,7 @@ static void guest_code(void)
 
 	/* Watchpoint */
 	reset_debug_state();
-	install_wp(PC(write_data));
+	install_wp(wpn, PC(write_data));
 	write_data = 'x';
 	GUEST_ASSERT_EQ(write_data, 'x');
 	GUEST_ASSERT_EQ(wp_data_addr, PC(write_data));
@@ -237,7 +249,7 @@ static void guest_code(void)
 	/* OS Lock blocking hardware-breakpoint */
 	reset_debug_state();
 	enable_os_lock();
-	install_hw_bp(PC(hw_bp2));
+	install_hw_bp(bpn, PC(hw_bp2));
 	hw_bp_addr = 0;
 	asm volatile("hw_bp2: nop");
 	GUEST_ASSERT_EQ(hw_bp_addr, 0);
@@ -249,7 +261,7 @@ static void guest_code(void)
 	enable_os_lock();
 	write_data = '\0';
 	wp_data_addr = 0;
-	install_wp(PC(write_data));
+	install_wp(wpn, PC(write_data));
 	write_data = 'x';
 	GUEST_ASSERT_EQ(write_data, 'x');
 	GUEST_ASSERT_EQ(wp_data_addr, 0);
@@ -337,6 +349,8 @@ int main(int argc, char *argv[])
 	vm_install_sync_handler(vm, VECTOR_SYNC_CURRENT,
 				ESR_EC_SVC64, guest_svc_handler);
 
+	/* Run tests with breakpoint#0 and watchpoint#0. */
+	vcpu_args_set(vcpu, 2, 0, 0);
 	for (stage = 0; stage < 11; stage++) {
 		vcpu_run(vcpu);
 
-- 
2.37.1.595.g718a3a8f04-goog

[PATCH 4/9] KVM: arm64: selftests: Add helpers to enable debug exceptions

[Reiji Watanabe]

Add helpers to enable breakpoint and watchpoint exceptions.

Signed-off-by: Reiji Watanabe <reijiw@google.com>
---
 .../selftests/kvm/aarch64/debug-exceptions.c  | 25 ++++++++++---------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
index 183ee16acb7d..713c7240b680 100644
--- a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
+++ b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
@@ -128,10 +128,20 @@ static void enable_os_lock(void)
 	GUEST_ASSERT(read_sysreg(oslsr_el1) & 2);
 }
 
+static void enable_debug_bwp_exception(void)
+{
+	uint32_t mdscr;
+
+	asm volatile("msr daifclr, #8");
+
+	mdscr = read_sysreg(mdscr_el1) | MDSCR_KDE | MDSCR_MDE;
+	write_sysreg(mdscr, mdscr_el1);
+	isb();
+}
+
 static void install_wp(uint8_t wpn, uint64_t addr)
 {
 	uint32_t wcr;
-	uint32_t mdscr;
 
 	wcr = DBGWCR_LEN8 | DBGWCR_RD | DBGWCR_WR | DBGWCR_EL1 | DBGWCR_E;
 	write_dbgwcr(wpn, wcr);
@@ -139,28 +149,19 @@ static void install_wp(uint8_t wpn, uint64_t addr)
 
 	isb();
 
-	asm volatile("msr daifclr, #8");
-
-	mdscr = read_sysreg(mdscr_el1) | MDSCR_KDE | MDSCR_MDE;
-	write_sysreg(mdscr, mdscr_el1);
-	isb();
+	enable_debug_bwp_exception();
 }
 
 static void install_hw_bp(uint8_t bpn, uint64_t addr)
 {
 	uint32_t bcr;
-	uint32_t mdscr;
 
 	bcr = DBGBCR_LEN8 | DBGBCR_EXEC | DBGBCR_EL1 | DBGBCR_E;
 	write_dbgbcr(bpn, bcr);
 	write_dbgbvr(bpn, addr);
 	isb();
 
-	asm volatile("msr daifclr, #8");
-
-	mdscr = read_sysreg(mdscr_el1) | MDSCR_KDE | MDSCR_MDE;
-	write_sysreg(mdscr, mdscr_el1);
-	isb();
+	enable_debug_bwp_exception();
 }
 
 static void install_ss(void)
-- 
2.37.1.595.g718a3a8f04-goog

[PATCH 5/9] KVM: arm64: selftests: Have debug_version() use cpuid_get_ufield() helper

[Reiji Watanabe]

Change debug_version() to use cpuid_get_ufield() to extract DebugVer
field from the AA64DFR0_EL1 register value.

Signed-off-by: Reiji Watanabe <reijiw@google.com>
---
 tools/testing/selftests/kvm/aarch64/debug-exceptions.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
index 713c7240b680..17b17359ac41 100644
--- a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
+++ b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
@@ -320,7 +320,7 @@ static int debug_version(struct kvm_vcpu *vcpu)
 	uint64_t id_aa64dfr0;
 
 	vcpu_get_reg(vcpu, KVM_ARM64_SYS_REG(SYS_ID_AA64DFR0_EL1), &id_aa64dfr0);
-	return id_aa64dfr0 & 0xf;
+	return cpuid_get_ufield(id_aa64dfr0, ID_AA64DFR0_DEBUGVER_SHIFT);
 }
 
 int main(int argc, char *argv[])
-- 
2.37.1.595.g718a3a8f04-goog

[PATCH 6/9] KVM: arm64: selftests: Change debug_version() to take ID_AA64DFR0_EL1

[Reiji Watanabe]

Change debug_version() to take the ID_AA64DFR0_EL1 value instead of
vcpu as an argument, and change its callsite to read ID_AA64DFR0_EL1
(and pass it to debug_version()).
Subsequent patches will reuse the register value in the callsite.

Signed-off-by: Reiji Watanabe <reijiw@google.com>
---
 tools/testing/selftests/kvm/aarch64/debug-exceptions.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
index 17b17359ac41..ab8860e3a9fa 100644
--- a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
+++ b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
@@ -315,11 +315,8 @@ static void guest_svc_handler(struct ex_regs *regs)
 	svc_addr = regs->pc;
 }
 
-static int debug_version(struct kvm_vcpu *vcpu)
+static int debug_version(uint64_t id_aa64dfr0)
 {
-	uint64_t id_aa64dfr0;
-
-	vcpu_get_reg(vcpu, KVM_ARM64_SYS_REG(SYS_ID_AA64DFR0_EL1), &id_aa64dfr0);
 	return cpuid_get_ufield(id_aa64dfr0, ID_AA64DFR0_DEBUGVER_SHIFT);
 }
 
@@ -329,6 +326,7 @@ int main(int argc, char *argv[])
 	struct kvm_vm *vm;
 	struct ucall uc;
 	int stage;
+	uint64_t aa64dfr0;
 
 	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
 	ucall_init(vm, NULL);
@@ -336,7 +334,8 @@ int main(int argc, char *argv[])
 	vm_init_descriptor_tables(vm);
 	vcpu_init_descriptor_tables(vcpu);
 
-	__TEST_REQUIRE(debug_version(vcpu) >= 6,
+	vcpu_get_reg(vcpu, KVM_ARM64_SYS_REG(SYS_ID_AA64DFR0_EL1), &aa64dfr0);
+	__TEST_REQUIRE(debug_version(aa64dfr0) >= 6,
 		       "Armv8 debug architecture not supported.");
 
 	vm_install_sync_handler(vm, VECTOR_SYNC_CURRENT,
-- 
2.37.1.595.g718a3a8f04-goog

[PATCH 7/9] KVM: arm64: selftests: Add a test case for a linked breakpoint

[Reiji Watanabe]

Currently, the debug-exceptions test doesn't have a test case for
a linked breakpoint. Add a test case for the linked breakpoint to
the test.

Signed-off-by: Reiji Watanabe <reijiw@google.com>

---
 .../selftests/kvm/aarch64/debug-exceptions.c  | 59 +++++++++++++++++--
 1 file changed, 55 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
index ab8860e3a9fa..9fccfeebccd3 100644
--- a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
+++ b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
@@ -11,6 +11,10 @@
 #define DBGBCR_EXEC	(0x0 << 3)
 #define DBGBCR_EL1	(0x1 << 1)
 #define DBGBCR_E	(0x1 << 0)
+#define DBGBCR_LBN_SHIFT	16
+#define DBGBCR_BT_SHIFT		20
+#define DBGBCR_BT_ADDR_LINK_CTX	(0x1 << DBGBCR_BT_SHIFT)
+#define DBGBCR_BT_CTX_LINK	(0x3 << DBGBCR_BT_SHIFT)
 
 #define DBGWCR_LEN8	(0xff << 5)
 #define DBGWCR_RD	(0x1 << 3)
@@ -21,7 +25,7 @@
 #define SPSR_D		(1 << 9)
 #define SPSR_SS		(1 << 21)
 
-extern unsigned char sw_bp, sw_bp2, hw_bp, hw_bp2, bp_svc, bp_brk, hw_wp, ss_start;
+extern unsigned char sw_bp, sw_bp2, hw_bp, hw_bp2, bp_svc, bp_brk, hw_wp, ss_start, hw_bp_ctx;
 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;
@@ -103,6 +107,7 @@ static void reset_debug_state(void)
 	isb();
 
 	write_sysreg(0, mdscr_el1);
+	write_sysreg(0, contextidr_el1);
 
 	/* Reset all bcr/bvr/wcr/wvr registers */
 	dfr0 = read_sysreg(id_aa64dfr0_el1);
@@ -164,6 +169,28 @@ static void install_hw_bp(uint8_t bpn, uint64_t addr)
 	enable_debug_bwp_exception();
 }
 
+void install_hw_bp_ctx(uint8_t addr_bp, uint8_t ctx_bp, uint64_t addr,
+		       uint64_t ctx)
+{
+	uint32_t addr_bcr, ctx_bcr;
+
+	/* Setup a context-aware breakpoint */
+	ctx_bcr = DBGBCR_LEN8 | DBGBCR_EXEC | DBGBCR_EL1 | DBGBCR_E |
+		  DBGBCR_BT_CTX_LINK;
+	write_dbgbcr(ctx_bp, ctx_bcr);
+	write_dbgbvr(ctx_bp, ctx);
+
+	/* Setup a linked breakpoint (linked to the context-aware breakpoint) */
+	addr_bcr = DBGBCR_LEN8 | DBGBCR_EXEC | DBGBCR_EL1 | DBGBCR_E |
+		   DBGBCR_BT_ADDR_LINK_CTX |
+		   ((uint32_t)ctx_bp << DBGBCR_LBN_SHIFT);
+	write_dbgbcr(addr_bp, addr_bcr);
+	write_dbgbvr(addr_bp, addr);
+	isb();
+
+	enable_debug_bwp_exception();
+}
+
 static void install_ss(void)
 {
 	uint32_t mdscr;
@@ -177,8 +204,10 @@ static void install_ss(void)
 
 static volatile char write_data;
 
-static void guest_code(uint8_t bpn, uint8_t wpn)
+static void guest_code(uint8_t bpn, uint8_t wpn, uint8_t ctx_bpn)
 {
+	uint64_t ctx = 0x1;	/* a random context number */
+
 	GUEST_SYNC(0);
 
 	/* Software-breakpoint */
@@ -281,6 +310,19 @@ static void guest_code(uint8_t bpn, uint8_t wpn)
 		     : : : "x0");
 	GUEST_ASSERT_EQ(ss_addr[0], 0);
 
+	/* Linked hardware-breakpoint */
+	hw_bp_addr = 0;
+	reset_debug_state();
+	install_hw_bp_ctx(bpn, ctx_bpn, PC(hw_bp_ctx), ctx);
+	/* Set context id */
+	write_sysreg(ctx, contextidr_el1);
+	isb();
+	asm volatile("hw_bp_ctx: nop");
+	write_sysreg(0, contextidr_el1);
+	GUEST_ASSERT_EQ(hw_bp_addr, PC(hw_bp_ctx));
+
+	GUEST_SYNC(10);
+
 	GUEST_DONE();
 }
 
@@ -327,6 +369,7 @@ int main(int argc, char *argv[])
 	struct ucall uc;
 	int stage;
 	uint64_t aa64dfr0;
+	uint8_t brps;
 
 	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
 	ucall_init(vm, NULL);
@@ -349,8 +392,16 @@ int main(int argc, char *argv[])
 	vm_install_sync_handler(vm, VECTOR_SYNC_CURRENT,
 				ESR_EC_SVC64, guest_svc_handler);
 
-	/* Run tests with breakpoint#0 and watchpoint#0. */
-	vcpu_args_set(vcpu, 2, 0, 0);
+	/* Number of breakpoints, minus 1 */
+	brps = cpuid_get_ufield(aa64dfr0, ID_AA64DFR0_BRPS_SHIFT);
+	__TEST_REQUIRE(brps > 0, "At least two breakpoints are required");
+
+	/*
+	 * Run tests with breakpoint#0 and watchpoint#0, and the higiest
+	 * numbered (context-aware) breakpoint.
+	 */
+	vcpu_args_set(vcpu, 3, 0, 0, brps);
+
 	for (stage = 0; stage < 11; stage++) {
 		vcpu_run(vcpu);
 
-- 
2.37.1.595.g718a3a8f04-goog

[PATCH 8/9] KVM: arm64: selftests: Add a test case for a linked watchpoint

[Reiji Watanabe]

Currently, the debug-exceptions test doesn't have a test case for
a linked watchpoint. Add a test case for the linked watchpoint to
the test.

Signed-off-by: Reiji Watanabe <reijiw@google.com>
---
 .../selftests/kvm/aarch64/debug-exceptions.c  | 35 +++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
index 9fccfeebccd3..dc94c85bb383 100644
--- a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
+++ b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
@@ -21,6 +21,9 @@
 #define DBGWCR_WR	(0x2 << 3)
 #define DBGWCR_EL1	(0x1 << 1)
 #define DBGWCR_E	(0x1 << 0)
+#define DBGWCR_LBN_SHIFT	16
+#define DBGWCR_WT_SHIFT		20
+#define DBGWCR_WT_LINK		(0x1 << DBGWCR_WT_SHIFT)
 
 #define SPSR_D		(1 << 9)
 #define SPSR_SS		(1 << 21)
@@ -169,6 +172,28 @@ static void install_hw_bp(uint8_t bpn, uint64_t addr)
 	enable_debug_bwp_exception();
 }
 
+static void install_wp_ctx(uint8_t addr_wp, uint8_t ctx_bp, uint64_t addr,
+			   uint64_t ctx)
+{
+	uint32_t wcr;
+	uint64_t ctx_bcr;
+
+	/* Setup a context-aware breakpoint */
+	ctx_bcr = DBGBCR_LEN8 | DBGBCR_EXEC | DBGBCR_EL1 | DBGBCR_E |
+		  DBGBCR_BT_CTX_LINK;
+	write_dbgbcr(ctx_bp, ctx_bcr);
+	write_dbgbvr(ctx_bp, ctx);
+
+	/* Setup a linked watchpoint (linked to the context-aware breakpoint) */
+	wcr = DBGWCR_LEN8 | DBGWCR_RD | DBGWCR_WR | DBGWCR_EL1 | DBGWCR_E |
+	      DBGWCR_WT_LINK | ((uint32_t)ctx_bp << DBGWCR_LBN_SHIFT);
+	write_dbgwcr(addr_wp, wcr);
+	write_dbgwvr(addr_wp, addr);
+	isb();
+
+	enable_debug_bwp_exception();
+}
+
 void install_hw_bp_ctx(uint8_t addr_bp, uint8_t ctx_bp, uint64_t addr,
 		       uint64_t ctx)
 {
@@ -323,6 +348,16 @@ static void guest_code(uint8_t bpn, uint8_t wpn, uint8_t ctx_bpn)
 
 	GUEST_SYNC(10);
 
+	/* Linked watchpoint */
+	reset_debug_state();
+	install_wp_ctx(wpn, ctx_bpn, PC(write_data), ctx);
+	/* Set context id */
+	write_sysreg(ctx, contextidr_el1);
+	isb();
+	write_data = 'x';
+	GUEST_ASSERT_EQ(write_data, 'x');
+	GUEST_ASSERT_EQ(wp_data_addr, PC(write_data));
+
 	GUEST_DONE();
 }
 
-- 
2.37.1.595.g718a3a8f04-goog

[PATCH 9/9] KVM: arm64: selftests: Test with every breakpoint/watchpoint

[Reiji Watanabe]

Currently, the debug-exceptions test always uses only
{break,watch}point#0 and the highest numbered context-aware
breakpoint. Modify the test to use all {break,watch}points and
context-aware breakpoints supported on the system.

Signed-off-by: Reiji Watanabe <reijiw@google.com>
---
 .../selftests/kvm/aarch64/debug-exceptions.c  | 77 +++++++++++++++----
 1 file changed, 61 insertions(+), 16 deletions(-)

diff --git a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
index dc94c85bb383..7aeab6ae887a 100644
--- a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
+++ b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
@@ -397,14 +397,12 @@ static int debug_version(uint64_t id_aa64dfr0)
 	return cpuid_get_ufield(id_aa64dfr0, ID_AA64DFR0_DEBUGVER_SHIFT);
 }
 
-int main(int argc, char *argv[])
+void run_test(uint8_t bpn, uint8_t wpn, uint8_t ctx_bpn)
 {
 	struct kvm_vcpu *vcpu;
 	struct kvm_vm *vm;
 	struct ucall uc;
 	int stage;
-	uint64_t aa64dfr0;
-	uint8_t brps;
 
 	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
 	ucall_init(vm, NULL);
@@ -412,10 +410,6 @@ int main(int argc, char *argv[])
 	vm_init_descriptor_tables(vm);
 	vcpu_init_descriptor_tables(vcpu);
 
-	vcpu_get_reg(vcpu, KVM_ARM64_SYS_REG(SYS_ID_AA64DFR0_EL1), &aa64dfr0);
-	__TEST_REQUIRE(debug_version(aa64dfr0) >= 6,
-		       "Armv8 debug architecture not supported.");
-
 	vm_install_sync_handler(vm, VECTOR_SYNC_CURRENT,
 				ESR_EC_BRK_INS, guest_sw_bp_handler);
 	vm_install_sync_handler(vm, VECTOR_SYNC_CURRENT,
@@ -427,15 +421,9 @@ int main(int argc, char *argv[])
 	vm_install_sync_handler(vm, VECTOR_SYNC_CURRENT,
 				ESR_EC_SVC64, guest_svc_handler);
 
-	/* Number of breakpoints, minus 1 */
-	brps = cpuid_get_ufield(aa64dfr0, ID_AA64DFR0_BRPS_SHIFT);
-	__TEST_REQUIRE(brps > 0, "At least two breakpoints are required");
-
-	/*
-	 * Run tests with breakpoint#0 and watchpoint#0, and the higiest
-	 * numbered (context-aware) breakpoint.
-	 */
-	vcpu_args_set(vcpu, 3, 0, 0, brps);
+	/* Specify bpn/wpn/ctx_bpn to be tested */
+	vcpu_args_set(vcpu, 3, bpn, wpn, ctx_bpn);
+	pr_debug("Use bpn#%d, wpn#%d and ctx_bpn#%d\n", bpn, wpn, ctx_bpn);
 
 	for (stage = 0; stage < 11; stage++) {
 		vcpu_run(vcpu);
@@ -458,5 +446,62 @@ int main(int argc, char *argv[])
 
 done:
 	kvm_vm_free(vm);
+}
+
+/*
+ * Run debug testing using the various breakpoint#, watchpoint# and
+ * context-aware breakpoint# with the given ID_AA64DFR0_EL1 configuration.
+ */
+void test_debug(uint64_t aa64dfr0)
+{
+	uint8_t brps, wrps, ctx_cmps;
+	uint8_t normal_brp_num, wrp_num, ctx_brp_base, ctx_brp_num;
+	int b, w, c;
+
+	brps = cpuid_get_ufield(aa64dfr0, ID_AA64DFR0_BRPS_SHIFT);
+	__TEST_REQUIRE(brps > 0, "At least two breakpoints are required");
+
+	wrps = cpuid_get_ufield(aa64dfr0, ID_AA64DFR0_WRPS_SHIFT);
+	ctx_cmps = cpuid_get_ufield(aa64dfr0, ID_AA64DFR0_CTX_CMPS_SHIFT);
+
+	pr_debug("%s brps:%d, wrps:%d, ctx_cmps:%d\n", __func__,
+		 brps, wrps, ctx_cmps);
+
+	/* Number of normal (non-context aware) breakpoints */
+	normal_brp_num = brps - ctx_cmps;
+
+	/* Number of watchpoints */
+	wrp_num = wrps + 1;
+
+	/* Number of context aware breakpoints */
+	ctx_brp_num = ctx_cmps + 1;
+
+	/* Lowest context aware breakpoint number */
+	ctx_brp_base = normal_brp_num;
+
+	/* Run tests with all supported breakpoints/watchpoints */
+	for (c = ctx_brp_base; c < ctx_brp_base + ctx_brp_num; c++) {
+		for (b = 0; b < normal_brp_num; b++) {
+			for (w = 0; w < wrp_num; w++)
+				run_test(b, w, c);
+		}
+	}
+}
+
+int main(int argc, char *argv[])
+{
+	struct kvm_vm *vm;
+	struct kvm_vcpu *vcpu;
+	uint64_t aa64dfr0;
+
+	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
+	vcpu_get_reg(vcpu, KVM_ARM64_SYS_REG(SYS_ID_AA64DFR0_EL1), &aa64dfr0);
+	kvm_vm_free(vm);
+
+	__TEST_REQUIRE(debug_version(aa64dfr0) >= 6,
+		       "Armv8 debug architecture not supported.");
+
+	/* Run debug tests with the default configuration */
+	test_debug(aa64dfr0);
 	return 0;
 }
-- 
2.37.1.595.g718a3a8f04-goog

Re: [PATCH 1/9] KVM: arm64: selftests: Add helpers to extract a field of an ID register

[Oliver Upton]

Hi Reiji,

On Wed, Aug 24, 2022 at 10:08:38PM -0700, Reiji Watanabe wrote:
> Introduce helpers to extract a field of an ID register.
> Subsequent patches will use those helpers.
> 
> Signed-off-by: Reiji Watanabe <reijiw@google.com>
> ---
>  .../selftests/kvm/include/aarch64/processor.h     |  2 ++
>  .../testing/selftests/kvm/lib/aarch64/processor.c | 15 +++++++++++++++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/tools/testing/selftests/kvm/include/aarch64/processor.h b/tools/testing/selftests/kvm/include/aarch64/processor.h
> index a8124f9dd68a..a9b4b4e0e592 100644
> --- a/tools/testing/selftests/kvm/include/aarch64/processor.h
> +++ b/tools/testing/selftests/kvm/include/aarch64/processor.h
> @@ -193,4 +193,6 @@ void smccc_hvc(uint32_t function_id, uint64_t arg0, uint64_t arg1,
>  
>  uint32_t guest_get_vcpuid(void);
>  
> +int cpuid_get_sfield(uint64_t val, int field_shift);
> +unsigned int cpuid_get_ufield(uint64_t val, int field_shift);
>  #endif /* SELFTEST_KVM_PROCESSOR_H */
> diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> index 6f5551368944..0b2ad46e7ff5 100644
> --- a/tools/testing/selftests/kvm/lib/aarch64/processor.c
> +++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> @@ -528,3 +528,18 @@ void smccc_hvc(uint32_t function_id, uint64_t arg0, uint64_t arg1,
>  		       [arg4] "r"(arg4), [arg5] "r"(arg5), [arg6] "r"(arg6)
>  		     : "x0", "x1", "x2", "x3", "x4", "x5", "x6", "x7");
>  }
> +
> +/* Helpers to get a signed/unsigned feature field from ID register value */
> +int cpuid_get_sfield(uint64_t val, int field_shift)
> +{
> +	int width = 4;
> +
> +	return (int64_t)(val << (64 - width - field_shift)) >> (64 - width);
> +}

I don't believe this helper is ever used.

> +unsigned int cpuid_get_ufield(uint64_t val, int field_shift)
> +{
> +	int width = 4;
> +
> +	return (uint64_t)(val << (64 - width - field_shift)) >> (64 - width);
> +}

I would recommend not open-coding this and instead make use of
ARM64_FEATURE_MASK(). You could pull in linux/bitfield.h to tools, or do
something like this:

  #define ARM64_FEATURE_GET(ftr, val)					\
  	  	  ((ARM64_FEATURE_MASK(ftr) & val) >> ftr##_SHIFT)

Slight preference for FIELD_{GET,SET}() as it matches the field
extraction in the kernel as well.

--
Thanks,
Oliver

Re: [PATCH 1/9] KVM: arm64: selftests: Add helpers to extract a field of an ID register

[Ricardo Koller]

On Thu, Aug 25, 2022 at 09:48:35AM -0700, Oliver Upton wrote:
> Hi Reiji,
> 
> On Wed, Aug 24, 2022 at 10:08:38PM -0700, Reiji Watanabe wrote:
> > Introduce helpers to extract a field of an ID register.
> > Subsequent patches will use those helpers.
> > 
> > Signed-off-by: Reiji Watanabe <reijiw@google.com>
> > ---
> >  .../selftests/kvm/include/aarch64/processor.h     |  2 ++
> >  .../testing/selftests/kvm/lib/aarch64/processor.c | 15 +++++++++++++++
> >  2 files changed, 17 insertions(+)
> > 
> > diff --git a/tools/testing/selftests/kvm/include/aarch64/processor.h b/tools/testing/selftests/kvm/include/aarch64/processor.h
> > index a8124f9dd68a..a9b4b4e0e592 100644
> > --- a/tools/testing/selftests/kvm/include/aarch64/processor.h
> > +++ b/tools/testing/selftests/kvm/include/aarch64/processor.h
> > @@ -193,4 +193,6 @@ void smccc_hvc(uint32_t function_id, uint64_t arg0, uint64_t arg1,
> >  
> >  uint32_t guest_get_vcpuid(void);
> >  
> > +int cpuid_get_sfield(uint64_t val, int field_shift);
> > +unsigned int cpuid_get_ufield(uint64_t val, int field_shift);
> >  #endif /* SELFTEST_KVM_PROCESSOR_H */
> > diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> > index 6f5551368944..0b2ad46e7ff5 100644
> > --- a/tools/testing/selftests/kvm/lib/aarch64/processor.c
> > +++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> > @@ -528,3 +528,18 @@ void smccc_hvc(uint32_t function_id, uint64_t arg0, uint64_t arg1,
> >  		       [arg4] "r"(arg4), [arg5] "r"(arg5), [arg6] "r"(arg6)
> >  		     : "x0", "x1", "x2", "x3", "x4", "x5", "x6", "x7");
> >  }
> > +
> > +/* Helpers to get a signed/unsigned feature field from ID register value */
> > +int cpuid_get_sfield(uint64_t val, int field_shift)
> > +{
> > +	int width = 4;
> > +
> > +	return (int64_t)(val << (64 - width - field_shift)) >> (64 - width);
> > +}
> 
> I don't believe this helper is ever used.
> 
> > +unsigned int cpuid_get_ufield(uint64_t val, int field_shift)
> > +{
> > +	int width = 4;
> > +
> > +	return (uint64_t)(val << (64 - width - field_shift)) >> (64 - width);
> > +}
> 
> I would recommend not open-coding this and instead make use of
> ARM64_FEATURE_MASK(). You could pull in linux/bitfield.h to tools, or do
> something like this:
> 
>   #define ARM64_FEATURE_GET(ftr, val)					\
>   	  	  ((ARM64_FEATURE_MASK(ftr) & val) >> ftr##_SHIFT)
> 
> Slight preference for FIELD_{GET,SET}() as it matches the field
> extraction in the kernel as well.

Was doing that with this commit:

	[PATCH v5 05/13] tools: Copy bitfield.h from the kernel sources

Maybe you could just use it given that it's already reviewed.

> 
> --
> Thanks,
> Oliver
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 3/9] KVM: arm64: selftests: Remove the hard-coded {b,w}pn#0 from debug-exceptions

[Oliver Upton]

On Wed, Aug 24, 2022 at 10:08:40PM -0700, Reiji Watanabe wrote:
> Remove the hard-coded {break,watch}point #0 from the guest_code()
> in debug-exceptions to allow {break,watch}point number to be
> specified.  Subsequent patches will add test cases for non-zero
> {break,watch}points.

Also worth mentioning that you're opportunistically zeroing the debug
registers as their values are UNKNOWN out of reset.

--
Thanks,
Oliver

Re: [PATCH 1/9] KVM: arm64: selftests: Add helpers to extract a field of an ID register

[Oliver Upton]

On Thu, Aug 25, 2022 at 09:52:53AM -0700, Ricardo Koller wrote:
> On Thu, Aug 25, 2022 at 09:48:35AM -0700, Oliver Upton wrote:
> > Hi Reiji,
> > 
> > On Wed, Aug 24, 2022 at 10:08:38PM -0700, Reiji Watanabe wrote:
> > > Introduce helpers to extract a field of an ID register.
> > > Subsequent patches will use those helpers.
> > > 
> > > Signed-off-by: Reiji Watanabe <reijiw@google.com>
> > > ---
> > >  .../selftests/kvm/include/aarch64/processor.h     |  2 ++
> > >  .../testing/selftests/kvm/lib/aarch64/processor.c | 15 +++++++++++++++
> > >  2 files changed, 17 insertions(+)
> > > 
> > > diff --git a/tools/testing/selftests/kvm/include/aarch64/processor.h b/tools/testing/selftests/kvm/include/aarch64/processor.h
> > > index a8124f9dd68a..a9b4b4e0e592 100644
> > > --- a/tools/testing/selftests/kvm/include/aarch64/processor.h
> > > +++ b/tools/testing/selftests/kvm/include/aarch64/processor.h
> > > @@ -193,4 +193,6 @@ void smccc_hvc(uint32_t function_id, uint64_t arg0, uint64_t arg1,
> > >  
> > >  uint32_t guest_get_vcpuid(void);
> > >  
> > > +int cpuid_get_sfield(uint64_t val, int field_shift);
> > > +unsigned int cpuid_get_ufield(uint64_t val, int field_shift);
> > >  #endif /* SELFTEST_KVM_PROCESSOR_H */
> > > diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> > > index 6f5551368944..0b2ad46e7ff5 100644
> > > --- a/tools/testing/selftests/kvm/lib/aarch64/processor.c
> > > +++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> > > @@ -528,3 +528,18 @@ void smccc_hvc(uint32_t function_id, uint64_t arg0, uint64_t arg1,
> > >  		       [arg4] "r"(arg4), [arg5] "r"(arg5), [arg6] "r"(arg6)
> > >  		     : "x0", "x1", "x2", "x3", "x4", "x5", "x6", "x7");
> > >  }
> > > +
> > > +/* Helpers to get a signed/unsigned feature field from ID register value */
> > > +int cpuid_get_sfield(uint64_t val, int field_shift)
> > > +{
> > > +	int width = 4;
> > > +
> > > +	return (int64_t)(val << (64 - width - field_shift)) >> (64 - width);
> > > +}
> > 
> > I don't believe this helper is ever used.
> > 
> > > +unsigned int cpuid_get_ufield(uint64_t val, int field_shift)
> > > +{
> > > +	int width = 4;
> > > +
> > > +	return (uint64_t)(val << (64 - width - field_shift)) >> (64 - width);
> > > +}
> > 
> > I would recommend not open-coding this and instead make use of
> > ARM64_FEATURE_MASK(). You could pull in linux/bitfield.h to tools, or do
> > something like this:
> > 
> >   #define ARM64_FEATURE_GET(ftr, val)					\
> >   	  	  ((ARM64_FEATURE_MASK(ftr) & val) >> ftr##_SHIFT)
> > 
> > Slight preference for FIELD_{GET,SET}() as it matches the field
> > extraction in the kernel as well.
> 
> Was doing that with this commit:
> 
> 	[PATCH v5 05/13] tools: Copy bitfield.h from the kernel sources
> 
> Maybe you could just use it given that it's already reviewed.

Oops, thanks for the reminder Ricardo! Yeah, let's go that route then.

--
Thanks,
Oliver

Re: [PATCH 4/9] KVM: arm64: selftests: Add helpers to enable debug exceptions

[Oliver Upton]

On Wed, Aug 24, 2022 at 10:08:41PM -0700, Reiji Watanabe wrote:
> Add helpers to enable breakpoint and watchpoint exceptions.
> 
> Signed-off-by: Reiji Watanabe <reijiw@google.com>
> ---
>  .../selftests/kvm/aarch64/debug-exceptions.c  | 25 ++++++++++---------
>  1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> index 183ee16acb7d..713c7240b680 100644
> --- a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> +++ b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> @@ -128,10 +128,20 @@ static void enable_os_lock(void)
>  	GUEST_ASSERT(read_sysreg(oslsr_el1) & 2);
>  }
>  
> +static void enable_debug_bwp_exception(void)

uber-nit: enable_monitor_debug_exceptions()

(more closely matches the definition of MDSCR_EL1.MDE)

With that:

Reviewed-by: Oliver Upton <oliver.upton@linux.dev>

--
Thanks,
Oliver

Re: [PATCH 5/9] KVM: arm64: selftests: Have debug_version() use cpuid_get_ufield() helper

[Oliver Upton]

On Wed, Aug 24, 2022 at 10:08:42PM -0700, Reiji Watanabe wrote:
> Change debug_version() to use cpuid_get_ufield() to extract DebugVer
> field from the AA64DFR0_EL1 register value.

Either squash this into the patch that adds the field accessors or
reorder it to immediately follow said patch.

aarch64_get_supported_page_sizes() is also due for a cleanup, as it
accesses the TGRANx fields of ID_AA64MMFR0_EL1.

--
Thanks,
Oliver

Re: [PATCH 1/9] KVM: arm64: selftests: Add helpers to extract a field of an ID register

[Reiji Watanabe]

Hi Oliver, Ricardo,

On Thu, Aug 25, 2022 at 9:58 AM Oliver Upton <oliver.upton@linux.dev> wrote:
>
> On Thu, Aug 25, 2022 at 09:52:53AM -0700, Ricardo Koller wrote:
> > On Thu, Aug 25, 2022 at 09:48:35AM -0700, Oliver Upton wrote:
> > > Hi Reiji,
> > >
> > > On Wed, Aug 24, 2022 at 10:08:38PM -0700, Reiji Watanabe wrote:
> > > > Introduce helpers to extract a field of an ID register.
> > > > Subsequent patches will use those helpers.
> > > >
> > > > Signed-off-by: Reiji Watanabe <reijiw@google.com>
> > > > ---
> > > >  .../selftests/kvm/include/aarch64/processor.h     |  2 ++
> > > >  .../testing/selftests/kvm/lib/aarch64/processor.c | 15 +++++++++++++++
> > > >  2 files changed, 17 insertions(+)
> > > >
> > > > diff --git a/tools/testing/selftests/kvm/include/aarch64/processor.h b/tools/testing/selftests/kvm/include/aarch64/processor.h
> > > > index a8124f9dd68a..a9b4b4e0e592 100644
> > > > --- a/tools/testing/selftests/kvm/include/aarch64/processor.h
> > > > +++ b/tools/testing/selftests/kvm/include/aarch64/processor.h
> > > > @@ -193,4 +193,6 @@ void smccc_hvc(uint32_t function_id, uint64_t arg0, uint64_t arg1,
> > > >
> > > >  uint32_t guest_get_vcpuid(void);
> > > >
> > > > +int cpuid_get_sfield(uint64_t val, int field_shift);
> > > > +unsigned int cpuid_get_ufield(uint64_t val, int field_shift);
> > > >  #endif /* SELFTEST_KVM_PROCESSOR_H */
> > > > diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> > > > index 6f5551368944..0b2ad46e7ff5 100644
> > > > --- a/tools/testing/selftests/kvm/lib/aarch64/processor.c
> > > > +++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> > > > @@ -528,3 +528,18 @@ void smccc_hvc(uint32_t function_id, uint64_t arg0, uint64_t arg1,
> > > >                  [arg4] "r"(arg4), [arg5] "r"(arg5), [arg6] "r"(arg6)
> > > >                : "x0", "x1", "x2", "x3", "x4", "x5", "x6", "x7");
> > > >  }
> > > > +
> > > > +/* Helpers to get a signed/unsigned feature field from ID register value */
> > > > +int cpuid_get_sfield(uint64_t val, int field_shift)
> > > > +{
> > > > + int width = 4;
> > > > +
> > > > + return (int64_t)(val << (64 - width - field_shift)) >> (64 - width);
> > > > +}
> > >
> > > I don't believe this helper is ever used.

I thought I was going to use this in the selftest for my ID register series.
(So, I originally would like to have similar helpers in cpufeture.h)
But, you are right. That test only extracts unsigned fields...

> > >
> > > > +unsigned int cpuid_get_ufield(uint64_t val, int field_shift)
> > > > +{
> > > > + int width = 4;
> > > > +
> > > > + return (uint64_t)(val << (64 - width - field_shift)) >> (64 - width);
> > > > +}
> > >
> > > I would recommend not open-coding this and instead make use of
> > > ARM64_FEATURE_MASK(). You could pull in linux/bitfield.h to tools, or do
> > > something like this:
> > >
> > >   #define ARM64_FEATURE_GET(ftr, val)                                       \
> > >               ((ARM64_FEATURE_MASK(ftr) & val) >> ftr##_SHIFT)
> > >
> > > Slight preference for FIELD_{GET,SET}() as it matches the field
> > > extraction in the kernel as well.
> >
> > Was doing that with this commit:
> >
> >       [PATCH v5 05/13] tools: Copy bitfield.h from the kernel sources
> >
> > Maybe you could just use it given that it's already reviewed.
>
> Oops, thanks for the reminder Ricardo! Yeah, let's go that route then.

Thank you for the information. I will use that instead.

Thank you,
Reiji

Re: [PATCH 3/9] KVM: arm64: selftests: Remove the hard-coded {b,w}pn#0 from debug-exceptions

[Reiji Watanabe]

Hi Oliver,

On Thu, Aug 25, 2022 at 9:55 AM Oliver Upton <oliver.upton@linux.dev> wrote:
>
> On Wed, Aug 24, 2022 at 10:08:40PM -0700, Reiji Watanabe wrote:
> > Remove the hard-coded {break,watch}point #0 from the guest_code()
> > in debug-exceptions to allow {break,watch}point number to be
> > specified.  Subsequent patches will add test cases for non-zero
> > {break,watch}points.
>
> Also worth mentioning that you're opportunistically zeroing the debug
> registers as their values are UNKNOWN out of reset.

Yes, I will add that description in v2.

Thank you,
Reiji

Re: [PATCH 4/9] KVM: arm64: selftests: Add helpers to enable debug exceptions

[Reiji Watanabe]

Hi Oliver,

On Thu, Aug 25, 2022 at 10:22 AM Oliver Upton <oliver.upton@linux.dev> wrote:
>
> On Wed, Aug 24, 2022 at 10:08:41PM -0700, Reiji Watanabe wrote:
> > Add helpers to enable breakpoint and watchpoint exceptions.
> >
> > Signed-off-by: Reiji Watanabe <reijiw@google.com>
> > ---
> >  .../selftests/kvm/aarch64/debug-exceptions.c  | 25 ++++++++++---------
> >  1 file changed, 13 insertions(+), 12 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> > index 183ee16acb7d..713c7240b680 100644
> > --- a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> > +++ b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> > @@ -128,10 +128,20 @@ static void enable_os_lock(void)
> >       GUEST_ASSERT(read_sysreg(oslsr_el1) & 2);
> >  }
> >
> > +static void enable_debug_bwp_exception(void)
>
> uber-nit: enable_monitor_debug_exceptions()
>
> (more closely matches the definition of MDSCR_EL1.MDE)

Thank you for the proposal. Sounds better!


> With that:
>
> Reviewed-by: Oliver Upton <oliver.upton@linux.dev>

Thank you for the review!
Reiji

Re: [PATCH 5/9] KVM: arm64: selftests: Have debug_version() use cpuid_get_ufield() helper

[Reiji Watanabe]

On Thu, Aug 25, 2022 at 10:29 AM Oliver Upton <oliver.upton@linux.dev> wrote:
>
> On Wed, Aug 24, 2022 at 10:08:42PM -0700, Reiji Watanabe wrote:
> > Change debug_version() to use cpuid_get_ufield() to extract DebugVer
> > field from the AA64DFR0_EL1 register value.
>
> Either squash this into the patch that adds the field accessors or
> reorder it to immediately follow said patch.
>
> aarch64_get_supported_page_sizes() is also due for a cleanup, as it
> accesses the TGRANx fields of ID_AA64MMFR0_EL1.

Sure, I will fix them.

Thank you,
Reiji

Re: [PATCH 7/9] KVM: arm64: selftests: Add a test case for a linked breakpoint

[Reiji Watanabe]

On Wed, Aug 24, 2022 at 10:10 PM Reiji Watanabe <reijiw@google.com> wrote:
>
> Currently, the debug-exceptions test doesn't have a test case for
> a linked breakpoint. Add a test case for the linked breakpoint to
> the test.
>
> Signed-off-by: Reiji Watanabe <reijiw@google.com>
>
> ---
>  .../selftests/kvm/aarch64/debug-exceptions.c  | 59 +++++++++++++++++--
>  1 file changed, 55 insertions(+), 4 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> index ab8860e3a9fa..9fccfeebccd3 100644
> --- a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> +++ b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> @@ -11,6 +11,10 @@
>  #define DBGBCR_EXEC    (0x0 << 3)
>  #define DBGBCR_EL1     (0x1 << 1)
>  #define DBGBCR_E       (0x1 << 0)
> +#define DBGBCR_LBN_SHIFT       16
> +#define DBGBCR_BT_SHIFT                20
> +#define DBGBCR_BT_ADDR_LINK_CTX        (0x1 << DBGBCR_BT_SHIFT)
> +#define DBGBCR_BT_CTX_LINK     (0x3 << DBGBCR_BT_SHIFT)
>
>  #define DBGWCR_LEN8    (0xff << 5)
>  #define DBGWCR_RD      (0x1 << 3)
> @@ -21,7 +25,7 @@
>  #define SPSR_D         (1 << 9)
>  #define SPSR_SS                (1 << 21)
>
> -extern unsigned char sw_bp, sw_bp2, hw_bp, hw_bp2, bp_svc, bp_brk, hw_wp, ss_start;
> +extern unsigned char sw_bp, sw_bp2, hw_bp, hw_bp2, bp_svc, bp_brk, hw_wp, ss_start, hw_bp_ctx;
>  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;
> @@ -103,6 +107,7 @@ static void reset_debug_state(void)
>         isb();
>
>         write_sysreg(0, mdscr_el1);
> +       write_sysreg(0, contextidr_el1);
>
>         /* Reset all bcr/bvr/wcr/wvr registers */
>         dfr0 = read_sysreg(id_aa64dfr0_el1);
> @@ -164,6 +169,28 @@ static void install_hw_bp(uint8_t bpn, uint64_t addr)
>         enable_debug_bwp_exception();
>  }
>
> +void install_hw_bp_ctx(uint8_t addr_bp, uint8_t ctx_bp, uint64_t addr,
> +                      uint64_t ctx)
> +{
> +       uint32_t addr_bcr, ctx_bcr;
> +
> +       /* Setup a context-aware breakpoint */
> +       ctx_bcr = DBGBCR_LEN8 | DBGBCR_EXEC | DBGBCR_EL1 | DBGBCR_E |
> +                 DBGBCR_BT_CTX_LINK;
> +       write_dbgbcr(ctx_bp, ctx_bcr);
> +       write_dbgbvr(ctx_bp, ctx);
> +
> +       /* Setup a linked breakpoint (linked to the context-aware breakpoint) */
> +       addr_bcr = DBGBCR_LEN8 | DBGBCR_EXEC | DBGBCR_EL1 | DBGBCR_E |
> +                  DBGBCR_BT_ADDR_LINK_CTX |
> +                  ((uint32_t)ctx_bp << DBGBCR_LBN_SHIFT);
> +       write_dbgbcr(addr_bp, addr_bcr);
> +       write_dbgbvr(addr_bp, addr);
> +       isb();
> +
> +       enable_debug_bwp_exception();
> +}
> +
>  static void install_ss(void)
>  {
>         uint32_t mdscr;
> @@ -177,8 +204,10 @@ static void install_ss(void)
>
>  static volatile char write_data;
>
> -static void guest_code(uint8_t bpn, uint8_t wpn)
> +static void guest_code(uint8_t bpn, uint8_t wpn, uint8_t ctx_bpn)
>  {
> +       uint64_t ctx = 0x1;     /* a random context number */
> +
>         GUEST_SYNC(0);
>
>         /* Software-breakpoint */
> @@ -281,6 +310,19 @@ static void guest_code(uint8_t bpn, uint8_t wpn)
>                      : : : "x0");
>         GUEST_ASSERT_EQ(ss_addr[0], 0);
>

I've just noticed that I should add GUEST_SYNC(10) here, use
GUEST_SYNC(11) for the following test case, and update the
stage limit value in the loop in userspace code.

Or I might consider removing the stage management code itself.
It doesn't appear to be very useful to me, and I would think
we could easily forget to update it :-)

Thank you,
Reiji

> +       /* Linked hardware-breakpoint */
> +       hw_bp_addr = 0;
> +       reset_debug_state();
> +       install_hw_bp_ctx(bpn, ctx_bpn, PC(hw_bp_ctx), ctx);
> +       /* Set context id */
> +       write_sysreg(ctx, contextidr_el1);
> +       isb();
> +       asm volatile("hw_bp_ctx: nop");
> +       write_sysreg(0, contextidr_el1);
> +       GUEST_ASSERT_EQ(hw_bp_addr, PC(hw_bp_ctx));
> +
> +       GUEST_SYNC(10);
> +
>         GUEST_DONE();
>  }
>
> @@ -327,6 +369,7 @@ int main(int argc, char *argv[])
>         struct ucall uc;
>         int stage;
>         uint64_t aa64dfr0;
> +       uint8_t brps;
>
>         vm = vm_create_with_one_vcpu(&vcpu, guest_code);
>         ucall_init(vm, NULL);
> @@ -349,8 +392,16 @@ int main(int argc, char *argv[])
>         vm_install_sync_handler(vm, VECTOR_SYNC_CURRENT,
>                                 ESR_EC_SVC64, guest_svc_handler);
>
> -       /* Run tests with breakpoint#0 and watchpoint#0. */
> -       vcpu_args_set(vcpu, 2, 0, 0);
> +       /* Number of breakpoints, minus 1 */
> +       brps = cpuid_get_ufield(aa64dfr0, ID_AA64DFR0_BRPS_SHIFT);
> +       __TEST_REQUIRE(brps > 0, "At least two breakpoints are required");
> +
> +       /*
> +        * Run tests with breakpoint#0 and watchpoint#0, and the higiest
> +        * numbered (context-aware) breakpoint.
> +        */
> +       vcpu_args_set(vcpu, 3, 0, 0, brps);
> +
>         for (stage = 0; stage < 11; stage++) {
>                 vcpu_run(vcpu);
>
> --
> 2.37.1.595.g718a3a8f04-goog
>

Re: [PATCH 2/9] KVM: arm64: selftests: Add write_dbg{b,w}{c,v}r helpers in debug-exceptions

[Ricardo Koller]

On Wed, Aug 24, 2022 at 10:08:39PM -0700, Reiji Watanabe wrote:
> Introduce helpers in the debug-exceptions test to write to
> dbg{b,w}{c,v}r registers. Those helpers will be useful for
> test cases that will be added to the test in subsequent patches.
>

Reviewed-by: Ricardo Koller <ricarkol@google.com>

> Signed-off-by: Reiji Watanabe <reijiw@google.com>
> ---
>  .../selftests/kvm/aarch64/debug-exceptions.c  | 72 +++++++++++++++++--
>  1 file changed, 68 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> index 2ee35cf9801e..51047e6b8db3 100644
> --- a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> +++ b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> @@ -28,6 +28,69 @@ static volatile uint64_t svc_addr;
>  static volatile uint64_t ss_addr[4], ss_idx;
>  #define  PC(v)  ((uint64_t)&(v))
>  
> +#define GEN_DEBUG_WRITE_REG(reg_name)			\
> +static void write_##reg_name(int num, uint64_t val)	\
> +{							\
> +	switch (num) {					\
> +	case 0:						\
> +		write_sysreg(val, reg_name##0_el1);	\
> +		break;					\
> +	case 1:						\
> +		write_sysreg(val, reg_name##1_el1);	\
> +		break;					\
> +	case 2:						\
> +		write_sysreg(val, reg_name##2_el1);	\
> +		break;					\
> +	case 3:						\
> +		write_sysreg(val, reg_name##3_el1);	\
> +		break;					\
> +	case 4:						\
> +		write_sysreg(val, reg_name##4_el1);	\
> +		break;					\
> +	case 5:						\
> +		write_sysreg(val, reg_name##5_el1);	\
> +		break;					\
> +	case 6:						\
> +		write_sysreg(val, reg_name##6_el1);	\
> +		break;					\
> +	case 7:						\
> +		write_sysreg(val, reg_name##7_el1);	\
> +		break;					\
> +	case 8:						\
> +		write_sysreg(val, reg_name##8_el1);	\
> +		break;					\
> +	case 9:						\
> +		write_sysreg(val, reg_name##9_el1);	\
> +		break;					\
> +	case 10:					\
> +		write_sysreg(val, reg_name##10_el1);	\
> +		break;					\
> +	case 11:					\
> +		write_sysreg(val, reg_name##11_el1);	\
> +		break;					\
> +	case 12:					\
> +		write_sysreg(val, reg_name##12_el1);	\
> +		break;					\
> +	case 13:					\
> +		write_sysreg(val, reg_name##13_el1);	\
> +		break;					\
> +	case 14:					\
> +		write_sysreg(val, reg_name##14_el1);	\
> +		break;					\
> +	case 15:					\
> +		write_sysreg(val, reg_name##15_el1);	\
> +		break;					\
> +	default:					\
> +		GUEST_ASSERT(0);			\
> +	}						\
> +}
> +
> +/* Define write_dbgbcr()/write_dbgbvr()/write_dbgwcr()/write_dbgwvr() */
> +GEN_DEBUG_WRITE_REG(dbgbcr)
> +GEN_DEBUG_WRITE_REG(dbgbvr)
> +GEN_DEBUG_WRITE_REG(dbgwcr)
> +GEN_DEBUG_WRITE_REG(dbgwvr)
> +
>  static void reset_debug_state(void)
>  {
>  	asm volatile("msr daifset, #8");
> @@ -59,8 +122,9 @@ static void install_wp(uint64_t addr)
>  	uint32_t mdscr;
>  
>  	wcr = DBGWCR_LEN8 | DBGWCR_RD | DBGWCR_WR | DBGWCR_EL1 | DBGWCR_E;
> -	write_sysreg(wcr, dbgwcr0_el1);
> -	write_sysreg(addr, dbgwvr0_el1);
> +	write_dbgwcr(0, wcr);
> +	write_dbgwvr(0, addr);
> +
>  	isb();
>  
>  	asm volatile("msr daifclr, #8");
> @@ -76,8 +140,8 @@ static void install_hw_bp(uint64_t addr)
>  	uint32_t mdscr;
>  
>  	bcr = DBGBCR_LEN8 | DBGBCR_EXEC | DBGBCR_EL1 | DBGBCR_E;
> -	write_sysreg(bcr, dbgbcr0_el1);
> -	write_sysreg(addr, dbgbvr0_el1);
> +	write_dbgbcr(0, bcr);
> +	write_dbgbvr(0, addr);
>  	isb();
>  
>  	asm volatile("msr daifclr, #8");
> -- 
> 2.37.1.595.g718a3a8f04-goog
> 

Re: [PATCH 3/9] KVM: arm64: selftests: Remove the hard-coded {b,w}pn#0 from debug-exceptions

[Ricardo Koller]

On Wed, Aug 24, 2022 at 10:08:40PM -0700, Reiji Watanabe wrote:
> Remove the hard-coded {break,watch}point #0 from the guest_code()
> in debug-exceptions to allow {break,watch}point number to be
> specified.  Subsequent patches will add test cases for non-zero
> {break,watch}points.
>
> Signed-off-by: Reiji Watanabe <reijiw@google.com>
> ---
>  .../selftests/kvm/aarch64/debug-exceptions.c  | 50 ++++++++++++-------
>  1 file changed, 32 insertions(+), 18 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> index 51047e6b8db3..183ee16acb7d 100644
> --- a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> +++ b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> @@ -93,6 +93,9 @@ GEN_DEBUG_WRITE_REG(dbgwvr)
>  
>  static void reset_debug_state(void)
>  {
> +	uint8_t brps, wrps, i;
> +	uint64_t dfr0;
> +
>  	asm volatile("msr daifset, #8");
>  
>  	write_sysreg(0, osdlr_el1);
> @@ -100,11 +103,20 @@ static void reset_debug_state(void)
>  	isb();
>  
>  	write_sysreg(0, mdscr_el1);
> -	/* This test only uses the first bp and wp slot. */
> -	write_sysreg(0, dbgbvr0_el1);
> -	write_sysreg(0, dbgbcr0_el1);
> -	write_sysreg(0, dbgwcr0_el1);
> -	write_sysreg(0, dbgwvr0_el1);
> +
> +	/* Reset all bcr/bvr/wcr/wvr registers */
> +	dfr0 = read_sysreg(id_aa64dfr0_el1);
> +	brps = cpuid_get_ufield(dfr0, ID_AA64DFR0_BRPS_SHIFT);

I guess this might have to change to ARM64_FEATURE_GET(). In any case:

Reviewed-by: Ricardo Koller <ricarkol@google.com>

(also assuming it includes the commit message clarification about reset
clearing all registers).

> +	for (i = 0; i <= brps; i++) {
> +		write_dbgbcr(i, 0);
> +		write_dbgbvr(i, 0);
> +	}
> +	wrps = cpuid_get_ufield(dfr0, ID_AA64DFR0_WRPS_SHIFT);
> +	for (i = 0; i <= wrps; i++) {
> +		write_dbgwcr(i, 0);
> +		write_dbgwvr(i, 0);
> +	}
> +
>  	isb();
>  }
>  
> @@ -116,14 +128,14 @@ static void enable_os_lock(void)
>  	GUEST_ASSERT(read_sysreg(oslsr_el1) & 2);
>  }
>  
> -static void install_wp(uint64_t addr)
> +static void install_wp(uint8_t wpn, uint64_t addr)
>  {
>  	uint32_t wcr;
>  	uint32_t mdscr;
>  
>  	wcr = DBGWCR_LEN8 | DBGWCR_RD | DBGWCR_WR | DBGWCR_EL1 | DBGWCR_E;
> -	write_dbgwcr(0, wcr);
> -	write_dbgwvr(0, addr);
> +	write_dbgwcr(wpn, wcr);
> +	write_dbgwvr(wpn, addr);
>  
>  	isb();
>  
> @@ -134,14 +146,14 @@ static void install_wp(uint64_t addr)
>  	isb();
>  }
>  
> -static void install_hw_bp(uint64_t addr)
> +static void install_hw_bp(uint8_t bpn, uint64_t addr)
>  {
>  	uint32_t bcr;
>  	uint32_t mdscr;
>  
>  	bcr = DBGBCR_LEN8 | DBGBCR_EXEC | DBGBCR_EL1 | DBGBCR_E;
> -	write_dbgbcr(0, bcr);
> -	write_dbgbvr(0, addr);
> +	write_dbgbcr(bpn, bcr);
> +	write_dbgbvr(bpn, addr);
>  	isb();
>  
>  	asm volatile("msr daifclr, #8");
> @@ -164,7 +176,7 @@ static void install_ss(void)
>  
>  static volatile char write_data;
>  
> -static void guest_code(void)
> +static void guest_code(uint8_t bpn, uint8_t wpn)
>  {
>  	GUEST_SYNC(0);
>  
> @@ -177,7 +189,7 @@ static void guest_code(void)
>  
>  	/* Hardware-breakpoint */
>  	reset_debug_state();
> -	install_hw_bp(PC(hw_bp));
> +	install_hw_bp(bpn, PC(hw_bp));
>  	asm volatile("hw_bp: nop");
>  	GUEST_ASSERT_EQ(hw_bp_addr, PC(hw_bp));
>  
> @@ -185,7 +197,7 @@ static void guest_code(void)
>  
>  	/* Hardware-breakpoint + svc */
>  	reset_debug_state();
> -	install_hw_bp(PC(bp_svc));
> +	install_hw_bp(bpn, PC(bp_svc));
>  	asm volatile("bp_svc: svc #0");
>  	GUEST_ASSERT_EQ(hw_bp_addr, PC(bp_svc));
>  	GUEST_ASSERT_EQ(svc_addr, PC(bp_svc) + 4);
> @@ -194,7 +206,7 @@ static void guest_code(void)
>  
>  	/* Hardware-breakpoint + software-breakpoint */
>  	reset_debug_state();
> -	install_hw_bp(PC(bp_brk));
> +	install_hw_bp(bpn, PC(bp_brk));
>  	asm volatile("bp_brk: brk #0");
>  	GUEST_ASSERT_EQ(sw_bp_addr, PC(bp_brk));
>  	GUEST_ASSERT_EQ(hw_bp_addr, PC(bp_brk));
> @@ -203,7 +215,7 @@ static void guest_code(void)
>  
>  	/* Watchpoint */
>  	reset_debug_state();
> -	install_wp(PC(write_data));
> +	install_wp(wpn, PC(write_data));
>  	write_data = 'x';
>  	GUEST_ASSERT_EQ(write_data, 'x');
>  	GUEST_ASSERT_EQ(wp_data_addr, PC(write_data));
> @@ -237,7 +249,7 @@ static void guest_code(void)
>  	/* OS Lock blocking hardware-breakpoint */
>  	reset_debug_state();
>  	enable_os_lock();
> -	install_hw_bp(PC(hw_bp2));
> +	install_hw_bp(bpn, PC(hw_bp2));
>  	hw_bp_addr = 0;
>  	asm volatile("hw_bp2: nop");
>  	GUEST_ASSERT_EQ(hw_bp_addr, 0);
> @@ -249,7 +261,7 @@ static void guest_code(void)
>  	enable_os_lock();
>  	write_data = '\0';
>  	wp_data_addr = 0;
> -	install_wp(PC(write_data));
> +	install_wp(wpn, PC(write_data));
>  	write_data = 'x';
>  	GUEST_ASSERT_EQ(write_data, 'x');
>  	GUEST_ASSERT_EQ(wp_data_addr, 0);
> @@ -337,6 +349,8 @@ int main(int argc, char *argv[])
>  	vm_install_sync_handler(vm, VECTOR_SYNC_CURRENT,
>  				ESR_EC_SVC64, guest_svc_handler);
>  
> +	/* Run tests with breakpoint#0 and watchpoint#0. */
> +	vcpu_args_set(vcpu, 2, 0, 0);
>  	for (stage = 0; stage < 11; stage++) {
>  		vcpu_run(vcpu);
>  
> -- 
> 2.37.1.595.g718a3a8f04-goog
> 

Re: [PATCH 4/9] KVM: arm64: selftests: Add helpers to enable debug exceptions

[Ricardo Koller]

On Thu, Aug 25, 2022 at 10:21:49AM -0700, Oliver Upton wrote:
> On Wed, Aug 24, 2022 at 10:08:41PM -0700, Reiji Watanabe wrote:
> > Add helpers to enable breakpoint and watchpoint exceptions.
> > 
> > Signed-off-by: Reiji Watanabe <reijiw@google.com>
> > ---
> >  .../selftests/kvm/aarch64/debug-exceptions.c  | 25 ++++++++++---------
> >  1 file changed, 13 insertions(+), 12 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> > index 183ee16acb7d..713c7240b680 100644
> > --- a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> > +++ b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> > @@ -128,10 +128,20 @@ static void enable_os_lock(void)
> >  	GUEST_ASSERT(read_sysreg(oslsr_el1) & 2);
> >  }
> >  
> > +static void enable_debug_bwp_exception(void)
> 
> uber-nit: enable_monitor_debug_exceptions()
> 
> (more closely matches the definition of MDSCR_EL1.MDE)

oh, didn't know the MDE was for monitor debug exc. Anyway:

Reviewed-by: Ricardo Koller <ricarkol@google.com>

> 
> With that:
> 
> Reviewed-by: Oliver Upton <oliver.upton@linux.dev>
> 
> --
> Thanks,
> Oliver
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 7/9] KVM: arm64: selftests: Add a test case for a linked breakpoint

[Ricardo Koller]

On Thu, Aug 25, 2022 at 06:29:34PM -0700, Reiji Watanabe wrote:
> On Wed, Aug 24, 2022 at 10:10 PM Reiji Watanabe <reijiw@google.com> wrote:
> >
> > Currently, the debug-exceptions test doesn't have a test case for
> > a linked breakpoint. Add a test case for the linked breakpoint to
> > the test.
> >
> > Signed-off-by: Reiji Watanabe <reijiw@google.com>
> >
> > ---
> >  .../selftests/kvm/aarch64/debug-exceptions.c  | 59 +++++++++++++++++--
> >  1 file changed, 55 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> > index ab8860e3a9fa..9fccfeebccd3 100644
> > --- a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> > +++ b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> > @@ -11,6 +11,10 @@
> >  #define DBGBCR_EXEC    (0x0 << 3)
> >  #define DBGBCR_EL1     (0x1 << 1)
> >  #define DBGBCR_E       (0x1 << 0)
> > +#define DBGBCR_LBN_SHIFT       16
> > +#define DBGBCR_BT_SHIFT                20
> > +#define DBGBCR_BT_ADDR_LINK_CTX        (0x1 << DBGBCR_BT_SHIFT)
> > +#define DBGBCR_BT_CTX_LINK     (0x3 << DBGBCR_BT_SHIFT)
> >
> >  #define DBGWCR_LEN8    (0xff << 5)
> >  #define DBGWCR_RD      (0x1 << 3)
> > @@ -21,7 +25,7 @@
> >  #define SPSR_D         (1 << 9)
> >  #define SPSR_SS                (1 << 21)
> >
> > -extern unsigned char sw_bp, sw_bp2, hw_bp, hw_bp2, bp_svc, bp_brk, hw_wp, ss_start;
> > +extern unsigned char sw_bp, sw_bp2, hw_bp, hw_bp2, bp_svc, bp_brk, hw_wp, ss_start, hw_bp_ctx;
> >  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;
> > @@ -103,6 +107,7 @@ static void reset_debug_state(void)
> >         isb();
> >
> >         write_sysreg(0, mdscr_el1);
> > +       write_sysreg(0, contextidr_el1);
> >
> >         /* Reset all bcr/bvr/wcr/wvr registers */
> >         dfr0 = read_sysreg(id_aa64dfr0_el1);
> > @@ -164,6 +169,28 @@ static void install_hw_bp(uint8_t bpn, uint64_t addr)
> >         enable_debug_bwp_exception();
> >  }
> >
> > +void install_hw_bp_ctx(uint8_t addr_bp, uint8_t ctx_bp, uint64_t addr,
> > +                      uint64_t ctx)
> > +{
> > +       uint32_t addr_bcr, ctx_bcr;
> > +
> > +       /* Setup a context-aware breakpoint */
> > +       ctx_bcr = DBGBCR_LEN8 | DBGBCR_EXEC | DBGBCR_EL1 | DBGBCR_E |
> > +                 DBGBCR_BT_CTX_LINK;
> > +       write_dbgbcr(ctx_bp, ctx_bcr);
> > +       write_dbgbvr(ctx_bp, ctx);
> > +
> > +       /* Setup a linked breakpoint (linked to the context-aware breakpoint) */
> > +       addr_bcr = DBGBCR_LEN8 | DBGBCR_EXEC | DBGBCR_EL1 | DBGBCR_E |
> > +                  DBGBCR_BT_ADDR_LINK_CTX |
> > +                  ((uint32_t)ctx_bp << DBGBCR_LBN_SHIFT);
> > +       write_dbgbcr(addr_bp, addr_bcr);
> > +       write_dbgbvr(addr_bp, addr);
> > +       isb();
> > +
> > +       enable_debug_bwp_exception();
> > +}
> > +
> >  static void install_ss(void)
> >  {
> >         uint32_t mdscr;
> > @@ -177,8 +204,10 @@ static void install_ss(void)
> >
> >  static volatile char write_data;
> >
> > -static void guest_code(uint8_t bpn, uint8_t wpn)
> > +static void guest_code(uint8_t bpn, uint8_t wpn, uint8_t ctx_bpn)
> >  {
> > +       uint64_t ctx = 0x1;     /* a random context number */
> > +
> >         GUEST_SYNC(0);
> >
> >         /* Software-breakpoint */
> > @@ -281,6 +310,19 @@ static void guest_code(uint8_t bpn, uint8_t wpn)
> >                      : : : "x0");
> >         GUEST_ASSERT_EQ(ss_addr[0], 0);
> >
> 
> I've just noticed that I should add GUEST_SYNC(10) here, use
> GUEST_SYNC(11) for the following test case, and update the
> stage limit value in the loop in userspace code.
> 
> Or I might consider removing the stage management code itself.
> It doesn't appear to be very useful to me, and I would think
> we could easily forget to update it :-)
> 
> Thank you,
> Reiji
>

Yes, it's better to remove it. The intention was to make sure the guest
generates the expected sequence of exits. In this case for example,
"1, .., 11, DONE" would be correct, but "1, .., 11, 12, DONE" would not.

> > +       /* Linked hardware-breakpoint */
> > +       hw_bp_addr = 0;
> > +       reset_debug_state();
> > +       install_hw_bp_ctx(bpn, ctx_bpn, PC(hw_bp_ctx), ctx);
> > +       /* Set context id */
> > +       write_sysreg(ctx, contextidr_el1);
> > +       isb();
> > +       asm volatile("hw_bp_ctx: nop");
> > +       write_sysreg(0, contextidr_el1);
> > +       GUEST_ASSERT_EQ(hw_bp_addr, PC(hw_bp_ctx));
> > +
> > +       GUEST_SYNC(10);
> > +
> >         GUEST_DONE();
> >  }
> >
> > @@ -327,6 +369,7 @@ int main(int argc, char *argv[])
> >         struct ucall uc;
> >         int stage;
> >         uint64_t aa64dfr0;
> > +       uint8_t brps;
> >
> >         vm = vm_create_with_one_vcpu(&vcpu, guest_code);
> >         ucall_init(vm, NULL);
> > @@ -349,8 +392,16 @@ int main(int argc, char *argv[])
> >         vm_install_sync_handler(vm, VECTOR_SYNC_CURRENT,
> >                                 ESR_EC_SVC64, guest_svc_handler);
> >
> > -       /* Run tests with breakpoint#0 and watchpoint#0. */
> > -       vcpu_args_set(vcpu, 2, 0, 0);
> > +       /* Number of breakpoints, minus 1 */
> > +       brps = cpuid_get_ufield(aa64dfr0, ID_AA64DFR0_BRPS_SHIFT);
> > +       __TEST_REQUIRE(brps > 0, "At least two breakpoints are required");
> > +
> > +       /*
> > +        * Run tests with breakpoint#0 and watchpoint#0, and the higiest
> > +        * numbered (context-aware) breakpoint.
> > +        */
> > +       vcpu_args_set(vcpu, 3, 0, 0, brps);
> > +
> >         for (stage = 0; stage < 11; stage++) {
> >                 vcpu_run(vcpu);
> >
> > --
> > 2.37.1.595.g718a3a8f04-goog
> >

Re: [PATCH 7/9] KVM: arm64: selftests: Add a test case for a linked breakpoint

[Ricardo Koller]

On Fri, Sep 09, 2022 at 01:18:28PM -0700, Ricardo Koller wrote:
> On Thu, Aug 25, 2022 at 06:29:34PM -0700, Reiji Watanabe wrote:
> > On Wed, Aug 24, 2022 at 10:10 PM Reiji Watanabe <reijiw@google.com> wrote:
> > >
> > > Currently, the debug-exceptions test doesn't have a test case for
> > > a linked breakpoint. Add a test case for the linked breakpoint to
> > > the test.
> > >
> > > Signed-off-by: Reiji Watanabe <reijiw@google.com>
> > >
> > > ---
> > >  .../selftests/kvm/aarch64/debug-exceptions.c  | 59 +++++++++++++++++--
> > >  1 file changed, 55 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> > > index ab8860e3a9fa..9fccfeebccd3 100644
> > > --- a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> > > +++ b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> > > @@ -11,6 +11,10 @@
> > >  #define DBGBCR_EXEC    (0x0 << 3)
> > >  #define DBGBCR_EL1     (0x1 << 1)
> > >  #define DBGBCR_E       (0x1 << 0)
> > > +#define DBGBCR_LBN_SHIFT       16
> > > +#define DBGBCR_BT_SHIFT                20
> > > +#define DBGBCR_BT_ADDR_LINK_CTX        (0x1 << DBGBCR_BT_SHIFT)
> > > +#define DBGBCR_BT_CTX_LINK     (0x3 << DBGBCR_BT_SHIFT)
> > >
> > >  #define DBGWCR_LEN8    (0xff << 5)
> > >  #define DBGWCR_RD      (0x1 << 3)
> > > @@ -21,7 +25,7 @@
> > >  #define SPSR_D         (1 << 9)
> > >  #define SPSR_SS                (1 << 21)
> > >
> > > -extern unsigned char sw_bp, sw_bp2, hw_bp, hw_bp2, bp_svc, bp_brk, hw_wp, ss_start;
> > > +extern unsigned char sw_bp, sw_bp2, hw_bp, hw_bp2, bp_svc, bp_brk, hw_wp, ss_start, hw_bp_ctx;
> > >  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;
> > > @@ -103,6 +107,7 @@ static void reset_debug_state(void)
> > >         isb();
> > >
> > >         write_sysreg(0, mdscr_el1);
> > > +       write_sysreg(0, contextidr_el1);
> > >
> > >         /* Reset all bcr/bvr/wcr/wvr registers */
> > >         dfr0 = read_sysreg(id_aa64dfr0_el1);
> > > @@ -164,6 +169,28 @@ static void install_hw_bp(uint8_t bpn, uint64_t addr)
> > >         enable_debug_bwp_exception();
> > >  }
> > >
> > > +void install_hw_bp_ctx(uint8_t addr_bp, uint8_t ctx_bp, uint64_t addr,
> > > +                      uint64_t ctx)
> > > +{
> > > +       uint32_t addr_bcr, ctx_bcr;
> > > +
> > > +       /* Setup a context-aware breakpoint */
> > > +       ctx_bcr = DBGBCR_LEN8 | DBGBCR_EXEC | DBGBCR_EL1 | DBGBCR_E |
> > > +                 DBGBCR_BT_CTX_LINK;
> > > +       write_dbgbcr(ctx_bp, ctx_bcr);
> > > +       write_dbgbvr(ctx_bp, ctx);
> > > +
> > > +       /* Setup a linked breakpoint (linked to the context-aware breakpoint) */
> > > +       addr_bcr = DBGBCR_LEN8 | DBGBCR_EXEC | DBGBCR_EL1 | DBGBCR_E |
> > > +                  DBGBCR_BT_ADDR_LINK_CTX |
> > > +                  ((uint32_t)ctx_bp << DBGBCR_LBN_SHIFT);
> > > +       write_dbgbcr(addr_bp, addr_bcr);
> > > +       write_dbgbvr(addr_bp, addr);
> > > +       isb();
> > > +
> > > +       enable_debug_bwp_exception();
> > > +}
> > > +
> > >  static void install_ss(void)
> > >  {
> > >         uint32_t mdscr;
> > > @@ -177,8 +204,10 @@ static void install_ss(void)
> > >
> > >  static volatile char write_data;
> > >
> > > -static void guest_code(uint8_t bpn, uint8_t wpn)
> > > +static void guest_code(uint8_t bpn, uint8_t wpn, uint8_t ctx_bpn)
> > >  {
> > > +       uint64_t ctx = 0x1;     /* a random context number */
> > > +
> > >         GUEST_SYNC(0);
> > >
> > >         /* Software-breakpoint */
> > > @@ -281,6 +310,19 @@ static void guest_code(uint8_t bpn, uint8_t wpn)
> > >                      : : : "x0");
> > >         GUEST_ASSERT_EQ(ss_addr[0], 0);
> > >
> > 
> > I've just noticed that I should add GUEST_SYNC(10) here, use
> > GUEST_SYNC(11) for the following test case, and update the
> > stage limit value in the loop in userspace code.
> > 
> > Or I might consider removing the stage management code itself.
> > It doesn't appear to be very useful to me, and I would think
> > we could easily forget to update it :-)
> > 
> > Thank you,
> > Reiji
> >
> 
> Yes, it's better to remove it. The intention was to make sure the guest
> generates the expected sequence of exits. In this case for example,
> "1, .., 11, DONE" would be correct, but "1, .., 11, 12, DONE" would not.

Sorry, the correct sequence should be "1, .., 10, DONE". And also, what
I meant to say is that *original* intention was to check that, which
wasn't actually completed as the incorrect sequence would also succeed.

> 
> > > +       /* Linked hardware-breakpoint */
> > > +       hw_bp_addr = 0;
> > > +       reset_debug_state();
> > > +       install_hw_bp_ctx(bpn, ctx_bpn, PC(hw_bp_ctx), ctx);
> > > +       /* Set context id */
> > > +       write_sysreg(ctx, contextidr_el1);
> > > +       isb();
> > > +       asm volatile("hw_bp_ctx: nop");
> > > +       write_sysreg(0, contextidr_el1);
> > > +       GUEST_ASSERT_EQ(hw_bp_addr, PC(hw_bp_ctx));
> > > +
> > > +       GUEST_SYNC(10);
> > > +
> > >         GUEST_DONE();
> > >  }
> > >
> > > @@ -327,6 +369,7 @@ int main(int argc, char *argv[])
> > >         struct ucall uc;
> > >         int stage;
> > >         uint64_t aa64dfr0;
> > > +       uint8_t brps;
> > >
> > >         vm = vm_create_with_one_vcpu(&vcpu, guest_code);
> > >         ucall_init(vm, NULL);
> > > @@ -349,8 +392,16 @@ int main(int argc, char *argv[])
> > >         vm_install_sync_handler(vm, VECTOR_SYNC_CURRENT,
> > >                                 ESR_EC_SVC64, guest_svc_handler);
> > >
> > > -       /* Run tests with breakpoint#0 and watchpoint#0. */
> > > -       vcpu_args_set(vcpu, 2, 0, 0);
> > > +       /* Number of breakpoints, minus 1 */
> > > +       brps = cpuid_get_ufield(aa64dfr0, ID_AA64DFR0_BRPS_SHIFT);
> > > +       __TEST_REQUIRE(brps > 0, "At least two breakpoints are required");
> > > +
> > > +       /*
> > > +        * Run tests with breakpoint#0 and watchpoint#0, and the higiest
> > > +        * numbered (context-aware) breakpoint.
> > > +        */
> > > +       vcpu_args_set(vcpu, 3, 0, 0, brps);
> > > +
> > >         for (stage = 0; stage < 11; stage++) {
> > >                 vcpu_run(vcpu);
> > >
> > > --
> > > 2.37.1.595.g718a3a8f04-goog
> > >

Re: [PATCH 7/9] KVM: arm64: selftests: Add a test case for a linked breakpoint

[Ricardo Koller]

On Wed, Aug 24, 2022 at 10:08:44PM -0700, Reiji Watanabe wrote:
> Currently, the debug-exceptions test doesn't have a test case for
> a linked breakpoint. Add a test case for the linked breakpoint to
> the test.

I would add some more detail, like the fact that this is a pair of
breakpoints: one is a context-aware breakpoint, and the other one
is an address breakpoint linked to the first one.

> 
> Signed-off-by: Reiji Watanabe <reijiw@google.com>
> 
> ---
>  .../selftests/kvm/aarch64/debug-exceptions.c  | 59 +++++++++++++++++--
>  1 file changed, 55 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> index ab8860e3a9fa..9fccfeebccd3 100644
> --- a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> +++ b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> @@ -11,6 +11,10 @@
>  #define DBGBCR_EXEC	(0x0 << 3)
>  #define DBGBCR_EL1	(0x1 << 1)
>  #define DBGBCR_E	(0x1 << 0)
> +#define DBGBCR_LBN_SHIFT	16
> +#define DBGBCR_BT_SHIFT		20
> +#define DBGBCR_BT_ADDR_LINK_CTX	(0x1 << DBGBCR_BT_SHIFT)
> +#define DBGBCR_BT_CTX_LINK	(0x3 << DBGBCR_BT_SHIFT)
>  
>  #define DBGWCR_LEN8	(0xff << 5)
>  #define DBGWCR_RD	(0x1 << 3)
> @@ -21,7 +25,7 @@
>  #define SPSR_D		(1 << 9)
>  #define SPSR_SS		(1 << 21)
>  
> -extern unsigned char sw_bp, sw_bp2, hw_bp, hw_bp2, bp_svc, bp_brk, hw_wp, ss_start;
> +extern unsigned char sw_bp, sw_bp2, hw_bp, hw_bp2, bp_svc, bp_brk, hw_wp, ss_start, hw_bp_ctx;
>  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;
> @@ -103,6 +107,7 @@ static void reset_debug_state(void)
>  	isb();
>  
>  	write_sysreg(0, mdscr_el1);
> +	write_sysreg(0, contextidr_el1);
>  
>  	/* Reset all bcr/bvr/wcr/wvr registers */
>  	dfr0 = read_sysreg(id_aa64dfr0_el1);
> @@ -164,6 +169,28 @@ static void install_hw_bp(uint8_t bpn, uint64_t addr)
>  	enable_debug_bwp_exception();
>  }
>  
> +void install_hw_bp_ctx(uint8_t addr_bp, uint8_t ctx_bp, uint64_t addr,
> +		       uint64_t ctx)
> +{
> +	uint32_t addr_bcr, ctx_bcr;
> +
> +	/* Setup a context-aware breakpoint */
> +	ctx_bcr = DBGBCR_LEN8 | DBGBCR_EXEC | DBGBCR_EL1 | DBGBCR_E |
> +		  DBGBCR_BT_CTX_LINK;
                               ^^^^^
                          isn't this a regular context-aware breakpoint?
			  the other one is the linked one.

> +	write_dbgbcr(ctx_bp, ctx_bcr);
> +	write_dbgbvr(ctx_bp, ctx);
> +
> +	/* Setup a linked breakpoint (linked to the context-aware breakpoint) */
> +	addr_bcr = DBGBCR_LEN8 | DBGBCR_EXEC | DBGBCR_EL1 | DBGBCR_E |
> +		   DBGBCR_BT_ADDR_LINK_CTX |
> +		   ((uint32_t)ctx_bp << DBGBCR_LBN_SHIFT);

Just a curiosity, can the context-aware one link to this one?

> +	write_dbgbcr(addr_bp, addr_bcr);
> +	write_dbgbvr(addr_bp, addr);
> +	isb();
> +
> +	enable_debug_bwp_exception();
> +}
> +
>  static void install_ss(void)
>  {
>  	uint32_t mdscr;
> @@ -177,8 +204,10 @@ static void install_ss(void)
>  
>  static volatile char write_data;
>  
> -static void guest_code(uint8_t bpn, uint8_t wpn)
> +static void guest_code(uint8_t bpn, uint8_t wpn, uint8_t ctx_bpn)
>  {
> +	uint64_t ctx = 0x1;	/* a random context number */

nit: make this number a bit more unlikely to happen by mistake.
I guess you could use all available 32 bits.

> +
>  	GUEST_SYNC(0);
>  
>  	/* Software-breakpoint */
> @@ -281,6 +310,19 @@ static void guest_code(uint8_t bpn, uint8_t wpn)
>  		     : : : "x0");
>  	GUEST_ASSERT_EQ(ss_addr[0], 0);
>  
> +	/* Linked hardware-breakpoint */
> +	hw_bp_addr = 0;
> +	reset_debug_state();
> +	install_hw_bp_ctx(bpn, ctx_bpn, PC(hw_bp_ctx), ctx);
> +	/* Set context id */
> +	write_sysreg(ctx, contextidr_el1);
> +	isb();
> +	asm volatile("hw_bp_ctx: nop");
> +	write_sysreg(0, contextidr_el1);
> +	GUEST_ASSERT_EQ(hw_bp_addr, PC(hw_bp_ctx));
> +
> +	GUEST_SYNC(10);
> +
>  	GUEST_DONE();
>  }
>  
> @@ -327,6 +369,7 @@ int main(int argc, char *argv[])
>  	struct ucall uc;
>  	int stage;
>  	uint64_t aa64dfr0;
> +	uint8_t brps;
>  
>  	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
>  	ucall_init(vm, NULL);
> @@ -349,8 +392,16 @@ int main(int argc, char *argv[])
>  	vm_install_sync_handler(vm, VECTOR_SYNC_CURRENT,
>  				ESR_EC_SVC64, guest_svc_handler);
>  
> -	/* Run tests with breakpoint#0 and watchpoint#0. */
> -	vcpu_args_set(vcpu, 2, 0, 0);
> +	/* Number of breakpoints, minus 1 */
> +	brps = cpuid_get_ufield(aa64dfr0, ID_AA64DFR0_BRPS_SHIFT);

If brps is "number of breakpoints", then there should be a "+ 1" above.
Otherwise brps is really "last breakpoint" (last_brp).

> +	__TEST_REQUIRE(brps > 0, "At least two breakpoints are required");

Yes, based on this test, brps is really "last breakpoint". I would
suggest changing the name to "last_brp" (or something similar).

> +
> +	/*
> +	 * Run tests with breakpoint#0 and watchpoint#0, and the higiest

	 * Run tests with breakpoint#0, watchpoint#0, and the highest
	
> +	 * numbered (context-aware) breakpoint.
> +	 */
> +	vcpu_args_set(vcpu, 3, 0, 0, brps);
> +
>  	for (stage = 0; stage < 11; stage++) {
>  		vcpu_run(vcpu);
>  
> -- 
> 2.37.1.595.g718a3a8f04-goog
> 

Re: [PATCH 3/9] KVM: arm64: selftests: Remove the hard-coded {b,w}pn#0 from debug-exceptions

[Reiji Watanabe]

Hi Ricardo,

On Fri, Sep 9, 2022 at 12:46 PM Ricardo Koller <ricarkol@google.com> wrote:
>
> On Wed, Aug 24, 2022 at 10:08:40PM -0700, Reiji Watanabe wrote:
> > Remove the hard-coded {break,watch}point #0 from the guest_code()
> > in debug-exceptions to allow {break,watch}point number to be
> > specified.  Subsequent patches will add test cases for non-zero
> > {break,watch}points.
> >
> > Signed-off-by: Reiji Watanabe <reijiw@google.com>
> > ---
> >  .../selftests/kvm/aarch64/debug-exceptions.c  | 50 ++++++++++++-------
> >  1 file changed, 32 insertions(+), 18 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> > index 51047e6b8db3..183ee16acb7d 100644
> > --- a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> > +++ b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> > @@ -93,6 +93,9 @@ GEN_DEBUG_WRITE_REG(dbgwvr)
> >
> >  static void reset_debug_state(void)
> >  {
> > +     uint8_t brps, wrps, i;
> > +     uint64_t dfr0;
> > +
> >       asm volatile("msr daifset, #8");
> >
> >       write_sysreg(0, osdlr_el1);
> > @@ -100,11 +103,20 @@ static void reset_debug_state(void)
> >       isb();
> >
> >       write_sysreg(0, mdscr_el1);
> > -     /* This test only uses the first bp and wp slot. */
> > -     write_sysreg(0, dbgbvr0_el1);
> > -     write_sysreg(0, dbgbcr0_el1);
> > -     write_sysreg(0, dbgwcr0_el1);
> > -     write_sysreg(0, dbgwvr0_el1);
> > +
> > +     /* Reset all bcr/bvr/wcr/wvr registers */
> > +     dfr0 = read_sysreg(id_aa64dfr0_el1);
> > +     brps = cpuid_get_ufield(dfr0, ID_AA64DFR0_BRPS_SHIFT);
>
> I guess this might have to change to ARM64_FEATURE_GET(). In any case:
>
> Reviewed-by: Ricardo Koller <ricarkol@google.com>
>
> (also assuming it includes the commit message clarification about reset
> clearing all registers).

Yes, I will fix those in V2.

Thank you for the review!
Reiji

Re: [PATCH 7/9] KVM: arm64: selftests: Add a test case for a linked breakpoint

[Reiji Watanabe]

Hi Ricardo,

Thank you for the review!

On Fri, Sep 9, 2022 at 2:01 PM Ricardo Koller <ricarkol@google.com> wrote:
>
> On Wed, Aug 24, 2022 at 10:08:44PM -0700, Reiji Watanabe wrote:
> > Currently, the debug-exceptions test doesn't have a test case for
> > a linked breakpoint. Add a test case for the linked breakpoint to
> > the test.
>
> I would add some more detail, like the fact that this is a pair of
> breakpoints: one is a context-aware breakpoint, and the other one
> is an address breakpoint linked to the first one.

Sure, I would add more detail.

>
> >
> > Signed-off-by: Reiji Watanabe <reijiw@google.com>
> >
> > ---
> >  .../selftests/kvm/aarch64/debug-exceptions.c  | 59 +++++++++++++++++--
> >  1 file changed, 55 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> > index ab8860e3a9fa..9fccfeebccd3 100644
> > --- a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> > +++ b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> > @@ -11,6 +11,10 @@
> >  #define DBGBCR_EXEC  (0x0 << 3)
> >  #define DBGBCR_EL1   (0x1 << 1)
> >  #define DBGBCR_E     (0x1 << 0)
> > +#define DBGBCR_LBN_SHIFT     16
> > +#define DBGBCR_BT_SHIFT              20
> > +#define DBGBCR_BT_ADDR_LINK_CTX      (0x1 << DBGBCR_BT_SHIFT)
> > +#define DBGBCR_BT_CTX_LINK   (0x3 << DBGBCR_BT_SHIFT)
> >
> >  #define DBGWCR_LEN8  (0xff << 5)
> >  #define DBGWCR_RD    (0x1 << 3)
> > @@ -21,7 +25,7 @@
> >  #define SPSR_D               (1 << 9)
> >  #define SPSR_SS              (1 << 21)
> >
> > -extern unsigned char sw_bp, sw_bp2, hw_bp, hw_bp2, bp_svc, bp_brk, hw_wp, ss_start;
> > +extern unsigned char sw_bp, sw_bp2, hw_bp, hw_bp2, bp_svc, bp_brk, hw_wp, ss_start, hw_bp_ctx;
> >  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;
> > @@ -103,6 +107,7 @@ static void reset_debug_state(void)
> >       isb();
> >
> >       write_sysreg(0, mdscr_el1);
> > +     write_sysreg(0, contextidr_el1);
> >
> >       /* Reset all bcr/bvr/wcr/wvr registers */
> >       dfr0 = read_sysreg(id_aa64dfr0_el1);
> > @@ -164,6 +169,28 @@ static void install_hw_bp(uint8_t bpn, uint64_t addr)
> >       enable_debug_bwp_exception();
> >  }
> >
> > +void install_hw_bp_ctx(uint8_t addr_bp, uint8_t ctx_bp, uint64_t addr,
> > +                    uint64_t ctx)
> > +{
> > +     uint32_t addr_bcr, ctx_bcr;
> > +
> > +     /* Setup a context-aware breakpoint */
> > +     ctx_bcr = DBGBCR_LEN8 | DBGBCR_EXEC | DBGBCR_EL1 | DBGBCR_E |
> > +               DBGBCR_BT_CTX_LINK;
>                                ^^^^^
>                           isn't this a regular context-aware breakpoint?
>                           the other one is the linked one.

That is one of the types that we could use only for context-aware
breakpoints (Linked Context ID Match breakpoint).  I should probably
have stated we use Linked Context ID Match breakpoint for the
context-aware breakpoint ?


>
> > +     write_dbgbcr(ctx_bp, ctx_bcr);
> > +     write_dbgbvr(ctx_bp, ctx);
> > +
> > +     /* Setup a linked breakpoint (linked to the context-aware breakpoint) */
> > +     addr_bcr = DBGBCR_LEN8 | DBGBCR_EXEC | DBGBCR_EL1 | DBGBCR_E |
> > +                DBGBCR_BT_ADDR_LINK_CTX |
> > +                ((uint32_t)ctx_bp << DBGBCR_LBN_SHIFT);
>
> Just a curiosity, can the context-aware one link to this one?

No, it can't (LBN field for the Context breakpoint is ignored).

>
> > +     write_dbgbcr(addr_bp, addr_bcr);
> > +     write_dbgbvr(addr_bp, addr);
> > +     isb();
> > +
> > +     enable_debug_bwp_exception();
> > +}
> > +
> >  static void install_ss(void)
> >  {
> >       uint32_t mdscr;
> > @@ -177,8 +204,10 @@ static void install_ss(void)
> >
> >  static volatile char write_data;
> >
> > -static void guest_code(uint8_t bpn, uint8_t wpn)
> > +static void guest_code(uint8_t bpn, uint8_t wpn, uint8_t ctx_bpn)
> >  {
> > +     uint64_t ctx = 0x1;     /* a random context number */
>
> nit: make this number a bit more unlikely to happen by mistake.
> I guess you could use all available 32 bits.

Sure, I could change it to some different number.


>
> > +
> >       GUEST_SYNC(0);
> >
> >       /* Software-breakpoint */
> > @@ -281,6 +310,19 @@ static void guest_code(uint8_t bpn, uint8_t wpn)
> >                    : : : "x0");
> >       GUEST_ASSERT_EQ(ss_addr[0], 0);
> >
> > +     /* Linked hardware-breakpoint */
> > +     hw_bp_addr = 0;
> > +     reset_debug_state();
> > +     install_hw_bp_ctx(bpn, ctx_bpn, PC(hw_bp_ctx), ctx);
> > +     /* Set context id */
> > +     write_sysreg(ctx, contextidr_el1);
> > +     isb();
> > +     asm volatile("hw_bp_ctx: nop");
> > +     write_sysreg(0, contextidr_el1);
> > +     GUEST_ASSERT_EQ(hw_bp_addr, PC(hw_bp_ctx));
> > +
> > +     GUEST_SYNC(10);
> > +
> >       GUEST_DONE();
> >  }
> >
> > @@ -327,6 +369,7 @@ int main(int argc, char *argv[])
> >       struct ucall uc;
> >       int stage;
> >       uint64_t aa64dfr0;
> > +     uint8_t brps;
> >
> >       vm = vm_create_with_one_vcpu(&vcpu, guest_code);
> >       ucall_init(vm, NULL);
> > @@ -349,8 +392,16 @@ int main(int argc, char *argv[])
> >       vm_install_sync_handler(vm, VECTOR_SYNC_CURRENT,
> >                               ESR_EC_SVC64, guest_svc_handler);
> >
> > -     /* Run tests with breakpoint#0 and watchpoint#0. */
> > -     vcpu_args_set(vcpu, 2, 0, 0);
> > +     /* Number of breakpoints, minus 1 */
> > +     brps = cpuid_get_ufield(aa64dfr0, ID_AA64DFR0_BRPS_SHIFT);
>
> If brps is "number of breakpoints", then there should be a "+ 1" above.
> Otherwise brps is really "last breakpoint" (last_brp).
>
> > +     __TEST_REQUIRE(brps > 0, "At least two breakpoints are required");
>
> Yes, based on this test, brps is really "last breakpoint". I would
> suggest changing the name to "last_brp" (or something similar).

The 'brps' I meant is simply 'BRPS' field value of ID_AA64DFR0_EL1.
I agree that it could be misleading.

The following patches use xxx_num for the number of watch/break points.
So, I am thinking of changing it brp_num to indicate the number of
breakpoints (and add 1).


> > +
> > +     /*
> > +      * Run tests with breakpoint#0 and watchpoint#0, and the higiest
>
>          * Run tests with breakpoint#0, watchpoint#0, and the highest

Will fix this.

Thank you,
Reiji

Re: [PATCH 7/9] KVM: arm64: selftests: Add a test case for a linked breakpoint

[Reiji Watanabe]

Hi Ricardo,

> > > > -static void guest_code(uint8_t bpn, uint8_t wpn)
> > > > +static void guest_code(uint8_t bpn, uint8_t wpn, uint8_t ctx_bpn)
> > > >  {
> > > > +       uint64_t ctx = 0x1;     /* a random context number */
> > > > +
> > > >         GUEST_SYNC(0);
> > > >
> > > >         /* Software-breakpoint */
> > > > @@ -281,6 +310,19 @@ static void guest_code(uint8_t bpn, uint8_t wpn)
> > > >                      : : : "x0");
> > > >         GUEST_ASSERT_EQ(ss_addr[0], 0);
> > > >
> > >
> > > I've just noticed that I should add GUEST_SYNC(10) here, use
> > > GUEST_SYNC(11) for the following test case, and update the
> > > stage limit value in the loop in userspace code.
> > >
> > > Or I might consider removing the stage management code itself.
> > > It doesn't appear to be very useful to me, and I would think
> > > we could easily forget to update it :-)
> > >
> > > Thank you,
> > > Reiji
> > >
> >
> > Yes, it's better to remove it. The intention was to make sure the guest
> > generates the expected sequence of exits. In this case for example,
> > "1, .., 11, DONE" would be correct, but "1, .., 11, 12, DONE" would not.
>
> Sorry, the correct sequence should be "1, .., 10, DONE". And also, what
> I meant to say is that *original* intention was to check that, which
> wasn't actually completed as the incorrect sequence would also succeed.

Thank you for the comments and explaining the original intention.
I will remove that.

Thank you,
Reiji