[PATCH v9 0/6] QEMU support for KVM Guest Debug on arm64

[PATCH v9 0/6] QEMU support for KVM Guest Debug on arm64

[Alex Bennée]

Hi,

Here is the latest patch set to support debugging of KVM guests on
arm64. The main changes since the last post are that the kernel
patches have now been mainlined. As the recent master has synced up
headers they are no longer needed for this series.

I've add a GDB Python based test into tests/guest-debug. However I
haven't plumbed it in to the "make check" machinery as we don't really
have a good solution for adding binaries for testing (especially in
TCG cases). This patch can be dropped if wanted and I'll just keep it
locally.

So in summary the changes are:

  - Kernel ABI now finalised and mainlined
  - Fixed the handling of watchpoints
  - Removed manual array fiddling in favour of g_array
  - Addressed most of Peter Maydell's review comments
  - The patch series is now checkpatch.pl clean

More detailed changelogs are attached to each patch.

GIT Repo:

The patch series is based off a recent master and can be found at:

https://github.com/stsquad/qemu
branch: kvm/guest-debug-v9


Alex Bennée (6):
  target-arm: kvm64 - introduce kvm_arm_init_debug()
  target-arm: kvm - implement software breakpoints
  target-arm: kvm - support for single step
  target-arm: kvm - add support for HW assisted debug
  target-arm: kvm - re-inject guest debug exceptions
  tests/guest-debug: introduce basic gdbstub tests

 target-arm/helper-a64.c           |  12 +-
 target-arm/kvm.c                  | 137 +++++++++++---
 target-arm/kvm64.c                | 366 ++++++++++++++++++++++++++++++++++++++
 target-arm/kvm_arm.h              |  38 ++++
 tests/guest-debug/test-gdbstub.py | 171 ++++++++++++++++++
 5 files changed, 696 insertions(+), 28 deletions(-)
 create mode 100644 tests/guest-debug/test-gdbstub.py

-- 
2.6.3

[PATCH v9 0/6] QEMU support for KVM Guest Debug on arm64

[Alex Bennée]

Hi,

Here is the latest patch set to support debugging of KVM guests on
arm64. The main changes since the last post are that the kernel
patches have now been mainlined. As the recent master has synced up
headers they are no longer needed for this series.

I've add a GDB Python based test into tests/guest-debug. However I
haven't plumbed it in to the "make check" machinery as we don't really
have a good solution for adding binaries for testing (especially in
TCG cases). This patch can be dropped if wanted and I'll just keep it
locally.

So in summary the changes are:

  - Kernel ABI now finalised and mainlined
  - Fixed the handling of watchpoints
  - Removed manual array fiddling in favour of g_array
  - Addressed most of Peter Maydell's review comments
  - The patch series is now checkpatch.pl clean

More detailed changelogs are attached to each patch.

GIT Repo:

The patch series is based off a recent master and can be found at:

https://github.com/stsquad/qemu
branch: kvm/guest-debug-v9


Alex Benn?e (6):
  target-arm: kvm64 - introduce kvm_arm_init_debug()
  target-arm: kvm - implement software breakpoints
  target-arm: kvm - support for single step
  target-arm: kvm - add support for HW assisted debug
  target-arm: kvm - re-inject guest debug exceptions
  tests/guest-debug: introduce basic gdbstub tests

 target-arm/helper-a64.c           |  12 +-
 target-arm/kvm.c                  | 137 +++++++++++---
 target-arm/kvm64.c                | 366 ++++++++++++++++++++++++++++++++++++++
 target-arm/kvm_arm.h              |  38 ++++
 tests/guest-debug/test-gdbstub.py | 171 ++++++++++++++++++
 5 files changed, 696 insertions(+), 28 deletions(-)
 create mode 100644 tests/guest-debug/test-gdbstub.py

-- 
2.6.3

[Qemu-devel] [PATCH v9 0/6] QEMU support for KVM Guest Debug on arm64

[Alex Bennée]

Hi,

Here is the latest patch set to support debugging of KVM guests on
arm64. The main changes since the last post are that the kernel
patches have now been mainlined. As the recent master has synced up
headers they are no longer needed for this series.

I've add a GDB Python based test into tests/guest-debug. However I
haven't plumbed it in to the "make check" machinery as we don't really
have a good solution for adding binaries for testing (especially in
TCG cases). This patch can be dropped if wanted and I'll just keep it
locally.

So in summary the changes are:

  - Kernel ABI now finalised and mainlined
  - Fixed the handling of watchpoints
  - Removed manual array fiddling in favour of g_array
  - Addressed most of Peter Maydell's review comments
  - The patch series is now checkpatch.pl clean

More detailed changelogs are attached to each patch.

GIT Repo:

The patch series is based off a recent master and can be found at:

https://github.com/stsquad/qemu
branch: kvm/guest-debug-v9


Alex Bennée (6):
  target-arm: kvm64 - introduce kvm_arm_init_debug()
  target-arm: kvm - implement software breakpoints
  target-arm: kvm - support for single step
  target-arm: kvm - add support for HW assisted debug
  target-arm: kvm - re-inject guest debug exceptions
  tests/guest-debug: introduce basic gdbstub tests

 target-arm/helper-a64.c           |  12 +-
 target-arm/kvm.c                  | 137 +++++++++++---
 target-arm/kvm64.c                | 366 ++++++++++++++++++++++++++++++++++++++
 target-arm/kvm_arm.h              |  38 ++++
 tests/guest-debug/test-gdbstub.py | 171 ++++++++++++++++++
 5 files changed, 696 insertions(+), 28 deletions(-)
 create mode 100644 tests/guest-debug/test-gdbstub.py

-- 
2.6.3

[PATCH v9 1/6] target-arm: kvm64 - introduce kvm_arm_init_debug()

[Alex Bennée]

As we haven't always had guest debug support we need to probe for it.
Additionally we don't do this in the start-up capability code so we
don't fall over on old kernels.

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

diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
index ceebfeb..d087794 100644
--- a/target-arm/kvm64.c
+++ b/target-arm/kvm64.c
@@ -25,6 +25,22 @@
 #include "internals.h"
 #include "hw/arm/arm.h"
 
+static bool have_guest_debug;
+
+/**
+ * kvm_arm_init_debug()
+ * @cs: CPUState
+ *
+ * Check for guest debug capabilities.
+ *
+ */
+static void kvm_arm_init_debug(CPUState *cs)
+{
+    have_guest_debug = kvm_check_extension(cs->kvm_state,
+                                           KVM_CAP_SET_GUEST_DEBUG);
+    return;
+}
+
 static inline void set_feature(uint64_t *features, int feature)
 {
     *features |= 1ULL << feature;
@@ -121,6 +137,8 @@ int kvm_arch_init_vcpu(CPUState *cs)
     }
     cpu->mp_affinity = mpidr & ARM64_AFFINITY_MASK;
 
+    kvm_arm_init_debug(cs);
+
     return kvm_arm_init_cpreg_list(cpu);
 }
 
-- 
2.6.3

[PATCH v9 1/6] target-arm: kvm64 - introduce kvm_arm_init_debug()

[Alex Bennée]

As we haven't always had guest debug support we need to probe for it.
Additionally we don't do this in the start-up capability code so we
don't fall over on old kernels.

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

diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
index ceebfeb..d087794 100644
--- a/target-arm/kvm64.c
+++ b/target-arm/kvm64.c
@@ -25,6 +25,22 @@
 #include "internals.h"
 #include "hw/arm/arm.h"
 
+static bool have_guest_debug;
+
+/**
+ * kvm_arm_init_debug()
+ * @cs: CPUState
+ *
+ * Check for guest debug capabilities.
+ *
+ */
+static void kvm_arm_init_debug(CPUState *cs)
+{
+    have_guest_debug = kvm_check_extension(cs->kvm_state,
+                                           KVM_CAP_SET_GUEST_DEBUG);
+    return;
+}
+
 static inline void set_feature(uint64_t *features, int feature)
 {
     *features |= 1ULL << feature;
@@ -121,6 +137,8 @@ int kvm_arch_init_vcpu(CPUState *cs)
     }
     cpu->mp_affinity = mpidr & ARM64_AFFINITY_MASK;
 
+    kvm_arm_init_debug(cs);
+
     return kvm_arm_init_cpreg_list(cpu);
 }
 
-- 
2.6.3

[Qemu-devel] [PATCH v9 1/6] target-arm: kvm64 - introduce kvm_arm_init_debug()

[Alex Bennée]

As we haven't always had guest debug support we need to probe for it.
Additionally we don't do this in the start-up capability code so we
don't fall over on old kernels.

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

diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
index ceebfeb..d087794 100644
--- a/target-arm/kvm64.c
+++ b/target-arm/kvm64.c
@@ -25,6 +25,22 @@
 #include "internals.h"
 #include "hw/arm/arm.h"
 
+static bool have_guest_debug;
+
+/**
+ * kvm_arm_init_debug()
+ * @cs: CPUState
+ *
+ * Check for guest debug capabilities.
+ *
+ */
+static void kvm_arm_init_debug(CPUState *cs)
+{
+    have_guest_debug = kvm_check_extension(cs->kvm_state,
+                                           KVM_CAP_SET_GUEST_DEBUG);
+    return;
+}
+
 static inline void set_feature(uint64_t *features, int feature)
 {
     *features |= 1ULL << feature;
@@ -121,6 +137,8 @@ int kvm_arch_init_vcpu(CPUState *cs)
     }
     cpu->mp_affinity = mpidr & ARM64_AFFINITY_MASK;
 
+    kvm_arm_init_debug(cs);
+
     return kvm_arm_init_cpreg_list(cpu);
 }
 
-- 
2.6.3

Re: [PATCH v9 1/6] target-arm: kvm64 - introduce kvm_arm_init_debug()

[Peter Maydell]

On 12 November 2015 at 16:20, Alex Bennée <alex.bennee@linaro.org> wrote:
> As we haven't always had guest debug support we need to probe for it.
> Additionally we don't do this in the start-up capability code so we
> don't fall over on old kernels.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  target-arm/kvm64.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
> index ceebfeb..d087794 100644
> --- a/target-arm/kvm64.c
> +++ b/target-arm/kvm64.c
> @@ -25,6 +25,22 @@
>  #include "internals.h"
>  #include "hw/arm/arm.h"
>
> +static bool have_guest_debug;
> +
> +/**
> + * kvm_arm_init_debug()
> + * @cs: CPUState
> + *
> + * Check for guest debug capabilities.
> + *
> + */
> +static void kvm_arm_init_debug(CPUState *cs)
> +{
> +    have_guest_debug = kvm_check_extension(cs->kvm_state,
> +                                           KVM_CAP_SET_GUEST_DEBUG);
> +    return;
> +}
> +
>  static inline void set_feature(uint64_t *features, int feature)
>  {
>      *features |= 1ULL << feature;
> @@ -121,6 +137,8 @@ int kvm_arch_init_vcpu(CPUState *cs)
>      }
>      cpu->mp_affinity = mpidr & ARM64_AFFINITY_MASK;
>
> +    kvm_arm_init_debug(cs);
> +
>      return kvm_arm_init_cpreg_list(cpu);
>  }

I assume in practice the kernel guarantees that either all
CPUs have the SET_GUEST_DEBUG cap, or none do :-)

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

thanks
-- PMM
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v9 1/6] target-arm: kvm64 - introduce kvm_arm_init_debug()

[Peter Maydell]

On 12 November 2015 at 16:20, Alex Benn?e <alex.bennee@linaro.org> wrote:
> As we haven't always had guest debug support we need to probe for it.
> Additionally we don't do this in the start-up capability code so we
> don't fall over on old kernels.
>
> Signed-off-by: Alex Benn?e <alex.bennee@linaro.org>
> ---
>  target-arm/kvm64.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
> index ceebfeb..d087794 100644
> --- a/target-arm/kvm64.c
> +++ b/target-arm/kvm64.c
> @@ -25,6 +25,22 @@
>  #include "internals.h"
>  #include "hw/arm/arm.h"
>
> +static bool have_guest_debug;
> +
> +/**
> + * kvm_arm_init_debug()
> + * @cs: CPUState
> + *
> + * Check for guest debug capabilities.
> + *
> + */
> +static void kvm_arm_init_debug(CPUState *cs)
> +{
> +    have_guest_debug = kvm_check_extension(cs->kvm_state,
> +                                           KVM_CAP_SET_GUEST_DEBUG);
> +    return;
> +}
> +
>  static inline void set_feature(uint64_t *features, int feature)
>  {
>      *features |= 1ULL << feature;
> @@ -121,6 +137,8 @@ int kvm_arch_init_vcpu(CPUState *cs)
>      }
>      cpu->mp_affinity = mpidr & ARM64_AFFINITY_MASK;
>
> +    kvm_arm_init_debug(cs);
> +
>      return kvm_arm_init_cpreg_list(cpu);
>  }

I assume in practice the kernel guarantees that either all
CPUs have the SET_GUEST_DEBUG cap, or none do :-)

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

thanks
-- PMM

Re: [Qemu-devel] [PATCH v9 1/6] target-arm: kvm64 - introduce kvm_arm_init_debug()

[Peter Maydell]

On 12 November 2015 at 16:20, Alex Bennée <alex.bennee@linaro.org> wrote:
> As we haven't always had guest debug support we need to probe for it.
> Additionally we don't do this in the start-up capability code so we
> don't fall over on old kernels.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  target-arm/kvm64.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
> index ceebfeb..d087794 100644
> --- a/target-arm/kvm64.c
> +++ b/target-arm/kvm64.c
> @@ -25,6 +25,22 @@
>  #include "internals.h"
>  #include "hw/arm/arm.h"
>
> +static bool have_guest_debug;
> +
> +/**
> + * kvm_arm_init_debug()
> + * @cs: CPUState
> + *
> + * Check for guest debug capabilities.
> + *
> + */
> +static void kvm_arm_init_debug(CPUState *cs)
> +{
> +    have_guest_debug = kvm_check_extension(cs->kvm_state,
> +                                           KVM_CAP_SET_GUEST_DEBUG);
> +    return;
> +}
> +
>  static inline void set_feature(uint64_t *features, int feature)
>  {
>      *features |= 1ULL << feature;
> @@ -121,6 +137,8 @@ int kvm_arch_init_vcpu(CPUState *cs)
>      }
>      cpu->mp_affinity = mpidr & ARM64_AFFINITY_MASK;
>
> +    kvm_arm_init_debug(cs);
> +
>      return kvm_arm_init_cpreg_list(cpu);
>  }

I assume in practice the kernel guarantees that either all
CPUs have the SET_GUEST_DEBUG cap, or none do :-)

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

thanks
-- PMM

Re: [PATCH v9 1/6] target-arm: kvm64 - introduce kvm_arm_init_debug()

[Peter Maydell]

On 20 November 2015 at 15:05, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 12 November 2015 at 16:20, Alex Bennée <alex.bennee@linaro.org> wrote:
>> As we haven't always had guest debug support we need to probe for it.
>> Additionally we don't do this in the start-up capability code so we
>> don't fall over on old kernels.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  target-arm/kvm64.c | 18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>>
>> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
>> index ceebfeb..d087794 100644
>> --- a/target-arm/kvm64.c
>> +++ b/target-arm/kvm64.c
>> @@ -25,6 +25,22 @@
>>  #include "internals.h"
>>  #include "hw/arm/arm.h"
>>
>> +static bool have_guest_debug;
>> +
>> +/**
>> + * kvm_arm_init_debug()
>> + * @cs: CPUState
>> + *
>> + * Check for guest debug capabilities.
>> + *
>> + */
>> +static void kvm_arm_init_debug(CPUState *cs)
>> +{
>> +    have_guest_debug = kvm_check_extension(cs->kvm_state,
>> +                                           KVM_CAP_SET_GUEST_DEBUG);
>> +    return;
>> +}
>> +
>>  static inline void set_feature(uint64_t *features, int feature)
>>  {
>>      *features |= 1ULL << feature;
>> @@ -121,6 +137,8 @@ int kvm_arch_init_vcpu(CPUState *cs)
>>      }
>>      cpu->mp_affinity = mpidr & ARM64_AFFINITY_MASK;
>>
>> +    kvm_arm_init_debug(cs);
>> +
>>      return kvm_arm_init_cpreg_list(cpu);
>>  }
>
> I assume in practice the kernel guarantees that either all
> CPUs have the SET_GUEST_DEBUG cap, or none do :-)
>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

...except I've just noticed that nothing else in this patchset
ever reads the have_guest_debug bool we just set, so what is
the purpose of this patch?

thanks
-- PMM
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v9 1/6] target-arm: kvm64 - introduce kvm_arm_init_debug()

[Peter Maydell]

On 20 November 2015 at 15:05, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 12 November 2015 at 16:20, Alex Benn?e <alex.bennee@linaro.org> wrote:
>> As we haven't always had guest debug support we need to probe for it.
>> Additionally we don't do this in the start-up capability code so we
>> don't fall over on old kernels.
>>
>> Signed-off-by: Alex Benn?e <alex.bennee@linaro.org>
>> ---
>>  target-arm/kvm64.c | 18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>>
>> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
>> index ceebfeb..d087794 100644
>> --- a/target-arm/kvm64.c
>> +++ b/target-arm/kvm64.c
>> @@ -25,6 +25,22 @@
>>  #include "internals.h"
>>  #include "hw/arm/arm.h"
>>
>> +static bool have_guest_debug;
>> +
>> +/**
>> + * kvm_arm_init_debug()
>> + * @cs: CPUState
>> + *
>> + * Check for guest debug capabilities.
>> + *
>> + */
>> +static void kvm_arm_init_debug(CPUState *cs)
>> +{
>> +    have_guest_debug = kvm_check_extension(cs->kvm_state,
>> +                                           KVM_CAP_SET_GUEST_DEBUG);
>> +    return;
>> +}
>> +
>>  static inline void set_feature(uint64_t *features, int feature)
>>  {
>>      *features |= 1ULL << feature;
>> @@ -121,6 +137,8 @@ int kvm_arch_init_vcpu(CPUState *cs)
>>      }
>>      cpu->mp_affinity = mpidr & ARM64_AFFINITY_MASK;
>>
>> +    kvm_arm_init_debug(cs);
>> +
>>      return kvm_arm_init_cpreg_list(cpu);
>>  }
>
> I assume in practice the kernel guarantees that either all
> CPUs have the SET_GUEST_DEBUG cap, or none do :-)
>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

...except I've just noticed that nothing else in this patchset
ever reads the have_guest_debug bool we just set, so what is
the purpose of this patch?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v9 1/6] target-arm: kvm64 - introduce kvm_arm_init_debug()

[Peter Maydell]

On 20 November 2015 at 15:05, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 12 November 2015 at 16:20, Alex Bennée <alex.bennee@linaro.org> wrote:
>> As we haven't always had guest debug support we need to probe for it.
>> Additionally we don't do this in the start-up capability code so we
>> don't fall over on old kernels.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  target-arm/kvm64.c | 18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>>
>> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
>> index ceebfeb..d087794 100644
>> --- a/target-arm/kvm64.c
>> +++ b/target-arm/kvm64.c
>> @@ -25,6 +25,22 @@
>>  #include "internals.h"
>>  #include "hw/arm/arm.h"
>>
>> +static bool have_guest_debug;
>> +
>> +/**
>> + * kvm_arm_init_debug()
>> + * @cs: CPUState
>> + *
>> + * Check for guest debug capabilities.
>> + *
>> + */
>> +static void kvm_arm_init_debug(CPUState *cs)
>> +{
>> +    have_guest_debug = kvm_check_extension(cs->kvm_state,
>> +                                           KVM_CAP_SET_GUEST_DEBUG);
>> +    return;
>> +}
>> +
>>  static inline void set_feature(uint64_t *features, int feature)
>>  {
>>      *features |= 1ULL << feature;
>> @@ -121,6 +137,8 @@ int kvm_arch_init_vcpu(CPUState *cs)
>>      }
>>      cpu->mp_affinity = mpidr & ARM64_AFFINITY_MASK;
>>
>> +    kvm_arm_init_debug(cs);
>> +
>>      return kvm_arm_init_cpreg_list(cpu);
>>  }
>
> I assume in practice the kernel guarantees that either all
> CPUs have the SET_GUEST_DEBUG cap, or none do :-)
>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

...except I've just noticed that nothing else in this patchset
ever reads the have_guest_debug bool we just set, so what is
the purpose of this patch?

thanks
-- PMM

Re: [PATCH v9 1/6] target-arm: kvm64 - introduce kvm_arm_init_debug()

[Alex Bennée]

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

> On 20 November 2015 at 15:05, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 12 November 2015 at 16:20, Alex Bennée <alex.bennee@linaro.org> wrote:
>>> As we haven't always had guest debug support we need to probe for it.
>>> Additionally we don't do this in the start-up capability code so we
>>> don't fall over on old kernels.
>>>
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>> ---
>>>  target-arm/kvm64.c | 18 ++++++++++++++++++
>>>  1 file changed, 18 insertions(+)
>>>
>>> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
>>> index ceebfeb..d087794 100644
>>> --- a/target-arm/kvm64.c
>>> +++ b/target-arm/kvm64.c
>>> @@ -25,6 +25,22 @@
>>>  #include "internals.h"
>>>  #include "hw/arm/arm.h"
>>>
>>> +static bool have_guest_debug;
>>> +
>>> +/**
>>> + * kvm_arm_init_debug()
>>> + * @cs: CPUState
>>> + *
>>> + * Check for guest debug capabilities.
>>> + *
>>> + */
>>> +static void kvm_arm_init_debug(CPUState *cs)
>>> +{
>>> +    have_guest_debug = kvm_check_extension(cs->kvm_state,
>>> +                                           KVM_CAP_SET_GUEST_DEBUG);
>>> +    return;
>>> +}
>>> +
>>>  static inline void set_feature(uint64_t *features, int feature)
>>>  {
>>>      *features |= 1ULL << feature;
>>> @@ -121,6 +137,8 @@ int kvm_arch_init_vcpu(CPUState *cs)
>>>      }
>>>      cpu->mp_affinity = mpidr & ARM64_AFFINITY_MASK;
>>>
>>> +    kvm_arm_init_debug(cs);
>>> +
>>>      return kvm_arm_init_cpreg_list(cpu);
>>>  }
>>
>> I assume in practice the kernel guarantees that either all
>> CPUs have the SET_GUEST_DEBUG cap, or none do :-)
>>
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> ...except I've just noticed that nothing else in this patchset
> ever reads the have_guest_debug bool we just set, so what is
> the purpose of this patch?

Oops, maybe to point out my stupidity ;-)

But yes the SET_GUEST_DEBUG cap is kernel wide.

>
> thanks
> -- PMM


-- 
Alex Bennée

[PATCH v9 1/6] target-arm: kvm64 - introduce kvm_arm_init_debug()

[Alex Bennée]

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

> On 20 November 2015 at 15:05, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 12 November 2015 at 16:20, Alex Benn?e <alex.bennee@linaro.org> wrote:
>>> As we haven't always had guest debug support we need to probe for it.
>>> Additionally we don't do this in the start-up capability code so we
>>> don't fall over on old kernels.
>>>
>>> Signed-off-by: Alex Benn?e <alex.bennee@linaro.org>
>>> ---
>>>  target-arm/kvm64.c | 18 ++++++++++++++++++
>>>  1 file changed, 18 insertions(+)
>>>
>>> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
>>> index ceebfeb..d087794 100644
>>> --- a/target-arm/kvm64.c
>>> +++ b/target-arm/kvm64.c
>>> @@ -25,6 +25,22 @@
>>>  #include "internals.h"
>>>  #include "hw/arm/arm.h"
>>>
>>> +static bool have_guest_debug;
>>> +
>>> +/**
>>> + * kvm_arm_init_debug()
>>> + * @cs: CPUState
>>> + *
>>> + * Check for guest debug capabilities.
>>> + *
>>> + */
>>> +static void kvm_arm_init_debug(CPUState *cs)
>>> +{
>>> +    have_guest_debug = kvm_check_extension(cs->kvm_state,
>>> +                                           KVM_CAP_SET_GUEST_DEBUG);
>>> +    return;
>>> +}
>>> +
>>>  static inline void set_feature(uint64_t *features, int feature)
>>>  {
>>>      *features |= 1ULL << feature;
>>> @@ -121,6 +137,8 @@ int kvm_arch_init_vcpu(CPUState *cs)
>>>      }
>>>      cpu->mp_affinity = mpidr & ARM64_AFFINITY_MASK;
>>>
>>> +    kvm_arm_init_debug(cs);
>>> +
>>>      return kvm_arm_init_cpreg_list(cpu);
>>>  }
>>
>> I assume in practice the kernel guarantees that either all
>> CPUs have the SET_GUEST_DEBUG cap, or none do :-)
>>
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> ...except I've just noticed that nothing else in this patchset
> ever reads the have_guest_debug bool we just set, so what is
> the purpose of this patch?

Oops, maybe to point out my stupidity ;-)

But yes the SET_GUEST_DEBUG cap is kernel wide.

>
> thanks
> -- PMM


-- 
Alex Benn?e

Re: [PATCH v9 1/6] target-arm: kvm64 - introduce kvm_arm_init_debug()

[Alex Bennée]

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

> On 20 November 2015 at 15:05, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 12 November 2015 at 16:20, Alex Bennée <alex.bennee@linaro.org> wrote:
>>> As we haven't always had guest debug support we need to probe for it.
>>> Additionally we don't do this in the start-up capability code so we
>>> don't fall over on old kernels.
>>>
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>> ---
>>>  target-arm/kvm64.c | 18 ++++++++++++++++++
>>>  1 file changed, 18 insertions(+)
>>>
>>> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
>>> index ceebfeb..d087794 100644
>>> --- a/target-arm/kvm64.c
>>> +++ b/target-arm/kvm64.c
>>> @@ -25,6 +25,22 @@
>>>  #include "internals.h"
>>>  #include "hw/arm/arm.h"
>>>
>>> +static bool have_guest_debug;
>>> +
>>> +/**
>>> + * kvm_arm_init_debug()
>>> + * @cs: CPUState
>>> + *
>>> + * Check for guest debug capabilities.
>>> + *
>>> + */
>>> +static void kvm_arm_init_debug(CPUState *cs)
>>> +{
>>> +    have_guest_debug = kvm_check_extension(cs->kvm_state,
>>> +                                           KVM_CAP_SET_GUEST_DEBUG);
>>> +    return;
>>> +}
>>> +
>>>  static inline void set_feature(uint64_t *features, int feature)
>>>  {
>>>      *features |= 1ULL << feature;
>>> @@ -121,6 +137,8 @@ int kvm_arch_init_vcpu(CPUState *cs)
>>>      }
>>>      cpu->mp_affinity = mpidr & ARM64_AFFINITY_MASK;
>>>
>>> +    kvm_arm_init_debug(cs);
>>> +
>>>      return kvm_arm_init_cpreg_list(cpu);
>>>  }
>>
>> I assume in practice the kernel guarantees that either all
>> CPUs have the SET_GUEST_DEBUG cap, or none do :-)
>>
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> ...except I've just noticed that nothing else in this patchset
> ever reads the have_guest_debug bool we just set, so what is
> the purpose of this patch?

Oops, maybe to point out my stupidity ;-)

But yes the SET_GUEST_DEBUG cap is kernel wide.

>
> thanks
> -- PMM


-- 
Alex Bennée

Re: [Qemu-devel] [PATCH v9 1/6] target-arm: kvm64 - introduce kvm_arm_init_debug()

[Alex Bennée]

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

> On 20 November 2015 at 15:05, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 12 November 2015 at 16:20, Alex Bennée <alex.bennee@linaro.org> wrote:
>>> As we haven't always had guest debug support we need to probe for it.
>>> Additionally we don't do this in the start-up capability code so we
>>> don't fall over on old kernels.
>>>
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>> ---
>>>  target-arm/kvm64.c | 18 ++++++++++++++++++
>>>  1 file changed, 18 insertions(+)
>>>
>>> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
>>> index ceebfeb..d087794 100644
>>> --- a/target-arm/kvm64.c
>>> +++ b/target-arm/kvm64.c
>>> @@ -25,6 +25,22 @@
>>>  #include "internals.h"
>>>  #include "hw/arm/arm.h"
>>>
>>> +static bool have_guest_debug;
>>> +
>>> +/**
>>> + * kvm_arm_init_debug()
>>> + * @cs: CPUState
>>> + *
>>> + * Check for guest debug capabilities.
>>> + *
>>> + */
>>> +static void kvm_arm_init_debug(CPUState *cs)
>>> +{
>>> +    have_guest_debug = kvm_check_extension(cs->kvm_state,
>>> +                                           KVM_CAP_SET_GUEST_DEBUG);
>>> +    return;
>>> +}
>>> +
>>>  static inline void set_feature(uint64_t *features, int feature)
>>>  {
>>>      *features |= 1ULL << feature;
>>> @@ -121,6 +137,8 @@ int kvm_arch_init_vcpu(CPUState *cs)
>>>      }
>>>      cpu->mp_affinity = mpidr & ARM64_AFFINITY_MASK;
>>>
>>> +    kvm_arm_init_debug(cs);
>>> +
>>>      return kvm_arm_init_cpreg_list(cpu);
>>>  }
>>
>> I assume in practice the kernel guarantees that either all
>> CPUs have the SET_GUEST_DEBUG cap, or none do :-)
>>
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> ...except I've just noticed that nothing else in this patchset
> ever reads the have_guest_debug bool we just set, so what is
> the purpose of this patch?

Oops, maybe to point out my stupidity ;-)

But yes the SET_GUEST_DEBUG cap is kernel wide.

>
> thanks
> -- PMM


-- 
Alex Bennée

[PATCH v9 2/6] target-arm: kvm - implement software breakpoints

[Alex Bennée]

These don't involve messing around with debug registers, just setting
the breakpoint instruction in memory. GDB will not use this mechanism if
it can't access the memory to write the breakpoint.

All the kernel has to do is ensure the hypervisor traps the breakpoint
exceptions and returns to userspace.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

--
v2
  - handle debug exit with new hsr exception info
  - add verbosity to UNIMP message
v3
  - sync with kvm_cpu_synchronize_state() before checking PC.
  - use internals.h defines
  - use env->pc
  - use proper format types
v9
  - add include for error_report
  - define a brk_insn constant
---
 target-arm/kvm.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 78 insertions(+), 12 deletions(-)

diff --git a/target-arm/kvm.c b/target-arm/kvm.c
index 79ef4c6..50f70ef 100644
--- a/target-arm/kvm.c
+++ b/target-arm/kvm.c
@@ -17,6 +17,7 @@
 
 #include "qemu-common.h"
 #include "qemu/timer.h"
+#include "qemu/error-report.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/kvm.h"
 #include "kvm_arm.h"
@@ -516,9 +517,60 @@ MemTxAttrs kvm_arch_post_run(CPUState *cs, struct kvm_run *run)
     return MEMTXATTRS_UNSPECIFIED;
 }
 
