diff mbox series

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

Message ID 20220825050846.3418868-4-reijiw@google.com (mailing list archive)
State New, archived
Headers show
Series KVM: arm64: selftests: Test linked {break,watch}points | expand

Commit Message

Reiji Watanabe Aug. 25, 2022, 5:08 a.m. UTC 
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(-)

Comments

Oliver Upton Aug. 25, 2022, 4:55 p.m. UTC | #1 
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
Reiji Watanabe Aug. 26, 2022, 12:53 a.m. UTC | #2 
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
Ricardo Koller Sept. 9, 2022, 7:46 p.m. UTC | #3 
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
>
Reiji Watanabe Sept. 10, 2022, 4:15 a.m. UTC | #4 
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
diff mbox series

Patch

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