[PATCH v1 0/4] fixes for kvm/arm64 guest debug

[PATCH v1 0/4] fixes for kvm/arm64 guest debug

[Alex Bennée]

Hi,

While at Connect I had a go at reproducing a problem that Ard had
reported to me about guest debug not working. I managed to replicate
it and uncovered a few other nits in the process. While upgrading to a
tip-of-tree gdb to get around:

  https://sourceware.org/bugzilla/show_bug.cgi?id=23127

works we certainly can be better at handling the DBGBVR bits in QEMU.
That fixed the additional BQL assertion and target_el assertion
failures are harder to trigger (you need to debug your debug while
debugging debug). However I've included the obvious fixes for that as
well.

Alex Bennée (4):
  target/arm64: properly handle DBGVR RESS bits
  target/arm64: hold BQL when calling do_interrupt()
  target/arm64: kvm debug set target_el when passing exception to guest
  tests/guest-debug: fix scoping of failcount

 target/arm/kvm64.c                | 15 ++++++++++++++-
 tests/guest-debug/test-gdbstub.py |  1 +
 2 files changed, 15 insertions(+), 1 deletion(-)

-- 
2.17.1

[PATCH v1 1/4] target/arm64: properly handle DBGVR RESS bits

[Alex Bennée]

This only fails with some (broken) versions of gdb but we should
treat the top bits of DBGBVR as RESS. As the hardware may have IMPDEF
approaches to writes to this register we apply the sign extension when
checking breakpoints.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/arm/kvm64.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index e0b8246283..80ad07ed0c 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -356,13 +356,23 @@ bool kvm_arm_hw_debug_active(CPUState *cs)
     return ((cur_hw_wps > 0) || (cur_hw_bps > 0));
 }
 