+/* See v8 ARM ARM D7.2.27 ESR_ELx, Exception Syndrome Register
+ *
+ * To minimise translating between kernel and user-space the kernel
+ * ABI just provides user-space with the full exception syndrome
+ * register value to be decoded in QEMU.
+ */
+
+static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
+{
+    struct kvm_debug_exit_arch *arch_info = &run->debug.arch;
+    int hsr_ec = arch_info->hsr >> ARM_EL_EC_SHIFT;
+    ARMCPU *cpu = ARM_CPU(cs);
+    CPUARMState *env = &cpu->env;
+
+    /* Ensure PC is synchronised */
+    kvm_cpu_synchronize_state(cs);
+
+    switch (hsr_ec) {
+    case EC_AA64_BKPT:
+        if (kvm_find_sw_breakpoint(cs, env->pc)) {
+            return true;
+        }
+        break;
+    default:
+        error_report("%s: unhandled debug exit (%"PRIx32", %"PRIx64")\n",
+                     __func__, arch_info->hsr, env->pc);
+    }
+
+    /* If we don't handle this it could be it really is for the
+       guest to handle */
+    qemu_log_mask(LOG_UNIMP,
+                  "%s: re-injecting exception not yet implemented"
+                  " (0x%"PRIx32", %"PRIx64")\n",
+                  __func__, hsr_ec, env->pc);
+
+    return false;
+}
+
 int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
 {
-    return 0;
+    int ret = 0;
+
+    switch (run->exit_reason) {
+    case KVM_EXIT_DEBUG:
+        if (kvm_handle_debug(cs, run)) {
+            ret = EXCP_DEBUG;
+        } /* otherwise return to guest */
+        break;
+    default:
+        qemu_log_mask(LOG_UNIMP, "%s: un-handled exit reason %d\n",
+                      __func__, run->exit_reason);
+        break;
+    }
+    return ret;
 }
 
 bool kvm_arch_stop_on_emulation_error(CPUState *cs)
@@ -543,14 +595,34 @@ int kvm_arch_on_sigbus(int code, void *addr)
 
 void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
 {
-    qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
+    if (kvm_sw_breakpoints_active(cs)) {
+        dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;
+    }
 }
 
-int kvm_arch_insert_sw_breakpoint(CPUState *cs,
-                                  struct kvm_sw_breakpoint *bp)
+/* C6.6.29 BRK instruction */
+static const uint32_t brk_insn = 0xd4200000;
+
+int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
 {
-    qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
-    return -EINVAL;
+
+    if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 0) ||
+        cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&brk_insn, 4, 1)) {
+        return -EINVAL;
+    }
+    return 0;
+}
+
+int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
+{
+    static uint32_t brk;
+
+    if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&brk, 4, 0) ||
+        brk != brk_insn ||
+        cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 1)) {
+        return -EINVAL;
+    }
+    return 0;
 }
 
 int kvm_arch_insert_hw_breakpoint(target_ulong addr,
@@ -567,12 +639,6 @@ int kvm_arch_remove_hw_breakpoint(target_ulong addr,
     return -EINVAL;
 }
 
-int kvm_arch_remove_sw_breakpoint(CPUState *cs,
-                                  struct kvm_sw_breakpoint *bp)
-{
-    qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
-    return -EINVAL;
-}
 
 void kvm_arch_remove_all_hw_breakpoints(void)
 {
-- 
2.6.3

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v9 2/6] target-arm: kvm - implement software breakpoints

[Alex Bennée]

These don't involve messing around with debug registers, just setting
the breakpoint instruction in memory. GDB will not use this mechanism if
it can't access the memory to write the breakpoint.

All the kernel has to do is ensure the hypervisor traps the breakpoint
exceptions and returns to userspace.

Signed-off-by: Alex Benn?e <alex.bennee@linaro.org>

--
v2
  - handle debug exit with new hsr exception info
  - add verbosity to UNIMP message
v3
  - sync with kvm_cpu_synchronize_state() before checking PC.
  - use internals.h defines
  - use env->pc
  - use proper format types
v9
  - add include for error_report
  - define a brk_insn constant
---
 target-arm/kvm.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 78 insertions(+), 12 deletions(-)

diff --git a/target-arm/kvm.c b/target-arm/kvm.c
index 79ef4c6..50f70ef 100644
--- a/target-arm/kvm.c
+++ b/target-arm/kvm.c
@@ -17,6 +17,7 @@
 
 #include "qemu-common.h"
 #include "qemu/timer.h"
+#include "qemu/error-report.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/kvm.h"
 #include "kvm_arm.h"
@@ -516,9 +517,60 @@ MemTxAttrs kvm_arch_post_run(CPUState *cs, struct kvm_run *run)
     return MEMTXATTRS_UNSPECIFIED;
 }
 
+/* See v8 ARM ARM D7.2.27 ESR_ELx, Exception Syndrome Register
+ *
+ * To minimise translating between kernel and user-space the kernel
+ * ABI just provides user-space with the full exception syndrome
+ * register value to be decoded in QEMU.
+ */
+
+static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
+{
+    struct kvm_debug_exit_arch *arch_info = &run->debug.arch;
+    int hsr_ec = arch_info->hsr >> ARM_EL_EC_SHIFT;
+    ARMCPU *cpu = ARM_CPU(cs);
+    CPUARMState *env = &cpu->env;
+
+    /* Ensure PC is synchronised */
+    kvm_cpu_synchronize_state(cs);
+
+    switch (hsr_ec) {
+    case EC_AA64_BKPT:
+        if (kvm_find_sw_breakpoint(cs, env->pc)) {
+            return true;
+        }
+        break;
+    default:
+        error_report("%s: unhandled debug exit (%"PRIx32", %"PRIx64")\n",
+                     __func__, arch_info->hsr, env->pc);
+    }
+
+    /* If we don't handle this it could be it really is for the
+       guest to handle */
+    qemu_log_mask(LOG_UNIMP,
+                  "%s: re-injecting exception not yet implemented"
+                  " (0x%"PRIx32", %"PRIx64")\n",
+                  __func__, hsr_ec, env->pc);
+
+    return false;
+}
+
 int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
 {
-    return 0;
+    int ret = 0;
+
+    switch (run->exit_reason) {
+    case KVM_EXIT_DEBUG:
+        if (kvm_handle_debug(cs, run)) {
+            ret = EXCP_DEBUG;
+        } /* otherwise return to guest */
+        break;
+    default:
+        qemu_log_mask(LOG_UNIMP, "%s: un-handled exit reason %d\n",
+                      __func__, run->exit_reason);
+        break;
+    }
+    return ret;
 }
 
 bool kvm_arch_stop_on_emulation_error(CPUState *cs)
@@ -543,14 +595,34 @@ int kvm_arch_on_sigbus(int code, void *addr)
 
 void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
 {
-    qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
+    if (kvm_sw_breakpoints_active(cs)) {
+        dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;
+    }
 }
 
-int kvm_arch_insert_sw_breakpoint(CPUState *cs,
-                                  struct kvm_sw_breakpoint *bp)
+/* C6.6.29 BRK instruction */
+static const uint32_t brk_insn = 0xd4200000;
+
+int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
 {
-    qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
-    return -EINVAL;
+
+    if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 0) ||
+        cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&brk_insn, 4, 1)) {
+        return -EINVAL;
+    }
+    return 0;
+}
+
+int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
+{
+    static uint32_t brk;
+
+    if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&brk, 4, 0) ||
+        brk != brk_insn ||
+        cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 1)) {
+        return -EINVAL;
+    }
+    return 0;
 }
 
 int kvm_arch_insert_hw_breakpoint(target_ulong addr,
@@ -567,12 +639,6 @@ int kvm_arch_remove_hw_breakpoint(target_ulong addr,
     return -EINVAL;
 }
 
-int kvm_arch_remove_sw_breakpoint(CPUState *cs,
-                                  struct kvm_sw_breakpoint *bp)
-{
-    qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
-    return -EINVAL;
-}
 
 void kvm_arch_remove_all_hw_breakpoints(void)
 {
-- 
2.6.3

[Qemu-devel] [PATCH v9 2/6] target-arm: kvm - implement software breakpoints

[Alex Bennée]

These don't involve messing around with debug registers, just setting
the breakpoint instruction in memory. GDB will not use this mechanism if
it can't access the memory to write the breakpoint.

All the kernel has to do is ensure the hypervisor traps the breakpoint
exceptions and returns to userspace.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

--
v2
  - handle debug exit with new hsr exception info
  - add verbosity to UNIMP message
v3
  - sync with kvm_cpu_synchronize_state() before checking PC.
  - use internals.h defines
  - use env->pc
  - use proper format types
v9
  - add include for error_report
  - define a brk_insn constant
---
 target-arm/kvm.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 78 insertions(+), 12 deletions(-)

diff --git a/target-arm/kvm.c b/target-arm/kvm.c
index 79ef4c6..50f70ef 100644
--- a/target-arm/kvm.c
+++ b/target-arm/kvm.c
@@ -17,6 +17,7 @@
 
 #include "qemu-common.h"
 #include "qemu/timer.h"
+#include "qemu/error-report.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/kvm.h"
 #include "kvm_arm.h"
@@ -516,9 +517,60 @@ MemTxAttrs kvm_arch_post_run(CPUState *cs, struct kvm_run *run)
     return MEMTXATTRS_UNSPECIFIED;
 }
 
+/* See v8 ARM ARM D7.2.27 ESR_ELx, Exception Syndrome Register
+ *
+ * To minimise translating between kernel and user-space the kernel
+ * ABI just provides user-space with the full exception syndrome
+ * register value to be decoded in QEMU.
+ */
+
+static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
+{
+    struct kvm_debug_exit_arch *arch_info = &run->debug.arch;
+    int hsr_ec = arch_info->hsr >> ARM_EL_EC_SHIFT;
+    ARMCPU *cpu = ARM_CPU(cs);
+    CPUARMState *env = &cpu->env;
+
+    /* Ensure PC is synchronised */
+    kvm_cpu_synchronize_state(cs);
+
+    switch (hsr_ec) {
+    case EC_AA64_BKPT:
+        if (kvm_find_sw_breakpoint(cs, env->pc)) {
+            return true;
+        }
+        break;
+    default:
+        error_report("%s: unhandled debug exit (%"PRIx32", %"PRIx64")\n",
+                     __func__, arch_info->hsr, env->pc);
+    }
+
+    /* If we don't handle this it could be it really is for the
+       guest to handle */
+    qemu_log_mask(LOG_UNIMP,
+                  "%s: re-injecting exception not yet implemented"
+                  " (0x%"PRIx32", %"PRIx64")\n",
+                  __func__, hsr_ec, env->pc);
+
+    return false;
+}
+
 int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
 {
-    return 0;
+    int ret = 0;
+
+    switch (run->exit_reason) {
+    case KVM_EXIT_DEBUG:
+        if (kvm_handle_debug(cs, run)) {
+            ret = EXCP_DEBUG;
+        } /* otherwise return to guest */
+        break;
+    default:
+        qemu_log_mask(LOG_UNIMP, "%s: un-handled exit reason %d\n",
+                      __func__, run->exit_reason);
+        break;
+    }
+    return ret;
 }
 
 bool kvm_arch_stop_on_emulation_error(CPUState *cs)
@@ -543,14 +595,34 @@ int kvm_arch_on_sigbus(int code, void *addr)
 
 void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
 {
-    qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
+    if (kvm_sw_breakpoints_active(cs)) {
+        dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;
+    }
 }
 
-int kvm_arch_insert_sw_breakpoint(CPUState *cs,
-                                  struct kvm_sw_breakpoint *bp)
+/* C6.6.29 BRK instruction */
+static const uint32_t brk_insn = 0xd4200000;
+
+int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
 {
-    qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
-    return -EINVAL;
+
+    if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 0) ||
+        cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&brk_insn, 4, 1)) {
+        return -EINVAL;
+    }
+    return 0;
+}
+
+int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
+{
+    static uint32_t brk;
+
+    if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&brk, 4, 0) ||
+        brk != brk_insn ||
+        cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 1)) {
+        return -EINVAL;
+    }
+    return 0;
 }
 
 int kvm_arch_insert_hw_breakpoint(target_ulong addr,
@@ -567,12 +639,6 @@ int kvm_arch_remove_hw_breakpoint(target_ulong addr,
     return -EINVAL;
 }
 
-int kvm_arch_remove_sw_breakpoint(CPUState *cs,
-                                  struct kvm_sw_breakpoint *bp)
-{
-    qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
-    return -EINVAL;
-}
 
 void kvm_arch_remove_all_hw_breakpoints(void)
 {
-- 
2.6.3

Re: [PATCH v9 2/6] target-arm: kvm - implement software breakpoints

[Peter Maydell]

On 12 November 2015 at 16:20, Alex Bennée <alex.bennee@linaro.org> wrote:
> These don't involve messing around with debug registers, just setting
> the breakpoint instruction in memory. GDB will not use this mechanism if
> it can't access the memory to write the breakpoint.
>
> All the kernel has to do is ensure the hypervisor traps the breakpoint
> exceptions and returns to userspace.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> --
> v2
>   - handle debug exit with new hsr exception info
>   - add verbosity to UNIMP message
> v3
>   - sync with kvm_cpu_synchronize_state() before checking PC.
>   - use internals.h defines
>   - use env->pc
>   - use proper format types
> v9
>   - add include for error_report
>   - define a brk_insn constant
> ---
>  target-arm/kvm.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 78 insertions(+), 12 deletions(-)
>
> diff --git a/target-arm/kvm.c b/target-arm/kvm.c
> index 79ef4c6..50f70ef 100644
> --- a/target-arm/kvm.c
> +++ b/target-arm/kvm.c
> @@ -17,6 +17,7 @@
>
>  #include "qemu-common.h"
>  #include "qemu/timer.h"
> +#include "qemu/error-report.h"
>  #include "sysemu/sysemu.h"
>  #include "sysemu/kvm.h"
>  #include "kvm_arm.h"
> @@ -516,9 +517,60 @@ MemTxAttrs kvm_arch_post_run(CPUState *cs, struct kvm_run *run)
>      return MEMTXATTRS_UNSPECIFIED;
>  }
>
> +/* See v8 ARM ARM D7.2.27 ESR_ELx, Exception Syndrome Register
> + *
> + * To minimise translating between kernel and user-space the kernel
> + * ABI just provides user-space with the full exception syndrome
> + * register value to be decoded in QEMU.
> + */
> +
> +static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
> +{
> +    struct kvm_debug_exit_arch *arch_info = &run->debug.arch;
> +    int hsr_ec = arch_info->hsr >> ARM_EL_EC_SHIFT;

This doesn't build for 32-bit ARM:

target-arm/kvm.c:530:27: error: ‘struct kvm_debug_exit_arch’ has no
member named ‘hsr’
     int hsr_ec = arch_info->hsr >> ARM_EL_EC_SHIFT;

> +    ARMCPU *cpu = ARM_CPU(cs);
> +    CPUARMState *env = &cpu->env;
> +
> +    /* Ensure PC is synchronised */
> +    kvm_cpu_synchronize_state(cs);
> +
> +    switch (hsr_ec) {
> +    case EC_AA64_BKPT:
> +        if (kvm_find_sw_breakpoint(cs, env->pc)) {
> +            return true;
> +        }
> +        break;
> +    default:
> +        error_report("%s: unhandled debug exit (%"PRIx32", %"PRIx64")\n",
> +                     __func__, arch_info->hsr, env->pc);
> +    }
> +
> +    /* If we don't handle this it could be it really is for the
> +       guest to handle */
> +    qemu_log_mask(LOG_UNIMP,
> +                  "%s: re-injecting exception not yet implemented"
> +                  " (0x%"PRIx32", %"PRIx64")\n",
> +                  __func__, hsr_ec, env->pc);
> +
> +    return false;
> +}
> +
>  int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
>  {
> -    return 0;
> +    int ret = 0;
> +
> +    switch (run->exit_reason) {
> +    case KVM_EXIT_DEBUG:
> +        if (kvm_handle_debug(cs, run)) {
> +            ret = EXCP_DEBUG;
> +        } /* otherwise return to guest */
> +        break;
> +    default:
> +        qemu_log_mask(LOG_UNIMP, "%s: un-handled exit reason %d\n",
> +                      __func__, run->exit_reason);
> +        break;
> +    }
> +    return ret;
>  }
>
>  bool kvm_arch_stop_on_emulation_error(CPUState *cs)
> @@ -543,14 +595,34 @@ int kvm_arch_on_sigbus(int code, void *addr)
>
>  void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
>  {
> -    qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
> +    if (kvm_sw_breakpoints_active(cs)) {
> +        dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;

This won't compile on 32-bit ARM either:

target-arm/kvm.c:634:38: error: ‘KVM_GUESTDBG_USE_SW_BP’ undeclared
(first use in this function)
         dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;

> +    }
>  }
>
> -int kvm_arch_insert_sw_breakpoint(CPUState *cs,
> -                                  struct kvm_sw_breakpoint *bp)
> +/* C6.6.29 BRK instruction */
> +static const uint32_t brk_insn = 0xd4200000;

This is the A64 breakpoint instruction, so why is it in the common-to-32-and-64
source file? How about the A32 and T16 breakpoint insns?

> +
> +int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
>  {
> -    qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
> -    return -EINVAL;
> +
> +    if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 0) ||
> +        cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&brk_insn, 4, 1)) {
> +        return -EINVAL;
> +    }

Should we allow insertion of sw breakpoint insns if the kernel doesn't
implement KVM_EXIT_DEBUG and reporting the ESR to us?

> +    return 0;
> +}
> +
> +int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> +{
> +    static uint32_t brk;
> +
> +    if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&brk, 4, 0) ||
> +        brk != brk_insn ||
> +        cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 1)) {
> +        return -EINVAL;
> +    }
> +    return 0;
>  }
>
>  int kvm_arch_insert_hw_breakpoint(target_ulong addr,
> @@ -567,12 +639,6 @@ int kvm_arch_remove_hw_breakpoint(target_ulong addr,
>      return -EINVAL;
>  }
>
> -int kvm_arch_remove_sw_breakpoint(CPUState *cs,
> -                                  struct kvm_sw_breakpoint *bp)
> -{
> -    qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
> -    return -EINVAL;
> -}
>
>  void kvm_arch_remove_all_hw_breakpoints(void)
>  {
> --
> 2.6.3

thanks
-- PMM

[PATCH v9 2/6] target-arm: kvm - implement software breakpoints

[Peter Maydell]

On 12 November 2015 at 16:20, Alex Benn?e <alex.bennee@linaro.org> wrote:
> These don't involve messing around with debug registers, just setting
> the breakpoint instruction in memory. GDB will not use this mechanism if
> it can't access the memory to write the breakpoint.
>
> All the kernel has to do is ensure the hypervisor traps the breakpoint
> exceptions and returns to userspace.
>
> Signed-off-by: Alex Benn?e <alex.bennee@linaro.org>
>
> --
> v2
>   - handle debug exit with new hsr exception info
>   - add verbosity to UNIMP message
> v3
>   - sync with kvm_cpu_synchronize_state() before checking PC.
>   - use internals.h defines
>   - use env->pc
>   - use proper format types
> v9
>   - add include for error_report
>   - define a brk_insn constant
> ---
>  target-arm/kvm.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 78 insertions(+), 12 deletions(-)
>
> diff --git a/target-arm/kvm.c b/target-arm/kvm.c
> index 79ef4c6..50f70ef 100644
> --- a/target-arm/kvm.c
> +++ b/target-arm/kvm.c
> @@ -17,6 +17,7 @@
>
>  #include "qemu-common.h"
>  #include "qemu/timer.h"
> +#include "qemu/error-report.h"
>  #include "sysemu/sysemu.h"
>  #include "sysemu/kvm.h"
>  #include "kvm_arm.h"
> @@ -516,9 +517,60 @@ MemTxAttrs kvm_arch_post_run(CPUState *cs, struct kvm_run *run)
>      return MEMTXATTRS_UNSPECIFIED;
>  }
>
> +/* See v8 ARM ARM D7.2.27 ESR_ELx, Exception Syndrome Register
> + *
> + * To minimise translating between kernel and user-space the kernel
> + * ABI just provides user-space with the full exception syndrome
> + * register value to be decoded in QEMU.
> + */
> +
> +static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
> +{
> +    struct kvm_debug_exit_arch *arch_info = &run->debug.arch;
> +    int hsr_ec = arch_info->hsr >> ARM_EL_EC_SHIFT;

This doesn't build for 32-bit ARM:

target-arm/kvm.c:530:27: error: ?struct kvm_debug_exit_arch? has no
member named ?hsr?
     int hsr_ec = arch_info->hsr >> ARM_EL_EC_SHIFT;

> +    ARMCPU *cpu = ARM_CPU(cs);
> +    CPUARMState *env = &cpu->env;
> +
> +    /* Ensure PC is synchronised */
> +    kvm_cpu_synchronize_state(cs);
> +
> +    switch (hsr_ec) {
> +    case EC_AA64_BKPT:
> +        if (kvm_find_sw_breakpoint(cs, env->pc)) {
> +            return true;
> +        }
> +        break;
> +    default:
> +        error_report("%s: unhandled debug exit (%"PRIx32", %"PRIx64")\n",
> +                     __func__, arch_info->hsr, env->pc);
> +    }
> +
> +    /* If we don't handle this it could be it really is for the
> +       guest to handle */
> +    qemu_log_mask(LOG_UNIMP,
> +                  "%s: re-injecting exception not yet implemented"
> +                  " (0x%"PRIx32", %"PRIx64")\n",
> +                  __func__, hsr_ec, env->pc);
> +
> +    return false;
> +}
> +
>  int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
>  {
> -    return 0;
> +    int ret = 0;
> +
> +    switch (run->exit_reason) {
> +    case KVM_EXIT_DEBUG:
> +        if (kvm_handle_debug(cs, run)) {
> +            ret = EXCP_DEBUG;
> +        } /* otherwise return to guest */
> +        break;
> +    default:
> +        qemu_log_mask(LOG_UNIMP, "%s: un-handled exit reason %d\n",
> +                      __func__, run->exit_reason);
> +        break;
> +    }
> +    return ret;
>  }
>
>  bool kvm_arch_stop_on_emulation_error(CPUState *cs)
> @@ -543,14 +595,34 @@ int kvm_arch_on_sigbus(int code, void *addr)
>
>  void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
>  {
> -    qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
> +    if (kvm_sw_breakpoints_active(cs)) {
> +        dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;

This won't compile on 32-bit ARM either:

target-arm/kvm.c:634:38: error: ?KVM_GUESTDBG_USE_SW_BP? undeclared
(first use in this function)
         dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;

> +    }
>  }
>
> -int kvm_arch_insert_sw_breakpoint(CPUState *cs,
> -                                  struct kvm_sw_breakpoint *bp)
> +/* C6.6.29 BRK instruction */
> +static const uint32_t brk_insn = 0xd4200000;

This is the A64 breakpoint instruction, so why is it in the common-to-32-and-64
source file? How about the A32 and T16 breakpoint insns?

> +
> +int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
>  {
> -    qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
> -    return -EINVAL;
> +
> +    if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 0) ||
> +        cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&brk_insn, 4, 1)) {
> +        return -EINVAL;
> +    }

Should we allow insertion of sw breakpoint insns if the kernel doesn't
implement KVM_EXIT_DEBUG and reporting the ESR to us?

> +    return 0;
> +}
> +
> +int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> +{
> +    static uint32_t brk;
> +
> +    if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&brk, 4, 0) ||
> +        brk != brk_insn ||
> +        cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 1)) {
> +        return -EINVAL;
> +    }
> +    return 0;
>  }
>
>  int kvm_arch_insert_hw_breakpoint(target_ulong addr,
> @@ -567,12 +639,6 @@ int kvm_arch_remove_hw_breakpoint(target_ulong addr,
>      return -EINVAL;
>  }
>
> -int kvm_arch_remove_sw_breakpoint(CPUState *cs,
> -                                  struct kvm_sw_breakpoint *bp)
> -{
> -    qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
> -    return -EINVAL;
> -}
>
>  void kvm_arch_remove_all_hw_breakpoints(void)
>  {
> --
> 2.6.3

thanks
-- PMM

Re: [Qemu-devel] [PATCH v9 2/6] target-arm: kvm - implement software breakpoints

[Peter Maydell]

On 12 November 2015 at 16:20, Alex Bennée <alex.bennee@linaro.org> wrote:
> These don't involve messing around with debug registers, just setting
> the breakpoint instruction in memory. GDB will not use this mechanism if
> it can't access the memory to write the breakpoint.
>
> All the kernel has to do is ensure the hypervisor traps the breakpoint
> exceptions and returns to userspace.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> --
> v2
>   - handle debug exit with new hsr exception info
>   - add verbosity to UNIMP message
> v3
>   - sync with kvm_cpu_synchronize_state() before checking PC.
>   - use internals.h defines
>   - use env->pc
>   - use proper format types
> v9
>   - add include for error_report
>   - define a brk_insn constant
> ---
>  target-arm/kvm.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 78 insertions(+), 12 deletions(-)
>
> diff --git a/target-arm/kvm.c b/target-arm/kvm.c
> index 79ef4c6..50f70ef 100644
> --- a/target-arm/kvm.c
> +++ b/target-arm/kvm.c
> @@ -17,6 +17,7 @@
>
>  #include "qemu-common.h"
>  #include "qemu/timer.h"
> +#include "qemu/error-report.h"
>  #include "sysemu/sysemu.h"
>  #include "sysemu/kvm.h"
>  #include "kvm_arm.h"
> @@ -516,9 +517,60 @@ MemTxAttrs kvm_arch_post_run(CPUState *cs, struct kvm_run *run)
>      return MEMTXATTRS_UNSPECIFIED;
>  }
>
> +/* See v8 ARM ARM D7.2.27 ESR_ELx, Exception Syndrome Register
> + *
> + * To minimise translating between kernel and user-space the kernel
> + * ABI just provides user-space with the full exception syndrome
> + * register value to be decoded in QEMU.
> + */
> +
> +static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
> +{
> +    struct kvm_debug_exit_arch *arch_info = &run->debug.arch;
> +    int hsr_ec = arch_info->hsr >> ARM_EL_EC_SHIFT;

This doesn't build for 32-bit ARM:

target-arm/kvm.c:530:27: error: ‘struct kvm_debug_exit_arch’ has no
member named ‘hsr’
     int hsr_ec = arch_info->hsr >> ARM_EL_EC_SHIFT;

> +    ARMCPU *cpu = ARM_CPU(cs);
> +    CPUARMState *env = &cpu->env;
> +
> +    /* Ensure PC is synchronised */
> +    kvm_cpu_synchronize_state(cs);
> +
> +    switch (hsr_ec) {
> +    case EC_AA64_BKPT:
> +        if (kvm_find_sw_breakpoint(cs, env->pc)) {
> +            return true;
> +        }
> +        break;
> +    default:
> +        error_report("%s: unhandled debug exit (%"PRIx32", %"PRIx64")\n",
> +                     __func__, arch_info->hsr, env->pc);
> +    }
> +
> +    /* If we don't handle this it could be it really is for the
> +       guest to handle */
> +    qemu_log_mask(LOG_UNIMP,
> +                  "%s: re-injecting exception not yet implemented"
> +                  " (0x%"PRIx32", %"PRIx64")\n",
> +                  __func__, hsr_ec, env->pc);
> +
> +    return false;
> +}
> +
>  int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
>  {
> -    return 0;
> +    int ret = 0;
> +
> +    switch (run->exit_reason) {
> +    case KVM_EXIT_DEBUG:
> +        if (kvm_handle_debug(cs, run)) {
> +            ret = EXCP_DEBUG;
> +        } /* otherwise return to guest */
> +        break;
> +    default:
> +        qemu_log_mask(LOG_UNIMP, "%s: un-handled exit reason %d\n",
> +                      __func__, run->exit_reason);
> +        break;
> +    }
> +    return ret;
>  }
>
>  bool kvm_arch_stop_on_emulation_error(CPUState *cs)
> @@ -543,14 +595,34 @@ int kvm_arch_on_sigbus(int code, void *addr)
>
>  void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
>  {
> -    qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
> +    if (kvm_sw_breakpoints_active(cs)) {
> +        dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;

This won't compile on 32-bit ARM either:

target-arm/kvm.c:634:38: error: ‘KVM_GUESTDBG_USE_SW_BP’ undeclared
(first use in this function)
         dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;

> +    }
>  }
>
> -int kvm_arch_insert_sw_breakpoint(CPUState *cs,
> -                                  struct kvm_sw_breakpoint *bp)
> +/* C6.6.29 BRK instruction */
> +static const uint32_t brk_insn = 0xd4200000;

This is the A64 breakpoint instruction, so why is it in the common-to-32-and-64
source file? How about the A32 and T16 breakpoint insns?

> +
> +int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
>  {
> -    qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
> -    return -EINVAL;
> +
> +    if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 0) ||
> +        cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&brk_insn, 4, 1)) {
> +        return -EINVAL;
> +    }

Should we allow insertion of sw breakpoint insns if the kernel doesn't
implement KVM_EXIT_DEBUG and reporting the ESR to us?

