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

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: [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

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

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

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