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

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

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