> +    return 0;
> +}
> +
> +int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> +{
> +    static uint32_t brk;
> +
> +    if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&brk, 4, 0) ||
> +        brk != brk_insn ||
> +        cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 1)) {
> +        return -EINVAL;
> +    }
> +    return 0;
>  }
>
>  int kvm_arch_insert_hw_breakpoint(target_ulong addr,
> @@ -567,12 +639,6 @@ int kvm_arch_remove_hw_breakpoint(target_ulong addr,
>      return -EINVAL;
>  }
>
> -int kvm_arch_remove_sw_breakpoint(CPUState *cs,
> -                                  struct kvm_sw_breakpoint *bp)
> -{
> -    qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
> -    return -EINVAL;
> -}
>
>  void kvm_arch_remove_all_hw_breakpoints(void)
>  {
> --
> 2.6.3

thanks
-- PMM

[PATCH v9 3/6] target-arm: kvm - support for single step

[Alex Bennée]

This adds support for single-step. There isn't much to do on the QEMU
side as after we set-up the request for single step via the debug ioctl
it is all handled within the kernel.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v2
  - convert to using HSR_EC
v3
  - use internals.h definitions
---
 target-arm/kvm.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/target-arm/kvm.c b/target-arm/kvm.c
index 50f70ef..d505a7e 100644
--- a/target-arm/kvm.c
+++ b/target-arm/kvm.c
@@ -535,6 +535,13 @@ static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
     kvm_cpu_synchronize_state(cs);
 
     switch (hsr_ec) {
+    case EC_SOFTWARESTEP:
+        if (cs->singlestep_enabled) {
+            return true;
+        } else {
+            error_report("Came out of SINGLE STEP when not enabled");
+        }
+        break;
     case EC_AA64_BKPT:
         if (kvm_find_sw_breakpoint(cs, env->pc)) {
             return true;
@@ -595,6 +602,9 @@ int kvm_arch_on_sigbus(int code, void *addr)
 
 void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
 {
+    if (cs->singlestep_enabled) {
+        dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_SINGLESTEP;
+    }
     if (kvm_sw_breakpoints_active(cs)) {
         dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;
     }
-- 
2.6.3

[PATCH v9 3/6] target-arm: kvm - support for single step

[Alex Bennée]

This adds support for single-step. There isn't much to do on the QEMU
side as after we set-up the request for single step via the debug ioctl
it is all handled within the kernel.

Signed-off-by: Alex Benn?e <alex.bennee@linaro.org>

---
v2
  - convert to using HSR_EC
v3
  - use internals.h definitions
---
 target-arm/kvm.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/target-arm/kvm.c b/target-arm/kvm.c
index 50f70ef..d505a7e 100644
--- a/target-arm/kvm.c
+++ b/target-arm/kvm.c
@@ -535,6 +535,13 @@ static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
     kvm_cpu_synchronize_state(cs);
 
     switch (hsr_ec) {
+    case EC_SOFTWARESTEP:
+        if (cs->singlestep_enabled) {
+            return true;
+        } else {
+            error_report("Came out of SINGLE STEP when not enabled");
+        }
+        break;
     case EC_AA64_BKPT:
         if (kvm_find_sw_breakpoint(cs, env->pc)) {
             return true;
@@ -595,6 +602,9 @@ int kvm_arch_on_sigbus(int code, void *addr)
 
 void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
 {
+    if (cs->singlestep_enabled) {
+        dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_SINGLESTEP;
+    }
     if (kvm_sw_breakpoints_active(cs)) {
         dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;
     }
-- 
2.6.3

[Qemu-devel] [PATCH v9 3/6] target-arm: kvm - support for single step

[Alex Bennée]

This adds support for single-step. There isn't much to do on the QEMU
side as after we set-up the request for single step via the debug ioctl
it is all handled within the kernel.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v2
  - convert to using HSR_EC
v3
  - use internals.h definitions
---
 target-arm/kvm.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/target-arm/kvm.c b/target-arm/kvm.c
index 50f70ef..d505a7e 100644
--- a/target-arm/kvm.c
+++ b/target-arm/kvm.c
@@ -535,6 +535,13 @@ static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
     kvm_cpu_synchronize_state(cs);
 
     switch (hsr_ec) {
+    case EC_SOFTWARESTEP:
+        if (cs->singlestep_enabled) {
+            return true;
+        } else {
+            error_report("Came out of SINGLE STEP when not enabled");
+        }
+        break;
     case EC_AA64_BKPT:
         if (kvm_find_sw_breakpoint(cs, env->pc)) {
             return true;
@@ -595,6 +602,9 @@ int kvm_arch_on_sigbus(int code, void *addr)
 
 void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
 {
+    if (cs->singlestep_enabled) {
+        dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_SINGLESTEP;
+    }
     if (kvm_sw_breakpoints_active(cs)) {
         dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;
     }
-- 
2.6.3

Re: [PATCH v9 3/6] target-arm: kvm - support for single step

[Peter Maydell]

On 12 November 2015 at 16:20, Alex Bennée <alex.bennee@linaro.org> wrote:
> This adds support for single-step. There isn't much to do on the QEMU
> side as after we set-up the request for single step via the debug ioctl
> it is all handled within the kernel.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> ---
> v2
>   - convert to using HSR_EC
> v3
>   - use internals.h definitions
> ---
>  target-arm/kvm.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/target-arm/kvm.c b/target-arm/kvm.c
> index 50f70ef..d505a7e 100644
> --- a/target-arm/kvm.c
> +++ b/target-arm/kvm.c
> @@ -535,6 +535,13 @@ static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
>      kvm_cpu_synchronize_state(cs);
>
>      switch (hsr_ec) {
> +    case EC_SOFTWARESTEP:
> +        if (cs->singlestep_enabled) {
> +            return true;
> +        } else {
> +            error_report("Came out of SINGLE STEP when not enabled");
> +        }
> +        break;
>      case EC_AA64_BKPT:
>          if (kvm_find_sw_breakpoint(cs, env->pc)) {
>              return true;
> @@ -595,6 +602,9 @@ int kvm_arch_on_sigbus(int code, void *addr)
>
>  void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
>  {
> +    if (cs->singlestep_enabled) {
> +        dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_SINGLESTEP;
> +    }

Doesn't kvm_update_guest_debug() already set these bits, or am
I misreading it?

>      if (kvm_sw_breakpoints_active(cs)) {
>          dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;
>      }
> --
> 2.6.3

thanks
-- PMM

[PATCH v9 3/6] target-arm: kvm - support for single step

[Peter Maydell]

On 12 November 2015 at 16:20, Alex Benn?e <alex.bennee@linaro.org> wrote:
> This adds support for single-step. There isn't much to do on the QEMU
> side as after we set-up the request for single step via the debug ioctl
> it is all handled within the kernel.
>
> Signed-off-by: Alex Benn?e <alex.bennee@linaro.org>
>
> ---
> v2
>   - convert to using HSR_EC
> v3
>   - use internals.h definitions
> ---
>  target-arm/kvm.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/target-arm/kvm.c b/target-arm/kvm.c
> index 50f70ef..d505a7e 100644
> --- a/target-arm/kvm.c
> +++ b/target-arm/kvm.c
> @@ -535,6 +535,13 @@ static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
>      kvm_cpu_synchronize_state(cs);
>
>      switch (hsr_ec) {
> +    case EC_SOFTWARESTEP:
> +        if (cs->singlestep_enabled) {
> +            return true;
> +        } else {
> +            error_report("Came out of SINGLE STEP when not enabled");
> +        }
> +        break;
>      case EC_AA64_BKPT:
>          if (kvm_find_sw_breakpoint(cs, env->pc)) {
>              return true;
> @@ -595,6 +602,9 @@ int kvm_arch_on_sigbus(int code, void *addr)
>
>  void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
>  {
> +    if (cs->singlestep_enabled) {
> +        dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_SINGLESTEP;
> +    }

Doesn't kvm_update_guest_debug() already set these bits, or am
I misreading it?

>      if (kvm_sw_breakpoints_active(cs)) {
>          dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;
>      }
> --
> 2.6.3

thanks
-- PMM

Re: [Qemu-devel] [PATCH v9 3/6] target-arm: kvm - support for single step

[Peter Maydell]

On 12 November 2015 at 16:20, Alex Bennée <alex.bennee@linaro.org> wrote:
> This adds support for single-step. There isn't much to do on the QEMU
> side as after we set-up the request for single step via the debug ioctl
> it is all handled within the kernel.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> ---
> v2
>   - convert to using HSR_EC
> v3
>   - use internals.h definitions
> ---
>  target-arm/kvm.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/target-arm/kvm.c b/target-arm/kvm.c
> index 50f70ef..d505a7e 100644
> --- a/target-arm/kvm.c
> +++ b/target-arm/kvm.c
> @@ -535,6 +535,13 @@ static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
>      kvm_cpu_synchronize_state(cs);
>
>      switch (hsr_ec) {
> +    case EC_SOFTWARESTEP:
> +        if (cs->singlestep_enabled) {
> +            return true;
> +        } else {
> +            error_report("Came out of SINGLE STEP when not enabled");
> +        }
> +        break;
>      case EC_AA64_BKPT:
>          if (kvm_find_sw_breakpoint(cs, env->pc)) {
>              return true;
> @@ -595,6 +602,9 @@ int kvm_arch_on_sigbus(int code, void *addr)
>
>  void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
>  {
> +    if (cs->singlestep_enabled) {
> +        dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_SINGLESTEP;
> +    }

Doesn't kvm_update_guest_debug() already set these bits, or am
I misreading it?

>      if (kvm_sw_breakpoints_active(cs)) {
>          dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;
>      }
> --
> 2.6.3

thanks
-- PMM

Re: [PATCH v9 3/6] target-arm: kvm - support for single step

[Alex Bennée]

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

> On 12 November 2015 at 16:20, Alex Bennée <alex.bennee@linaro.org> wrote:
>> This adds support for single-step. There isn't much to do on the QEMU
>> side as after we set-up the request for single step via the debug ioctl
>> it is all handled within the kernel.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>
>> ---
>> v2
>>   - convert to using HSR_EC
>> v3
>>   - use internals.h definitions
>> ---
>>  target-arm/kvm.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/target-arm/kvm.c b/target-arm/kvm.c
>> index 50f70ef..d505a7e 100644
>> --- a/target-arm/kvm.c
>> +++ b/target-arm/kvm.c
>> @@ -535,6 +535,13 @@ static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
>>      kvm_cpu_synchronize_state(cs);
>>
>>      switch (hsr_ec) {
>> +    case EC_SOFTWARESTEP:
>> +        if (cs->singlestep_enabled) {
>> +            return true;
>> +        } else {
>> +            error_report("Came out of SINGLE STEP when not enabled");
>> +        }
>> +        break;
>>      case EC_AA64_BKPT:
>>          if (kvm_find_sw_breakpoint(cs, env->pc)) {
>>              return true;
>> @@ -595,6 +602,9 @@ int kvm_arch_on_sigbus(int code, void *addr)
>>
>>  void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
>>  {
>> +    if (cs->singlestep_enabled) {
>> +        dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_SINGLESTEP;
>> +    }
>
> Doesn't kvm_update_guest_debug() already set these bits, or am
> I misreading it?

Yeah. This raises an interesting problem about what to do when we don't
have the capability. I could suppress those bits in the update function
but that seems a bit hacky.

Looking at the GDB capability code there doesn't seem to report
breakpoint capability short of just failing when you try to set one.

>
>>      if (kvm_sw_breakpoints_active(cs)) {
>>          dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;
>>      }
>> --
>> 2.6.3
>
> thanks
> -- PMM


--
Alex Bennée
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v9 3/6] target-arm: kvm - support for single step

[Alex Bennée]

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

> On 12 November 2015 at 16:20, Alex Benn?e <alex.bennee@linaro.org> wrote:
>> This adds support for single-step. There isn't much to do on the QEMU
>> side as after we set-up the request for single step via the debug ioctl
>> it is all handled within the kernel.
>>
>> Signed-off-by: Alex Benn?e <alex.bennee@linaro.org>
>>
>> ---
>> v2
>>   - convert to using HSR_EC
>> v3
>>   - use internals.h definitions
>> ---
>>  target-arm/kvm.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/target-arm/kvm.c b/target-arm/kvm.c
>> index 50f70ef..d505a7e 100644
>> --- a/target-arm/kvm.c
>> +++ b/target-arm/kvm.c
>> @@ -535,6 +535,13 @@ static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
>>      kvm_cpu_synchronize_state(cs);
>>
>>      switch (hsr_ec) {
>> +    case EC_SOFTWARESTEP:
>> +        if (cs->singlestep_enabled) {
>> +            return true;
>> +        } else {
>> +            error_report("Came out of SINGLE STEP when not enabled");
>> +        }
>> +        break;
>>      case EC_AA64_BKPT:
>>          if (kvm_find_sw_breakpoint(cs, env->pc)) {
>>              return true;
>> @@ -595,6 +602,9 @@ int kvm_arch_on_sigbus(int code, void *addr)
>>
>>  void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
>>  {
>> +    if (cs->singlestep_enabled) {
>> +        dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_SINGLESTEP;
>> +    }
>
> Doesn't kvm_update_guest_debug() already set these bits, or am
> I misreading it?

Yeah. This raises an interesting problem about what to do when we don't
have the capability. I could suppress those bits in the update function
but that seems a bit hacky.

Looking at the GDB capability code there doesn't seem to report
breakpoint capability short of just failing when you try to set one.

>
>>      if (kvm_sw_breakpoints_active(cs)) {
>>          dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;
>>      }
>> --
>> 2.6.3
>
> thanks
> -- PMM


--
Alex Benn?e

Re: [Qemu-devel] [PATCH v9 3/6] target-arm: kvm - support for single step

[Alex Bennée]

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

> On 12 November 2015 at 16:20, Alex Bennée <alex.bennee@linaro.org> wrote:
>> This adds support for single-step. There isn't much to do on the QEMU
>> side as after we set-up the request for single step via the debug ioctl
>> it is all handled within the kernel.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>
>> ---
>> v2
>>   - convert to using HSR_EC
>> v3
>>   - use internals.h definitions
>> ---
>>  target-arm/kvm.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/target-arm/kvm.c b/target-arm/kvm.c
>> index 50f70ef..d505a7e 100644
>> --- a/target-arm/kvm.c
>> +++ b/target-arm/kvm.c
>> @@ -535,6 +535,13 @@ static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
>>      kvm_cpu_synchronize_state(cs);
>>
>>      switch (hsr_ec) {
>> +    case EC_SOFTWARESTEP:
>> +        if (cs->singlestep_enabled) {
>> +            return true;
>> +        } else {
>> +            error_report("Came out of SINGLE STEP when not enabled");
>> +        }
>> +        break;
>>      case EC_AA64_BKPT:
>>          if (kvm_find_sw_breakpoint(cs, env->pc)) {
>>              return true;
>> @@ -595,6 +602,9 @@ int kvm_arch_on_sigbus(int code, void *addr)
>>
>>  void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
>>  {
>> +    if (cs->singlestep_enabled) {
>> +        dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_SINGLESTEP;
>> +    }
>
> Doesn't kvm_update_guest_debug() already set these bits, or am
> I misreading it?

Yeah. This raises an interesting problem about what to do when we don't
have the capability. I could suppress those bits in the update function
but that seems a bit hacky.

Looking at the GDB capability code there doesn't seem to report
breakpoint capability short of just failing when you try to set one.

>
>>      if (kvm_sw_breakpoints_active(cs)) {
>>          dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;
>>      }
>> --
>> 2.6.3
>
> thanks
> -- PMM


--
Alex Bennée

[PATCH v9 4/6] target-arm: kvm - add support for HW assisted debug

[Alex Bennée]

This adds basic support for HW assisted debug. The ioctl interface to
KVM allows us to pass an implementation defined number of break and
watch point registers. When KVM_GUESTDBG_USE_HW is specified these
debug registers will be installed in place on the world switch into the
guest.

The hardware is actually capable of more advanced matching but it is
unclear if this expressiveness is available via the gdbstub protocol.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v2
  - correct setting of PMC/BAS/MASK
  - improved commentary
  - added helper function to check watchpoint in range
  - fix find/deletion of watchpoints
v3
  - use internals.h definitions
v6
  - KVM_GUESTDBG_USE_HW_BP->KVM_GUESTDBG_USE_HW
  - renamed some helper functions to avoid confusion
v9
  - fix merge conflicts on re-base
  - rm asm/ptrace.h include
  - add additional commentry for hw breakpoints
  - explain gdb's model for HW bkpts
  - fix up spacing, formatting as per checkpatch
  - better PAC values
  - use is_power_of_2
  - use _arm_ fn naming and add docs
  - add a CPUWatchpoint structure for reporting
  - replace manual array manipulation with g_array abstraction
---
 target-arm/kvm.c     |  38 +++---
 target-arm/kvm64.c   | 352 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 target-arm/kvm_arm.h |  38 ++++++
 3 files changed, 406 insertions(+), 22 deletions(-)

diff --git a/target-arm/kvm.c b/target-arm/kvm.c
index d505a7e..1f57e92 100644
--- a/target-arm/kvm.c
+++ b/target-arm/kvm.c
@@ -547,6 +547,20 @@ static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
             return true;
         }
         break;
+    case EC_BREAKPOINT:
+        if (kvm_arm_find_hw_breakpoint(cs, env->pc)) {
+            return true;
+        }
+        break;
+    case EC_WATCHPOINT:
+    {
+        CPUWatchpoint *wp = kvm_arm_find_hw_watchpoint(cs, arch_info->far);
+        if (wp) {
+            cs->watchpoint_hit = wp;
+            return true;
+        }
+        break;
+    }
     default:
         error_report("%s: unhandled debug exit (%"PRIx32", %"PRIx64")\n",
                      __func__, arch_info->hsr, env->pc);
@@ -608,6 +622,10 @@ void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
     if (kvm_sw_breakpoints_active(cs)) {
         dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;
     }
+    if (kvm_arm_hw_debug_active(cs)) {
+        dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_HW;
+        kvm_arm_copy_hw_debug_data(&dbg->arch);
+    }
 }
 
 /* C6.6.29 BRK instruction */
@@ -635,26 +653,6 @@ int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
     return 0;
 }
 
-int kvm_arch_insert_hw_breakpoint(target_ulong addr,
-                                  target_ulong len, int type)
-{
-    qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
-    return -EINVAL;
-}
-
-int kvm_arch_remove_hw_breakpoint(target_ulong addr,
-                                  target_ulong len, int type)
-{
-    qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
-    return -EINVAL;
-}
-
-
-void kvm_arch_remove_all_hw_breakpoints(void)
-{
-    qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
-}
-
 void kvm_arch_init_irq_routing(KVMState *s)
 {
 }
diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
index d087794..c468324 100644
--- a/target-arm/kvm64.c
+++ b/target-arm/kvm64.c
@@ -2,6 +2,7 @@
  * ARM implementation of KVM hooks, 64 bit specific code
  *
  * Copyright Mian-M. Hamayun 2013, Virtual Open Systems
+ * Copyright Alex Bennée 2014, Linaro
  *
  * This work is licensed under the terms of the GNU GPL, version 2 or later.
  * See the COPYING file in the top-level directory.
@@ -12,12 +13,17 @@
 #include <sys/types.h>
 #include <sys/ioctl.h>
 #include <sys/mman.h>
+#include <sys/ptrace.h>
 
+#include <linux/elf.h>
 #include <linux/kvm.h>
 
 #include "config-host.h"
 #include "qemu-common.h"
 #include "qemu/timer.h"
+#include "qemu/host-utils.h"
+#include "qemu/error-report.h"
+#include "exec/gdbstub.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/kvm.h"
 #include "kvm_arm.h"
@@ -27,20 +33,362 @@
 
 static bool have_guest_debug;
 
+/*
+ * Although the ARM implementation of hardware assisted debugging
+ * allows for different breakpoints per-core the current GDB interface
+ * treats them as a global pool of registers (which seems to be the
+ * case for x86, ppc and s390). As a result we store one copy of
+ * registers which is used for all active cores.
+ *
+ * Write access is serialised by virtue of the GDB protocol which
+ * updates things. Read access (i.e. when the values are copied to the
+ * vCPU) is also gated by GDBs run control.
+ *
+ * This is not unreasonable as most of the time debugging kernels you
+ * never know which core will eventually execute your function.
+ */
+
+typedef struct {
+    uint64_t bcr;
+    uint64_t bvr;
+} HWBreakpoint;
+
+/* The watchpoint registers can cover more area than the requested
+ * watchpoint so we need to store the additional information
+ * somewhere. We also need to supply a CPUWatchpoint to the GDB stub
+ * when the watchpoint is hit.
+ */
+typedef struct {
+    uint64_t wcr;
+    uint64_t wvr;
+    CPUWatchpoint details;
+} HWWatchpoint;
+
+/* Maximum and current break/watch point counts */
+int max_hw_bps, max_hw_wps;
+GArray *hw_breakpoints, *hw_watchpoints;
+
+#define cur_hw_wps      (hw_watchpoints->len)
+#define cur_hw_bps      (hw_breakpoints->len)
+#define get_hw_bp(i)    (&g_array_index(hw_breakpoints, HWBreakpoint, i))
+#define get_hw_wp(i)    (&g_array_index(hw_watchpoints, HWWatchpoint, i))
+
 /**
- * kvm_arm_init_debug()
+ * kvm_arm_init_debug() - check for guest debug capabilities
  * @cs: CPUState
  *
- * Check for guest debug capabilities.
+ * kvm_check_extension returns 0 if we have no debug registers or the
+ * number we have.
  *
  */
 static void kvm_arm_init_debug(CPUState *cs)
 {
     have_guest_debug = kvm_check_extension(cs->kvm_state,
                                            KVM_CAP_SET_GUEST_DEBUG);
+
+    max_hw_wps = kvm_check_extension(cs->kvm_state, KVM_CAP_GUEST_DEBUG_HW_WPS);
+    hw_watchpoints = g_array_sized_new(true, true,
+                                       sizeof(HWWatchpoint), max_hw_wps);
+
+    max_hw_bps = kvm_check_extension(cs->kvm_state, KVM_CAP_GUEST_DEBUG_HW_BPS);
+    hw_breakpoints = g_array_sized_new(true, true,
+                                       sizeof(HWBreakpoint), max_hw_bps);
     return;
 }
 
+/**
+ * insert_hw_breakpoint()
+ * @addr: address of breakpoint
+ *
+ * See ARM ARM D2.9.1 for details but here we are only going to create
+ * simple un-linked breakpoints (i.e. we don't chain breakpoints
+ * together to match address and context or vmid). The hardware is
+ * capable of fancier matching but that will require exposing that
+ * fanciness to GDB's interface
+ *
+ * D7.3.2 DBGBCR<n>_EL1, Debug Breakpoint Control Registers
+ *
+ *  31  24 23  20 19   16 15 14  13  12   9 8   5 4    3 2   1  0
+ * +------+------+-------+-----+----+------+-----+------+-----+---+
+ * | RES0 |  BT  |  LBN  | SSC | HMC| RES0 | BAS | RES0 | PMC | E |
+ * +------+------+-------+-----+----+------+-----+------+-----+---+
+ *
+ * BT: Breakpoint type (0 = unlinked address match)
+ * LBN: Linked BP number (0 = unused)
+ * SSC/HMC/PMC: Security, Higher and Priv access control (Table D-12)
+ * BAS: Byte Address Select (RES1 for AArch64)
+ * E: Enable bit
+ */
+static int insert_hw_breakpoint(target_ulong addr)
+{
+    HWBreakpoint brk = {
+        .bcr = 0x1,                             /* BCR E=1, enable */
+        .bvr = addr
+    };
+
+    if (cur_hw_bps >= max_hw_bps) {
+        return -ENOBUFS;
+    }
+
+    brk.bcr = deposit32(brk.bcr, 1, 2, 0x3);   /* PMC = 11 */
+    brk.bcr = deposit32(brk.bcr, 5, 4, 0xf);   /* BAS = RES1 */
+
+    g_array_append_val(hw_breakpoints, brk);
+
+    return 0;
+}
+
+/**
+ * delete_hw_breakpoint()
+ * @pc: address of breakpoint
+ *
+ * Delete a breakpoint and shuffle any above down
+ */
+
+static int delete_hw_breakpoint(target_ulong pc)
+{
+    int i;
+    for (i = 0; i < hw_breakpoints->len; i++) {
+        HWBreakpoint *brk = get_hw_bp(i);
+        if (brk->bvr == pc) {
+            g_array_remove_index(hw_breakpoints, i);
+            return 0;
+        }
+    }
+    return -ENOENT;
+}
+
+/**
+ * insert_hw_watchpoint()
+ * @addr: address of watch point
+ * @len: size of area
+ * @type: type of watch point
+ *
+ * See ARM ARM D2.10. As with the breakpoints we can do some advanced
+ * stuff if we want to. The watch points can be linked with the break
+ * points above to make them context aware. However for simplicity
+ * currently we only deal with simple read/write watch points.
+ *
+ * D7.3.11 DBGWCR<n>_EL1, Debug Watchpoint Control Registers
+ *
+ *  31  29 28   24 23  21  20  19 16 15 14  13   12  5 4   3 2   1  0
+ * +------+-------+------+----+-----+-----+-----+-----+-----+-----+---+
+ * | RES0 |  MASK | RES0 | WT | LBN | SSC | HMC | BAS | LSC | PAC | E |
+ * +------+-------+------+----+-----+-----+-----+-----+-----+-----+---+
+ *
+ * MASK: num bits addr mask (0=none,01/10=res,11=3 bits (8 bytes))
+ * WT: 0 - unlinked, 1 - linked (not currently used)
+ * LBN: Linked BP number (not currently used)
+ * SSC/HMC/PAC: Security, Higher and Priv access control (Table D2-11)
+ * BAS: Byte Address Select
+ * LSC: Load/Store control (01: load, 10: store, 11: both)
+ * E: Enable
+ *
+ * The bottom 2 bits of the value register are masked. Therefore to
+ * break on any sizes smaller than an unaligned word you need to set
+ * MASK=0, BAS=bit per byte in question. For larger regions (^2) you
+ * need to ensure you mask the address as required and set BAS=0xff
+ */
+
+static int insert_hw_watchpoint(target_ulong addr,
+                                target_ulong len, int type)
+{
+    HWWatchpoint wp = {
+        .wcr = 1, /* E=1, enable */
+        .wvr = addr & (~0x7ULL),
+        .details = { .vaddr = addr, .len = len}
+    };
+
+    if (cur_hw_wps >= max_hw_wps) {
+        return -ENOBUFS;
+    }
+
+    /*
+     * HMC=0 SSC=0 PAC=3 will hit EL0 or EL1, any security state,
+     * valid whether EL3 is implemented or not
+     */
+    wp.wcr = deposit32(wp.wcr, 1, 2, 3);
+
+    switch (type) {
+    case GDB_WATCHPOINT_READ:
+        wp.wcr = deposit32(wp.wcr, 3, 2, 1);
+        wp.details.flags = BP_MEM_READ;
+        break;
+    case GDB_WATCHPOINT_WRITE:
+        wp.wcr = deposit32(wp.wcr, 3, 2, 2);
+        wp.details.flags = BP_MEM_WRITE;
+        break;
+    case GDB_WATCHPOINT_ACCESS:
+        wp.wcr = deposit32(wp.wcr, 3, 2, 3);
+        wp.details.flags = BP_MEM_ACCESS;
+        break;
+    default:
+        g_assert_not_reached();
+        break;
+    }
+    if (len <= 8) {
+        /* we align the address and set the bits in BAS */
+        int off = addr & 0x7;
+        int bas = (1 << len)-1;
+
+        wp.wcr = deposit32(wp.wcr, 5 + off, 8 - off, bas);
+    } else {
+        /* For ranges above 8 bytes we need to be a power of 2 */
+        if (is_power_of_2(len)) {
+            int bits = ctz64(len);
+
+            wp.wvr &= ~((1 << bits)-1);
+            wp.wcr = deposit32(wp.wcr, 24, 4, bits);
+            wp.wcr = deposit32(wp.wcr, 5, 8, 0xff);
+        } else {
+            return -ENOBUFS;
+        }
+    }
+
+    g_array_append_val(hw_watchpoints, wp);
+    return 0;
+}
+
+
+static bool check_watchpoint_in_range(int i, target_ulong addr)
+{
+    HWWatchpoint *wp = get_hw_wp(i);
+    uint64_t addr_top, addr_bottom = wp->wvr;
+    int bas = extract32(wp->wcr, 5, 8);
+    int mask = extract32(wp->wcr, 24, 4);
+
+    if (mask) {
+        addr_top = addr_bottom + (1 << mask);
+    } else {
+        /* BAS must be contiguous but can offset against the base
+         * address in DBGWVR */
+        addr_bottom = addr_bottom + ctz32(bas);
+        addr_top = addr_bottom + clo32(bas);
+    }
+
+    if (addr >= addr_bottom && addr <= addr_top) {
+        return true;
+    }
+
+    return false;
+}
+
+/**
+ * delete_hw_watchpoint()
+ * @addr: address of breakpoint
+ *
+ * Delete a breakpoint and shuffle any above down
+ */
+
+static int delete_hw_watchpoint(target_ulong addr,
+                                target_ulong len, int type)
+{
+    int i;
+    for (i = 0; i < cur_hw_wps; i++) {
+        if (check_watchpoint_in_range(i, addr)) {
+            g_array_remove_index(hw_watchpoints, i);
+            return 0;
+        }
+    }
+    return -ENOENT;
+}
+
+
+int kvm_arch_insert_hw_breakpoint(target_ulong addr,
+                                  target_ulong len, int type)
+{
+    switch (type) {
+    case GDB_BREAKPOINT_HW:
+        return insert_hw_breakpoint(addr);
+        break;
+    case GDB_WATCHPOINT_READ:
+    case GDB_WATCHPOINT_WRITE:
+    case GDB_WATCHPOINT_ACCESS:
+        return insert_hw_watchpoint(addr, len, type);
+    default:
+        return -ENOSYS;
+    }
+}
+
+int kvm_arch_remove_hw_breakpoint(target_ulong addr,
+                                  target_ulong len, int type)
+{
+    switch (type) {
+    case GDB_BREAKPOINT_HW:
+        return delete_hw_breakpoint(addr);
+        break;
+    case GDB_WATCHPOINT_READ:
+    case GDB_WATCHPOINT_WRITE:
+    case GDB_WATCHPOINT_ACCESS:
+        return delete_hw_watchpoint(addr, len, type);
+    default:
+        return -ENOSYS;
+    }
+}
+
+
+void kvm_arch_remove_all_hw_breakpoints(void)
+{
+    if (cur_hw_wps > 0) {
+        g_array_remove_range(hw_watchpoints, 0, cur_hw_wps);
+    }
+    if (cur_hw_bps > 0) {
+        g_array_remove_range(hw_breakpoints, 0, cur_hw_bps);
+    }
+}
+
+void kvm_arm_copy_hw_debug_data(struct kvm_guest_debug_arch *ptr)
+{
+    int i;
+    memset(ptr, 0, sizeof(struct kvm_guest_debug_arch));
+
+    for (i = 0; i < max_hw_wps; i++) {
+        HWWatchpoint *wp = get_hw_wp(i);
+        ptr->dbg_wcr[i] = wp->wcr;
+        ptr->dbg_wvr[i] = wp->wvr;
+    }
+    for (i = 0; i < max_hw_bps; i++) {
+        HWBreakpoint *bp = get_hw_bp(i);
+        ptr->dbg_bcr[i] = bp->bcr;
+        ptr->dbg_bvr[i] = bp->bvr;
+    }
+}
+
+bool kvm_arm_hw_debug_active(CPUState *cs)
+{
+    return ((cur_hw_wps > 0) || (cur_hw_bps > 0)) ? true : false;
+}
+
+bool kvm_arm_find_hw_breakpoint(CPUState *cpu, target_ulong pc)
+{
+    if (kvm_arm_hw_debug_active(cpu)) {
+        int i;
+
+        for (i = 0; i < cur_hw_bps; i++) {
+            HWBreakpoint *bp = get_hw_bp(i);
+            if (bp->bvr == pc) {
+                return true;
+            }
+        }
+    }
+    return false;
+}
+
+CPUWatchpoint *kvm_arm_find_hw_watchpoint(CPUState *cpu, target_ulong addr)
+{
+    if (kvm_arm_hw_debug_active(cpu)) {
+        int i;
+
+        for (i = 0; i < cur_hw_wps; i++) {
+            if (check_watchpoint_in_range(i, addr)) {
+                return &get_hw_wp(i)->details;
+            }
+        }
+    }
+    return NULL;
+}
+
+
 static inline void set_feature(uint64_t *features, int feature)
 {
     *features |= 1ULL << feature;
diff --git a/target-arm/kvm_arm.h b/target-arm/kvm_arm.h
index b516041..da88658 100644
--- a/target-arm/kvm_arm.h
+++ b/target-arm/kvm_arm.h
@@ -215,4 +215,42 @@ static inline const char *gic_class_name(void)
  */
 const char *gicv3_class_name(void);
 
+/**
+ * kvm_arm_hw_debug_active:
+ *
+ * Return TRUE is any hardware breakpoints in use.
+ */
+
+bool kvm_arm_hw_debug_active(CPUState *cs);
+
+/**
+ * kvm_arm_copy_hw_debug_data:
+ *
+ * @ptr: kvm_guest_debug_arch structure
+ *
+ * Copy the architecture specific debug registers into the
+ * kvm_guest_debug ioctl structure.
+ */
+struct kvm_guest_debug_arch;
+
+void kvm_arm_copy_hw_debug_data(struct kvm_guest_debug_arch *ptr);
+
+/**
+ * kvm_arm_find_hw_breakpoint:
+ * @cpu: CPUState
+ * @pc: pc of breakpoint
+ *
+ * Return TRUE if the pc matches one of our breakpoints.
+ */
+bool kvm_arm_find_hw_breakpoint(CPUState *cpu, target_ulong pc);
+
+/**
+ * kvm_arm_find_hw_watchpoint:
+ * @cpu: CPUState
+ * @addr: address of watchpoint
+ *
+ * Return the CPUWatchpoint structure the addr matches one of our watchpoints.
+ */
+CPUWatchpoint *kvm_arm_find_hw_watchpoint(CPUState *cpu, target_ulong addr);
+
 #endif
-- 
2.6.3

[PATCH v9 4/6] target-arm: kvm - add support for HW assisted debug

[Alex Bennée]

This adds basic support for HW assisted debug. The ioctl interface to
KVM allows us to pass an implementation defined number of break and
watch point registers. When KVM_GUESTDBG_USE_HW is specified these
debug registers will be installed in place on the world switch into the
guest.

The hardware is actually capable of more advanced matching but it is
unclear if this expressiveness is available via the gdbstub protocol.

Signed-off-by: Alex Benn?e <alex.bennee@linaro.org>

---
v2
  - correct setting of PMC/BAS/MASK
  - improved commentary
  - added helper function to check watchpoint in range
  - fix find/deletion of watchpoints
v3
  - use internals.h definitions
v6
  - KVM_GUESTDBG_USE_HW_BP->KVM_GUESTDBG_USE_HW
  - renamed some helper functions to avoid confusion
v9
  - fix merge conflicts on re-base
  - rm asm/ptrace.h include
  - add additional commentry for hw breakpoints
  - explain gdb's model for HW bkpts
  - fix up spacing, formatting as per checkpatch
  - better PAC values
  - use is_power_of_2
  - use _arm_ fn naming and add docs
  - add a CPUWatchpoint structure for reporting
  - replace manual array manipulation with g_array abstraction
---
 target-arm/kvm.c     |  38 +++---
 target-arm/kvm64.c   | 352 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 target-arm/kvm_arm.h |  38 ++++++
 3 files changed, 406 insertions(+), 22 deletions(-)

diff --git a/target-arm/kvm.c b/target-arm/kvm.c
index d505a7e..1f57e92 100644
--- a/target-arm/kvm.c
+++ b/target-arm/kvm.c
@@ -547,6 +547,20 @@ static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
             return true;
         }
         break;
+    case EC_BREAKPOINT:
+        if (kvm_arm_find_hw_breakpoint(cs, env->pc)) {
+            return true;
+        }
+        break;
+    case EC_WATCHPOINT:
+    {
+        CPUWatchpoint *wp = kvm_arm_find_hw_watchpoint(cs, arch_info->far);
+        if (wp) {
+            cs->watchpoint_hit = wp;
+            return true;
+        }
+        break;
+    }
     default:
         error_report("%s: unhandled debug exit (%"PRIx32", %"PRIx64")\n",
                      __func__, arch_info->hsr, env->pc);
@@ -608,6 +622,10 @@ void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
     if (kvm_sw_breakpoints_active(cs)) {
         dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;
     }
+    if (kvm_arm_hw_debug_active(cs)) {
+        dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_HW;
+        kvm_arm_copy_hw_debug_data(&dbg->arch);
+    }
 }
 
 /* C6.6.29 BRK instruction */
@@ -635,26 +653,6 @@ int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
     return 0;
 }
 
-int kvm_arch_insert_hw_breakpoint(target_ulong addr,
-                                  target_ulong len, int type)
-{
-    qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
-    return -EINVAL;
-}
-
-int kvm_arch_remove_hw_breakpoint(target_ulong addr,
-                                  target_ulong len, int type)
-{
-    qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
-    return -EINVAL;
-}
-
-
-void kvm_arch_remove_all_hw_breakpoints(void)
-{
-    qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
-}
-
 void kvm_arch_init_irq_routing(KVMState *s)
 {
 }
diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
index d087794..c468324 100644
--- a/target-arm/kvm64.c
+++ b/target-arm/kvm64.c
@@ -2,6 +2,7 @@
  * ARM implementation of KVM hooks, 64 bit specific code
  *
  * Copyright Mian-M. Hamayun 2013, Virtual Open Systems
+ * Copyright Alex Benn?e 2014, Linaro
  *
  * This work is licensed under the terms of the GNU GPL, version 2 or later.
  * See the COPYING file in the top-level directory.
@@ -12,12 +13,17 @@
 #include <sys/types.h>
 #include <sys/ioctl.h>
 #include <sys/mman.h>
+#include <sys/ptrace.h>
 
+#include <linux/elf.h>
 #include <linux/kvm.h>
 
 #include "config-host.h"
 #include "qemu-common.h"
 #include "qemu/timer.h"
+#include "qemu/host-utils.h"
+#include "qemu/error-report.h"
+#include "exec/gdbstub.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/kvm.h"
 #include "kvm_arm.h"
@@ -27,20 +33,362 @@
 
 static bool have_guest_debug;
 
+/*
+ * Although the ARM implementation of hardware assisted debugging
+ * allows for different breakpoints per-core the current GDB interface
+ * treats them as a global pool of registers (which seems to be the
+ * case for x86, ppc and s390). As a result we store one copy of
+ * registers which is used for all active cores.
+ *
+ * Write access is serialised by virtue of the GDB protocol which
+ * updates things. Read access (i.e. when the values are copied to the
+ * vCPU) is also gated by GDBs run control.
+ *
+ * This is not unreasonable as most of the time debugging kernels you
+ * never know which core will eventually execute your function.
+ */
+
+typedef struct {
+    uint64_t bcr;
+    uint64_t bvr;
+} HWBreakpoint;
+
+/* The watchpoint registers can cover more area than the requested
+ * watchpoint so we need to store the additional information
+ * somewhere. We also need to supply a CPUWatchpoint to the GDB stub
+ * when the watchpoint is hit.
+ */
+typedef struct {
+    uint64_t wcr;
+    uint64_t wvr;
+    CPUWatchpoint details;
+} HWWatchpoint;
+
+/* Maximum and current break/watch point counts */
+int max_hw_bps, max_hw_wps;
+GArray *hw_breakpoints, *hw_watchpoints;
+
+#define cur_hw_wps      (hw_watchpoints->len)
+#define cur_hw_bps      (hw_breakpoints->len)
+#define get_hw_bp(i)    (&g_array_index(hw_breakpoints, HWBreakpoint, i))
+#define get_hw_wp(i)    (&g_array_index(hw_watchpoints, HWWatchpoint, i))
+
 /**
- * kvm_arm_init_debug()
+ * kvm_arm_init_debug() - check for guest debug capabilities
  * @cs: CPUState
  *
- * Check for guest debug capabilities.
+ * kvm_check_extension returns 0 if we have no debug registers or the
+ * number we have.
  *
  */
 static void kvm_arm_init_debug(CPUState *cs)
 {
     have_guest_debug = kvm_check_extension(cs->kvm_state,
                                            KVM_CAP_SET_GUEST_DEBUG);
+
+    max_hw_wps = kvm_check_extension(cs->kvm_state, KVM_CAP_GUEST_DEBUG_HW_WPS);
+    hw_watchpoints = g_array_sized_new(true, true,
+                                       sizeof(HWWatchpoint), max_hw_wps);
+
+    max_hw_bps = kvm_check_extension(cs->kvm_state, KVM_CAP_GUEST_DEBUG_HW_BPS);
+    hw_breakpoints = g_array_sized_new(true, true,
+                                       sizeof(HWBreakpoint), max_hw_bps);
     return;
 }
 
+/**
+ * insert_hw_breakpoint()
+ * @addr: address of breakpoint
+ *
+ * See ARM ARM D2.9.1 for details but here we are only going to create
+ * simple un-linked breakpoints (i.e. we don't chain breakpoints
+ * together to match address and context or vmid). The hardware is
+ * capable of fancier matching but that will require exposing that
+ * fanciness to GDB's interface
+ *
+ * D7.3.2 DBGBCR<n>_EL1, Debug Breakpoint Control Registers
+ *
+ *  31  24 23  20 19   16 15 14  13  12   9 8   5 4    3 2   1  0
+ * +------+------+-------+-----+----+------+-----+------+-----+---+
+ * | RES0 |  BT  |  LBN  | SSC | HMC| RES0 | BAS | RES0 | PMC | E |
+ * +------+------+-------+-----+----+------+-----+------+-----+---+
+ *
+ * BT: Breakpoint type (0 = unlinked address match)
+ * LBN: Linked BP number (0 = unused)
+ * SSC/HMC/PMC: Security, Higher and Priv access control (Table D-12)
+ * BAS: Byte Address Select (RES1 for AArch64)
+ * E: Enable bit
+ */
+static int insert_hw_breakpoint(target_ulong addr)
+{
+    HWBreakpoint brk = {
+        .bcr = 0x1,                             /* BCR E=1, enable */
+        .bvr = addr
+    };
+
+    if (cur_hw_bps >= max_hw_bps) {
+        return -ENOBUFS;
+    }
+
+    brk.bcr = deposit32(brk.bcr, 1, 2, 0x3);   /* PMC = 11 */
+    brk.bcr = deposit32(brk.bcr, 5, 4, 0xf);   /* BAS = RES1 */
+
+    g_array_append_val(hw_breakpoints, brk);
+
+    return 0;
+}
+
+/**
+ * delete_hw_breakpoint()
+ * @pc: address of breakpoint
+ *
+ * Delete a breakpoint and shuffle any above down
+ */
+
+static int delete_hw_breakpoint(target_ulong pc)
+{
+    int i;
+    for (i = 0; i < hw_breakpoints->len; i++) {
+        HWBreakpoint *brk = get_hw_bp(i);
+        if (brk->bvr == pc) {
+            g_array_remove_index(hw_breakpoints, i);
+            return 0;
+        }
+    }
+    return -ENOENT;
+}
+
+/**
+ * insert_hw_watchpoint()
+ * @addr: address of watch point
+ * @len: size of area
+ * @type: type of watch point
+ *
+ * See ARM ARM D2.10. As with the breakpoints we can do some advanced
+ * stuff if we want to. The watch points can be linked with the break
+ * points above to make them context aware. However for simplicity
+ * currently we only deal with simple read/write watch points.
+ *
+ * D7.3.11 DBGWCR<n>_EL1, Debug Watchpoint Control Registers
+ *
+ *  31  29 28   24 23  21  20  19 16 15 14  13   12  5 4   3 2   1  0
+ * +------+-------+------+----+-----+-----+-----+-----+-----+-----+---+
+ * | RES0 |  MASK | RES0 | WT | LBN | SSC | HMC | BAS | LSC | PAC | E |
+ * +------+-------+------+----+-----+-----+-----+-----+-----+-----+---+
+ *
+ * MASK: num bits addr mask (0=none,01/10=res,11=3 bits (8 bytes))
+ * WT: 0 - unlinked, 1 - linked (not currently used)
+ * LBN: Linked BP number (not currently used)
+ * SSC/HMC/PAC: Security, Higher and Priv access control (Table D2-11)
+ * BAS: Byte Address Select
+ * LSC: Load/Store control (01: load, 10: store, 11: both)
+ * E: Enable
+ *
+ * The bottom 2 bits of the value register are masked. Therefore to
+ * break on any sizes smaller than an unaligned word you need to set
+ * MASK=0, BAS=bit per byte in question. For larger regions (^2) you
+ * need to ensure you mask the address as required and set BAS=0xff
+ */
+
+static int insert_hw_watchpoint(target_ulong addr,
+                                target_ulong len, int type)
+{
+    HWWatchpoint wp = {
+        .wcr = 1, /* E=1, enable */
+        .wvr = addr & (~0x7ULL),
+        .details = { .vaddr = addr, .len = len}
+    };
+
+    if (cur_hw_wps >= max_hw_wps) {
+        return -ENOBUFS;
+    }
+
+    /*
+     * HMC=0 SSC=0 PAC=3 will hit EL0 or EL1, any security state,
+     * valid whether EL3 is implemented or not
+     */
+    wp.wcr = deposit32(wp.wcr, 1, 2, 3);
+
+    switch (type) {
+    case GDB_WATCHPOINT_READ:
+        wp.wcr = deposit32(wp.wcr, 3, 2, 1);
+        wp.details.flags = BP_MEM_READ;
+        break;
+    case GDB_WATCHPOINT_WRITE:
+        wp.wcr = deposit32(wp.wcr, 3, 2, 2);
+        wp.details.flags = BP_MEM_WRITE;
+        break;
+    case GDB_WATCHPOINT_ACCESS:
+        wp.wcr = deposit32(wp.wcr, 3, 2, 3);
+        wp.details.flags = BP_MEM_ACCESS;
+        break;
+    default:
+        g_assert_not_reached();
+        break;
+    }
+    if (len <= 8) {
+        /* we align the address and set the bits in BAS */
+        int off = addr & 0x7;
+        int bas = (1 << len)-1;
+
+        wp.wcr = deposit32(wp.wcr, 5 + off, 8 - off, bas);
+    } else {
+        /* For ranges above 8 bytes we need to be a power of 2 */
+        if (is_power_of_2(len)) {
+            int bits = ctz64(len);
+
+            wp.wvr &= ~((1 << bits)-1);
+            wp.wcr = deposit32(wp.wcr, 24, 4, bits);
+            wp.wcr = deposit32(wp.wcr, 5, 8, 0xff);
+        } else {
+            return -ENOBUFS;
+        }
+    }
+
+    g_array_append_val(hw_watchpoints, wp);
+    return 0;
+}
+
+
+static bool check_watchpoint_in_range(int i, target_ulong addr)
+{
+    HWWatchpoint *wp = get_hw_wp(i);
+    uint64_t addr_top, addr_bottom = wp->wvr;
+    int bas = extract32(wp->wcr, 5, 8);
+    int mask = extract32(wp->wcr, 24, 4);
+
+    if (mask) {
+        addr_top = addr_bottom + (1 << mask);
+    } else {
+        /* BAS must be contiguous but can offset against the base
+         * address in DBGWVR */
+        addr_bottom = addr_bottom + ctz32(bas);
+        addr_top = addr_bottom + clo32(bas);
+    }
+
+    if (addr >= addr_bottom && addr <= addr_top) {
+        return true;
+    }
+
+    return false;
+}
+
+/**
+ * delete_hw_watchpoint()
+ * @addr: address of breakpoint
+ *
+ * Delete a breakpoint and shuffle any above down
+ */
+
+static int delete_hw_watchpoint(target_ulong addr,
+                                target_ulong len, int type)
+{
+    int i;
+    for (i = 0; i < cur_hw_wps; i++) {
+        if (check_watchpoint_in_range(i, addr)) {
+            g_array_remove_index(hw_watchpoints, i);
+            return 0;
+        }
+    }
+    return -ENOENT;
+}
+
+
+int kvm_arch_insert_hw_breakpoint(target_ulong addr,
+                                  target_ulong len, int type)
+{
+    switch (type) {
+    case GDB_BREAKPOINT_HW:
+        return insert_hw_breakpoint(addr);
+        break;
+    case GDB_WATCHPOINT_READ:
+    case GDB_WATCHPOINT_WRITE:
+    case GDB_WATCHPOINT_ACCESS:
+        return insert_hw_watchpoint(addr, len, type);
+    default:
+        return -ENOSYS;
+    }
+}
+
+int kvm_arch_remove_hw_breakpoint(target_ulong addr,
+                                  target_ulong len, int type)
+{
+    switch (type) {
+    case GDB_BREAKPOINT_HW:
+        return delete_hw_breakpoint(addr);
+        break;
+    case GDB_WATCHPOINT_READ:
+    case GDB_WATCHPOINT_WRITE:
+    case GDB_WATCHPOINT_ACCESS:
+        return delete_hw_watchpoint(addr, len, type);
+    default:
+        return -ENOSYS;
+    }
+}
+
+
+void kvm_arch_remove_all_hw_breakpoints(void)
+{
+    if (cur_hw_wps > 0) {
+        g_array_remove_range(hw_watchpoints, 0, cur_hw_wps);
+    }
+    if (cur_hw_bps > 0) {
+        g_array_remove_range(hw_breakpoints, 0, cur_hw_bps);
+    }
+}
+
+void kvm_arm_copy_hw_debug_data(struct kvm_guest_debug_arch *ptr)
+{
+    int i;
+    memset(ptr, 0, sizeof(struct kvm_guest_debug_arch));
+
+    for (i = 0; i < max_hw_wps; i++) {
+        HWWatchpoint *wp = get_hw_wp(i);
+        ptr->dbg_wcr[i] = wp->wcr;
+        ptr->dbg_wvr[i] = wp->wvr;
+    }
+    for (i = 0; i < max_hw_bps; i++) {
+        HWBreakpoint *bp = get_hw_bp(i);
+        ptr->dbg_bcr[i] = bp->bcr;
+        ptr->dbg_bvr[i] = bp->bvr;
+    }
+}
+
+bool kvm_arm_hw_debug_active(CPUState *cs)
+{
+    return ((cur_hw_wps > 0) || (cur_hw_bps > 0)) ? true : false;
+}
+
+bool kvm_arm_find_hw_breakpoint(CPUState *cpu, target_ulong pc)
+{
+    if (kvm_arm_hw_debug_active(cpu)) {
+        int i;
+
+        for (i = 0; i < cur_hw_bps; i++) {
+            HWBreakpoint *bp = get_hw_bp(i);
+            if (bp->bvr == pc) {
+                return true;
+            }
+        }
+    }
+    return false;
+}
+
+CPUWatchpoint *kvm_arm_find_hw_watchpoint(CPUState *cpu, target_ulong addr)
+{
+    if (kvm_arm_hw_debug_active(cpu)) {
+        int i;
+
+        for (i = 0; i < cur_hw_wps; i++) {
+            if (check_watchpoint_in_range(i, addr)) {
+                return &get_hw_wp(i)->details;
+            }
+        }
+    }
+    return NULL;
+}
+
+
 static inline void set_feature(uint64_t *features, int feature)
 {
     *features |= 1ULL << feature;
diff --git a/target-arm/kvm_arm.h b/target-arm/kvm_arm.h
index b516041..da88658 100644
--- a/target-arm/kvm_arm.h
+++ b/target-arm/kvm_arm.h
@@ -215,4 +215,42 @@ static inline const char *gic_class_name(void)
  */
 const char *gicv3_class_name(void);
 
+/**
+ * kvm_arm_hw_debug_active:
+ *
+ * Return TRUE is any hardware breakpoints in use.
+ */
+
+bool kvm_arm_hw_debug_active(CPUState *cs);
+
+/**
+ * kvm_arm_copy_hw_debug_data:
+ *
+ * @ptr: kvm_guest_debug_arch structure
+ *
+ * Copy the architecture specific debug registers into the
+ * kvm_guest_debug ioctl structure.
+ */
+struct kvm_guest_debug_arch;
+
+void kvm_arm_copy_hw_debug_data(struct kvm_guest_debug_arch *ptr);
+
+/**
+ * kvm_arm_find_hw_breakpoint:
+ * @cpu: CPUState
+ * @pc: pc of breakpoint
+ *
+ * Return TRUE if the pc matches one of our breakpoints.
+ */
+bool kvm_arm_find_hw_breakpoint(CPUState *cpu, target_ulong pc);
+
+/**
+ * kvm_arm_find_hw_watchpoint:
+ * @cpu: CPUState
+ * @addr: address of watchpoint
+ *
+ * Return the CPUWatchpoint structure the addr matches one of our watchpoints.
+ */
+CPUWatchpoint *kvm_arm_find_hw_watchpoint(CPUState *cpu, target_ulong addr);
+
 #endif
-- 
2.6.3

[Qemu-devel] [PATCH v9 4/6] target-arm: kvm - add support for HW assisted debug

[Alex Bennée]

This adds basic support for HW assisted debug. The ioctl interface to
KVM allows us to pass an implementation defined number of break and
watch point registers. When KVM_GUESTDBG_USE_HW is specified these
debug registers will be installed in place on the world switch into the
guest.

The hardware is actually capable of more advanced matching but it is
unclear if this expressiveness is available via the gdbstub protocol.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v2
  - correct setting of PMC/BAS/MASK
  - improved commentary
  - added helper function to check watchpoint in range
  - fix find/deletion of watchpoints
v3
  - use internals.h definitions
v6
  - KVM_GUESTDBG_USE_HW_BP->KVM_GUESTDBG_USE_HW
  - renamed some helper functions to avoid confusion
v9
  - fix merge conflicts on re-base
  - rm asm/ptrace.h include
  - add additional commentry for hw breakpoints
  - explain gdb's model for HW bkpts
  - fix up spacing, formatting as per checkpatch
  - better PAC values
  - use is_power_of_2
  - use _arm_ fn naming and add docs
  - add a CPUWatchpoint structure for reporting
  - replace manual array manipulation with g_array abstraction
---
 target-arm/kvm.c     |  38 +++---
 target-arm/kvm64.c   | 352 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 target-arm/kvm_arm.h |  38 ++++++
 3 files changed, 406 insertions(+), 22 deletions(-)

diff --git a/target-arm/kvm.c b/target-arm/kvm.c
index d505a7e..1f57e92 100644
--- a/target-arm/kvm.c
+++ b/target-arm/kvm.c
@@ -547,6 +547,20 @@ static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
             return true;
         }
         break;
+    case EC_BREAKPOINT:
+        if (kvm_arm_find_hw_breakpoint(cs, env->pc)) {
+            return true;
+        }
+        break;
+    case EC_WATCHPOINT:
+    {
+        CPUWatchpoint *wp = kvm_arm_find_hw_watchpoint(cs, arch_info->far);
+        if (wp) {
+            cs->watchpoint_hit = wp;
+            return true;
+        }
+        break;
+    }
     default:
         error_report("%s: unhandled debug exit (%"PRIx32", %"PRIx64")\n",
                      __func__, arch_info->hsr, env->pc);
@@ -608,6 +622,10 @@ void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
     if (kvm_sw_breakpoints_active(cs)) {
         dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;
     }
+    if (kvm_arm_hw_debug_active(cs)) {
+        dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_HW;
+        kvm_arm_copy_hw_debug_data(&dbg->arch);
+    }
 }
 
 /* C6.6.29 BRK instruction */
@@ -635,26 +653,6 @@ int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
     return 0;
 }
 
-int kvm_arch_insert_hw_breakpoint(target_ulong addr,
-                                  target_ulong len, int type)
-{
-    qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
-    return -EINVAL;
-}
-
-int kvm_arch_remove_hw_breakpoint(target_ulong addr,
-                                  target_ulong len, int type)
-{
-    qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
-    return -EINVAL;
-}
-
-
-void kvm_arch_remove_all_hw_breakpoints(void)
-{
-    qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
-}
-
 void kvm_arch_init_irq_routing(KVMState *s)
 {
 }
diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
index d087794..c468324 100644
--- a/target-arm/kvm64.c
+++ b/target-arm/kvm64.c
@@ -2,6 +2,7 @@
  * ARM implementation of KVM hooks, 64 bit specific code
  *
  * Copyright Mian-M. Hamayun 2013, Virtual Open Systems
+ * Copyright Alex Bennée 2014, Linaro
  *
  * This work is licensed under the terms of the GNU GPL, version 2 or later.
  * See the COPYING file in the top-level directory.
@@ -12,12 +13,17 @@
 #include <sys/types.h>
 #include <sys/ioctl.h>
 #include <sys/mman.h>
+#include <sys/ptrace.h>
 
+#include <linux/elf.h>
 #include <linux/kvm.h>
 
 #include "config-host.h"
 #include "qemu-common.h"
 #include "qemu/timer.h"
+#include "qemu/host-utils.h"
+#include "qemu/error-report.h"
+#include "exec/gdbstub.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/kvm.h"
 #include "kvm_arm.h"
@@ -27,20 +33,362 @@
 
 static bool have_guest_debug;
 
+/*
+ * Although the ARM implementation of hardware assisted debugging
+ * allows for different breakpoints per-core the current GDB interface
+ * treats them as a global pool of registers (which seems to be the
+ * case for x86, ppc and s390). As a result we store one copy of
+ * registers which is used for all active cores.
+ *
+ * Write access is serialised by virtue of the GDB protocol which
+ * updates things. Read access (i.e. when the values are copied to the
+ * vCPU) is also gated by GDBs run control.
+ *
+ * This is not unreasonable as most of the time debugging kernels you
+ * never know which core will eventually execute your function.
+ */
+
+typedef struct {
+    uint64_t bcr;
+    uint64_t bvr;
+} HWBreakpoint;
+
+/* The watchpoint registers can cover more area than the requested
+ * watchpoint so we need to store the additional information
+ * somewhere. We also need to supply a CPUWatchpoint to the GDB stub
+ * when the watchpoint is hit.
+ */
+typedef struct {
+    uint64_t wcr;
+    uint64_t wvr;
+    CPUWatchpoint details;
+} HWWatchpoint;
+
+/* Maximum and current break/watch point counts */
+int max_hw_bps, max_hw_wps;
+GArray *hw_breakpoints, *hw_watchpoints;
+
+#define cur_hw_wps      (hw_watchpoints->len)
+#define cur_hw_bps      (hw_breakpoints->len)
+#define get_hw_bp(i)    (&g_array_index(hw_breakpoints, HWBreakpoint, i))
+#define get_hw_wp(i)    (&g_array_index(hw_watchpoints, HWWatchpoint, i))
+
 /**
- * kvm_arm_init_debug()
+ * kvm_arm_init_debug() - check for guest debug capabilities
  * @cs: CPUState
  *
- * Check for guest debug capabilities.
+ * kvm_check_extension returns 0 if we have no debug registers or the
+ * number we have.
  *
  */
 static void kvm_arm_init_debug(CPUState *cs)
 {
     have_guest_debug = kvm_check_extension(cs->kvm_state,
                                            KVM_CAP_SET_GUEST_DEBUG);
+
+    max_hw_wps = kvm_check_extension(cs->kvm_state, KVM_CAP_GUEST_DEBUG_HW_WPS);
+    hw_watchpoints = g_array_sized_new(true, true,
+                                       sizeof(HWWatchpoint), max_hw_wps);
+
+    max_hw_bps = kvm_check_extension(cs->kvm_state, KVM_CAP_GUEST_DEBUG_HW_BPS);
+    hw_breakpoints = g_array_sized_new(true, true,
+                                       sizeof(HWBreakpoint), max_hw_bps);
     return;
 }
 
+/**
+ * insert_hw_breakpoint()
+ * @addr: address of breakpoint
+ *
+ * See ARM ARM D2.9.1 for details but here we are only going to create
+ * simple un-linked breakpoints (i.e. we don't chain breakpoints
+ * together to match address and context or vmid). The hardware is
+ * capable of fancier matching but that will require exposing that
+ * fanciness to GDB's interface
+ *
+ * D7.3.2 DBGBCR<n>_EL1, Debug Breakpoint Control Registers
+ *
+ *  31  24 23  20 19   16 15 14  13  12   9 8   5 4    3 2   1  0
+ * +------+------+-------+-----+----+------+-----+------+-----+---+
+ * | RES0 |  BT  |  LBN  | SSC | HMC| RES0 | BAS | RES0 | PMC | E |
+ * +------+------+-------+-----+----+------+-----+------+-----+---+
+ *
+ * BT: Breakpoint type (0 = unlinked address match)
+ * LBN: Linked BP number (0 = unused)
+ * SSC/HMC/PMC: Security, Higher and Priv access control (Table D-12)
+ * BAS: Byte Address Select (RES1 for AArch64)
+ * E: Enable bit
+ */
+static int insert_hw_breakpoint(target_ulong addr)
+{
+    HWBreakpoint brk = {
+        .bcr = 0x1,                             /* BCR E=1, enable */
+        .bvr = addr
+    };
+
+    if (cur_hw_bps >= max_hw_bps) {
+        return -ENOBUFS;
+    }
+
+    brk.bcr = deposit32(brk.bcr, 1, 2, 0x3);   /* PMC = 11 */
+    brk.bcr = deposit32(brk.bcr, 5, 4, 0xf);   /* BAS = RES1 */
+
+    g_array_append_val(hw_breakpoints, brk);
+
+    return 0;
+}
+
+/**
+ * delete_hw_breakpoint()
+ * @pc: address of breakpoint
+ *
+ * Delete a breakpoint and shuffle any above down
+ */
+
+static int delete_hw_breakpoint(target_ulong pc)
+{
+    int i;
+    for (i = 0; i < hw_breakpoints->len; i++) {
+        HWBreakpoint *brk = get_hw_bp(i);
+        if (brk->bvr == pc) {
+            g_array_remove_index(hw_breakpoints, i);
+            return 0;
+        }
+    }
+    return -ENOENT;
+}
+
+/**
+ * insert_hw_watchpoint()
+ * @addr: address of watch point
+ * @len: size of area
+ * @type: type of watch point
+ *
+ * See ARM ARM D2.10. As with the breakpoints we can do some advanced
+ * stuff if we want to. The watch points can be linked with the break
+ * points above to make them context aware. However for simplicity
+ * currently we only deal with simple read/write watch points.
+ *
+ * D7.3.11 DBGWCR<n>_EL1, Debug Watchpoint Control Registers
+ *
+ *  31  29 28   24 23  21  20  19 16 15 14  13   12  5 4   3 2   1  0
+ * +------+-------+------+----+-----+-----+-----+-----+-----+-----+---+
+ * | RES0 |  MASK | RES0 | WT | LBN | SSC | HMC | BAS | LSC | PAC | E |
+ * +------+-------+------+----+-----+-----+-----+-----+-----+-----+---+
+ *
+ * MASK: num bits addr mask (0=none,01/10=res,11=3 bits (8 bytes))
+ * WT: 0 - unlinked, 1 - linked (not currently used)
+ * LBN: Linked BP number (not currently used)
+ * SSC/HMC/PAC: Security, Higher and Priv access control (Table D2-11)
+ * BAS: Byte Address Select
+ * LSC: Load/Store control (01: load, 10: store, 11: both)
+ * E: Enable
+ *
+ * The bottom 2 bits of the value register are masked. Therefore to
+ * break on any sizes smaller than an unaligned word you need to set
+ * MASK=0, BAS=bit per byte in question. For larger regions (^2) you
+ * need to ensure you mask the address as required and set BAS=0xff
+ */
+
+static int insert_hw_watchpoint(target_ulong addr,
+                                target_ulong len, int type)
+{
+    HWWatchpoint wp = {
+        .wcr = 1, /* E=1, enable */
+        .wvr = addr & (~0x7ULL),
+        .details = { .vaddr = addr, .len = len}
+    };
+
+    if (cur_hw_wps >= max_hw_wps) {
+        return -ENOBUFS;
+    }
+
+    /*
+     * HMC=0 SSC=0 PAC=3 will hit EL0 or EL1, any security state,
+     * valid whether EL3 is implemented or not
+     */
+    wp.wcr = deposit32(wp.wcr, 1, 2, 3);
+
+    switch (type) {
+    case GDB_WATCHPOINT_READ:
+        wp.wcr = deposit32(wp.wcr, 3, 2, 1);
+        wp.details.flags = BP_MEM_READ;
+        break;
+    case GDB_WATCHPOINT_WRITE:
+        wp.wcr = deposit32(wp.wcr, 3, 2, 2);
+        wp.details.flags = BP_MEM_WRITE;
+        break;
+    case GDB_WATCHPOINT_ACCESS:
+        wp.wcr = deposit32(wp.wcr, 3, 2, 3);
+        wp.details.flags = BP_MEM_ACCESS;
+        break;
+    default:
+        g_assert_not_reached();
+        break;
+    }
+    if (len <= 8) {
+        /* we align the address and set the bits in BAS */
+        int off = addr & 0x7;
+        int bas = (1 << len)-1;
+
+        wp.wcr = deposit32(wp.wcr, 5 + off, 8 - off, bas);
+    } else {
+        /* For ranges above 8 bytes we need to be a power of 2 */
+        if (is_power_of_2(len)) {
+            int bits = ctz64(len);
+
+            wp.wvr &= ~((1 << bits)-1);
+            wp.wcr = deposit32(wp.wcr, 24, 4, bits);
+            wp.wcr = deposit32(wp.wcr, 5, 8, 0xff);
+        } else {
+            return -ENOBUFS;
+        }
+    }
+
+    g_array_append_val(hw_watchpoints, wp);
+    return 0;
+}
+
+
+static bool check_watchpoint_in_range(int i, target_ulong addr)
+{
+    HWWatchpoint *wp = get_hw_wp(i);
+    uint64_t addr_top, addr_bottom = wp->wvr;
+    int bas = extract32(wp->wcr, 5, 8);
+    int mask = extract32(wp->wcr, 24, 4);
+
+    if (mask) {
+        addr_top = addr_bottom + (1 << mask);
+    } else {
+        /* BAS must be contiguous but can offset against the base
+         * address in DBGWVR */
+        addr_bottom = addr_bottom + ctz32(bas);
+        addr_top = addr_bottom + clo32(bas);
+    }
+
+    if (addr >= addr_bottom && addr <= addr_top) {
+        return true;
+    }
+
+    return false;
+}
+
+/**
+ * delete_hw_watchpoint()
+ * @addr: address of breakpoint
+ *
+ * Delete a breakpoint and shuffle any above down
+ */
+
+static int delete_hw_watchpoint(target_ulong addr,
+                                target_ulong len, int type)
+{
+    int i;
+    for (i = 0; i < cur_hw_wps; i++) {
+        if (check_watchpoint_in_range(i, addr)) {
+            g_array_remove_index(hw_watchpoints, i);
+            return 0;
+        }
+    }
+    return -ENOENT;
+}
+
+
+int kvm_arch_insert_hw_breakpoint(target_ulong addr,
+                                  target_ulong len, int type)
+{
+    switch (type) {
+    case GDB_BREAKPOINT_HW:
+        return insert_hw_breakpoint(addr);
+        break;
+    case GDB_WATCHPOINT_READ:
+    case GDB_WATCHPOINT_WRITE:
+    case GDB_WATCHPOINT_ACCESS:
+        return insert_hw_watchpoint(addr, len, type);
+    default:
+        return -ENOSYS;
+    }
+}
+
+int kvm_arch_remove_hw_breakpoint(target_ulong addr,
+                                  target_ulong len, int type)
+{
+    switch (type) {
+    case GDB_BREAKPOINT_HW:
+        return delete_hw_breakpoint(addr);
+        break;
+    case GDB_WATCHPOINT_READ:
+    case GDB_WATCHPOINT_WRITE:
+    case GDB_WATCHPOINT_ACCESS:
+        return delete_hw_watchpoint(addr, len, type);
+    default:
+        return -ENOSYS;
+    }
+}
+
+
+void kvm_arch_remove_all_hw_breakpoints(void)
+{
+    if (cur_hw_wps > 0) {
+        g_array_remove_range(hw_watchpoints, 0, cur_hw_wps);
+    }
+    if (cur_hw_bps > 0) {
+        g_array_remove_range(hw_breakpoints, 0, cur_hw_bps);
+    }
+}
+
+void kvm_arm_copy_hw_debug_data(struct kvm_guest_debug_arch *ptr)
+{
+    int i;
+    memset(ptr, 0, sizeof(struct kvm_guest_debug_arch));
+
+    for (i = 0; i < max_hw_wps; i++) {
+        HWWatchpoint *wp = get_hw_wp(i);
+        ptr->dbg_wcr[i] = wp->wcr;
+        ptr->dbg_wvr[i] = wp->wvr;
+    }
+    for (i = 0; i < max_hw_bps; i++) {
+        HWBreakpoint *bp = get_hw_bp(i);
+        ptr->dbg_bcr[i] = bp->bcr;
+        ptr->dbg_bvr[i] = bp->bvr;
+    }
+}
+
+bool kvm_arm_hw_debug_active(CPUState *cs)
+{
+    return ((cur_hw_wps > 0) || (cur_hw_bps > 0)) ? true : false;
+}
+
+bool kvm_arm_find_hw_breakpoint(CPUState *cpu, target_ulong pc)
+{
+    if (kvm_arm_hw_debug_active(cpu)) {
+        int i;
+
+        for (i = 0; i < cur_hw_bps; i++) {
+            HWBreakpoint *bp = get_hw_bp(i);
+            if (bp->bvr == pc) {
+                return true;
+            }
+        }
+    }
+    return false;
+}
+
+CPUWatchpoint *kvm_arm_find_hw_watchpoint(CPUState *cpu, target_ulong addr)
+{
+    if (kvm_arm_hw_debug_active(cpu)) {
+        int i;
+
+        for (i = 0; i < cur_hw_wps; i++) {
+            if (check_watchpoint_in_range(i, addr)) {
+                return &get_hw_wp(i)->details;
+            }
+        }
+    }
+    return NULL;
+}
+
+
 static inline void set_feature(uint64_t *features, int feature)
 {
     *features |= 1ULL << feature;
diff --git a/target-arm/kvm_arm.h b/target-arm/kvm_arm.h
index b516041..da88658 100644
--- a/target-arm/kvm_arm.h
+++ b/target-arm/kvm_arm.h
@@ -215,4 +215,42 @@ static inline const char *gic_class_name(void)
  */
 const char *gicv3_class_name(void);
 
+/**
+ * kvm_arm_hw_debug_active:
+ *
+ * Return TRUE is any hardware breakpoints in use.
+ */
+
+bool kvm_arm_hw_debug_active(CPUState *cs);
+
+/**
+ * kvm_arm_copy_hw_debug_data:
+ *
+ * @ptr: kvm_guest_debug_arch structure
+ *
+ * Copy the architecture specific debug registers into the
+ * kvm_guest_debug ioctl structure.
+ */
+struct kvm_guest_debug_arch;
+
+void kvm_arm_copy_hw_debug_data(struct kvm_guest_debug_arch *ptr);
+
+/**
+ * kvm_arm_find_hw_breakpoint:
+ * @cpu: CPUState
+ * @pc: pc of breakpoint
+ *
+ * Return TRUE if the pc matches one of our breakpoints.
+ */
+bool kvm_arm_find_hw_breakpoint(CPUState *cpu, target_ulong pc);
+
+/**
+ * kvm_arm_find_hw_watchpoint:
+ * @cpu: CPUState
+ * @addr: address of watchpoint
+ *
+ * Return the CPUWatchpoint structure the addr matches one of our watchpoints.
+ */
+CPUWatchpoint *kvm_arm_find_hw_watchpoint(CPUState *cpu, target_ulong addr);
+
 #endif
-- 
2.6.3

Re: [PATCH v9 4/6] target-arm: kvm - add support for HW assisted debug

[Peter Maydell]

On 12 November 2015 at 16:20, Alex Bennée <alex.bennee@linaro.org> wrote:
> This adds basic support for HW assisted debug. The ioctl interface to
> KVM allows us to pass an implementation defined number of break and
> watch point registers. When KVM_GUESTDBG_USE_HW is specified these
> debug registers will be installed in place on the world switch into the
> guest.
>
> The hardware is actually capable of more advanced matching but it is
> unclear if this expressiveness is available via the gdbstub protocol.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> ---
> v2
>   - correct setting of PMC/BAS/MASK
>   - improved commentary
>   - added helper function to check watchpoint in range
>   - fix find/deletion of watchpoints
> v3
>   - use internals.h definitions
> v6
>   - KVM_GUESTDBG_USE_HW_BP->KVM_GUESTDBG_USE_HW
>   - renamed some helper functions to avoid confusion
> v9
>   - fix merge conflicts on re-base
>   - rm asm/ptrace.h include
>   - add additional commentry for hw breakpoints
>   - explain gdb's model for HW bkpts
>   - fix up spacing, formatting as per checkpatch
>   - better PAC values
>   - use is_power_of_2
>   - use _arm_ fn naming and add docs
>   - add a CPUWatchpoint structure for reporting
>   - replace manual array manipulation with g_array abstraction
> ---
>  target-arm/kvm.c     |  38 +++---
>  target-arm/kvm64.c   | 352 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  target-arm/kvm_arm.h |  38 ++++++
>  3 files changed, 406 insertions(+), 22 deletions(-)
>
> diff --git a/target-arm/kvm.c b/target-arm/kvm.c
> index d505a7e..1f57e92 100644
> --- a/target-arm/kvm.c
> +++ b/target-arm/kvm.c
> @@ -547,6 +547,20 @@ static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
>              return true;
>          }
>          break;
> +    case EC_BREAKPOINT:
> +        if (kvm_arm_find_hw_breakpoint(cs, env->pc)) {
> +            return true;
> +        }
> +        break;
> +    case EC_WATCHPOINT:
> +    {
> +        CPUWatchpoint *wp = kvm_arm_find_hw_watchpoint(cs, arch_info->far);

This won't compile for 32-bit ARM.

> +        if (wp) {
> +            cs->watchpoint_hit = wp;
> +            return true;
> +        }
> +        break;
> +    }
>      default:
>          error_report("%s: unhandled debug exit (%"PRIx32", %"PRIx64")\n",
>                       __func__, arch_info->hsr, env->pc);
> @@ -608,6 +622,10 @@ void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
>      if (kvm_sw_breakpoints_active(cs)) {
>          dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;
>      }
> +    if (kvm_arm_hw_debug_active(cs)) {
> +        dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_HW;
> +        kvm_arm_copy_hw_debug_data(&dbg->arch);
> +    }
>  }
>
>  /* C6.6.29 BRK instruction */
> @@ -635,26 +653,6 @@ int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
>      return 0;
>  }
>
> -int kvm_arch_insert_hw_breakpoint(target_ulong addr,
> -                                  target_ulong len, int type)
> -{
> -    qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
> -    return -EINVAL;
> -}

You've moved these functions into kvm64.c but haven't provided
a stub version in kvm32.c...

> -
> -int kvm_arch_remove_hw_breakpoint(target_ulong addr,
> -                                  target_ulong len, int type)
> -{
> -    qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
> -    return -EINVAL;
> -}
> -
> -
> -void kvm_arch_remove_all_hw_breakpoints(void)
> -{
> -    qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
> -}
> -
>  void kvm_arch_init_irq_routing(KVMState *s)
>  {
>  }
> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
> index d087794..c468324 100644
> --- a/target-arm/kvm64.c
> +++ b/target-arm/kvm64.c
> @@ -2,6 +2,7 @@
>   * ARM implementation of KVM hooks, 64 bit specific code
>   *
>   * Copyright Mian-M. Hamayun 2013, Virtual Open Systems
> + * Copyright Alex Bennée 2014, Linaro
>   *
>   * This work is licensed under the terms of the GNU GPL, version 2 or later.
>   * See the COPYING file in the top-level directory.
> @@ -12,12 +13,17 @@
>  #include <sys/types.h>
>  #include <sys/ioctl.h>
>  #include <sys/mman.h>
> +#include <sys/ptrace.h>
>
> +#include <linux/elf.h>
>  #include <linux/kvm.h>
>
>  #include "config-host.h"
>  #include "qemu-common.h"
>  #include "qemu/timer.h"
> +#include "qemu/host-utils.h"
> +#include "qemu/error-report.h"
> +#include "exec/gdbstub.h"
>  #include "sysemu/sysemu.h"
>  #include "sysemu/kvm.h"
>  #include "kvm_arm.h"
> @@ -27,20 +33,362 @@
>
>  static bool have_guest_debug;
>
> +/*
> + * Although the ARM implementation of hardware assisted debugging
> + * allows for different breakpoints per-core the current GDB interface

Comma after "per-core".

> + * treats them as a global pool of registers (which seems to be the
> + * case for x86, ppc and s390). As a result we store one copy of
> + * registers which is used for all active cores.
> + *
> + * Write access is serialised by virtue of the GDB protocol which
> + * updates things. Read access (i.e. when the values are copied to the
> + * vCPU) is also gated by GDBs run control.

"GDB's".

> + *
> + * This is not unreasonable as most of the time debugging kernels you
> + * never know which core will eventually execute your function.
> + */
> +
> +typedef struct {
> +    uint64_t bcr;
> +    uint64_t bvr;
> +} HWBreakpoint;
> +
> +/* The watchpoint registers can cover more area than the requested
> + * watchpoint so we need to store the additional information
> + * somewhere. We also need to supply a CPUWatchpoint to the GDB stub
> + * when the watchpoint is hit.
> + */
> +typedef struct {
> +    uint64_t wcr;
> +    uint64_t wvr;
> +    CPUWatchpoint details;
> +} HWWatchpoint;
> +
> +/* Maximum and current break/watch point counts */
> +int max_hw_bps, max_hw_wps;
> +GArray *hw_breakpoints, *hw_watchpoints;
> +
> +#define cur_hw_wps      (hw_watchpoints->len)
> +#define cur_hw_bps      (hw_breakpoints->len)
> +#define get_hw_bp(i)    (&g_array_index(hw_breakpoints, HWBreakpoint, i))
> +#define get_hw_wp(i)    (&g_array_index(hw_watchpoints, HWWatchpoint, i))
> +
>  /**
> - * kvm_arm_init_debug()
> + * kvm_arm_init_debug() - check for guest debug capabilities
>   * @cs: CPUState
>   *
> - * Check for guest debug capabilities.
> + * kvm_check_extension returns 0 if we have no debug registers or the
> + * number we have.

this would be easier to read phrased as "kvm_check_extension returns
the number of debug registers we have (which might be none)".

>   *
>   */
>  static void kvm_arm_init_debug(CPUState *cs)
>  {
>      have_guest_debug = kvm_check_extension(cs->kvm_state,
>                                             KVM_CAP_SET_GUEST_DEBUG);
> +
> +    max_hw_wps = kvm_check_extension(cs->kvm_state, KVM_CAP_GUEST_DEBUG_HW_WPS);
> +    hw_watchpoints = g_array_sized_new(true, true,
> +                                       sizeof(HWWatchpoint), max_hw_wps);
> +
> +    max_hw_bps = kvm_check_extension(cs->kvm_state, KVM_CAP_GUEST_DEBUG_HW_BPS);
> +    hw_breakpoints = g_array_sized_new(true, true,
> +                                       sizeof(HWBreakpoint), max_hw_bps);
>      return;
>  }
>
> +/**
> + * insert_hw_breakpoint()
> + * @addr: address of breakpoint
> + *
> + * See ARM ARM D2.9.1 for details but here we are only going to create
> + * simple un-linked breakpoints (i.e. we don't chain breakpoints
> + * together to match address and context or vmid). The hardware is
> + * capable of fancier matching but that will require exposing that
> + * fanciness to GDB's interface
> + *
> + * D7.3.2 DBGBCR<n>_EL1, Debug Breakpoint Control Registers
> + *
> + *  31  24 23  20 19   16 15 14  13  12   9 8   5 4    3 2   1  0
> + * +------+------+-------+-----+----+------+-----+------+-----+---+
> + * | RES0 |  BT  |  LBN  | SSC | HMC| RES0 | BAS | RES0 | PMC | E |
> + * +------+------+-------+-----+----+------+-----+------+-----+---+
> + *
> + * BT: Breakpoint type (0 = unlinked address match)
> + * LBN: Linked BP number (0 = unused)
> + * SSC/HMC/PMC: Security, Higher and Priv access control (Table D-12)
> + * BAS: Byte Address Select (RES1 for AArch64)
> + * E: Enable bit
> + */
> +static int insert_hw_breakpoint(target_ulong addr)
> +{
> +    HWBreakpoint brk = {
> +        .bcr = 0x1,                             /* BCR E=1, enable */
> +        .bvr = addr
> +    };
> +
> +    if (cur_hw_bps >= max_hw_bps) {
> +        return -ENOBUFS;
> +    }
> +
> +    brk.bcr = deposit32(brk.bcr, 1, 2, 0x3);   /* PMC = 11 */
> +    brk.bcr = deposit32(brk.bcr, 5, 4, 0xf);   /* BAS = RES1 */
> +
> +    g_array_append_val(hw_breakpoints, brk);
> +
> +    return 0;
> +}
> +
> +/**
> + * delete_hw_breakpoint()
> + * @pc: address of breakpoint
> + *
> + * Delete a breakpoint and shuffle any above down
> + */
> +
> +static int delete_hw_breakpoint(target_ulong pc)
> +{
> +    int i;
> +    for (i = 0; i < hw_breakpoints->len; i++) {
> +        HWBreakpoint *brk = get_hw_bp(i);
> +        if (brk->bvr == pc) {
> +            g_array_remove_index(hw_breakpoints, i);
> +            return 0;
> +        }
> +    }
> +    return -ENOENT;
> +}
> +
> +/**
> + * insert_hw_watchpoint()
> + * @addr: address of watch point
> + * @len: size of area
> + * @type: type of watch point
> + *
> + * See ARM ARM D2.10. As with the breakpoints we can do some advanced
> + * stuff if we want to. The watch points can be linked with the break
> + * points above to make them context aware. However for simplicity
> + * currently we only deal with simple read/write watch points.
> + *
> + * D7.3.11 DBGWCR<n>_EL1, Debug Watchpoint Control Registers
> + *
> + *  31  29 28   24 23  21  20  19 16 15 14  13   12  5 4   3 2   1  0
> + * +------+-------+------+----+-----+-----+-----+-----+-----+-----+---+
> + * | RES0 |  MASK | RES0 | WT | LBN | SSC | HMC | BAS | LSC | PAC | E |
> + * +------+-------+------+----+-----+-----+-----+-----+-----+-----+---+
> + *
> + * MASK: num bits addr mask (0=none,01/10=res,11=3 bits (8 bytes))
> + * WT: 0 - unlinked, 1 - linked (not currently used)
> + * LBN: Linked BP number (not currently used)
> + * SSC/HMC/PAC: Security, Higher and Priv access control (Table D2-11)
> + * BAS: Byte Address Select
> + * LSC: Load/Store control (01: load, 10: store, 11: both)
> + * E: Enable
> + *
> + * The bottom 2 bits of the value register are masked. Therefore to
> + * break on any sizes smaller than an unaligned word you need to set
> + * MASK=0, BAS=bit per byte in question. For larger regions (^2) you
> + * need to ensure you mask the address as required and set BAS=0xff
> + */
> +
> +static int insert_hw_watchpoint(target_ulong addr,
> +                                target_ulong len, int type)
> +{
> +    HWWatchpoint wp = {
> +        .wcr = 1, /* E=1, enable */
> +        .wvr = addr & (~0x7ULL),
> +        .details = { .vaddr = addr, .len = len}

Should at least have a space after "len".

> +    };
> +
> +    if (cur_hw_wps >= max_hw_wps) {
> +        return -ENOBUFS;
> +    }
> +
> +    /*
> +     * HMC=0 SSC=0 PAC=3 will hit EL0 or EL1, any security state,
> +     * valid whether EL3 is implemented or not
> +     */
> +    wp.wcr = deposit32(wp.wcr, 1, 2, 3);
> +
> +    switch (type) {
> +    case GDB_WATCHPOINT_READ:
> +        wp.wcr = deposit32(wp.wcr, 3, 2, 1);
> +        wp.details.flags = BP_MEM_READ;
> +        break;
> +    case GDB_WATCHPOINT_WRITE:
> +        wp.wcr = deposit32(wp.wcr, 3, 2, 2);
> +        wp.details.flags = BP_MEM_WRITE;
> +        break;
> +    case GDB_WATCHPOINT_ACCESS:
> +        wp.wcr = deposit32(wp.wcr, 3, 2, 3);
> +        wp.details.flags = BP_MEM_ACCESS;
> +        break;
> +    default:
> +        g_assert_not_reached();
> +        break;
> +    }
> +    if (len <= 8) {
> +        /* we align the address and set the bits in BAS */
> +        int off = addr & 0x7;
> +        int bas = (1 << len)-1;

Missing spaces around operator "-" (here and below).

> +
> +        wp.wcr = deposit32(wp.wcr, 5 + off, 8 - off, bas);
> +    } else {
> +        /* For ranges above 8 bytes we need to be a power of 2 */
> +        if (is_power_of_2(len)) {
> +            int bits = ctz64(len);
> +
> +            wp.wvr &= ~((1 << bits)-1);
> +            wp.wcr = deposit32(wp.wcr, 24, 4, bits);
> +            wp.wcr = deposit32(wp.wcr, 5, 8, 0xff);
> +        } else {
> +            return -ENOBUFS;
> +        }
> +    }
> +
> +    g_array_append_val(hw_watchpoints, wp);
> +    return 0;
> +}
> +
> +
> +static bool check_watchpoint_in_range(int i, target_ulong addr)
> +{
> +    HWWatchpoint *wp = get_hw_wp(i);
> +    uint64_t addr_top, addr_bottom = wp->wvr;
> +    int bas = extract32(wp->wcr, 5, 8);
> +    int mask = extract32(wp->wcr, 24, 4);
> +
> +    if (mask) {
> +        addr_top = addr_bottom + (1 << mask);
> +    } else {
> +        /* BAS must be contiguous but can offset against the base
> +         * address in DBGWVR */
> +        addr_bottom = addr_bottom + ctz32(bas);
> +        addr_top = addr_bottom + clo32(bas);
> +    }
> +
> +    if (addr >= addr_bottom && addr <= addr_top) {
> +        return true;
> +    }
> +
> +    return false;
> +}
> +
> +/**
> + * delete_hw_watchpoint()
> + * @addr: address of breakpoint
> + *
> + * Delete a breakpoint and shuffle any above down
> + */
> +
> +static int delete_hw_watchpoint(target_ulong addr,
> +                                target_ulong len, int type)
> +{
> +    int i;
> +    for (i = 0; i < cur_hw_wps; i++) {
> +        if (check_watchpoint_in_range(i, addr)) {
> +            g_array_remove_index(hw_watchpoints, i);
> +            return 0;
> +        }
> +    }
> +    return -ENOENT;
> +}
> +
> +
> +int kvm_arch_insert_hw_breakpoint(target_ulong addr,
> +                                  target_ulong len, int type)
> +{
> +    switch (type) {
> +    case GDB_BREAKPOINT_HW:
> +        return insert_hw_breakpoint(addr);
> +        break;
> +    case GDB_WATCHPOINT_READ:
> +    case GDB_WATCHPOINT_WRITE:
> +    case GDB_WATCHPOINT_ACCESS:
> +        return insert_hw_watchpoint(addr, len, type);
> +    default:
> +        return -ENOSYS;
> +    }
> +}
> +
> +int kvm_arch_remove_hw_breakpoint(target_ulong addr,
> +                                  target_ulong len, int type)
> +{
> +    switch (type) {
> +    case GDB_BREAKPOINT_HW:
> +        return delete_hw_breakpoint(addr);
> +        break;
> +    case GDB_WATCHPOINT_READ:
> +    case GDB_WATCHPOINT_WRITE:
> +    case GDB_WATCHPOINT_ACCESS:
> +        return delete_hw_watchpoint(addr, len, type);
> +    default:
> +        return -ENOSYS;
> +    }
> +}
> +
> +
> +void kvm_arch_remove_all_hw_breakpoints(void)
> +{
> +    if (cur_hw_wps > 0) {
> +        g_array_remove_range(hw_watchpoints, 0, cur_hw_wps);
> +    }
> +    if (cur_hw_bps > 0) {
> +        g_array_remove_range(hw_breakpoints, 0, cur_hw_bps);
> +    }
> +}
> +
> +void kvm_arm_copy_hw_debug_data(struct kvm_guest_debug_arch *ptr)
> +{
> +    int i;
> +    memset(ptr, 0, sizeof(struct kvm_guest_debug_arch));
> +
> +    for (i = 0; i < max_hw_wps; i++) {
> +        HWWatchpoint *wp = get_hw_wp(i);
> +        ptr->dbg_wcr[i] = wp->wcr;
> +        ptr->dbg_wvr[i] = wp->wvr;
> +    }
> +    for (i = 0; i < max_hw_bps; i++) {
> +        HWBreakpoint *bp = get_hw_bp(i);
> +        ptr->dbg_bcr[i] = bp->bcr;
> +        ptr->dbg_bvr[i] = bp->bvr;
> +    }
> +}
> +
> +bool kvm_arm_hw_debug_active(CPUState *cs)
> +{
> +    return ((cur_hw_wps > 0) || (cur_hw_bps > 0)) ? true : false;

You don't nede the ?: here.

> +}
> +
> +bool kvm_arm_find_hw_breakpoint(CPUState *cpu, target_ulong pc)
> +{
> +    if (kvm_arm_hw_debug_active(cpu)) {

Do we need to do this check? I think the loop condition
will DTRT if there aren't any current breakpoints.

> +        int i;
> +
> +        for (i = 0; i < cur_hw_bps; i++) {
> +            HWBreakpoint *bp = get_hw_bp(i);
> +            if (bp->bvr == pc) {
> +                return true;
> +            }
> +        }
> +    }
> +    return false;
> +}
> +
> +CPUWatchpoint *kvm_arm_find_hw_watchpoint(CPUState *cpu, target_ulong addr)
> +{
> +    if (kvm_arm_hw_debug_active(cpu)) {

Similarly here.

> +        int i;
> +
> +        for (i = 0; i < cur_hw_wps; i++) {
> +            if (check_watchpoint_in_range(i, addr)) {
> +                return &get_hw_wp(i)->details;
> +            }
> +        }
> +    }
> +    return NULL;
> +}
> +
> +
>  static inline void set_feature(uint64_t *features, int feature)
>  {
>      *features |= 1ULL << feature;
> diff --git a/target-arm/kvm_arm.h b/target-arm/kvm_arm.h
> index b516041..da88658 100644
> --- a/target-arm/kvm_arm.h
> +++ b/target-arm/kvm_arm.h
> @@ -215,4 +215,42 @@ static inline const char *gic_class_name(void)
>   */
>  const char *gicv3_class_name(void);
>
> +/**
> + * kvm_arm_hw_debug_active:
> + *
> + * Return TRUE is any hardware breakpoints in use.

"if".

> + */
> +
> +bool kvm_arm_hw_debug_active(CPUState *cs);
> +
> +/**
> + * kvm_arm_copy_hw_debug_data:
> + *
> + * @ptr: kvm_guest_debug_arch structure
> + *
> + * Copy the architecture specific debug registers into the
> + * kvm_guest_debug ioctl structure.
> + */
> +struct kvm_guest_debug_arch;
> +
> +void kvm_arm_copy_hw_debug_data(struct kvm_guest_debug_arch *ptr);
> +
> +/**
> + * kvm_arm_find_hw_breakpoint:
> + * @cpu: CPUState
> + * @pc: pc of breakpoint
> + *
> + * Return TRUE if the pc matches one of our breakpoints.
> + */
> +bool kvm_arm_find_hw_breakpoint(CPUState *cpu, target_ulong pc);
> +
> +/**
> + * kvm_arm_find_hw_watchpoint:
> + * @cpu: CPUState
> + * @addr: address of watchpoint
> + *
> + * Return the CPUWatchpoint structure the addr matches one of our watchpoints.

Missing "if" ? (or needs rephrasing)

> + */
> +CPUWatchpoint *kvm_arm_find_hw_watchpoint(CPUState *cpu, target_ulong addr);
> +
>  #endif
> --
> 2.6.3
>

thanks
-- PMM
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v9 4/6] target-arm: kvm - add support for HW assisted debug

[Peter Maydell]

On 12 November 2015 at 16:20, Alex Benn?e <alex.bennee@linaro.org> wrote:
> This adds basic support for HW assisted debug. The ioctl interface to
> KVM allows us to pass an implementation defined number of break and
> watch point registers. When KVM_GUESTDBG_USE_HW is specified these
> debug registers will be installed in place on the world switch into the
> guest.
>
> The hardware is actually capable of more advanced matching but it is
> unclear if this expressiveness is available via the gdbstub protocol.
>
> Signed-off-by: Alex Benn?e <alex.bennee@linaro.org>
>
> ---
> v2
>   - correct setting of PMC/BAS/MASK
>   - improved commentary
>   - added helper function to check watchpoint in range
>   - fix find/deletion of watchpoints
> v3
>   - use internals.h definitions
> v6
>   - KVM_GUESTDBG_USE_HW_BP->KVM_GUESTDBG_USE_HW
>   - renamed some helper functions to avoid confusion
> v9
>   - fix merge conflicts on re-base
>   - rm asm/ptrace.h include
>   - add additional commentry for hw breakpoints
>   - explain gdb's model for HW bkpts
>   - fix up spacing, formatting as per checkpatch
>   - better PAC values
>   - use is_power_of_2
>   - use _arm_ fn naming and add docs
>   - add a CPUWatchpoint structure for reporting
>   - replace manual array manipulation with g_array abstraction
> ---
>  target-arm/kvm.c     |  38 +++---
>  target-arm/kvm64.c   | 352 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  target-arm/kvm_arm.h |  38 ++++++
>  3 files changed, 406 insertions(+), 22 deletions(-)
>
> diff --git a/target-arm/kvm.c b/target-arm/kvm.c
> index d505a7e..1f57e92 100644
> --- a/target-arm/kvm.c
> +++ b/target-arm/kvm.c
> @@ -547,6 +547,20 @@ static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
>              return true;
>          }
>          break;
> +    case EC_BREAKPOINT:
> +        if (kvm_arm_find_hw_breakpoint(cs, env->pc)) {
> +            return true;
> +        }
> +        break;
> +    case EC_WATCHPOINT:
> +    {
> +        CPUWatchpoint *wp = kvm_arm_find_hw_watchpoint(cs, arch_info->far);

This won't compile for 32-bit ARM.

> +        if (wp) {
> +            cs->watchpoint_hit = wp;
> +            return true;
> +        }
> +        break;
> +    }
>      default:
>          error_report("%s: unhandled debug exit (%"PRIx32", %"PRIx64")\n",
>                       __func__, arch_info->hsr, env->pc);
> @@ -608,6 +622,10 @@ void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
>      if (kvm_sw_breakpoints_active(cs)) {
>          dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;
>      }
> +    if (kvm_arm_hw_debug_active(cs)) {
> +        dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_HW;
> +        kvm_arm_copy_hw_debug_data(&dbg->arch);
> +    }
>  }
>
>  /* C6.6.29 BRK instruction */
> @@ -635,26 +653,6 @@ int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
>      return 0;
>  }
>
> -int kvm_arch_insert_hw_breakpoint(target_ulong addr,
> -                                  target_ulong len, int type)
> -{
> -    qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
> -    return -EINVAL;
> -}

You've moved these functions into kvm64.c but haven't provided
a stub version in kvm32.c...

> -
> -int kvm_arch_remove_hw_breakpoint(target_ulong addr,
> -                                  target_ulong len, int type)
> -{
> -    qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
> -    return -EINVAL;
> -}
> -
> -
> -void kvm_arch_remove_all_hw_breakpoints(void)
> -{
> -    qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
> -}
> -
>  void kvm_arch_init_irq_routing(KVMState *s)
>  {
>  }
> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
> index d087794..c468324 100644
> --- a/target-arm/kvm64.c
> +++ b/target-arm/kvm64.c
> @@ -2,6 +2,7 @@
>   * ARM implementation of KVM hooks, 64 bit specific code
>   *
>   * Copyright Mian-M. Hamayun 2013, Virtual Open Systems
> + * Copyright Alex Benn?e 2014, Linaro
>   *
>   * This work is licensed under the terms of the GNU GPL, version 2 or later.
>   * See the COPYING file in the top-level directory.
> @@ -12,12 +13,17 @@
>  #include <sys/types.h>
>  #include <sys/ioctl.h>
>  #include <sys/mman.h>
> +#include <sys/ptrace.h>
>
> +#include <linux/elf.h>
>  #include <linux/kvm.h>
>
>  #include "config-host.h"
>  #include "qemu-common.h"
>  #include "qemu/timer.h"
> +#include "qemu/host-utils.h"
> +#include "qemu/error-report.h"
> +#include "exec/gdbstub.h"
>  #include "sysemu/sysemu.h"
>  #include "sysemu/kvm.h"
>  #include "kvm_arm.h"
> @@ -27,20 +33,362 @@
>
>  static bool have_guest_debug;
>
> +/*
> + * Although the ARM implementation of hardware assisted debugging
> + * allows for different breakpoints per-core the current GDB interface

Comma after "per-core".

> + * treats them as a global pool of registers (which seems to be the
> + * case for x86, ppc and s390). As a result we store one copy of
> + * registers which is used for all active cores.
> + *
> + * Write access is serialised by virtue of the GDB protocol which
> + * updates things. Read access (i.e. when the values are copied to the
> + * vCPU) is also gated by GDBs run control.

"GDB's".

> + *
> + * This is not unreasonable as most of the time debugging kernels you
> + * never know which core will eventually execute your function.
> + */
> +
> +typedef struct {
> +    uint64_t bcr;
> +    uint64_t bvr;
> +} HWBreakpoint;
> +
> +/* The watchpoint registers can cover more area than the requested
> + * watchpoint so we need to store the additional information
> + * somewhere. We also need to supply a CPUWatchpoint to the GDB stub
> + * when the watchpoint is hit.
> + */
> +typedef struct {
> +    uint64_t wcr;
> +    uint64_t wvr;
> +    CPUWatchpoint details;
> +} HWWatchpoint;
> +
> +/* Maximum and current break/watch point counts */
> +int max_hw_bps, max_hw_wps;
> +GArray *hw_breakpoints, *hw_watchpoints;
> +
> +#define cur_hw_wps      (hw_watchpoints->len)
> +#define cur_hw_bps      (hw_breakpoints->len)
> +#define get_hw_bp(i)    (&g_array_index(hw_breakpoints, HWBreakpoint, i))
> +#define get_hw_wp(i)    (&g_array_index(hw_watchpoints, HWWatchpoint, i))
> +
>  /**
> - * kvm_arm_init_debug()
> + * kvm_arm_init_debug() - check for guest debug capabilities
>   * @cs: CPUState
>   *
> - * Check for guest debug capabilities.
> + * kvm_check_extension returns 0 if we have no debug registers or the
> + * number we have.

this would be easier to read phrased as "kvm_check_extension returns
the number of debug registers we have (which might be none)".

>   *
>   */
>  static void kvm_arm_init_debug(CPUState *cs)
>  {
>      have_guest_debug = kvm_check_extension(cs->kvm_state,
>                                             KVM_CAP_SET_GUEST_DEBUG);
> +
> +    max_hw_wps = kvm_check_extension(cs->kvm_state, KVM_CAP_GUEST_DEBUG_HW_WPS);
> +    hw_watchpoints = g_array_sized_new(true, true,
> +                                       sizeof(HWWatchpoint), max_hw_wps);
> +
> +    max_hw_bps = kvm_check_extension(cs->kvm_state, KVM_CAP_GUEST_DEBUG_HW_BPS);
> +    hw_breakpoints = g_array_sized_new(true, true,
> +                                       sizeof(HWBreakpoint), max_hw_bps);
>      return;
>  }
>
> +/**
> + * insert_hw_breakpoint()
> + * @addr: address of breakpoint
> + *
> + * See ARM ARM D2.9.1 for details but here we are only going to create
> + * simple un-linked breakpoints (i.e. we don't chain breakpoints
> + * together to match address and context or vmid). The hardware is
> + * capable of fancier matching but that will require exposing that
> + * fanciness to GDB's interface
> + *
> + * D7.3.2 DBGBCR<n>_EL1, Debug Breakpoint Control Registers
> + *
> + *  31  24 23  20 19   16 15 14  13  12   9 8   5 4    3 2   1  0
> + * +------+------+-------+-----+----+------+-----+------+-----+---+
> + * | RES0 |  BT  |  LBN  | SSC | HMC| RES0 | BAS | RES0 | PMC | E |
> + * +------+------+-------+-----+----+------+-----+------+-----+---+
> + *
> + * BT: Breakpoint type (0 = unlinked address match)
> + * LBN: Linked BP number (0 = unused)
> + * SSC/HMC/PMC: Security, Higher and Priv access control (Table D-12)
> + * BAS: Byte Address Select (RES1 for AArch64)
> + * E: Enable bit
> + */
> +static int insert_hw_breakpoint(target_ulong addr)
> +{
> +    HWBreakpoint brk = {
> +        .bcr = 0x1,                             /* BCR E=1, enable */
> +        .bvr = addr
> +    };
> +
> +    if (cur_hw_bps >= max_hw_bps) {
> +        return -ENOBUFS;
> +    }
> +
> +    brk.bcr = deposit32(brk.bcr, 1, 2, 0x3);   /* PMC = 11 */
> +    brk.bcr = deposit32(brk.bcr, 5, 4, 0xf);   /* BAS = RES1 */
> +
> +    g_array_append_val(hw_breakpoints, brk);
> +
> +    return 0;
> +}
> +
> +/**
> + * delete_hw_breakpoint()
> + * @pc: address of breakpoint
> + *
> + * Delete a breakpoint and shuffle any above down
> + */
> +
> +static int delete_hw_breakpoint(target_ulong pc)
> +{
> +    int i;
> +    for (i = 0; i < hw_breakpoints->len; i++) {
> +        HWBreakpoint *brk = get_hw_bp(i);
> +        if (brk->bvr == pc) {
> +            g_array_remove_index(hw_breakpoints, i);
> +            return 0;
> +        }
> +    }
> +    return -ENOENT;
> +}
> +
> +/**
> + * insert_hw_watchpoint()
> + * @addr: address of watch point
> + * @len: size of area
> + * @type: type of watch point
> + *
> + * See ARM ARM D2.10. As with the breakpoints we can do some advanced
> + * stuff if we want to. The watch points can be linked with the break
> + * points above to make them context aware. However for simplicity
> + * currently we only deal with simple read/write watch points.
> + *
> + * D7.3.11 DBGWCR<n>_EL1, Debug Watchpoint Control Registers
> + *
> + *  31  29 28   24 23  21  20  19 16 15 14  13   12  5 4   3 2   1  0
> + * +------+-------+------+----+-----+-----+-----+-----+-----+-----+---+
> + * | RES0 |  MASK | RES0 | WT | LBN | SSC | HMC | BAS | LSC | PAC | E |
> + * +------+-------+------+----+-----+-----+-----+-----+-----+-----+---+
> + *
> + * MASK: num bits addr mask (0=none,01/10=res,11=3 bits (8 bytes))
> + * WT: 0 - unlinked, 1 - linked (not currently used)
> + * LBN: Linked BP number (not currently used)
> + * SSC/HMC/PAC: Security, Higher and Priv access control (Table D2-11)
> + * BAS: Byte Address Select
> + * LSC: Load/Store control (01: load, 10: store, 11: both)
> + * E: Enable
> + *
> + * The bottom 2 bits of the value register are masked. Therefore to
> + * break on any sizes smaller than an unaligned word you need to set
> + * MASK=0, BAS=bit per byte in question. For larger regions (^2) you
> + * need to ensure you mask the address as required and set BAS=0xff
> + */
> +
> +static int insert_hw_watchpoint(target_ulong addr,
> +                                target_ulong len, int type)
> +{
> +    HWWatchpoint wp = {
> +        .wcr = 1, /* E=1, enable */
> +        .wvr = addr & (~0x7ULL),
> +        .details = { .vaddr = addr, .len = len}

Should at least have a space after "len".

> +    };
> +
> +    if (cur_hw_wps >= max_hw_wps) {
> +        return -ENOBUFS;
> +    }
> +
> +    /*
> +     * HMC=0 SSC=0 PAC=3 will hit EL0 or EL1, any security state,
> +     * valid whether EL3 is implemented or not
> +     */
> +    wp.wcr = deposit32(wp.wcr, 1, 2, 3);
> +
> +    switch (type) {
> +    case GDB_WATCHPOINT_READ:
> +        wp.wcr = deposit32(wp.wcr, 3, 2, 1);
> +        wp.details.flags = BP_MEM_READ;
> +        break;
> +    case GDB_WATCHPOINT_WRITE:
> +        wp.wcr = deposit32(wp.wcr, 3, 2, 2);
> +        wp.details.flags = BP_MEM_WRITE;
> +        break;
> +    case GDB_WATCHPOINT_ACCESS:
> +        wp.wcr = deposit32(wp.wcr, 3, 2, 3);
> +        wp.details.flags = BP_MEM_ACCESS;
> +        break;
> +    default:
> +        g_assert_not_reached();
> +        break;
> +    }
> +    if (len <= 8) {
> +        /* we align the address and set the bits in BAS */
> +        int off = addr & 0x7;
> +        int bas = (1 << len)-1;

Missing spaces around operator "-" (here and below).

> +
> +        wp.wcr = deposit32(wp.wcr, 5 + off, 8 - off, bas);
> +    } else {
> +        /* For ranges above 8 bytes we need to be a power of 2 */
> +        if (is_power_of_2(len)) {
> +            int bits = ctz64(len);
> +
> +            wp.wvr &= ~((1 << bits)-1);
> +            wp.wcr = deposit32(wp.wcr, 24, 4, bits);
> +            wp.wcr = deposit32(wp.wcr, 5, 8, 0xff);
> +        } else {
> +            return -ENOBUFS;
> +        }
> +    }
> +
> +    g_array_append_val(hw_watchpoints, wp);
> +    return 0;
> +}
> +
> +
> +static bool check_watchpoint_in_range(int i, target_ulong addr)
> +{
> +    HWWatchpoint *wp = get_hw_wp(i);
> +    uint64_t addr_top, addr_bottom = wp->wvr;
> +    int bas = extract32(wp->wcr, 5, 8);
> +    int mask = extract32(wp->wcr, 24, 4);
> +
> +    if (mask) {
> +        addr_top = addr_bottom + (1 << mask);
> +    } else {
> +        /* BAS must be contiguous but can offset against the base
> +         * address in DBGWVR */
> +        addr_bottom = addr_bottom + ctz32(bas);
> +        addr_top = addr_bottom + clo32(bas);
> +    }
> +
> +    if (addr >= addr_bottom && addr <= addr_top) {
> +        return true;
> +    }
> +
> +    return false;
> +}
> +
> +/**
> + * delete_hw_watchpoint()
> + * @addr: address of breakpoint
> + *
> + * Delete a breakpoint and shuffle any above down
> + */
> +
> +static int delete_hw_watchpoint(target_ulong addr,
> +                                target_ulong len, int type)
> +{
> +    int i;
> +    for (i = 0; i < cur_hw_wps; i++) {
> +        if (check_watchpoint_in_range(i, addr)) {
> +            g_array_remove_index(hw_watchpoints, i);
> +            return 0;
> +        }
> +    }
> +    return -ENOENT;
> +}
> +
> +
> +int kvm_arch_insert_hw_breakpoint(target_ulong addr,
> +                                  target_ulong len, int type)
> +{
> +    switch (type) {
> +    case GDB_BREAKPOINT_HW:
> +        return insert_hw_breakpoint(addr);
> +        break;
> +    case GDB_WATCHPOINT_READ:
> +    case GDB_WATCHPOINT_WRITE:
> +    case GDB_WATCHPOINT_ACCESS:
> +        return insert_hw_watchpoint(addr, len, type);
> +    default:
> +        return -ENOSYS;
> +    }
> +}
> +
> +int kvm_arch_remove_hw_breakpoint(target_ulong addr,
> +                                  target_ulong len, int type)
> +{
> +    switch (type) {
> +    case GDB_BREAKPOINT_HW:
> +        return delete_hw_breakpoint(addr);
> +        break;
> +    case GDB_WATCHPOINT_READ:
> +    case GDB_WATCHPOINT_WRITE:
> +    case GDB_WATCHPOINT_ACCESS:
> +        return delete_hw_watchpoint(addr, len, type);
> +    default:
> +        return -ENOSYS;
> +    }
> +}
> +
> +
> +void kvm_arch_remove_all_hw_breakpoints(void)
> +{
> +    if (cur_hw_wps > 0) {
> +        g_array_remove_range(hw_watchpoints, 0, cur_hw_wps);
> +    }
> +    if (cur_hw_bps > 0) {
> +        g_array_remove_range(hw_breakpoints, 0, cur_hw_bps);
> +    }
> +}
> +
> +void kvm_arm_copy_hw_debug_data(struct kvm_guest_debug_arch *ptr)
> +{
> +    int i;
> +    memset(ptr, 0, sizeof(struct kvm_guest_debug_arch));
> +
> +    for (i = 0; i < max_hw_wps; i++) {
> +        HWWatchpoint *wp = get_hw_wp(i);
> +        ptr->dbg_wcr[i] = wp->wcr;
> +        ptr->dbg_wvr[i] = wp->wvr;
> +    }
> +    for (i = 0; i < max_hw_bps; i++) {
> +        HWBreakpoint *bp = get_hw_bp(i);
> +        ptr->dbg_bcr[i] = bp->bcr;
> +        ptr->dbg_bvr[i] = bp->bvr;
> +    }
> +}
> +
> +bool kvm_arm_hw_debug_active(CPUState *cs)
> +{
> +    return ((cur_hw_wps > 0) || (cur_hw_bps > 0)) ? true : false;

You don't nede the ?: here.

> +}
> +
> +bool kvm_arm_find_hw_breakpoint(CPUState *cpu, target_ulong pc)
> +{
> +    if (kvm_arm_hw_debug_active(cpu)) {

Do we need to do this check? I think the loop condition
will DTRT if there aren't any current breakpoints.

> +        int i;
> +
> +        for (i = 0; i < cur_hw_bps; i++) {
> +            HWBreakpoint *bp = get_hw_bp(i);
> +            if (bp->bvr == pc) {
> +                return true;
> +            }
> +        }
> +    }
> +    return false;
> +}
> +
> +CPUWatchpoint *kvm_arm_find_hw_watchpoint(CPUState *cpu, target_ulong addr)
> +{
> +    if (kvm_arm_hw_debug_active(cpu)) {

Similarly here.

> +        int i;
> +
> +        for (i = 0; i < cur_hw_wps; i++) {
> +            if (check_watchpoint_in_range(i, addr)) {
> +                return &get_hw_wp(i)->details;
> +            }
> +        }
> +    }
> +    return NULL;
> +}
> +
> +
>  static inline void set_feature(uint64_t *features, int feature)
>  {
>      *features |= 1ULL << feature;
> diff --git a/target-arm/kvm_arm.h b/target-arm/kvm_arm.h
> index b516041..da88658 100644
> --- a/target-arm/kvm_arm.h
> +++ b/target-arm/kvm_arm.h
> @@ -215,4 +215,42 @@ static inline const char *gic_class_name(void)
>   */
>  const char *gicv3_class_name(void);
>
> +/**
> + * kvm_arm_hw_debug_active:
> + *
> + * Return TRUE is any hardware breakpoints in use.

"if".

> + */
> +
> +bool kvm_arm_hw_debug_active(CPUState *cs);
> +
> +/**
> + * kvm_arm_copy_hw_debug_data:
> + *
> + * @ptr: kvm_guest_debug_arch structure
> + *
> + * Copy the architecture specific debug registers into the
> + * kvm_guest_debug ioctl structure.
> + */
> +struct kvm_guest_debug_arch;
> +
> +void kvm_arm_copy_hw_debug_data(struct kvm_guest_debug_arch *ptr);
> +
> +/**
> + * kvm_arm_find_hw_breakpoint:
> + * @cpu: CPUState
> + * @pc: pc of breakpoint
> + *
> + * Return TRUE if the pc matches one of our breakpoints.
> + */
> +bool kvm_arm_find_hw_breakpoint(CPUState *cpu, target_ulong pc);
> +
> +/**
> + * kvm_arm_find_hw_watchpoint:
> + * @cpu: CPUState
> + * @addr: address of watchpoint
> + *
> + * Return the CPUWatchpoint structure the addr matches one of our watchpoints.

Missing "if" ? (or needs rephrasing)

> + */
> +CPUWatchpoint *kvm_arm_find_hw_watchpoint(CPUState *cpu, target_ulong addr);
> +
>  #endif
> --
> 2.6.3
>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v9 4/6] target-arm: kvm - add support for HW assisted debug

[Peter Maydell]

On 12 November 2015 at 16:20, Alex Bennée <alex.bennee@linaro.org> wrote:
> This adds basic support for HW assisted debug. The ioctl interface to
> KVM allows us to pass an implementation defined number of break and
> watch point registers. When KVM_GUESTDBG_USE_HW is specified these
> debug registers will be installed in place on the world switch into the
> guest.
>
> The hardware is actually capable of more advanced matching but it is
> unclear if this expressiveness is available via the gdbstub protocol.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> ---
> v2
>   - correct setting of PMC/BAS/MASK
>   - improved commentary
>   - added helper function to check watchpoint in range
>   - fix find/deletion of watchpoints
> v3
>   - use internals.h definitions
> v6
>   - KVM_GUESTDBG_USE_HW_BP->KVM_GUESTDBG_USE_HW
>   - renamed some helper functions to avoid confusion
> v9
>   - fix merge conflicts on re-base
>   - rm asm/ptrace.h include
>   - add additional commentry for hw breakpoints
>   - explain gdb's model for HW bkpts
>   - fix up spacing, formatting as per checkpatch
>   - better PAC values
>   - use is_power_of_2
>   - use _arm_ fn naming and add docs
>   - add a CPUWatchpoint structure for reporting
>   - replace manual array manipulation with g_array abstraction
> ---
>  target-arm/kvm.c     |  38 +++---
>  target-arm/kvm64.c   | 352 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  target-arm/kvm_arm.h |  38 ++++++
>  3 files changed, 406 insertions(+), 22 deletions(-)
>
> diff --git a/target-arm/kvm.c b/target-arm/kvm.c
> index d505a7e..1f57e92 100644
> --- a/target-arm/kvm.c
> +++ b/target-arm/kvm.c
> @@ -547,6 +547,20 @@ static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
>              return true;
>          }
>          break;
> +    case EC_BREAKPOINT:
> +        if (kvm_arm_find_hw_breakpoint(cs, env->pc)) {
> +            return true;
> +        }
> +        break;
> +    case EC_WATCHPOINT:
> +    {
> +        CPUWatchpoint *wp = kvm_arm_find_hw_watchpoint(cs, arch_info->far);

This won't compile for 32-bit ARM.

> +        if (wp) {
> +            cs->watchpoint_hit = wp;
> +            return true;
> +        }
> +        break;
> +    }
>      default:
>          error_report("%s: unhandled debug exit (%"PRIx32", %"PRIx64")\n",
>                       __func__, arch_info->hsr, env->pc);
> @@ -608,6 +622,10 @@ void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
>      if (kvm_sw_breakpoints_active(cs)) {
>          dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;
>      }
> +    if (kvm_arm_hw_debug_active(cs)) {
> +        dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_HW;
> +        kvm_arm_copy_hw_debug_data(&dbg->arch);
> +    }
>  }
>
>  /* C6.6.29 BRK instruction */
> @@ -635,26 +653,6 @@ int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
>      return 0;
>  }
>
> -int kvm_arch_insert_hw_breakpoint(target_ulong addr,
> -                                  target_ulong len, int type)
> -{
> -    qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
> -    return -EINVAL;
> -}

You've moved these functions into kvm64.c but haven't provided
a stub version in kvm32.c...

> -
> -int kvm_arch_remove_hw_breakpoint(target_ulong addr,
> -                                  target_ulong len, int type)
> -{
> -    qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
> -    return -EINVAL;
> -}
> -
> -
> -void kvm_arch_remove_all_hw_breakpoints(void)
> -{
> -    qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
> -}
> -
>  void kvm_arch_init_irq_routing(KVMState *s)
>  {
>  }
> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
> index d087794..c468324 100644
> --- a/target-arm/kvm64.c
> +++ b/target-arm/kvm64.c
> @@ -2,6 +2,7 @@
>   * ARM implementation of KVM hooks, 64 bit specific code
>   *
>   * Copyright Mian-M. Hamayun 2013, Virtual Open Systems
> + * Copyright Alex Bennée 2014, Linaro
>   *
>   * This work is licensed under the terms of the GNU GPL, version 2 or later.
>   * See the COPYING file in the top-level directory.
> @@ -12,12 +13,17 @@
>  #include <sys/types.h>
>  #include <sys/ioctl.h>
>  #include <sys/mman.h>
> +#include <sys/ptrace.h>
>
> +#include <linux/elf.h>
>  #include <linux/kvm.h>
>
>  #include "config-host.h"
>  #include "qemu-common.h"
>  #include "qemu/timer.h"
> +#include "qemu/host-utils.h"
> +#include "qemu/error-report.h"
> +#include "exec/gdbstub.h"
>  #include "sysemu/sysemu.h"
>  #include "sysemu/kvm.h"
>  #include "kvm_arm.h"
> @@ -27,20 +33,362 @@
>
>  static bool have_guest_debug;
>
> +/*
> + * Although the ARM implementation of hardware assisted debugging
> + * allows for different breakpoints per-core the current GDB interface

Comma after "per-core".

> + * treats them as a global pool of registers (which seems to be the
> + * case for x86, ppc and s390). As a result we store one copy of
> + * registers which is used for all active cores.
> + *
> + * Write access is serialised by virtue of the GDB protocol which
> + * updates things. Read access (i.e. when the values are copied to the
> + * vCPU) is also gated by GDBs run control.

"GDB's".

> + *
> + * This is not unreasonable as most of the time debugging kernels you
> + * never know which core will eventually execute your function.
> + */
> +
> +typedef struct {
> +    uint64_t bcr;
> +    uint64_t bvr;
> +} HWBreakpoint;
> +
> +/* The watchpoint registers can cover more area than the requested
> + * watchpoint so we need to store the additional information
> + * somewhere. We also need to supply a CPUWatchpoint to the GDB stub
> + * when the watchpoint is hit.
> + */
> +typedef struct {
> +    uint64_t wcr;
> +    uint64_t wvr;
> +    CPUWatchpoint details;
> +} HWWatchpoint;
> +
> +/* Maximum and current break/watch point counts */
> +int max_hw_bps, max_hw_wps;
> +GArray *hw_breakpoints, *hw_watchpoints;
> +
> +#define cur_hw_wps      (hw_watchpoints->len)
> +#define cur_hw_bps      (hw_breakpoints->len)
> +#define get_hw_bp(i)    (&g_array_index(hw_breakpoints, HWBreakpoint, i))
> +#define get_hw_wp(i)    (&g_array_index(hw_watchpoints, HWWatchpoint, i))
> +
>  /**
> - * kvm_arm_init_debug()
> + * kvm_arm_init_debug() - check for guest debug capabilities
>   * @cs: CPUState
>   *
> - * Check for guest debug capabilities.
> + * kvm_check_extension returns 0 if we have no debug registers or the
> + * number we have.

this would be easier to read phrased as "kvm_check_extension returns
the number of debug registers we have (which might be none)".

>   *
>   */
>  static void kvm_arm_init_debug(CPUState *cs)
>  {
>      have_guest_debug = kvm_check_extension(cs->kvm_state,
>                                             KVM_CAP_SET_GUEST_DEBUG);
> +
> +    max_hw_wps = kvm_check_extension(cs->kvm_state, KVM_CAP_GUEST_DEBUG_HW_WPS);
> +    hw_watchpoints = g_array_sized_new(true, true,
> +                                       sizeof(HWWatchpoint), max_hw_wps);
> +
> +    max_hw_bps = kvm_check_extension(cs->kvm_state, KVM_CAP_GUEST_DEBUG_HW_BPS);
> +    hw_breakpoints = g_array_sized_new(true, true,
> +                                       sizeof(HWBreakpoint), max_hw_bps);
>      return;
>  }
>
> +/**
> + * insert_hw_breakpoint()
> + * @addr: address of breakpoint
> + *
> + * See ARM ARM D2.9.1 for details but here we are only going to create
> + * simple un-linked breakpoints (i.e. we don't chain breakpoints
> + * together to match address and context or vmid). The hardware is
> + * capable of fancier matching but that will require exposing that
> + * fanciness to GDB's interface
> + *
> + * D7.3.2 DBGBCR<n>_EL1, Debug Breakpoint Control Registers
> + *
> + *  31  24 23  20 19   16 15 14  13  12   9 8   5 4    3 2   1  0
> + * +------+------+-------+-----+----+------+-----+------+-----+---+
> + * | RES0 |  BT  |  LBN  | SSC | HMC| RES0 | BAS | RES0 | PMC | E |
> + * +------+------+-------+-----+----+------+-----+------+-----+---+
> + *
> + * BT: Breakpoint type (0 = unlinked address match)
> + * LBN: Linked BP number (0 = unused)
> + * SSC/HMC/PMC: Security, Higher and Priv access control (Table D-12)
> + * BAS: Byte Address Select (RES1 for AArch64)
> + * E: Enable bit
> + */
> +static int insert_hw_breakpoint(target_ulong addr)
> +{
> +    HWBreakpoint brk = {
> +        .bcr = 0x1,                             /* BCR E=1, enable */
> +        .bvr = addr
> +    };
> +
> +    if (cur_hw_bps >= max_hw_bps) {
> +        return -ENOBUFS;
> +    }
> +
> +    brk.bcr = deposit32(brk.bcr, 1, 2, 0x3);   /* PMC = 11 */
> +    brk.bcr = deposit32(brk.bcr, 5, 4, 0xf);   /* BAS = RES1 */
> +
> +    g_array_append_val(hw_breakpoints, brk);
> +
> +    return 0;
> +}
> +
> +/**
> + * delete_hw_breakpoint()
> + * @pc: address of breakpoint
> + *
> + * Delete a breakpoint and shuffle any above down
> + */
> +
> +static int delete_hw_breakpoint(target_ulong pc)
> +{
> +    int i;
> +    for (i = 0; i < hw_breakpoints->len; i++) {
> +        HWBreakpoint *brk = get_hw_bp(i);
> +        if (brk->bvr == pc) {
> +            g_array_remove_index(hw_breakpoints, i);
> +            return 0;
> +        }
> +    }
> +    return -ENOENT;
> +}
> +
> +/**
> + * insert_hw_watchpoint()
> + * @addr: address of watch point
> + * @len: size of area
> + * @type: type of watch point
> + *
> + * See ARM ARM D2.10. As with the breakpoints we can do some advanced
> + * stuff if we want to. The watch points can be linked with the break
> + * points above to make them context aware. However for simplicity
> + * currently we only deal with simple read/write watch points.
> + *
> + * D7.3.11 DBGWCR<n>_EL1, Debug Watchpoint Control Registers
> + *
> + *  31  29 28   24 23  21  20  19 16 15 14  13   12  5 4   3 2   1  0
> + * +------+-------+------+----+-----+-----+-----+-----+-----+-----+---+
> + * | RES0 |  MASK | RES0 | WT | LBN | SSC | HMC | BAS | LSC | PAC | E |
> + * +------+-------+------+----+-----+-----+-----+-----+-----+-----+---+
> + *
> + * MASK: num bits addr mask (0=none,01/10=res,11=3 bits (8 bytes))
> + * WT: 0 - unlinked, 1 - linked (not currently used)
> + * LBN: Linked BP number (not currently used)
> + * SSC/HMC/PAC: Security, Higher and Priv access control (Table D2-11)
> + * BAS: Byte Address Select
> + * LSC: Load/Store control (01: load, 10: store, 11: both)
> + * E: Enable
> + *
> + * The bottom 2 bits of the value register are masked. Therefore to
> + * break on any sizes smaller than an unaligned word you need to set
> + * MASK=0, BAS=bit per byte in question. For larger regions (^2) you
> + * need to ensure you mask the address as required and set BAS=0xff
> + */
> +
> +static int insert_hw_watchpoint(target_ulong addr,
> +                                target_ulong len, int type)
> +{
> +    HWWatchpoint wp = {
> +        .wcr = 1, /* E=1, enable */
> +        .wvr = addr & (~0x7ULL),
> +        .details = { .vaddr = addr, .len = len}

Should at least have a space after "len".

> +    };
> +
> +    if (cur_hw_wps >= max_hw_wps) {
> +        return -ENOBUFS;
> +    }
> +
> +    /*
> +     * HMC=0 SSC=0 PAC=3 will hit EL0 or EL1, any security state,
> +     * valid whether EL3 is implemented or not
> +     */
> +    wp.wcr = deposit32(wp.wcr, 1, 2, 3);
> +
> +    switch (type) {
> +    case GDB_WATCHPOINT_READ:
> +        wp.wcr = deposit32(wp.wcr, 3, 2, 1);
> +        wp.details.flags = BP_MEM_READ;
> +        break;
> +    case GDB_WATCHPOINT_WRITE:
> +        wp.wcr = deposit32(wp.wcr, 3, 2, 2);
> +        wp.details.flags = BP_MEM_WRITE;
> +        break;
> +    case GDB_WATCHPOINT_ACCESS:
> +        wp.wcr = deposit32(wp.wcr, 3, 2, 3);
> +        wp.details.flags = BP_MEM_ACCESS;
> +        break;
> +    default:
> +        g_assert_not_reached();
> +        break;
> +    }
> +    if (len <= 8) {
> +        /* we align the address and set the bits in BAS */
> +        int off = addr & 0x7;
> +        int bas = (1 << len)-1;

Missing spaces around operator "-" (here and below).

> +
> +        wp.wcr = deposit32(wp.wcr, 5 + off, 8 - off, bas);
> +    } else {
> +        /* For ranges above 8 bytes we need to be a power of 2 */
> +        if (is_power_of_2(len)) {
> +            int bits = ctz64(len);
> +
> +            wp.wvr &= ~((1 << bits)-1);
> +            wp.wcr = deposit32(wp.wcr, 24, 4, bits);
> +            wp.wcr = deposit32(wp.wcr, 5, 8, 0xff);
> +        } else {
> +            return -ENOBUFS;
> +        }
> +    }
> +
> +    g_array_append_val(hw_watchpoints, wp);
> +    return 0;
> +}
> +
> +
> +static bool check_watchpoint_in_range(int i, target_ulong addr)
> +{
> +    HWWatchpoint *wp = get_hw_wp(i);
> +    uint64_t addr_top, addr_bottom = wp->wvr;
> +    int bas = extract32(wp->wcr, 5, 8);
> +    int mask = extract32(wp->wcr, 24, 4);
> +
> +    if (mask) {
> +        addr_top = addr_bottom + (1 << mask);
> +    } else {
> +        /* BAS must be contiguous but can offset against the base
> +         * address in DBGWVR */
> +        addr_bottom = addr_bottom + ctz32(bas);
> +        addr_top = addr_bottom + clo32(bas);
> +    }
> +
> +    if (addr >= addr_bottom && addr <= addr_top) {
> +        return true;
> +    }
> +
> +    return false;
> +}
> +
> +/**
> + * delete_hw_watchpoint()
> + * @addr: address of breakpoint
> + *
> + * Delete a breakpoint and shuffle any above down
> + */
> +
> +static int delete_hw_watchpoint(target_ulong addr,
> +                                target_ulong len, int type)
> +{
> +    int i;
> +    for (i = 0; i < cur_hw_wps; i++) {
> +        if (check_watchpoint_in_range(i, addr)) {
> +            g_array_remove_index(hw_watchpoints, i);
> +            return 0;
> +        }
> +    }
> +    return -ENOENT;
> +}
> +
> +
> +int kvm_arch_insert_hw_breakpoint(target_ulong addr,
> +                                  target_ulong len, int type)
> +{
> +    switch (type) {
> +    case GDB_BREAKPOINT_HW:
> +        return insert_hw_breakpoint(addr);
> +        break;
> +    case GDB_WATCHPOINT_READ:
> +    case GDB_WATCHPOINT_WRITE:
> +    case GDB_WATCHPOINT_ACCESS:
> +        return insert_hw_watchpoint(addr, len, type);
> +    default:
> +        return -ENOSYS;
> +    }
> +}
> +
> +int kvm_arch_remove_hw_breakpoint(target_ulong addr,
> +                                  target_ulong len, int type)
> +{
> +    switch (type) {
> +    case GDB_BREAKPOINT_HW:
> +        return delete_hw_breakpoint(addr);
> +        break;
> +    case GDB_WATCHPOINT_READ:
> +    case GDB_WATCHPOINT_WRITE:
> +    case GDB_WATCHPOINT_ACCESS:
> +        return delete_hw_watchpoint(addr, len, type);
> +    default:
> +        return -ENOSYS;
> +    }
> +}
> +
> +
> +void kvm_arch_remove_all_hw_breakpoints(void)
> +{
> +    if (cur_hw_wps > 0) {
> +        g_array_remove_range(hw_watchpoints, 0, cur_hw_wps);
> +    }
> +    if (cur_hw_bps > 0) {
> +        g_array_remove_range(hw_breakpoints, 0, cur_hw_bps);
> +    }
> +}
> +
> +void kvm_arm_copy_hw_debug_data(struct kvm_guest_debug_arch *ptr)
> +{
> +    int i;
> +    memset(ptr, 0, sizeof(struct kvm_guest_debug_arch));
> +
> +    for (i = 0; i < max_hw_wps; i++) {
> +        HWWatchpoint *wp = get_hw_wp(i);
> +        ptr->dbg_wcr[i] = wp->wcr;
> +        ptr->dbg_wvr[i] = wp->wvr;
> +    }
> +    for (i = 0; i < max_hw_bps; i++) {
> +        HWBreakpoint *bp = get_hw_bp(i);
> +        ptr->dbg_bcr[i] = bp->bcr;
> +        ptr->dbg_bvr[i] = bp->bvr;
> +    }
> +}
> +
> +bool kvm_arm_hw_debug_active(CPUState *cs)
> +{
> +    return ((cur_hw_wps > 0) || (cur_hw_bps > 0)) ? true : false;

You don't nede the ?: here.

> +}
> +
> +bool kvm_arm_find_hw_breakpoint(CPUState *cpu, target_ulong pc)
> +{
> +    if (kvm_arm_hw_debug_active(cpu)) {

Do we need to do this check? I think the loop condition
will DTRT if there aren't any current breakpoints.

> +        int i;
> +
> +        for (i = 0; i < cur_hw_bps; i++) {
> +            HWBreakpoint *bp = get_hw_bp(i);
> +            if (bp->bvr == pc) {
> +                return true;
> +            }
> +        }
> +    }
> +    return false;
> +}
> +
> +CPUWatchpoint *kvm_arm_find_hw_watchpoint(CPUState *cpu, target_ulong addr)
> +{
> +    if (kvm_arm_hw_debug_active(cpu)) {

Similarly here.

> +        int i;
> +
> +        for (i = 0; i < cur_hw_wps; i++) {
> +            if (check_watchpoint_in_range(i, addr)) {
> +                return &get_hw_wp(i)->details;
> +            }
> +        }
> +    }
> +    return NULL;
> +}
> +
> +
>  static inline void set_feature(uint64_t *features, int feature)
>  {
>      *features |= 1ULL << feature;
> diff --git a/target-arm/kvm_arm.h b/target-arm/kvm_arm.h
> index b516041..da88658 100644
> --- a/target-arm/kvm_arm.h
> +++ b/target-arm/kvm_arm.h
> @@ -215,4 +215,42 @@ static inline const char *gic_class_name(void)
>   */
>  const char *gicv3_class_name(void);
>
> +/**
> + * kvm_arm_hw_debug_active:
> + *
> + * Return TRUE is any hardware breakpoints in use.

"if".

> + */
> +
> +bool kvm_arm_hw_debug_active(CPUState *cs);
> +
> +/**
> + * kvm_arm_copy_hw_debug_data:
> + *
> + * @ptr: kvm_guest_debug_arch structure
> + *
> + * Copy the architecture specific debug registers into the
> + * kvm_guest_debug ioctl structure.
> + */
> +struct kvm_guest_debug_arch;
> +
> +void kvm_arm_copy_hw_debug_data(struct kvm_guest_debug_arch *ptr);
> +
> +/**
> + * kvm_arm_find_hw_breakpoint:
> + * @cpu: CPUState
> + * @pc: pc of breakpoint
> + *
> + * Return TRUE if the pc matches one of our breakpoints.
> + */
> +bool kvm_arm_find_hw_breakpoint(CPUState *cpu, target_ulong pc);
> +
> +/**
> + * kvm_arm_find_hw_watchpoint:
> + * @cpu: CPUState
> + * @addr: address of watchpoint
> + *
> + * Return the CPUWatchpoint structure the addr matches one of our watchpoints.

Missing "if" ? (or needs rephrasing)

> + */
> +CPUWatchpoint *kvm_arm_find_hw_watchpoint(CPUState *cpu, target_ulong addr);
> +
>  #endif
> --
> 2.6.3
>

thanks
-- PMM

[PATCH v9 5/6] target-arm: kvm - re-inject guest debug exceptions

[Alex Bennée]

From: Alex Bennée <alex@bennee.com>

If we can't find details for the debug exception in our debug state
then we can assume the exception is due to debugging inside the guest.
To inject the exception into the guest state we re-use the TCG exception
code (do_interupt).

However while guest debugging is in effect we currently can't handle the
guest using single step which is heavily used by GDB.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v5:
  - new for v5
---
 target-arm/helper-a64.c | 12 ++++++++++--
 target-arm/kvm.c        | 27 +++++++++++++++++++--------
 2 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
index deb8dbe..fc3ccdf 100644
--- a/target-arm/helper-a64.c
+++ b/target-arm/helper-a64.c
@@ -25,6 +25,7 @@
 #include "qemu/bitops.h"
 #include "internals.h"
 #include "qemu/crc32c.h"
+#include "sysemu/kvm.h"
 #include <zlib.h> /* For crc32 */
 
 /* C2.4.7 Multiply and divide */
@@ -469,7 +470,8 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
                   new_el);
     if (qemu_loglevel_mask(CPU_LOG_INT)
         && !excp_is_internal(cs->exception_index)) {
-        qemu_log_mask(CPU_LOG_INT, "...with ESR 0x%" PRIx32 "\n",
+        qemu_log_mask(CPU_LOG_INT, "...with ESR %x/0x%" PRIx32 "\n",
+                      env->exception.syndrome >> ARM_EL_EC_SHIFT,
                       env->exception.syndrome);
     }
 
@@ -535,6 +537,12 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
     aarch64_restore_sp(env, new_el);
 
     env->pc = addr;
-    cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
+
+    qemu_log_mask(CPU_LOG_INT, "...to EL%d PC 0x%" PRIx64 " PSTATE 0x%x\n",
+                  new_el, env->pc, pstate_read(env));
+
+    if (!kvm_enabled()) {
+        cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
+    }
 }
 #endif
diff --git a/target-arm/kvm.c b/target-arm/kvm.c
index 1f57e92..4ac177a 100644
--- a/target-arm/kvm.c
+++ b/target-arm/kvm.c
@@ -529,9 +529,10 @@ static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
     struct kvm_debug_exit_arch *arch_info = &run->debug.arch;
     int hsr_ec = arch_info->hsr >> ARM_EL_EC_SHIFT;
     ARMCPU *cpu = ARM_CPU(cs);
+    CPUClass *cc = CPU_GET_CLASS(cs);
     CPUARMState *env = &cpu->env;
 
-    /* Ensure PC is synchronised */
+    /* Ensure all state is synchronised */
     kvm_cpu_synchronize_state(cs);
 
     switch (hsr_ec) {
@@ -539,7 +540,14 @@ static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
         if (cs->singlestep_enabled) {
             return true;
         } else {
-            error_report("Came out of SINGLE STEP when not enabled");
+            /*
+             * The kernel should have supressed the guests ability to
+             * single step at this point so something has gone wrong.
+             */
+            error_report("%s: guest single-step while debugging unsupported"
+                         " (%"PRIx64", %"PRIx32")\n",
+                         __func__, env->pc, arch_info->hsr);
+            return false;
         }
         break;
     case EC_AA64_BKPT:
@@ -564,14 +572,17 @@ static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
     default:
         error_report("%s: unhandled debug exit (%"PRIx32", %"PRIx64")\n",
                      __func__, arch_info->hsr, env->pc);
+        return false;
     }
 
-    /* If we don't handle this it could be it really is for the
-       guest to handle */
-    qemu_log_mask(LOG_UNIMP,
-                  "%s: re-injecting exception not yet implemented"
-                  " (0x%"PRIx32", %"PRIx64")\n",
-                  __func__, hsr_ec, env->pc);
+    /* If we are not handling the debug exception it must belong to
+     * the guest. Let's re-use the existing TCG interrupt code to set
+     * everything up properly
+     */
+    cs->exception_index = EXCP_BKPT;
+    env->exception.syndrome = arch_info->hsr;
+    env->exception.vaddress = arch_info->far;
+    cc->do_interrupt(cs);
 
     return false;
 }
-- 
2.6.3

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v9 5/6] target-arm: kvm - re-inject guest debug exceptions

[Alex Bennée]

From: Alex Benn?e <alex@bennee.com>

If we can't find details for the debug exception in our debug state
then we can assume the exception is due to debugging inside the guest.
To inject the exception into the guest state we re-use the TCG exception
code (do_interupt).

However while guest debugging is in effect we currently can't handle the
guest using single step which is heavily used by GDB.

Signed-off-by: Alex Benn?e <alex.bennee@linaro.org>

---
v5:
  - new for v5
---
 target-arm/helper-a64.c | 12 ++++++++++--
 target-arm/kvm.c        | 27 +++++++++++++++++++--------
 2 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
index deb8dbe..fc3ccdf 100644
--- a/target-arm/helper-a64.c
+++ b/target-arm/helper-a64.c
@@ -25,6 +25,7 @@
 #include "qemu/bitops.h"
 #include "internals.h"
 #include "qemu/crc32c.h"
+#include "sysemu/kvm.h"
 #include <zlib.h> /* For crc32 */
 
 /* C2.4.7 Multiply and divide */
@@ -469,7 +470,8 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
                   new_el);
     if (qemu_loglevel_mask(CPU_LOG_INT)
         && !excp_is_internal(cs->exception_index)) {
-        qemu_log_mask(CPU_LOG_INT, "...with ESR 0x%" PRIx32 "\n",
+        qemu_log_mask(CPU_LOG_INT, "...with ESR %x/0x%" PRIx32 "\n",
+                      env->exception.syndrome >> ARM_EL_EC_SHIFT,
                       env->exception.syndrome);
     }
 
@@ -535,6 +537,12 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
     aarch64_restore_sp(env, new_el);
 
     env->pc = addr;
-    cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
+
+    qemu_log_mask(CPU_LOG_INT, "...to EL%d PC 0x%" PRIx64 " PSTATE 0x%x\n",
+                  new_el, env->pc, pstate_read(env));
+
+    if (!kvm_enabled()) {
+        cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
+    }
 }
 #endif
diff --git a/target-arm/kvm.c b/target-arm/kvm.c
index 1f57e92..4ac177a 100644
--- a/target-arm/kvm.c
+++ b/target-arm/kvm.c
@@ -529,9 +529,10 @@ static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
     struct kvm_debug_exit_arch *arch_info = &run->debug.arch;
     int hsr_ec = arch_info->hsr >> ARM_EL_EC_SHIFT;
     ARMCPU *cpu = ARM_CPU(cs);
+    CPUClass *cc = CPU_GET_CLASS(cs);
     CPUARMState *env = &cpu->env;
 
-    /* Ensure PC is synchronised */
+    /* Ensure all state is synchronised */
     kvm_cpu_synchronize_state(cs);
 
     switch (hsr_ec) {
@@ -539,7 +540,14 @@ static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
         if (cs->singlestep_enabled) {
             return true;
         } else {
-            error_report("Came out of SINGLE STEP when not enabled");
+            /*
+             * The kernel should have supressed the guests ability to
+             * single step at this point so something has gone wrong.
+             */
+            error_report("%s: guest single-step while debugging unsupported"
+                         " (%"PRIx64", %"PRIx32")\n",
+                         __func__, env->pc, arch_info->hsr);
+            return false;
         }
         break;
     case EC_AA64_BKPT:
@@ -564,14 +572,17 @@ static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
     default:
         error_report("%s: unhandled debug exit (%"PRIx32", %"PRIx64")\n",
                      __func__, arch_info->hsr, env->pc);
+        return false;
     }
 
-    /* If we don't handle this it could be it really is for the
-       guest to handle */
-    qemu_log_mask(LOG_UNIMP,
-                  "%s: re-injecting exception not yet implemented"
-                  " (0x%"PRIx32", %"PRIx64")\n",
-                  __func__, hsr_ec, env->pc);
+    /* If we are not handling the debug exception it must belong to
+     * the guest. Let's re-use the existing TCG interrupt code to set
+     * everything up properly
+     */
+    cs->exception_index = EXCP_BKPT;
+    env->exception.syndrome = arch_info->hsr;
+    env->exception.vaddress = arch_info->far;
+    cc->do_interrupt(cs);
 
     return false;
 }
-- 
2.6.3

[Qemu-devel] [PATCH v9 5/6] target-arm: kvm - re-inject guest debug exceptions

[Alex Bennée]

From: Alex Bennée <alex@bennee.com>

If we can't find details for the debug exception in our debug state
then we can assume the exception is due to debugging inside the guest.
To inject the exception into the guest state we re-use the TCG exception
code (do_interupt).

However while guest debugging is in effect we currently can't handle the
guest using single step which is heavily used by GDB.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v5:
  - new for v5
---
 target-arm/helper-a64.c | 12 ++++++++++--
 target-arm/kvm.c        | 27 +++++++++++++++++++--------
 2 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
index deb8dbe..fc3ccdf 100644
--- a/target-arm/helper-a64.c
+++ b/target-arm/helper-a64.c
@@ -25,6 +25,7 @@
 #include "qemu/bitops.h"
 #include "internals.h"
 #include "qemu/crc32c.h"
+#include "sysemu/kvm.h"
 #include <zlib.h> /* For crc32 */
 
 /* C2.4.7 Multiply and divide */
@@ -469,7 +470,8 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
                   new_el);
     if (qemu_loglevel_mask(CPU_LOG_INT)
         && !excp_is_internal(cs->exception_index)) {
-        qemu_log_mask(CPU_LOG_INT, "...with ESR 0x%" PRIx32 "\n",
+        qemu_log_mask(CPU_LOG_INT, "...with ESR %x/0x%" PRIx32 "\n",
+                      env->exception.syndrome >> ARM_EL_EC_SHIFT,
                       env->exception.syndrome);
     }
 
@@ -535,6 +537,12 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
     aarch64_restore_sp(env, new_el);
 
     env->pc = addr;
-    cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
+
+    qemu_log_mask(CPU_LOG_INT, "...to EL%d PC 0x%" PRIx64 " PSTATE 0x%x\n",
+                  new_el, env->pc, pstate_read(env));
+
+    if (!kvm_enabled()) {
+        cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
+    }
 }
 #endif
diff --git a/target-arm/kvm.c b/target-arm/kvm.c
index 1f57e92..4ac177a 100644
--- a/target-arm/kvm.c
+++ b/target-arm/kvm.c
@@ -529,9 +529,10 @@ static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
     struct kvm_debug_exit_arch *arch_info = &run->debug.arch;
     int hsr_ec = arch_info->hsr >> ARM_EL_EC_SHIFT;
     ARMCPU *cpu = ARM_CPU(cs);
+    CPUClass *cc = CPU_GET_CLASS(cs);
     CPUARMState *env = &cpu->env;
 
-    /* Ensure PC is synchronised */
+    /* Ensure all state is synchronised */
     kvm_cpu_synchronize_state(cs);
 
     switch (hsr_ec) {
@@ -539,7 +540,14 @@ static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
         if (cs->singlestep_enabled) {
             return true;
         } else {
-            error_report("Came out of SINGLE STEP when not enabled");
+            /*
+             * The kernel should have supressed the guests ability to
+             * single step at this point so something has gone wrong.
+             */
+            error_report("%s: guest single-step while debugging unsupported"
+                         " (%"PRIx64", %"PRIx32")\n",
+                         __func__, env->pc, arch_info->hsr);
+            return false;
         }
         break;
     case EC_AA64_BKPT:
@@ -564,14 +572,17 @@ static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
     default:
         error_report("%s: unhandled debug exit (%"PRIx32", %"PRIx64")\n",
                      __func__, arch_info->hsr, env->pc);
+        return false;
     }
 
-    /* If we don't handle this it could be it really is for the
-       guest to handle */
-    qemu_log_mask(LOG_UNIMP,
-                  "%s: re-injecting exception not yet implemented"
-                  " (0x%"PRIx32", %"PRIx64")\n",
-                  __func__, hsr_ec, env->pc);
+    /* If we are not handling the debug exception it must belong to
+     * the guest. Let's re-use the existing TCG interrupt code to set
+     * everything up properly
+     */
+    cs->exception_index = EXCP_BKPT;
+    env->exception.syndrome = arch_info->hsr;
+    env->exception.vaddress = arch_info->far;
+    cc->do_interrupt(cs);
 
     return false;
 }
-- 
2.6.3

Re: [PATCH v9 5/6] target-arm: kvm - re-inject guest debug exceptions

[Peter Maydell]

On 12 November 2015 at 16:20, Alex Bennée <alex.bennee@linaro.org> wrote:
> From: Alex Bennée <alex@bennee.com>
>
> If we can't find details for the debug exception in our debug state
> then we can assume the exception is due to debugging inside the guest.
> To inject the exception into the guest state we re-use the TCG exception
> code (do_interupt).

"do_interrupt".

>
> However while guest debugging is in effect we currently can't handle the
> guest using single step which is heavily used by GDB.

Can you expand this to be clearer about what the problem is here?
Is this a thing fixed by this commit or a remaining issue after it?

> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> ---
> v5:
>   - new for v5
> ---
>  target-arm/helper-a64.c | 12 ++++++++++--
>  target-arm/kvm.c        | 27 +++++++++++++++++++--------
>  2 files changed, 29 insertions(+), 10 deletions(-)
>
> diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
> index deb8dbe..fc3ccdf 100644
> --- a/target-arm/helper-a64.c
> +++ b/target-arm/helper-a64.c
> @@ -25,6 +25,7 @@
>  #include "qemu/bitops.h"
>  #include "internals.h"
>  #include "qemu/crc32c.h"
> +#include "sysemu/kvm.h"
>  #include <zlib.h> /* For crc32 */
>
>  /* C2.4.7 Multiply and divide */
> @@ -469,7 +470,8 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
>                    new_el);
>      if (qemu_loglevel_mask(CPU_LOG_INT)
>          && !excp_is_internal(cs->exception_index)) {
> -        qemu_log_mask(CPU_LOG_INT, "...with ESR 0x%" PRIx32 "\n",
> +        qemu_log_mask(CPU_LOG_INT, "...with ESR %x/0x%" PRIx32 "\n",
> +                      env->exception.syndrome >> ARM_EL_EC_SHIFT,
>                        env->exception.syndrome);
>      }
>
> @@ -535,6 +537,12 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
>      aarch64_restore_sp(env, new_el);
>
>      env->pc = addr;
> -    cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
> +
> +    qemu_log_mask(CPU_LOG_INT, "...to EL%d PC 0x%" PRIx64 " PSTATE 0x%x\n",
> +                  new_el, env->pc, pstate_read(env));
> +
> +    if (!kvm_enabled()) {
> +        cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
> +    }
>  }
>  #endif
> diff --git a/target-arm/kvm.c b/target-arm/kvm.c
> index 1f57e92..4ac177a 100644
> --- a/target-arm/kvm.c
> +++ b/target-arm/kvm.c
> @@ -529,9 +529,10 @@ static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
>      struct kvm_debug_exit_arch *arch_info = &run->debug.arch;
>      int hsr_ec = arch_info->hsr >> ARM_EL_EC_SHIFT;
>      ARMCPU *cpu = ARM_CPU(cs);
> +    CPUClass *cc = CPU_GET_CLASS(cs);
>      CPUARMState *env = &cpu->env;
>
> -    /* Ensure PC is synchronised */
> +    /* Ensure all state is synchronised */

You might as well have just written the comment like that to start with :-)

>      kvm_cpu_synchronize_state(cs);
>
>      switch (hsr_ec) {
> @@ -539,7 +540,14 @@ static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
>          if (cs->singlestep_enabled) {
>              return true;
>          } else {
> -            error_report("Came out of SINGLE STEP when not enabled");
> +            /*
> +             * The kernel should have supressed the guests ability to

"suppressed". "guest's".

> +             * single step at this point so something has gone wrong.
> +             */
> +            error_report("%s: guest single-step while debugging unsupported"
> +                         " (%"PRIx64", %"PRIx32")\n",
> +                         __func__, env->pc, arch_info->hsr);
> +            return false;

Why didn't we just write the error_report this way to start with?

>          }
>          break;
>      case EC_AA64_BKPT:
> @@ -564,14 +572,17 @@ static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
>      default:
>          error_report("%s: unhandled debug exit (%"PRIx32", %"PRIx64")\n",
>                       __func__, arch_info->hsr, env->pc);
> +        return false;

Might as well put this "return false;" in the original code that
adds the default case, rather than changing the control flow in this
patch.

>      }
>
> -    /* If we don't handle this it could be it really is for the
> -       guest to handle */
> -    qemu_log_mask(LOG_UNIMP,
> -                  "%s: re-injecting exception not yet implemented"
> -                  " (0x%"PRIx32", %"PRIx64")\n",
> -                  __func__, hsr_ec, env->pc);
> +    /* If we are not handling the debug exception it must belong to
> +     * the guest. Let's re-use the existing TCG interrupt code to set
> +     * everything up properly

Missing trailing ".".

> +     */
> +    cs->exception_index = EXCP_BKPT;
> +    env->exception.syndrome = arch_info->hsr;
> +    env->exception.vaddress = arch_info->far;

You need to set env->exception.target_el to 1 as well.

> +    cc->do_interrupt(cs);
>
>      return false;
>  }
> --
> 2.6.3
>

thanks
-- PMM

[PATCH v9 5/6] target-arm: kvm - re-inject guest debug exceptions

[Peter Maydell]

On 12 November 2015 at 16:20, Alex Benn?e <alex.bennee@linaro.org> wrote:
> From: Alex Benn?e <alex@bennee.com>
>
> If we can't find details for the debug exception in our debug state
> then we can assume the exception is due to debugging inside the guest.
> To inject the exception into the guest state we re-use the TCG exception
> code (do_interupt).

"do_interrupt".

>
> However while guest debugging is in effect we currently can't handle the
> guest using single step which is heavily used by GDB.

Can you expand this to be clearer about what the problem is here?
Is this a thing fixed by this commit or a remaining issue after it?

> Signed-off-by: Alex Benn?e <alex.bennee@linaro.org>
>
> ---
> v5:
>   - new for v5
> ---
>  target-arm/helper-a64.c | 12 ++++++++++--
>  target-arm/kvm.c        | 27 +++++++++++++++++++--------
>  2 files changed, 29 insertions(+), 10 deletions(-)
>
> diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
> index deb8dbe..fc3ccdf 100644
> --- a/target-arm/helper-a64.c
> +++ b/target-arm/helper-a64.c
> @@ -25,6 +25,7 @@
>  #include "qemu/bitops.h"
>  #include "internals.h"
>  #include "qemu/crc32c.h"
> +#include "sysemu/kvm.h"
>  #include <zlib.h> /* For crc32 */
>
>  /* C2.4.7 Multiply and divide */
> @@ -469,7 +470,8 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
>                    new_el);
>      if (qemu_loglevel_mask(CPU_LOG_INT)
>          && !excp_is_internal(cs->exception_index)) {
> -        qemu_log_mask(CPU_LOG_INT, "...with ESR 0x%" PRIx32 "\n",
> +        qemu_log_mask(CPU_LOG_INT, "...with ESR %x/0x%" PRIx32 "\n",
> +                      env->exception.syndrome >> ARM_EL_EC_SHIFT,
>                        env->exception.syndrome);
>      }
>
> @@ -535,6 +537,12 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
>      aarch64_restore_sp(env, new_el);
>
>      env->pc = addr;
> -    cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
> +
> +    qemu_log_mask(CPU_LOG_INT, "...to EL%d PC 0x%" PRIx64 " PSTATE 0x%x\n",
> +                  new_el, env->pc, pstate_read(env));
> +
> +    if (!kvm_enabled()) {
> +        cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
> +    }
>  }
>  #endif
> diff --git a/target-arm/kvm.c b/target-arm/kvm.c
> index 1f57e92..4ac177a 100644
> --- a/target-arm/kvm.c
> +++ b/target-arm/kvm.c
> @@ -529,9 +529,10 @@ static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
>      struct kvm_debug_exit_arch *arch_info = &run->debug.arch;
>      int hsr_ec = arch_info->hsr >> ARM_EL_EC_SHIFT;
>      ARMCPU *cpu = ARM_CPU(cs);
> +    CPUClass *cc = CPU_GET_CLASS(cs);
>      CPUARMState *env = &cpu->env;
>
> -    /* Ensure PC is synchronised */
> +    /* Ensure all state is synchronised */

You might as well have just written the comment like that to start with :-)

>      kvm_cpu_synchronize_state(cs);
>
>      switch (hsr_ec) {
> @@ -539,7 +540,14 @@ static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
>          if (cs->singlestep_enabled) {
>              return true;
>          } else {
> -            error_report("Came out of SINGLE STEP when not enabled");
> +            /*
> +             * The kernel should have supressed the guests ability to

"suppressed". "guest's".

> +             * single step at this point so something has gone wrong.
> +             */
> +            error_report("%s: guest single-step while debugging unsupported"
> +                         " (%"PRIx64", %"PRIx32")\n",
> +                         __func__, env->pc, arch_info->hsr);
> +            return false;

Why didn't we just write the error_report this way to start with?

>          }
>          break;
>      case EC_AA64_BKPT:
> @@ -564,14 +572,17 @@ static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
>      default:
>          error_report("%s: unhandled debug exit (%"PRIx32", %"PRIx64")\n",
>                       __func__, arch_info->hsr, env->pc);
> +        return false;

Might as well put this "return false;" in the original code that
adds the default case, rather than changing the control flow in this
patch.

>      }
>
> -    /* If we don't handle this it could be it really is for the
> -       guest to handle */
> -    qemu_log_mask(LOG_UNIMP,
> -                  "%s: re-injecting exception not yet implemented"
> -                  " (0x%"PRIx32", %"PRIx64")\n",
> -                  __func__, hsr_ec, env->pc);
> +    /* If we are not handling the debug exception it must belong to
> +     * the guest. Let's re-use the existing TCG interrupt code to set
> +     * everything up properly

Missing trailing ".".

> +     */
> +    cs->exception_index = EXCP_BKPT;
> +    env->exception.syndrome = arch_info->hsr;
> +    env->exception.vaddress = arch_info->far;

You need to set env->exception.target_el to 1 as well.

> +    cc->do_interrupt(cs);
>
>      return false;
>  }
> --
> 2.6.3
>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v9 5/6] target-arm: kvm - re-inject guest debug exceptions