+/*
+ * We shouldn't rely on gdb correctly setting the top bits of DBGBVR
+ * and the HW lists the top bits a RESS - sign-extending the top bit
+ * of the VA address. As it is IMPDEF if the write is either a sign
+ * extension or kept as is we might fix it up before we compare with
+ * the correctly reported and sign extended address.
+ */
+
 static bool find_hw_breakpoint(CPUState *cpu, target_ulong pc)
 {
     int i;
 
     for (i = 0; i < cur_hw_bps; i++) {
         HWBreakpoint *bp = get_hw_bp(i);
-        if (bp->bvr == pc) {
+        target_ulong bvr = bp->bvr;
+        bvr |= extract64(bvr, 52, 1) ? MAKE_64BIT_MASK(53, 11) : 0;
+        if (bvr == pc) {
             return true;
         }
     }
-- 
2.17.1

Re: [PATCH v1 1/4] target/arm64: properly handle DBGVR RESS bits

[Peter Maydell]

On 26 September 2018 at 12:20, Alex Bennée <alex.bennee@linaro.org> wrote:
> This only fails with some (broken) versions of gdb but we should
> treat the top bits of DBGBVR as RESS. As the hardware may have IMPDEF
> approaches to writes to this register we apply the sign extension when
> checking breakpoints.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  target/arm/kvm64.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> index e0b8246283..80ad07ed0c 100644
> --- a/target/arm/kvm64.c
> +++ b/target/arm/kvm64.c
> @@ -356,13 +356,23 @@ bool kvm_arm_hw_debug_active(CPUState *cs)
>      return ((cur_hw_wps > 0) || (cur_hw_bps > 0));
>  }
>
> +/*
> + * We shouldn't rely on gdb correctly setting the top bits of DBGBVR
> + * and the HW lists the top bits a RESS - sign-extending the top bit
> + * of the VA address. As it is IMPDEF if the write is either a sign
> + * extension or kept as is we might fix it up before we compare with
> + * the correctly reported and sign extended address.
> + */
> +
>  static bool find_hw_breakpoint(CPUState *cpu, target_ulong pc)
>  {
>      int i;
>
>      for (i = 0; i < cur_hw_bps; i++) {
>          HWBreakpoint *bp = get_hw_bp(i);
> -        if (bp->bvr == pc) {
> +        target_ulong bvr = bp->bvr;
> +        bvr |= extract64(bvr, 52, 1) ? MAKE_64BIT_MASK(53, 11) : 0;
> +        if (bvr == pc) {
>              return true;
>          }
>      }

Shouldn't we be sanitizing the addresses we get from gdb
before we put them into the hardware watchpoint registers,
rather than doing the sign extension when we read the registers?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v1 1/4] target/arm64: properly handle DBGVR RESS bits

[Peter Maydell]

On 26 September 2018 at 12:20, Alex Bennée <alex.bennee@linaro.org> wrote:
> This only fails with some (broken) versions of gdb but we should
> treat the top bits of DBGBVR as RESS. As the hardware may have IMPDEF
> approaches to writes to this register we apply the sign extension when
> checking breakpoints.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  target/arm/kvm64.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> index e0b8246283..80ad07ed0c 100644
> --- a/target/arm/kvm64.c
> +++ b/target/arm/kvm64.c
> @@ -356,13 +356,23 @@ bool kvm_arm_hw_debug_active(CPUState *cs)
>      return ((cur_hw_wps > 0) || (cur_hw_bps > 0));
>  }
>
> +/*
> + * We shouldn't rely on gdb correctly setting the top bits of DBGBVR
> + * and the HW lists the top bits a RESS - sign-extending the top bit
> + * of the VA address. As it is IMPDEF if the write is either a sign
> + * extension or kept as is we might fix it up before we compare with
> + * the correctly reported and sign extended address.
> + */
> +
>  static bool find_hw_breakpoint(CPUState *cpu, target_ulong pc)
>  {
>      int i;
>
>      for (i = 0; i < cur_hw_bps; i++) {
>          HWBreakpoint *bp = get_hw_bp(i);
> -        if (bp->bvr == pc) {
> +        target_ulong bvr = bp->bvr;
> +        bvr |= extract64(bvr, 52, 1) ? MAKE_64BIT_MASK(53, 11) : 0;
> +        if (bvr == pc) {
>              return true;
>          }
>      }

Shouldn't we be sanitizing the addresses we get from gdb
before we put them into the hardware watchpoint registers,
rather than doing the sign extension when we read the registers?

thanks
-- PMM

Re: [PATCH v1 1/4] target/arm64: properly handle DBGVR RESS bits

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 26 September 2018 at 12:20, Alex Bennée <alex.bennee@linaro.org> wrote:
>> This only fails with some (broken) versions of gdb but we should
>> treat the top bits of DBGBVR as RESS. As the hardware may have IMPDEF
>> approaches to writes to this register we apply the sign extension when
>> checking breakpoints.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  target/arm/kvm64.c | 12 +++++++++++-
>>  1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
>> index e0b8246283..80ad07ed0c 100644
>> --- a/target/arm/kvm64.c
>> +++ b/target/arm/kvm64.c
>> @@ -356,13 +356,23 @@ bool kvm_arm_hw_debug_active(CPUState *cs)
>>      return ((cur_hw_wps > 0) || (cur_hw_bps > 0));
>>  }
>>
>> +/*
>> + * We shouldn't rely on gdb correctly setting the top bits of DBGBVR
>> + * and the HW lists the top bits a RESS - sign-extending the top bit
>> + * of the VA address. As it is IMPDEF if the write is either a sign
>> + * extension or kept as is we might fix it up before we compare with
>> + * the correctly reported and sign extended address.
>> + */
>> +
>>  static bool find_hw_breakpoint(CPUState *cpu, target_ulong pc)
>>  {
>>      int i;
>>
>>      for (i = 0; i < cur_hw_bps; i++) {
>>          HWBreakpoint *bp = get_hw_bp(i);
>> -        if (bp->bvr == pc) {
>> +        target_ulong bvr = bp->bvr;
>> +        bvr |= extract64(bvr, 52, 1) ? MAKE_64BIT_MASK(53, 11) : 0;
>> +        if (bvr == pc) {
>>              return true;
>>          }
>>      }
>
> Shouldn't we be sanitizing the addresses we get from gdb
> before we put them into the hardware watchpoint registers,
> rather than doing the sign extension when we read the registers?

I guess that works too. I'll switch it around.

--
Alex Bennée

Re: [Qemu-devel] [PATCH v1 1/4] target/arm64: properly handle DBGVR RESS bits

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 26 September 2018 at 12:20, Alex Bennée <alex.bennee@linaro.org> wrote:
>> This only fails with some (broken) versions of gdb but we should
>> treat the top bits of DBGBVR as RESS. As the hardware may have IMPDEF
>> approaches to writes to this register we apply the sign extension when
>> checking breakpoints.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  target/arm/kvm64.c | 12 +++++++++++-
>>  1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
>> index e0b8246283..80ad07ed0c 100644
>> --- a/target/arm/kvm64.c
>> +++ b/target/arm/kvm64.c
>> @@ -356,13 +356,23 @@ bool kvm_arm_hw_debug_active(CPUState *cs)
>>      return ((cur_hw_wps > 0) || (cur_hw_bps > 0));
>>  }
>>
>> +/*
>> + * We shouldn't rely on gdb correctly setting the top bits of DBGBVR
>> + * and the HW lists the top bits a RESS - sign-extending the top bit
>> + * of the VA address. As it is IMPDEF if the write is either a sign
>> + * extension or kept as is we might fix it up before we compare with
>> + * the correctly reported and sign extended address.
>> + */
>> +
>>  static bool find_hw_breakpoint(CPUState *cpu, target_ulong pc)
>>  {
>>      int i;
>>
>>      for (i = 0; i < cur_hw_bps; i++) {
>>          HWBreakpoint *bp = get_hw_bp(i);
>> -        if (bp->bvr == pc) {
>> +        target_ulong bvr = bp->bvr;
>> +        bvr |= extract64(bvr, 52, 1) ? MAKE_64BIT_MASK(53, 11) : 0;
>> +        if (bvr == pc) {
>>              return true;
>>          }
>>      }
>
> Shouldn't we be sanitizing the addresses we get from gdb
> before we put them into the hardware watchpoint registers,
> rather than doing the sign extension when we read the registers?

I guess that works too. I'll switch it around.

--
Alex Bennée

[PATCH v1 2/4] target/arm64: hold BQL when calling do_interrupt()

[Alex Bennée]

Fix the assertion failure when running interrupts.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/arm/kvm64.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index 80ad07ed0c..346e1f1a73 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -984,7 +984,9 @@ bool kvm_arm_handle_debug(CPUState *cs, struct kvm_debug_exit_arch *debug_exit)
     cs->exception_index = EXCP_BKPT;
     env->exception.syndrome = debug_exit->hsr;
     env->exception.vaddress = debug_exit->far;
+    qemu_mutex_lock_iothread();
     cc->do_interrupt(cs);
+    qemu_mutex_unlock_iothread();
 
     return false;
 }
-- 
2.17.1

Re: [PATCH v1 2/4] target/arm64: hold BQL when calling do_interrupt()

[Peter Maydell]

On 26 September 2018 at 12:20, Alex Bennée <alex.bennee@linaro.org> wrote:
> Fix the assertion failure when running interrupts.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  target/arm/kvm64.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> index 80ad07ed0c..346e1f1a73 100644
> --- a/target/arm/kvm64.c
> +++ b/target/arm/kvm64.c
> @@ -984,7 +984,9 @@ bool kvm_arm_handle_debug(CPUState *cs, struct kvm_debug_exit_arch *debug_exit)
>      cs->exception_index = EXCP_BKPT;
>      env->exception.syndrome = debug_exit->hsr;
>      env->exception.vaddress = debug_exit->far;
> +    qemu_mutex_lock_iothread();
>      cc->do_interrupt(cs);
> +    qemu_mutex_unlock_iothread();
>
>      return false;
>  }
> --
> 2.17.1

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v1 2/4] target/arm64: hold BQL when calling do_interrupt()

[Peter Maydell]

On 26 September 2018 at 12:20, Alex Bennée <alex.bennee@linaro.org> wrote:
> Fix the assertion failure when running interrupts.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  target/arm/kvm64.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> index 80ad07ed0c..346e1f1a73 100644
> --- a/target/arm/kvm64.c
> +++ b/target/arm/kvm64.c
> @@ -984,7 +984,9 @@ bool kvm_arm_handle_debug(CPUState *cs, struct kvm_debug_exit_arch *debug_exit)
>      cs->exception_index = EXCP_BKPT;
>      env->exception.syndrome = debug_exit->hsr;
>      env->exception.vaddress = debug_exit->far;
> +    qemu_mutex_lock_iothread();
>      cc->do_interrupt(cs);
> +    qemu_mutex_unlock_iothread();
>
>      return false;
>  }
> --
> 2.17.1

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH v1 3/4] target/arm64: kvm debug set target_el when passing exception to guest

