On 12 November 2015 at 16:20, Alex Bennée <alex.bennee@xxxxxxxxxx> wrote:
> From: Alex Bennée <alex@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
>
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