[Peter Maydell]

On 12 November 2015 at 16:20, Alex Bennée <alex.bennee@linaro.org> wrote:
> From: Alex Bennée <alex@bennee.com>
>
> If we can't find details for the debug exception in our debug state
> then we can assume the exception is due to debugging inside the guest.
> To inject the exception into the guest state we re-use the TCG exception
> code (do_interupt).

"do_interrupt".

>
> However while guest debugging is in effect we currently can't handle the
> guest using single step which is heavily used by GDB.

Can you expand this to be clearer about what the problem is here?
Is this a thing fixed by this commit or a remaining issue after it?

> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> ---
> v5:
>   - new for v5
> ---
>  target-arm/helper-a64.c | 12 ++++++++++--
>  target-arm/kvm.c        | 27 +++++++++++++++++++--------
>  2 files changed, 29 insertions(+), 10 deletions(-)
>
> diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
> index deb8dbe..fc3ccdf 100644
> --- a/target-arm/helper-a64.c
> +++ b/target-arm/helper-a64.c
> @@ -25,6 +25,7 @@
>  #include "qemu/bitops.h"
>  #include "internals.h"
>  #include "qemu/crc32c.h"
> +#include "sysemu/kvm.h"
>  #include <zlib.h> /* For crc32 */
>
>  /* C2.4.7 Multiply and divide */
> @@ -469,7 +470,8 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
>                    new_el);
>      if (qemu_loglevel_mask(CPU_LOG_INT)
>          && !excp_is_internal(cs->exception_index)) {
> -        qemu_log_mask(CPU_LOG_INT, "...with ESR 0x%" PRIx32 "\n",
> +        qemu_log_mask(CPU_LOG_INT, "...with ESR %x/0x%" PRIx32 "\n",
> +                      env->exception.syndrome >> ARM_EL_EC_SHIFT,
>                        env->exception.syndrome);
>      }
>
> @@ -535,6 +537,12 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
>      aarch64_restore_sp(env, new_el);
>
>      env->pc = addr;
> -    cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
> +
> +    qemu_log_mask(CPU_LOG_INT, "...to EL%d PC 0x%" PRIx64 " PSTATE 0x%x\n",
> +                  new_el, env->pc, pstate_read(env));
> +
> +    if (!kvm_enabled()) {
> +        cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
> +    }
>  }
>  #endif
> diff --git a/target-arm/kvm.c b/target-arm/kvm.c
> index 1f57e92..4ac177a 100644
> --- a/target-arm/kvm.c
> +++ b/target-arm/kvm.c
> @@ -529,9 +529,10 @@ static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
>      struct kvm_debug_exit_arch *arch_info = &run->debug.arch;
>      int hsr_ec = arch_info->hsr >> ARM_EL_EC_SHIFT;
>      ARMCPU *cpu = ARM_CPU(cs);
> +    CPUClass *cc = CPU_GET_CLASS(cs);
>      CPUARMState *env = &cpu->env;
>
> -    /* Ensure PC is synchronised */
> +    /* Ensure all state is synchronised */