[Alex Bennée]

When we are debugging the guest all exception come our way but might
be for the guests own debug exceptions. We use the ->do_interrupt()
infrastructure to do this however we are missing a full setup of the
exception structure causing an assert later down the line.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/arm/kvm64.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index 346e1f1a73..9ceff1884c 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -984,6 +984,7 @@ bool kvm_arm_handle_debug(CPUState *cs, struct kvm_debug_exit_arch *debug_exit)
     cs->exception_index = EXCP_BKPT;
     env->exception.syndrome = debug_exit->hsr;
     env->exception.vaddress = debug_exit->far;
+    env->exception.target_el = 1;
     qemu_mutex_lock_iothread();
     cc->do_interrupt(cs);
     qemu_mutex_unlock_iothread();
-- 
2.17.1

Re: [PATCH v1 3/4] target/arm64: kvm debug set target_el when passing exception to guest

[Peter Maydell]

On 26 September 2018 at 12:20, Alex Bennée <alex.bennee@linaro.org> wrote:
> When we are debugging the guest all exception come our way but might

"exceptions"

> be for the guests own debug exceptions. We use the ->do_interrupt()

"guest's"

> infrastructure to do this however we are missing a full setup of the

"to inject the exception into the guest. However, "

> exception structure causing an assert later down the line.

