| Message ID | 20181213115503.24188-2-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Fix kvm guest debugging of AA32 guests on AA64 | expand |
Hi Alex, Thanks again for looking into this. On Thu, 13 Dec 2018 at 12:55, Alex Bennée <alex.bennee@linaro.org> wrote: > > When supported by the hardware we can run AA32 guests or even AA64 EL1 > code with AA32 EL0 mode code. Inserting a AA64 break point into AA32 > code tends to break things. This is especially acute with gdb as it > inserts temporary breakpoints when stepping through code. > > The heuristic of checking the current mode works but it's not perfect. > A user could be placing a break point in code after a mode switch and > that will still fail. However there doesn't seem to be a way to force > a hbreak by default. According to "set breakpoint auto-hw on": > > This is the default behavior. When GDB sets a breakpoint, it will try > to use the target memory map to decide if software or hardware > breakpoint must be used. > > Reported-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > Cc: Omair Javaid <omair.javaid@linaro.org> > --- > target/arm/kvm64.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c > index 0a502091e7..dd564a59b7 100644 > --- a/target/arm/kvm64.c > +++ b/target/arm/kvm64.c > @@ -989,14 +989,20 @@ int kvm_arch_get_registers(CPUState *cs) > return ret; > } > > -/* C6.6.29 BRK instruction */ > +/* BRK (A64) and BKPT (A32) instructions */ > static const uint32_t brk_insn = 0xd4200000; > +static const uint32_t bkpt_insn = 0xe1200070; We'll need to cheat here, and use an opcode that is a BKPT instruction in both ARM and Thumb modes, i.e., 0xe120be70 (where the lower 16 bits constitute a T1 bkpt opcode with imm=0x70) > > int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp) > { > + CPUARMState *env = &ARM_CPU(cs)->env; > + int el = arm_current_el(env); > + bool is_aa64 = arm_el_is_aa64(env, el); > + const uint32_t *bpi = is_aa64 ? &brk_insn : &bkpt_insn; > + > if (have_guest_debug) { > 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)) { > + cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)bpi, 4, 1)) { Should we be dealing with endianness here? > return -EINVAL; > } > return 0; > @@ -1012,7 +1018,7 @@ int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp) > > if (have_guest_debug) { > if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&brk, 4, 0) || > - brk != brk_insn || > + !(brk == brk_insn || brk == bkpt_insn) || > cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 1)) { > return -EINVAL; > } > @@ -1055,6 +1061,7 @@ bool kvm_arm_handle_debug(CPUState *cs, struct kvm_debug_exit_arch *debug_exit) > return false; > } > break; > + case EC_AA32_BKPT: > case EC_AA64_BKPT: > if (kvm_find_sw_breakpoint(cs, env->pc)) { > return true; > -- > 2.17.1 >
Ard Biesheuvel <ard.biesheuvel@linaro.org> writes: > Hi Alex, > > Thanks again for looking into this. > > On Thu, 13 Dec 2018 at 12:55, Alex Bennée <alex.bennee@linaro.org> wrote: <snip> > >> >> int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp) >> { >> + CPUARMState *env = &ARM_CPU(cs)->env; >> + int el = arm_current_el(env); >> + bool is_aa64 = arm_el_is_aa64(env, el); >> + const uint32_t *bpi = is_aa64 ? &brk_insn : &bkpt_insn; >> + >> if (have_guest_debug) { >> 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)) { >> + cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)bpi, 4, 1)) { > > Should we be dealing with endianness here? > <snip> I don't think so - everything eventually ends up (ld|st)n_p which deals with the endianness details. -- Alex Bennée
On 12/13/18 5:55 AM, Alex Bennée wrote: > int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp) > { > + CPUARMState *env = &ARM_CPU(cs)->env; > + int el = arm_current_el(env); > + bool is_aa64 = arm_el_is_aa64(env, el); This will assert for el == 0; for that you need is_a64(env). r~
On 12/13/18 8:55 AM, Alex Bennée wrote: > > Ard Biesheuvel <ard.biesheuvel@linaro.org> writes: > >> Hi Alex, >> >> Thanks again for looking into this. >> >> On Thu, 13 Dec 2018 at 12:55, Alex Bennée <alex.bennee@linaro.org> wrote: > <snip> >> >>> >>> int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp) >>> { >>> + CPUARMState *env = &ARM_CPU(cs)->env; >>> + int el = arm_current_el(env); >>> + bool is_aa64 = arm_el_is_aa64(env, el); >>> + const uint32_t *bpi = is_aa64 ? &brk_insn : &bkpt_insn; >>> + >>> if (have_guest_debug) { >>> 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)) { >>> + cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)bpi, 4, 1)) { >> >> Should we be dealing with endianness here? >> > <snip> > > I don't think so - everything eventually ends up (ld|st)n_p which deals > with the endianness details. I think Ard is right. You need to consider dynamic endianness with bswap_code(arm_sctlr_b(env)) r~ r~
On Thu, 13 Dec 2018 at 16:55, Alex Bennée <alex.bennee@linaro.org> wrote: > > When supported by the hardware we can run AA32 guests or even AA64 EL1 > code with AA32 EL0 mode code. Inserting a AA64 break point into AA32 > code tends to break things. This is especially acute with gdb as it > inserts temporary breakpoints when stepping through code. Hi Alex, Are you expecting GDB to switch targets (from AArch64 to AArch32 or vice versa) when there is mode switch for example a AArch64 binary calling a AArch32 library function. I think GDB will behave nicely when doing 1 type of debug but seamless switching between modes is still tricky for GDB. There has been some work in last few months on this but not sure where it has reached. > > The heuristic of checking the current mode works but it's not perfect. > A user could be placing a break point in code after a mode switch and > that will still fail. However there doesn't seem to be a way to force > a hbreak by default. According to "set breakpoint auto-hw on": On mode switch we ll have to switch target architecture on GDB side and GDB may not be able to do that seamlessly. Have you tried debugging such an example? > > This is the default behavior. When GDB sets a breakpoint, it will try > to use the target memory map to decide if software or hardware > breakpoint must be used. > > Reported-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > Cc: Omair Javaid <omair.javaid@linaro.org> > --- > target/arm/kvm64.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c > index 0a502091e7..dd564a59b7 100644 > --- a/target/arm/kvm64.c > +++ b/target/arm/kvm64.c > @@ -989,14 +989,20 @@ int kvm_arch_get_registers(CPUState *cs) > return ret; > } > > -/* C6.6.29 BRK instruction */ > +/* BRK (A64) and BKPT (A32) instructions */ > static const uint32_t brk_insn = 0xd4200000; > +static const uint32_t bkpt_insn = 0xe1200070; > > int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp) > { > + CPUARMState *env = &ARM_CPU(cs)->env; > + int el = arm_current_el(env); > + bool is_aa64 = arm_el_is_aa64(env, el); > + const uint32_t *bpi = is_aa64 ? &brk_insn : &bkpt_insn; > + > if (have_guest_debug) { > 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)) { > + cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)bpi, 4, 1)) { > return -EINVAL; > } > return 0; > @@ -1012,7 +1018,7 @@ int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp) > > if (have_guest_debug) { > if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&brk, 4, 0) || > - brk != brk_insn || > + !(brk == brk_insn || brk == bkpt_insn) || > cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 1)) { > return -EINVAL; > } > @@ -1055,6 +1061,7 @@ bool kvm_arm_handle_debug(CPUState *cs, struct kvm_debug_exit_arch *debug_exit) > return false; > } > break; > + case EC_AA32_BKPT: > case EC_AA64_BKPT: > if (kvm_find_sw_breakpoint(cs, env->pc)) { > return true; > -- > 2.17.1 >
On 12/14/18 2:37 AM, Omair Javaid wrote: > Are you expecting GDB to switch targets (from AArch64 to AArch32 or > vice versa) when there is mode switch for example a AArch64 binary > calling a AArch32 library function. Mode changes happen only at privilege level changes. E.g. AArch32 binary makes a system call to an AArch64 kernel, or an AArch32 guest kernel takes an i/o emulation trap to an AArch64 hypervisor. But for this sort of system debugging, it would be nice to be able to single-step through the whole operation. r~
Richard Henderson <richard.henderson@linaro.org> writes: > On 12/13/18 8:55 AM, Alex Bennée wrote: >> >> Ard Biesheuvel <ard.biesheuvel@linaro.org> writes: >> >>> Hi Alex, >>> >>> Thanks again for looking into this. >>> >>> On Thu, 13 Dec 2018 at 12:55, Alex Bennée <alex.bennee@linaro.org> wrote: >> <snip> >>> >>>> >>>> int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp) >>>> { >>>> + CPUARMState *env = &ARM_CPU(cs)->env; >>>> + int el = arm_current_el(env); >>>> + bool is_aa64 = arm_el_is_aa64(env, el); >>>> + const uint32_t *bpi = is_aa64 ? &brk_insn : &bkpt_insn; >>>> + >>>> if (have_guest_debug) { >>>> 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)) { >>>> + cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)bpi, 4, 1)) { >>> >>> Should we be dealing with endianness here? >>> >> <snip> >> >> I don't think so - everything eventually ends up (ld|st)n_p which deals >> with the endianness details. > > I think Ard is right. You need to consider dynamic endianness with > > bswap_code(arm_sctlr_b(env)) *sigh* I guess. It of course still a heuristic that can break because we don't know if the system will have switched mode by the time it gets to the breakpoint. -- Alex Bennée
On Fri, 14 Dec 2018 at 17:26, Alex Bennée <alex.bennee@linaro.org> wrote: > > > Richard Henderson <richard.henderson@linaro.org> writes: > > > On 12/13/18 8:55 AM, Alex Bennée wrote: > >> > >> Ard Biesheuvel <ard.biesheuvel@linaro.org> writes: > >> > >>> Hi Alex, > >>> > >>> Thanks again for looking into this. > >>> > >>> On Thu, 13 Dec 2018 at 12:55, Alex Bennée <alex.bennee@linaro.org> wrote: > >> <snip> > >>> > >>>> > >>>> int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp) > >>>> { > >>>> + CPUARMState *env = &ARM_CPU(cs)->env; > >>>> + int el = arm_current_el(env); > >>>> + bool is_aa64 = arm_el_is_aa64(env, el); > >>>> + const uint32_t *bpi = is_aa64 ? &brk_insn : &bkpt_insn; > >>>> + > >>>> if (have_guest_debug) { > >>>> 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)) { > >>>> + cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)bpi, 4, 1)) { > >>> > >>> Should we be dealing with endianness here? > >>> > >> <snip> > >> > >> I don't think so - everything eventually ends up (ld|st)n_p which deals > >> with the endianness details. > > > > I think Ard is right. You need to consider dynamic endianness with > > > > bswap_code(arm_sctlr_b(env)) > > *sigh* I guess. It of course still a heuristic that can break because we > don't know if the system will have switched mode by the time it gets to > the breakpoint. > Actually, I was referring to the QEMU/host side. Instruction encodings are always little endian in ARMv7 and v8 (which is all KVM cares about in any case), but I guess it is [theoretically?] possible that we are running a BE QEMU?
diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c index 0a502091e7..dd564a59b7 100644 --- a/target/arm/kvm64.c +++ b/target/arm/kvm64.c @@ -989,14 +989,20 @@ int kvm_arch_get_registers(CPUState *cs) return ret; } -/* C6.6.29 BRK instruction */ +/* BRK (A64) and BKPT (A32) instructions */ static const uint32_t brk_insn = 0xd4200000; +static const uint32_t bkpt_insn = 0xe1200070; int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp) { + CPUARMState *env = &ARM_CPU(cs)->env; + int el = arm_current_el(env); + bool is_aa64 = arm_el_is_aa64(env, el); + const uint32_t *bpi = is_aa64 ? &brk_insn : &bkpt_insn; + if (have_guest_debug) { 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)) { + cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)bpi, 4, 1)) { return -EINVAL; } return 0; @@ -1012,7 +1018,7 @@ int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp) if (have_guest_debug) { if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&brk, 4, 0) || - brk != brk_insn || + !(brk == brk_insn || brk == bkpt_insn) || cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 1)) { return -EINVAL; } @@ -1055,6 +1061,7 @@ bool kvm_arm_handle_debug(CPUState *cs, struct kvm_debug_exit_arch *debug_exit) return false; } break; + case EC_AA32_BKPT: case EC_AA64_BKPT: if (kvm_find_sw_breakpoint(cs, env->pc)) { return true;
When supported by the hardware we can run AA32 guests or even AA64 EL1 code with AA32 EL0 mode code. Inserting a AA64 break point into AA32 code tends to break things. This is especially acute with gdb as it inserts temporary breakpoints when stepping through code. The heuristic of checking the current mode works but it's not perfect. A user could be placing a break point in code after a mode switch and that will still fail. However there doesn't seem to be a way to force a hbreak by default. According to "set breakpoint auto-hw on": This is the default behavior. When GDB sets a breakpoint, it will try to use the target memory map to decide if software or hardware breakpoint must be used. Reported-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> Cc: Omair Javaid <omair.javaid@linaro.org> --- target/arm/kvm64.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) -- 2.17.1