You might as well have just written the comment like that to start with :-)

>      kvm_cpu_synchronize_state(cs);
>
>      switch (hsr_ec) {
> @@ -539,7 +540,14 @@ static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
>          if (cs->singlestep_enabled) {
>              return true;
>          } else {
> -            error_report("Came out of SINGLE STEP when not enabled");
> +            /*
> +             * The kernel should have supressed the guests ability to

"suppressed". "guest's".

> +             * single step at this point so something has gone wrong.
> +             */
> +            error_report("%s: guest single-step while debugging unsupported"
> +                         " (%"PRIx64", %"PRIx32")\n",
> +                         __func__, env->pc, arch_info->hsr);
> +            return false;

Why didn't we just write the error_report this way to start with?

>          }
>          break;
>      case EC_AA64_BKPT:
> @@ -564,14 +572,17 @@ static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
>      default:
>          error_report("%s: unhandled debug exit (%"PRIx32", %"PRIx64")\n",
>                       __func__, arch_info->hsr, env->pc);
> +        return false;

Might as well put this "return false;" in the original code that
adds the default case, rather than changing the control flow in this
patch.

>      }
>
> -    /* If we don't handle this it could be it really is for the
> -       guest to handle */
> -    qemu_log_mask(LOG_UNIMP,
> -                  "%s: re-injecting exception not yet implemented"
> -                  " (0x%"PRIx32", %"PRIx64")\n",
> -                  __func__, hsr_ec, env->pc);
> +    /* If we are not handling the debug exception it must belong to
> +     * the guest. Let's re-use the existing TCG interrupt code to set
> +     * everything up properly

Missing trailing ".".

> +     */
> +    cs->exception_index = EXCP_BKPT;
> +    env->exception.syndrome = arch_info->hsr;
> +    env->exception.vaddress = arch_info->far;