"structure, "

>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  target/arm/kvm64.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> index 346e1f1a73..9ceff1884c 100644
> --- a/target/arm/kvm64.c
> +++ b/target/arm/kvm64.c
> @@ -984,6 +984,7 @@ bool kvm_arm_handle_debug(CPUState *cs, struct kvm_debug_exit_arch *debug_exit)
>      cs->exception_index = EXCP_BKPT;
>      env->exception.syndrome = debug_exit->hsr;
>      env->exception.vaddress = debug_exit->far;
> +    env->exception.target_el = 1;
>      qemu_mutex_lock_iothread();
>      cc->do_interrupt(cs);
>      qemu_mutex_unlock_iothread();
> --
> 2.17.1
>


Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v1 3/4] target/arm64: kvm debug set target_el when passing exception to guest

[Peter Maydell]

On 26 September 2018 at 12:20, Alex Bennée <alex.bennee@linaro.org> wrote:
> When we are debugging the guest all exception come our way but might

"exceptions"

> be for the guests own debug exceptions. We use the ->do_interrupt()

"guest's"

> infrastructure to do this however we are missing a full setup of the

"to inject the exception into the guest. However, "

> exception structure causing an assert later down the line.

"structure, "

>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  target/arm/kvm64.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> index 346e1f1a73..9ceff1884c 100644
> --- a/target/arm/kvm64.c
> +++ b/target/arm/kvm64.c
> @@ -984,6 +984,7 @@ bool kvm_arm_handle_debug(CPUState *cs, struct kvm_debug_exit_arch *debug_exit)
>      cs->exception_index = EXCP_BKPT;
>      env->exception.syndrome = debug_exit->hsr;
>      env->exception.vaddress = debug_exit->far;
> +    env->exception.target_el = 1;
>      qemu_mutex_lock_iothread();
>      cc->do_interrupt(cs);
>      qemu_mutex_unlock_iothread();
> --
> 2.17.1
>


Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH v1 4/4] tests/guest-debug: fix scoping of failcount

[Alex Bennée]

You should declare you are using a global version of a variable before
you attempt to modify it in a function.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/guest-debug/test-gdbstub.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/guest-debug/test-gdbstub.py b/tests/guest-debug/test-gdbstub.py
index 474d2c5c65..7bfc95b187 100644
--- a/tests/guest-debug/test-gdbstub.py
+++ b/tests/guest-debug/test-gdbstub.py
@@ -16,6 +16,7 @@ def report(cond, msg):
         print ("PASS: %s" % (msg))
     else:
         print ("FAIL: %s" % (msg))
+        global failcount
         failcount += 1
 
 
-- 
2.17.1

Re: [Qemu-devel] [PATCH v1 4/4] tests/guest-debug: fix scoping of failcount

[Peter Maydell]

On 26 September 2018 at 12:20, Alex Bennée <alex.bennee@linaro.org> wrote:
> You should declare you are using a global version of a variable before
> you attempt to modify it in a function.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  tests/guest-debug/test-gdbstub.py | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/tests/guest-debug/test-gdbstub.py b/tests/guest-debug/test-gdbstub.py
> index 474d2c5c65..7bfc95b187 100644
> --- a/tests/guest-debug/test-gdbstub.py
> +++ b/tests/guest-debug/test-gdbstub.py
> @@ -16,6 +16,7 @@ def report(cond, msg):
>          print ("PASS: %s" % (msg))
>      else:
>          print ("FAIL: %s" % (msg))
> +        global failcount
>          failcount += 1


Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Incidentally, if we ever get above 127 tests in this file,
the "exit(failcount)" at the bottom of the script will
not DTRT :-)

thanks
-- PMM