From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55186) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YwX7e-0006n5-MB for qemu-devel@nongnu.org; Sun, 24 May 2015 10:43:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YwX7a-0004AC-9D for qemu-devel@nongnu.org; Sun, 24 May 2015 10:43:26 -0400 Received: from mout.web.de ([212.227.17.11]:51185) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YwX7Z-0004A5-W0 for qemu-devel@nongnu.org; Sun, 24 May 2015 10:43:22 -0400 Message-ID: <5561E380.5010003@web.de> Date: Sun, 24 May 2015 16:43:12 +0200 From: Jan Kiszka MIME-Version: 1.0 References: <20141022113831.9548.36452.stgit@PASHA-ISP> <54B37FCA.4020805@siemens.com> <000801d02e41$75c1a000$6144e000$@Dovgaluk@ispras.ru> <54B38612.6020903@siemens.com> <54B38C03.9000608@redhat.com> In-Reply-To: <54B38C03.9000608@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="udb8xqnbMIgGj82WPAnmiRDS1u9AuFcMq" Subject: Re: [Qemu-devel] [PATCH] i386: fix breakpoints handling in icount mode List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini , Pavel Dovgaluk , qemu-devel@nongnu.org Cc: batuzovk@ispras.ru, zealot351@gmail.com, maria.klimushenkova@ispras.ru, mark.burton@greensocs.com, fred.konrad@greensocs.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --udb8xqnbMIgGj82WPAnmiRDS1u9AuFcMq Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 2015-01-12 09:55, Paolo Bonzini wrote: > On 12/01/2015 09:30, Jan Kiszka wrote: >> I think this would only cure a symptom, but it doesn't explain why we >> now hit cpu_handle_guest_debug which we do not before the patch: >=20 > That means we now exit with EXCP_DEBUG and we didn't before? >=20 > Something like this would be a more complete fix (it works if you have > both gdb and CPU breakpoints), but I'm not sure if it's also a band-aid= > for the symptoms. >=20 > diff --git a/cpu-exec.c b/cpu-exec.c > index a4f0eff..56139ac 100644 > --- a/cpu-exec.c > +++ b/cpu-exec.c > @@ -302,7 +302,7 @@ static inline TranslationBlock *tb_find_fast(CPUArc= hState *env) > return tb; > } > =20 > -static void cpu_handle_debug_exception(CPUArchState *env) > +static int cpu_handle_debug_exception(CPUArchState *env) > { > CPUState *cpu =3D ENV_GET_CPU(env); > CPUClass *cc =3D CPU_GET_CLASS(cpu); > @@ -314,7 +314,7 @@ static void cpu_handle_debug_exception(CPUArchState= *env) > } > } > =20 > - cc->debug_excp_handler(cpu); > + return cc->debug_excp_handler(cpu); > } > =20 > /* main execution loop */ > @@ -375,12 +375,15 @@ int cpu_exec(CPUArchState *env) > if (cpu->exception_index >=3D 0) { > if (cpu->exception_index >=3D EXCP_INTERRUPT) { > /* exit request from the cpu execution loop */ > - ret =3D cpu->exception_index; > - if (ret =3D=3D EXCP_DEBUG) { > - cpu_handle_debug_exception(env); > + if (cpu->exception_index =3D=3D EXCP_DEBUG) { > + ret =3D cpu_handle_debug_exception(env); > + } else { > + ret =3D cpu->exception_index; > + } > + if (ret >=3D 0) { This condition is always true for both 0 and EXCP_DEBUG. > + cpu->exception_index =3D -1; > + break; > } > - cpu->exception_index =3D -1; > - break; > } else { > #if defined(CONFIG_USER_ONLY) > /* if user mode only, we simulate a fake exception= > diff --git a/include/qom/cpu.h b/include/qom/cpu.h > index 2098f1c..c1d6c20 100644 > --- a/include/qom/cpu.h > +++ b/include/qom/cpu.h > @@ -95,7 +95,8 @@ struct TranslationBlock; > * @get_phys_page_debug: Callback for obtaining a physical address. > * @gdb_read_register: Callback for letting GDB read a register. > * @gdb_write_register: Callback for letting GDB write a register. > - * @debug_excp_handler: Callback for handling debug exceptions. > + * @debug_excp_handler: Callback for handling debug exceptions. Shoul= d > + * return either #EXCP_DEBUG or zero. > * @vmsd: State description for migration. > * @gdb_num_core_regs: Number of core registers accessible to GDB. > * @gdb_core_xml_file: File name for core registers GDB XML descriptio= n. > @@ -140,7 +141,7 @@ typedef struct CPUClass { > hwaddr (*get_phys_page_debug)(CPUState *cpu, vaddr addr); > int (*gdb_read_register)(CPUState *cpu, uint8_t *buf, int reg); > int (*gdb_write_register)(CPUState *cpu, uint8_t *buf, int reg); > - void (*debug_excp_handler)(CPUState *cpu); > + int (*debug_excp_handler)(CPUState *cpu); > =20 > int (*write_elf64_note)(WriteCoreDumpFunction f, CPUState *cpu, > int cpuid, void *opaque); > diff --git a/qom/cpu.c b/qom/cpu.c > index 9c68fa4..e86fec5 100644 > --- a/qom/cpu.c > +++ b/qom/cpu.c > @@ -193,6 +193,11 @@ static bool cpu_common_virtio_is_big_endian(CPUSta= te *cpu) > return target_words_bigendian(); > } > =20 > +static int cpu_common_debug_excp_handler(CPUState *cpu) > +{ > + return EXCP_DEBUG; > +} > + > static void cpu_common_noop(CPUState *cpu) > { > } > @@ -340,7 +345,7 @@ static void cpu_class_init(ObjectClass *klass, void= *data) > k->gdb_read_register =3D cpu_common_gdb_read_register; > k->gdb_write_register =3D cpu_common_gdb_write_register; > k->virtio_is_big_endian =3D cpu_common_virtio_is_big_endian; > - k->debug_excp_handler =3D cpu_common_noop; > + k->debug_excp_handler =3D cpu_common_debug_excp_handler; > k->cpu_exec_enter =3D cpu_common_noop; > k->cpu_exec_exit =3D cpu_common_noop; > k->cpu_exec_interrupt =3D cpu_common_exec_interrupt; > diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c > index 2bed914..40b7f79 100644 > --- a/target-arm/op_helper.c > +++ b/target-arm/op_helper.c > @@ -732,7 +732,7 @@ static bool check_breakpoints(ARMCPU *cpu) > return false; > } > =20 > -void arm_debug_excp_handler(CPUState *cs) > +int arm_debug_excp_handler(CPUState *cs) > { > /* Called by core code when a watchpoint or breakpoint fires; > * need to check which one and raise the appropriate exception. > @@ -756,9 +756,9 @@ void arm_debug_excp_handler(CPUState *cs) > } > env->exception.vaddress =3D wp_hit->hitaddr; > raise_exception(env, EXCP_DATA_ABORT); > - } else { > - cpu_resume_from_signal(cs, NULL); > + return 0; > } > + cpu_resume_from_signal(cs, NULL); > } > } else { > if (check_breakpoints(cpu)) { > @@ -771,8 +771,10 @@ void arm_debug_excp_handler(CPUState *cs) > } > /* FAR is UNKNOWN, so doesn't need setting */ > raise_exception(env, EXCP_PREFETCH_ABORT); > + return 0; > } > } > + return EXCP_DEBUG; > } > =20 > /* ??? Flag setting arithmetic is awkward because we need to do compar= isons. > diff --git a/target-i386/helper.c b/target-i386/helper.c > index 4f1ddf7..a313424 100644 > --- a/target-i386/helper.c > +++ b/target-i386/helper.c > @@ -1004,17 +1004,19 @@ bool check_hw_breakpoints(CPUX86State *env, boo= l force_dr6_update) > return hit_enabled; > } > =20 > -void breakpoint_handler(CPUState *cs) > +int breakpoint_handler(CPUState *cs) > { > X86CPU *cpu =3D X86_CPU(cs); > CPUX86State *env =3D &cpu->env; > CPUBreakpoint *bp; > + int ret =3D EXCP_DEBUG; > =20 > if (cs->watchpoint_hit) { > if (cs->watchpoint_hit->flags & BP_CPU) { > cs->watchpoint_hit =3D NULL; > if (check_hw_breakpoints(env, false)) { > raise_exception(env, EXCP01_DB); > + ret =3D 0; > } else { > cpu_resume_from_signal(cs, NULL); > } > @@ -1025,11 +1027,13 @@ void breakpoint_handler(CPUState *cs) > if (bp->flags & BP_CPU) { > check_hw_breakpoints(env, true); > raise_exception(env, EXCP01_DB); > + ret =3D 0; > } > break; > } > } > } > + return ret; > } > =20 > typedef struct MCEInjectionParams { > diff --git a/target-lm32/helper.c b/target-lm32/helper.c > index 7a41f29..088d3fa 100644 > --- a/target-lm32/helper.c > +++ b/target-lm32/helper.c > @@ -125,17 +125,19 @@ static bool check_watchpoints(CPULM32State *env) > return false; > } > =20 > -void lm32_debug_excp_handler(CPUState *cs) > +int lm32_debug_excp_handler(CPUState *cs) > { > LM32CPU *cpu =3D LM32_CPU(cs); > CPULM32State *env =3D &cpu->env; > CPUBreakpoint *bp; > + int ret =3D EXCP_DEBUG; > =20 > if (cs->watchpoint_hit) { > if (cs->watchpoint_hit->flags & BP_CPU) { > cs->watchpoint_hit =3D NULL; > if (check_watchpoints(env)) { > raise_exception(env, EXCP_WATCHPOINT); > + ret =3D 0; > } else { > cpu_resume_from_signal(cs, NULL); > } > @@ -145,11 +147,13 @@ void lm32_debug_excp_handler(CPUState *cs) > if (bp->pc =3D=3D env->pc) { > if (bp->flags & BP_CPU) { > raise_exception(env, EXCP_BREAKPOINT); > + ret =3D 0; > } > break; > } > } > } > + return ret; > } > =20 > void lm32_cpu_do_interrupt(CPUState *cs) > diff --git a/target-xtensa/helper.c b/target-xtensa/helper.c > index d84d259..52f50a2 100644 > --- a/target-xtensa/helper.c > +++ b/target-xtensa/helper.c > @@ -79,7 +79,7 @@ static uint32_t check_hw_breakpoints(CPUXtensaState *= env) > return 0; > } > =20 > -void xtensa_breakpoint_handler(CPUState *cs) > +int xtensa_breakpoint_handler(CPUState *cs) > { > XtensaCPU *cpu =3D XTENSA_CPU(cs); > CPUXtensaState *env =3D &cpu->env; > @@ -92,10 +92,12 @@ void xtensa_breakpoint_handler(CPUState *cs) > cause =3D check_hw_breakpoints(env); > if (cause) { > debug_exception_env(env, cause); > + return 0; > } > cpu_resume_from_signal(cs, NULL); > } > } > + return EXCP_DEBUG; > } > =20 > XtensaCPU *cpu_xtensa_init(const char *cpu_model) >=20 >=20 Lost track of this: This doesn't build (EXCP_DEBUG not available to qom/cpu.c, wrong breakpoint_handler prototype) and then, when the build issues are fixed, it doesn't work. Playing a bit with the code, I found out that this cures the issue: diff --git a/target-i386/translate.c b/target-i386/translate.c index 305ce50..57b607d 100644 --- a/target-i386/translate.c +++ b/target-i386/translate.c @@ -8006,6 +8006,7 @@ static inline void gen_intermediate_code_internal(X= 86CPU *cpu, if (bp->pc =3D=3D pc_ptr && !((bp->flags & BP_CPU) && (tb->flags & HF_RF_MASK)))= { gen_debug(dc, pc_ptr - dc->cs_base); + pc_ptr =3D disas_insn(env, dc, pc_ptr); goto done_generating; } } pc_ptr is used at the end of the function to calculate the tb size. I suspect that the difference prevents that the breakpoint event is associated with the stored location. Can someone explain this more properly? Then I would happily pass patch credits. Jan --udb8xqnbMIgGj82WPAnmiRDS1u9AuFcMq Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iEYEARECAAYFAlVh44AACgkQitSsb3rl5xRvsgCgmEsrQ4z2extHFuaOuvqYGIuR HL0AoMyvBy8B9h8Auz7tMv+fHT/DlKlL =g0Oc -----END PGP SIGNATURE----- --udb8xqnbMIgGj82WPAnmiRDS1u9AuFcMq--