You need to set env->exception.target_el to 1 as well.

> +    cc->do_interrupt(cs);
>
>      return false;
>  }
> --
> 2.6.3
>

thanks
-- PMM

[PATCH v9 6/6] tests/guest-debug: introduce basic gdbstub tests

[Alex Bennée]

From: Alex Bennée <alex@bennee.com>

The aim of these tests is to combine with an appropriate kernel
image (with symbol-file vmlinux) and check it behaves as it should.
Given a kernel it checks:

  - single step
  - software breakpoint
  - hardware breakpoint
  - access, read and write watchpoints

On success it returns 0 to the calling process.

I've not plumbed this into the "make check" logic though as we need a
solution for providing non-host binaries to the tests. However the test
is structured to work with pretty much any Linux kernel image as it
uses the basic kernel_init code which is common across architectures.

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

diff --git a/tests/guest-debug/test-gdbstub.py b/tests/guest-debug/test-gdbstub.py
new file mode 100644
index 0000000..aa53567
--- /dev/null
+++ b/tests/guest-debug/test-gdbstub.py
@@ -0,0 +1,171 @@
+#
+# This script needs to be run on startup
+# qemu -kernel ${KERNEL} -s -S
+# and then:
+# gdb ${KERNEL}.vmlinux -x ${QEMU_SRC}/tests/guest-debug/test-gdbstub.py
+
+import gdb
+
+failcount = 0
+
+def report(cond, msg):
+    "Report success/fail of test"
+    if cond:
+        print "PASS: %s" % (msg)
+    else:
+        print "FAIL: %s" % (msg)
+        failcount += 1
+
+def check_step():
+    "Step an instruction, check it moved."
+    start_pc = gdb.parse_and_eval('$pc')
+    gdb.execute("si")
+    end_pc = gdb.parse_and_eval('$pc')
+
+    return not (start_pc == end_pc)
+
+
+def check_break(sym_name):
+    "Setup breakpoint, continue and check we stopped."
+    sym, ok = gdb.lookup_symbol(sym_name)
+    bp = gdb.Breakpoint(sym_name)
+
+    gdb.execute("c")
+
+    # hopefully we came back
+    end_pc = gdb.parse_and_eval('$pc')
+    print "%s == %s %d" % (end_pc, sym.value(), bp.hit_count)
+    bp.delete()
+
+    # can we test we hit bp?
+    return end_pc == sym.value()
+
+
+# We need to do hbreak manually as the python interface doesn't export it
+def check_hbreak(sym_name):
+    "Setup hardware breakpoint, continue and check we stopped."
+    sym, ok = gdb.lookup_symbol(sym_name)
+    gdb.execute("hbreak %s" % (sym_name))
+    gdb.execute("c")
+
+    # hopefully we came back
+    end_pc = gdb.parse_and_eval('$pc')
+    print "%s == %s" % (end_pc, sym.value())
+
+    if end_pc == sym.value():
+        gdb.execute("d 1")
+        return True
+    else:
+        return False
+
+
+class WatchPoint(gdb.Breakpoint):
+
+    def get_wpstr(self, sym_name):
+        "Setup sym and wp_str for given symbol."
+        self.sym, ok = gdb.lookup_symbol(sym_name)
+        wp_addr = gdb.parse_and_eval(sym_name).address
+        self.wp_str = '*(%(type)s)(&%(address)s)' % dict(
+            type = wp_addr.type, address = sym_name)
+
+        return(self.wp_str)
+
+    def __init__(self, sym_name, type):
+        wp_str = self.get_wpstr(sym_name)
+        super(WatchPoint, self).__init__(wp_str, gdb.BP_WATCHPOINT, type)
+
+    def stop(self):
+        end_pc = gdb.parse_and_eval('$pc')
+        print "HIT WP @ %s" % (end_pc)
+        return True
+
+
+def do_one_watch(sym, wtype, text):
+
+    wp = WatchPoint(sym, wtype)
+    gdb.execute("c")
+    report_str = "%s for %s (%s)" % (text, sym, wp.sym.value())
+
+    if wp.hit_count > 0:
+        report(True, report_str)
+        wp.delete()
+    else:
+        report(False, report_str)
+
+
+def check_watches(sym_name):
+    "Watch a symbol for any access."
+
+    # Should hit for any read
+    do_one_watch(sym_name, gdb.WP_ACCESS, "awatch")
+
+    # Again should hit for reads
+    do_one_watch(sym_name, gdb.WP_READ, "rwatch")
+
+    # Finally when it is written
+    do_one_watch(sym_name, gdb.WP_WRITE, "watch")
+
+
+class CatchBreakpoint(gdb.Breakpoint):
+    def __init__(self, sym_name):
+        super(CatchBreakpoint, self).__init__(sym_name)
+        self.sym, ok = gdb.lookup_symbol(sym_name)
+
+    def stop(self):
+        end_pc = gdb.parse_and_eval ('$pc')
+        print "CB: %s == %s" % (end_pc, self.sym.value())
+        if end_pc == sym.value():
+            report(False, "Hit final catchpoint")
+
+
+def run_test():
+    "Run throught the tests one by one"
+
+    print "Checking we can step the first few instructions"
+    step_ok = 0
+    for i in xrange(3):
+        if check_step():
+            step_ok += 1
+
+    report(step_ok == 3, "single step in boot code")
+
+    print "Checking HW breakpoint works"
+    break_ok = check_hbreak("kernel_init")
+    report(break_ok, "hbreak @ kernel_init")
+
+    # Can't set this up until we are in the kernel proper
+    # if we make it to run_init_process we've over-run and
+    # one of the tests failed
+    print "Setup catch-all for run_init_process"
+    cbp = CatchBreakpoint("run_init_process")
+    cpb2 = CatchBreakpoint("try_to_run_init_process")
+
+    print "Checking Normal breakpoint works"
+    break_ok = check_break("wait_for_completion")
+    report(break_ok, "break @ wait_for_completion")
+
+    print "Checking watchpoint works"
+    check_watches("system_state")
+
+#
+# This runs as the script it sourced (via -x)
+#
+
+try:
+    print "Connecting to remote"
+    gdb.execute("target remote localhost:1234")
+
+    # These are not very useful in scripts
+    gdb.execute("set pagination off")
+    gdb.execute("set confirm off")
+
+    # Run the actual tests
+    run_test()
+
+except:
+    print "GDB Exception while running test"
+    failcount += 1
+
+# Finally kill the inferior and exit gdb with a count of failures
+gdb.execute("kill")
+exit(failcount)
-- 
2.6.3

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v9 6/6] tests/guest-debug: introduce basic gdbstub tests

[Alex Bennée]

From: Alex Benn?e <alex@bennee.com>

The aim of these tests is to combine with an appropriate kernel
image (with symbol-file vmlinux) and check it behaves as it should.
Given a kernel it checks:

  - single step
  - software breakpoint
  - hardware breakpoint
  - access, read and write watchpoints

On success it returns 0 to the calling process.

I've not plumbed this into the "make check" logic though as we need a
solution for providing non-host binaries to the tests. However the test
is structured to work with pretty much any Linux kernel image as it
uses the basic kernel_init code which is common across architectures.

Signed-off-by: Alex Benn?e <alex.bennee@linaro.org>
---
 tests/guest-debug/test-gdbstub.py | 171 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 171 insertions(+)
 create mode 100644 tests/guest-debug/test-gdbstub.py

diff --git a/tests/guest-debug/test-gdbstub.py b/tests/guest-debug/test-gdbstub.py
new file mode 100644
index 0000000..aa53567
--- /dev/null
+++ b/tests/guest-debug/test-gdbstub.py
@@ -0,0 +1,171 @@
+#
+# This script needs to be run on startup
+# qemu -kernel ${KERNEL} -s -S
+# and then:
+# gdb ${KERNEL}.vmlinux -x ${QEMU_SRC}/tests/guest-debug/test-gdbstub.py
+
+import gdb
+
+failcount = 0
+
+def report(cond, msg):
+    "Report success/fail of test"
+    if cond:
+        print "PASS: %s" % (msg)
+    else:
+        print "FAIL: %s" % (msg)
+        failcount += 1
+
+def check_step():
+    "Step an instruction, check it moved."
+    start_pc = gdb.parse_and_eval('$pc')
+    gdb.execute("si")
+    end_pc = gdb.parse_and_eval('$pc')
+
+    return not (start_pc == end_pc)
+
+
+def check_break(sym_name):
+    "Setup breakpoint, continue and check we stopped."
+    sym, ok = gdb.lookup_symbol(sym_name)
+    bp = gdb.Breakpoint(sym_name)
+
+    gdb.execute("c")
+
+    # hopefully we came back
+    end_pc = gdb.parse_and_eval('$pc')
+    print "%s == %s %d" % (end_pc, sym.value(), bp.hit_count)
+    bp.delete()
+
+    # can we test we hit bp?
+    return end_pc == sym.value()
+
+
+# We need to do hbreak manually as the python interface doesn't export it
+def check_hbreak(sym_name):
+    "Setup hardware breakpoint, continue and check we stopped."
+    sym, ok = gdb.lookup_symbol(sym_name)
+    gdb.execute("hbreak %s" % (sym_name))
+    gdb.execute("c")
+
+    # hopefully we came back
+    end_pc = gdb.parse_and_eval('$pc')
+    print "%s == %s" % (end_pc, sym.value())
+
+    if end_pc == sym.value():
+        gdb.execute("d 1")
+        return True
+    else:
+        return False
+
+
+class WatchPoint(gdb.Breakpoint):
+
+    def get_wpstr(self, sym_name):
+        "Setup sym and wp_str for given symbol."
+        self.sym, ok = gdb.lookup_symbol(sym_name)
+        wp_addr = gdb.parse_and_eval(sym_name).address
+        self.wp_str = '*(%(type)s)(&%(address)s)' % dict(
+            type = wp_addr.type, address = sym_name)
+
+        return(self.wp_str)
+
+    def __init__(self, sym_name, type):
+        wp_str = self.get_wpstr(sym_name)
+        super(WatchPoint, self).__init__(wp_str, gdb.BP_WATCHPOINT, type)
+
+    def stop(self):
+        end_pc = gdb.parse_and_eval('$pc')
+        print "HIT WP @ %s" % (end_pc)
+        return True
+
+
+def do_one_watch(sym, wtype, text):
+
+    wp = WatchPoint(sym, wtype)
+    gdb.execute("c")
+    report_str = "%s for %s (%s)" % (text, sym, wp.sym.value())
+
+    if wp.hit_count > 0:
+        report(True, report_str)
+        wp.delete()
+    else:
+        report(False, report_str)
+
+
+def check_watches(sym_name):
+    "Watch a symbol for any access."
+
+    # Should hit for any read
+    do_one_watch(sym_name, gdb.WP_ACCESS, "awatch")
+
+    # Again should hit for reads
+    do_one_watch(sym_name, gdb.WP_READ, "rwatch")
+
+    # Finally when it is written
+    do_one_watch(sym_name, gdb.WP_WRITE, "watch")
+
+
+class CatchBreakpoint(gdb.Breakpoint):
+    def __init__(self, sym_name):
+        super(CatchBreakpoint, self).__init__(sym_name)
+        self.sym, ok = gdb.lookup_symbol(sym_name)
+
+    def stop(self):
+        end_pc = gdb.parse_and_eval ('$pc')
+        print "CB: %s == %s" % (end_pc, self.sym.value())
+        if end_pc == sym.value():
+            report(False, "Hit final catchpoint")
+
+
+def run_test():
+    "Run throught the tests one by one"
+
+    print "Checking we can step the first few instructions"
+    step_ok = 0
+    for i in xrange(3):
+        if check_step():
+            step_ok += 1
+
+    report(step_ok == 3, "single step in boot code")
+
+    print "Checking HW breakpoint works"
+    break_ok = check_hbreak("kernel_init")
+    report(break_ok, "hbreak @ kernel_init")
+
+    # Can't set this up until we are in the kernel proper
+    # if we make it to run_init_process we've over-run and
+    # one of the tests failed
+    print "Setup catch-all for run_init_process"
+    cbp = CatchBreakpoint("run_init_process")
+    cpb2 = CatchBreakpoint("try_to_run_init_process")
+
+    print "Checking Normal breakpoint works"
+    break_ok = check_break("wait_for_completion")
+    report(break_ok, "break @ wait_for_completion")
+
+    print "Checking watchpoint works"
+    check_watches("system_state")
+
+#
+# This runs as the script it sourced (via -x)
+#
+
+try:
+    print "Connecting to remote"
+    gdb.execute("target remote localhost:1234")
+
+    # These are not very useful in scripts
+    gdb.execute("set pagination off")
+    gdb.execute("set confirm off")
+
+    # Run the actual tests
+    run_test()
+
+except:
+    print "GDB Exception while running test"
+    failcount += 1
+
+# Finally kill the inferior and exit gdb with a count of failures
+gdb.execute("kill")
+exit(failcount)
-- 
2.6.3

[Qemu-devel] [PATCH v9 6/6] tests/guest-debug: introduce basic gdbstub tests

[Alex Bennée]

From: Alex Bennée <alex@bennee.com>

The aim of these tests is to combine with an appropriate kernel
image (with symbol-file vmlinux) and check it behaves as it should.
Given a kernel it checks:

  - single step
  - software breakpoint
  - hardware breakpoint
  - access, read and write watchpoints

On success it returns 0 to the calling process.

I've not plumbed this into the "make check" logic though as we need a
solution for providing non-host binaries to the tests. However the test
is structured to work with pretty much any Linux kernel image as it
uses the basic kernel_init code which is common across architectures.

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

diff --git a/tests/guest-debug/test-gdbstub.py b/tests/guest-debug/test-gdbstub.py
new file mode 100644
index 0000000..aa53567
--- /dev/null
+++ b/tests/guest-debug/test-gdbstub.py
@@ -0,0 +1,171 @@
+#
+# This script needs to be run on startup
+# qemu -kernel ${KERNEL} -s -S
+# and then:
+# gdb ${KERNEL}.vmlinux -x ${QEMU_SRC}/tests/guest-debug/test-gdbstub.py
+
+import gdb
+
+failcount = 0
+
+def report(cond, msg):
+    "Report success/fail of test"
+    if cond:
+        print "PASS: %s" % (msg)
+    else:
+        print "FAIL: %s" % (msg)
+        failcount += 1
+
+def check_step():
+    "Step an instruction, check it moved."
+    start_pc = gdb.parse_and_eval('$pc')
+    gdb.execute("si")
+    end_pc = gdb.parse_and_eval('$pc')
+
+    return not (start_pc == end_pc)
+
+
+def check_break(sym_name):
+    "Setup breakpoint, continue and check we stopped."
+    sym, ok = gdb.lookup_symbol(sym_name)
+    bp = gdb.Breakpoint(sym_name)
+
+    gdb.execute("c")
+
+    # hopefully we came back
+    end_pc = gdb.parse_and_eval('$pc')
+    print "%s == %s %d" % (end_pc, sym.value(), bp.hit_count)
+    bp.delete()
+
+    # can we test we hit bp?
+    return end_pc == sym.value()
+
+
+# We need to do hbreak manually as the python interface doesn't export it
+def check_hbreak(sym_name):
+    "Setup hardware breakpoint, continue and check we stopped."
+    sym, ok = gdb.lookup_symbol(sym_name)
+    gdb.execute("hbreak %s" % (sym_name))
+    gdb.execute("c")
+
+    # hopefully we came back
+    end_pc = gdb.parse_and_eval('$pc')
+    print "%s == %s" % (end_pc, sym.value())
+
+    if end_pc == sym.value():
+        gdb.execute("d 1")
+        return True
+    else:
+        return False
+
+
+class WatchPoint(gdb.Breakpoint):
+
+    def get_wpstr(self, sym_name):
+        "Setup sym and wp_str for given symbol."
+        self.sym, ok = gdb.lookup_symbol(sym_name)
+        wp_addr = gdb.parse_and_eval(sym_name).address
+        self.wp_str = '*(%(type)s)(&%(address)s)' % dict(
+            type = wp_addr.type, address = sym_name)
+
+        return(self.wp_str)
+
+    def __init__(self, sym_name, type):
+        wp_str = self.get_wpstr(sym_name)
+        super(WatchPoint, self).__init__(wp_str, gdb.BP_WATCHPOINT, type)
+
+    def stop(self):
+        end_pc = gdb.parse_and_eval('$pc')
+        print "HIT WP @ %s" % (end_pc)
+        return True
+
+
+def do_one_watch(sym, wtype, text):
+
+    wp = WatchPoint(sym, wtype)
+    gdb.execute("c")
+    report_str = "%s for %s (%s)" % (text, sym, wp.sym.value())
+
+    if wp.hit_count > 0:
+        report(True, report_str)
+        wp.delete()
+    else:
+        report(False, report_str)
+
+
+def check_watches(sym_name):
+    "Watch a symbol for any access."
+
+    # Should hit for any read
+    do_one_watch(sym_name, gdb.WP_ACCESS, "awatch")
+
+    # Again should hit for reads
+    do_one_watch(sym_name, gdb.WP_READ, "rwatch")
+
+    # Finally when it is written
+    do_one_watch(sym_name, gdb.WP_WRITE, "watch")
+
+
+class CatchBreakpoint(gdb.Breakpoint):
+    def __init__(self, sym_name):
+        super(CatchBreakpoint, self).__init__(sym_name)
+        self.sym, ok = gdb.lookup_symbol(sym_name)
+
+    def stop(self):
+        end_pc = gdb.parse_and_eval ('$pc')
+        print "CB: %s == %s" % (end_pc, self.sym.value())
+        if end_pc == sym.value():
+            report(False, "Hit final catchpoint")
+
+
+def run_test():
+    "Run throught the tests one by one"
+
+    print "Checking we can step the first few instructions"
+    step_ok = 0
+    for i in xrange(3):
+        if check_step():
+            step_ok += 1
+
+    report(step_ok == 3, "single step in boot code")
+
+    print "Checking HW breakpoint works"
+    break_ok = check_hbreak("kernel_init")
+    report(break_ok, "hbreak @ kernel_init")
+
+    # Can't set this up until we are in the kernel proper
+    # if we make it to run_init_process we've over-run and
+    # one of the tests failed
+    print "Setup catch-all for run_init_process"
+    cbp = CatchBreakpoint("run_init_process")
+    cpb2 = CatchBreakpoint("try_to_run_init_process")
+
+    print "Checking Normal breakpoint works"
+    break_ok = check_break("wait_for_completion")
+    report(break_ok, "break @ wait_for_completion")
+
+    print "Checking watchpoint works"
+    check_watches("system_state")
+
+#
+# This runs as the script it sourced (via -x)
+#
+
+try:
+    print "Connecting to remote"
+    gdb.execute("target remote localhost:1234")
+
+    # These are not very useful in scripts
+    gdb.execute("set pagination off")
+    gdb.execute("set confirm off")
+
+    # Run the actual tests
+    run_test()
+
+except:
+    print "GDB Exception while running test"
+    failcount += 1
+
+# Finally kill the inferior and exit gdb with a count of failures
+gdb.execute("kill")
+exit(failcount)
-- 
2.6.3

Re: [PATCH v9 6/6] tests/guest-debug: introduce basic gdbstub tests

[Peter Maydell]

On 12 November 2015 at 16:20, Alex Bennée <alex.bennee@linaro.org> wrote:
> From: Alex Bennée <alex@bennee.com>
>
> The aim of these tests is to combine with an appropriate kernel
> image (with symbol-file vmlinux) and check it behaves as it should.
> Given a kernel it checks:
>
>   - single step
>   - software breakpoint
>   - hardware breakpoint
>   - access, read and write watchpoints
>
> On success it returns 0 to the calling process.
>
> I've not plumbed this into the "make check" logic though as we need a
> solution for providing non-host binaries to the tests. However the test
> is structured to work with pretty much any Linux kernel image as it
> uses the basic kernel_init code which is common across architectures.

Do these tests pass if you run them on the TCG QEMU, just out
of interest?

I'm not a great fan of tests that aren't in 'make check'
because IME they just bitrot, but as you say we have no
sensible approach for handling tests that need to run real
guest code :-(

thanks
-- PMM

[PATCH v9 6/6] tests/guest-debug: introduce basic gdbstub tests

[Peter Maydell]

On 12 November 2015 at 16:20, Alex Benn?e <alex.bennee@linaro.org> wrote:
> From: Alex Benn?e <alex@bennee.com>
>
> The aim of these tests is to combine with an appropriate kernel
> image (with symbol-file vmlinux) and check it behaves as it should.
> Given a kernel it checks:
>
>   - single step
>   - software breakpoint
>   - hardware breakpoint
>   - access, read and write watchpoints
>
> On success it returns 0 to the calling process.
>
> I've not plumbed this into the "make check" logic though as we need a
> solution for providing non-host binaries to the tests. However the test
> is structured to work with pretty much any Linux kernel image as it
> uses the basic kernel_init code which is common across architectures.

Do these tests pass if you run them on the TCG QEMU, just out
of interest?

I'm not a great fan of tests that aren't in 'make check'
because IME they just bitrot, but as you say we have no
sensible approach for handling tests that need to run real
guest code :-(

thanks
-- PMM

Re: [Qemu-devel] [PATCH v9 6/6] tests/guest-debug: introduce basic gdbstub tests

[Peter Maydell]

On 12 November 2015 at 16:20, Alex Bennée <alex.bennee@linaro.org> wrote:
> From: Alex Bennée <alex@bennee.com>
>
> The aim of these tests is to combine with an appropriate kernel
> image (with symbol-file vmlinux) and check it behaves as it should.
> Given a kernel it checks:
>
>   - single step
>   - software breakpoint
>   - hardware breakpoint
>   - access, read and write watchpoints
>
> On success it returns 0 to the calling process.
>
> I've not plumbed this into the "make check" logic though as we need a
> solution for providing non-host binaries to the tests. However the test
> is structured to work with pretty much any Linux kernel image as it
> uses the basic kernel_init code which is common across architectures.

Do these tests pass if you run them on the TCG QEMU, just out
of interest?

I'm not a great fan of tests that aren't in 'make check'
because IME they just bitrot, but as you say we have no
sensible approach for handling tests that need to run real
guest code :-(

thanks
-- PMM

Re: [PATCH v9 6/6] tests/guest-debug: introduce basic gdbstub tests

[Alex Bennée]

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

> On 12 November 2015 at 16:20, Alex Bennée <alex.bennee@linaro.org> wrote:
>> From: Alex Bennée <alex@bennee.com>
>>
>> The aim of these tests is to combine with an appropriate kernel
>> image (with symbol-file vmlinux) and check it behaves as it should.
>> Given a kernel it checks:
>>
>>   - single step
>>   - software breakpoint
>>   - hardware breakpoint
>>   - access, read and write watchpoints
>>
>> On success it returns 0 to the calling process.
>>
>> I've not plumbed this into the "make check" logic though as we need a
>> solution for providing non-host binaries to the tests. However the test
>> is structured to work with pretty much any Linux kernel image as it
>> uses the basic kernel_init code which is common across architectures.
>
> Do these tests pass if you run them on the TCG QEMU, just out
> of interest?

You'll be glad to know they do.

> I'm not a great fan of tests that aren't in 'make check'
> because IME they just bitrot, but as you say we have no
> sensible approach for handling tests that need to run real
> guest code :-(

I was pondering if a git sub-project with large file support would work.
We could add pre-built binaries to the tree with appropriate meta-data
(src tree, version, config) to rebuild if required.

There would be some degree of trust implied in the original builder
though. Maybe a signed commit?

>
> thanks
> -- PMM


--
Alex Bennée

[PATCH v9 6/6] tests/guest-debug: introduce basic gdbstub tests

[Alex Bennée]

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

> On 12 November 2015 at 16:20, Alex Benn?e <alex.bennee@linaro.org> wrote:
>> From: Alex Benn?e <alex@bennee.com>
>>
>> The aim of these tests is to combine with an appropriate kernel
>> image (with symbol-file vmlinux) and check it behaves as it should.
>> Given a kernel it checks:
>>
>>   - single step
>>   - software breakpoint
>>   - hardware breakpoint
>>   - access, read and write watchpoints
>>
>> On success it returns 0 to the calling process.
>>
>> I've not plumbed this into the "make check" logic though as we need a
>> solution for providing non-host binaries to the tests. However the test
>> is structured to work with pretty much any Linux kernel image as it
>> uses the basic kernel_init code which is common across architectures.
>
> Do these tests pass if you run them on the TCG QEMU, just out
> of interest?

You'll be glad to know they do.

> I'm not a great fan of tests that aren't in 'make check'
> because IME they just bitrot, but as you say we have no
> sensible approach for handling tests that need to run real
> guest code :-(

I was pondering if a git sub-project with large file support would work.
We could add pre-built binaries to the tree with appropriate meta-data
(src tree, version, config) to rebuild if required.

There would be some degree of trust implied in the original builder
though. Maybe a signed commit?

>
> thanks
> -- PMM


--
Alex Benn?e

Re: [PATCH v9 6/6] tests/guest-debug: introduce basic gdbstub tests

[Alex Bennée]

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

> On 12 November 2015 at 16:20, Alex Bennée <alex.bennee@linaro.org> wrote:
>> From: Alex Bennée <alex@bennee.com>
>>
>> The aim of these tests is to combine with an appropriate kernel
>> image (with symbol-file vmlinux) and check it behaves as it should.
>> Given a kernel it checks:
>>
>>   - single step
>>   - software breakpoint
>>   - hardware breakpoint
>>   - access, read and write watchpoints
>>
>> On success it returns 0 to the calling process.
>>
>> I've not plumbed this into the "make check" logic though as we need a
>> solution for providing non-host binaries to the tests. However the test
>> is structured to work with pretty much any Linux kernel image as it
>> uses the basic kernel_init code which is common across architectures.
>
> Do these tests pass if you run them on the TCG QEMU, just out
> of interest?

You'll be glad to know they do.

> I'm not a great fan of tests that aren't in 'make check'
> because IME they just bitrot, but as you say we have no
> sensible approach for handling tests that need to run real
> guest code :-(

I was pondering if a git sub-project with large file support would work.
We could add pre-built binaries to the tree with appropriate meta-data
(src tree, version, config) to rebuild if required.

There would be some degree of trust implied in the original builder
though. Maybe a signed commit?

>
> thanks
> -- PMM


--
Alex Bennée

Re: [Qemu-devel] [PATCH v9 6/6] tests/guest-debug: introduce basic gdbstub tests

[Alex Bennée]

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

> On 12 November 2015 at 16:20, Alex Bennée <alex.bennee@linaro.org> wrote:
>> From: Alex Bennée <alex@bennee.com>
>>
>> The aim of these tests is to combine with an appropriate kernel
>> image (with symbol-file vmlinux) and check it behaves as it should.
>> Given a kernel it checks:
>>
>>   - single step
>>   - software breakpoint
>>   - hardware breakpoint
>>   - access, read and write watchpoints
>>
>> On success it returns 0 to the calling process.
>>
>> I've not plumbed this into the "make check" logic though as we need a
>> solution for providing non-host binaries to the tests. However the test
>> is structured to work with pretty much any Linux kernel image as it
>> uses the basic kernel_init code which is common across architectures.
>
> Do these tests pass if you run them on the TCG QEMU, just out
> of interest?

You'll be glad to know they do.

> I'm not a great fan of tests that aren't in 'make check'
> because IME they just bitrot, but as you say we have no
> sensible approach for handling tests that need to run real
> guest code :-(

I was pondering if a git sub-project with large file support would work.
We could add pre-built binaries to the tree with appropriate meta-data
(src tree, version, config) to rebuild if required.

There would be some degree of trust implied in the original builder
though. Maybe a signed commit?

>
> thanks
> -- PMM


--
Alex Bennée