From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48854) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WwSpq-0006LI-BJ for qemu-devel@nongnu.org; Mon, 16 Jun 2014 05:04:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WwSpk-0006kW-PW for qemu-devel@nongnu.org; Mon, 16 Jun 2014 05:04:14 -0400 Message-ID: <539EB306.9050501@suse.de> Date: Mon, 16 Jun 2014 11:04:06 +0200 From: Alexander Graf MIME-Version: 1.0 References: <1402412796-17299-1-git-send-email-Bharat.Bhushan@freescale.com> <1402412796-17299-4-git-send-email-Bharat.Bhushan@freescale.com> <539853F4.2030001@suse.de> <539ADF84.4040007@suse.de> <5a6feafe37d14d94acd58705d76afdf8@DM2PR03MB574.namprd03.prod.outlook.com> In-Reply-To: <5a6feafe37d14d94acd58705d76afdf8@DM2PR03MB574.namprd03.prod.outlook.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 3/3] ppc debug: Add debug stub support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Bharat.Bhushan@freescale.com" Cc: "qemu-ppc@nongnu.org" , "qemu-devel@nongnu.org" On 16.06.14 06:27, Bharat.Bhushan@freescale.com wrote: > >> -----Original Message----- >> From: Alexander Graf [mailto:agraf@suse.de] >> Sent: Friday, June 13, 2014 4:55 PM >> To: Bhushan Bharat-R65777 >> Cc: qemu-ppc@nongnu.org; qemu-devel@nongnu.org >> Subject: Re: [PATCH 3/3] ppc debug: Add debug stub support >> >> >> On 12.06.14 09:05, Bharat.Bhushan@freescale.com wrote: >>>> -----Original Message----- >>>> From: Alexander Graf [mailto:agraf@suse.de] >>>> Sent: Wednesday, June 11, 2014 6:35 PM >>>> To: Bhushan Bharat-R65777 >>>> Cc: qemu-ppc@nongnu.org; qemu-devel@nongnu.org >>>> Subject: Re: [PATCH 3/3] ppc debug: Add debug stub support >>>> >>>> On 06/10/2014 05:06 PM, Bharat Bhushan wrote: >>>>> This patch adds software breakpoint, hardware breakpoint and >>>>> hardware watchpoint support for ppc. If the debug interrupt is not >>>>> handled then this is injected to guest. >>>>> >>>>> Signed-off-by: Bharat Bhushan >>>>> --- >>>>> hw/ppc/e500.c | 1 + >>>>> target-ppc/kvm.c | 304 >> ++++++++++++++++++++++++++++++++++++++++++++++--- >>>> -- >>>>> target-ppc/kvm_ppc.h | 1 + >>>>> 3 files changed, 278 insertions(+), 28 deletions(-) >>>>> >>>>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c index a973c18..514c595 >>>>> 100644 >>>>> --- a/hw/ppc/e500.c >>>>> +++ b/hw/ppc/e500.c >>>>> @@ -853,6 +853,7 @@ void ppce500_init(MachineState *machine, >>>>> PPCE500Params >>>> *params) >>>>> if (kvm_enabled()) { >>>>> kvmppc_init(); >>>>> } >>>>> + kvmppc_e500_hw_breakpoint_init(); >>>>> } >>>>> >>>>> static int e500_ccsr_initfn(SysBusDevice *dev) diff --git >>>>> a/target-ppc/kvm.c b/target-ppc/kvm.c index 1d2384d..f5fbec6 100644 >>>>> --- a/target-ppc/kvm.c >>>>> +++ b/target-ppc/kvm.c >>>>> @@ -38,6 +38,7 @@ >>>>> #include "hw/ppc/ppc.h" >>>>> #include "sysemu/watchdog.h" >>>>> #include "trace.h" >>>>> +#include "exec/gdbstub.h" >>>>> >>>>> //#define DEBUG_KVM >>>>> >>>>> @@ -768,6 +769,38 @@ static int kvm_put_vpa(CPUState *cs) >>>>> >>>>> static int kvmppc_inject_debug_exception(CPUState *cs) >>>>> { >>>>> + PowerPCCPU *cpu = POWERPC_CPU(cs); >>>>> + CPUPPCState *env = &cpu->env; >>>>> + struct kvm_sregs sregs; >>>>> + int ret; >>>>> + >>>>> + if (!cap_booke_sregs) { >>>>> + return -1; >>>>> + } >>>>> + >>>>> + ret = kvm_vcpu_ioctl(cs, KVM_GET_SREGS, &sregs); >>>>> + if (ret < 0) { >>>>> + return -1; >>>>> + } >>>>> + >>>> I don't think any of this code should ever run for non-e500, no? >>> You mean the code below in this function? >> Yeah :). > Why you think accessing sregs (cssr0/1, dsrr0/1 and ioctl) is e500 specific. Are not these valid for 4xx as well? Ah, misunderstanding. I don't really care about 440 - we don't seem to have any users there. However, I do care about book3s - and the code isn't compatible with book3s at all ;). > >>>>> + if (sregs.u.e.features & KVM_SREGS_E_ED) { >>>> Hrm - we never seem to set E_ED in kvm? >>> Uhh, you are right. Going through the whole discussion about interrupt >> injection to guest I found that one patch missed for upstream. >>> Will send that patch >>> >>>>> + sregs.u.e.dsrr0 = env->nip; >>>>> + sregs.u.e.dsrr1 = env->msr; >>>>> + } else { >>>>> + sregs.u.e.csrr0 = env->nip; >>>>> + sregs.u.e.csrr1 = env->msr; >>>>> + } >>>>> + >>>>> + sregs.u.e.update_special = KVM_SREGS_E_UPDATE_DBSR; >>>>> + sregs.u.e.dbsr = env->spr[SPR_BOOKE_DBSR]; >>>>> + >>>>> + ret = kvm_vcpu_ioctl(cs, KVM_SET_SREGS, &sregs); >>>>> + if (ret < 0) { >>>>> + return -1; >>>>> + } >>>>> + >>>>> + env->pending_interrupts &= ~(1 << PPC_INTERRUPT_DEBUG); >>>>> + >>>>> return 0; >>>>> } >>>>> >>>>> @@ -1275,6 +1308,239 @@ static int >>>>> kvmppc_handle_dcr_write(CPUPPCState *env, >>>> uint32_t dcrn, uint32_t dat >>>>> return 0; >>>>> } >>>>> >>>>> +int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct >>>>> +kvm_sw_breakpoint *bp) { >>>>> + uint32_t sc = tswap32(debug_inst_opcode); >>>> Heh - this will become a lot of fun for real LE host as well as guest >> systems. >>> I am trying to understand the problem here, We want to byteswap opcode only if >> it is mixed endian (host and guest are of different endianess) case? >> >> Yes :). >> >>>> For now just remove the tswap and add a comment that this needs fixing for >> LE. >>>>> + >>>>> + if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 0) >> || >>>>> + cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&sc, 4, 1)) { >>>>> + return -EINVAL; >>>>> + } >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> +int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct >>>>> +kvm_sw_breakpoint *bp) { >>>>> + uint32_t sc; >>>>> + >>>>> + if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&sc, 4, 0) || >>>>> + sc != tswap32(debug_inst_opcode) || >>>> Same here. >>>> >>>> In fact, neither of the 2 operations are in a fast path. Can't we >>>> just fetch the debug inst opcode on demand in a function here? >>> Ok will do that. >>> >>>> That will allow for easier byte >>>> swapping depending on the guest's MSR.LE setting later as well. >>>> >>>>> + cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 1)) >> { >>>>> + return -EINVAL; >>>>> + } >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static struct HWBreakpoint { >>>>> + target_ulong addr; >>>>> + int type; >>>>> +} hw_breakpoint[6]; >>>>> + >>>>> +static int nb_hw_breakpoint; >>>>> +static int nb_hw_watchpoint; >>>>> +static int max_hw_breakpoint = 4; >>>>> +static int max_hw_watchpoint = 2; >>>>> + >>>>> +void kvmppc_e500_hw_breakpoint_init(void) >>>>> +{ >>>>> + max_hw_breakpoint = 2; >>>>> + max_hw_watchpoint = 2; >>>> Can we somehow get this information from kvm and set it in kvm_arch_init? >>> Will add one_reg to get this information. >> We can also fall back to always make this 2 when there's no other way to >> enumerate. But setting the default to 4/2 and then revert to 2/2 hard codedly >> seems odd. >> >> Btw, where does the 4/2 come from? > Because 44x have 4 IACs. Ah, ok. Just ignore 440. > >>>> Worst >>>> case we'll have to look at the cpu class and derive it from there, >>>> but it really should live solely inside the kvm file. >>>> >>>>> +} >>>>> + >>>>> +static int find_hw_breakpoint(target_ulong addr, int type) { >>>>> + int n; >>>>> + >>>>> + for (n = 0; n < nb_hw_breakpoint + nb_hw_watchpoint; n++) { >>>>> + if (hw_breakpoint[n].addr == addr && hw_breakpoint[n].type == type) >> { >>>>> + return n; >>>>> + } >>>>> + } >>>>> + >>>>> + return -1; >>>>> +} >>>>> + >>>>> +static int find_hw_watchpoint(target_ulong addr, int *flag) { >>>>> + int n; >>>>> + >>>>> + n = find_hw_breakpoint(addr, GDB_WATCHPOINT_ACCESS); >>>>> + if (n >= 0) { >>>>> + *flag = BP_MEM_ACCESS; >>>>> + return n; >>>>> + } >>>>> + >>>>> + n = find_hw_breakpoint(addr, GDB_WATCHPOINT_WRITE); >>>>> + if (n >= 0) { >>>>> + *flag = BP_MEM_WRITE; >>>>> + return n; >>>>> + } >>>>> + >>>>> + n = find_hw_breakpoint(addr, GDB_WATCHPOINT_READ); >>>>> + if (n >= 0) { >>>>> + *flag = BP_MEM_READ; >>>>> + return n; >>>>> + } >>>>> + >>>>> + return -1; >>>>> +} >>>>> + >>>>> +int kvm_arch_insert_hw_breakpoint(target_ulong addr, >>>>> + target_ulong len, int type) { >>>>> + hw_breakpoint[nb_hw_breakpoint + nb_hw_watchpoint].addr = addr; >>>>> + hw_breakpoint[nb_hw_breakpoint + nb_hw_watchpoint].type = type; >>>>> + >>>>> + switch (type) { >>>>> + case GDB_BREAKPOINT_HW: >>>>> + if (nb_hw_breakpoint >= max_hw_breakpoint) { >>>>> + return -ENOBUFS; >>>>> + } >>>>> + >>>>> + if (find_hw_breakpoint(addr, type) >= 0) { >>>>> + return -EEXIST; >>>>> + } >>>>> + >>>>> + nb_hw_breakpoint++; >>>>> + break; >>>>> + >>>>> + case GDB_WATCHPOINT_WRITE: >>>>> + case GDB_WATCHPOINT_READ: >>>>> + case GDB_WATCHPOINT_ACCESS: >>>>> + if (nb_hw_watchpoint >= max_hw_watchpoint) { >>>>> + return -ENOBUFS; >>>>> + } >>>>> + >>>>> + if (find_hw_breakpoint(addr, type) >= 0) { >>>>> + return -EEXIST; >>>>> + } >>>>> + >>>>> + nb_hw_watchpoint++; >>>>> + break; >>>>> + >>>>> + default: >>>>> + return -ENOSYS; >>>>> + } >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> +int kvm_arch_remove_hw_breakpoint(target_ulong addr, >>>>> + target_ulong len, int type) { >>>>> + int n; >>>>> + >>>>> + n = find_hw_breakpoint(addr, type); >>>>> + if (n < 0) { >>>>> + return -ENOENT; >>>>> + } >>>>> + >>>>> + switch (type) { >>>>> + case GDB_BREAKPOINT_HW: >>>>> + nb_hw_breakpoint--; >>>>> + break; >>>>> + >>>>> + case GDB_WATCHPOINT_WRITE: >>>>> + case GDB_WATCHPOINT_READ: >>>>> + case GDB_WATCHPOINT_ACCESS: >>>>> + nb_hw_watchpoint--; >>>>> + break; >>>>> + >>>>> + default: >>>>> + return -ENOSYS; >>>>> + } >>>>> + hw_breakpoint[n] = hw_breakpoint[nb_hw_breakpoint + >>>>> + nb_hw_watchpoint]; >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> +void kvm_arch_remove_all_hw_breakpoints(void) >>>>> +{ >>>>> + nb_hw_breakpoint = nb_hw_watchpoint = 0; } >>>>> + >>>>> +static CPUWatchpoint hw_watchpoint; >>>>> + >>>>> + >>>>> +static int kvm_handle_debug(PowerPCCPU *cpu, struct kvm_run *run) { >>>>> + CPUState *cs = CPU(cpu); >>>>> + CPUPPCState *env = &cpu->env; >>>>> + int handle = 0; >>>>> + int n; >>>>> + int flag = 0; >>>>> + struct kvm_debug_exit_arch *arch_info = &run->debug.arch; >>>>> + >>>>> + if (cs->singlestep_enabled) { >>>>> + handle = 1; >>>>> + } else if (arch_info->status) { >>>>> + if (arch_info->status & KVMPPC_DEBUG_BREAKPOINT) { >>>>> + n = find_hw_breakpoint(arch_info->address, GDB_BREAKPOINT_HW); >>>>> + if (n >= 0) { >>>>> + handle = 1; >>>>> + } >>>>> + } else if (arch_info->status & (KVMPPC_DEBUG_WATCH_READ | >>>>> + KVMPPC_DEBUG_WATCH_WRITE)) { >>>>> + n = find_hw_watchpoint(arch_info->address, &flag); >>>>> + if (n >= 0) { >>>>> + handle = 1; >>>>> + cs->watchpoint_hit = &hw_watchpoint; >>>>> + hw_watchpoint.vaddr = hw_breakpoint[n].addr; >>>>> + hw_watchpoint.flags = flag; >>>>> + } >>>>> + } >>>>> + } else if (kvm_find_sw_breakpoint(cs, arch_info->address)) { >>>>> + handle = 1; >>>>> + } >>>>> + >>>>> + cpu_synchronize_state(cs); >>>>> + if (handle) { >>>>> + env->spr[SPR_BOOKE_DBSR] = 0; >>>> This is pretty e500 specific. >>> You mean accessing DBSR, right ? >>> To handle core specific stuff, should we add some generic function like >> kvm_ppc_set/clear_debug_event() in hw/ppc/e500_debug.c, hw/ppc/ppc4xx_debug.c >> etc ? >> >> I don't think this is hardware, it's part of the core. So it'd belong to target- >> ppc/. Probably part of the excp helper file. > Is not this (dbsr) is booke specific and not just e500 specific? Should this be accessed under POWERPC_MMU_BOOKE and POWERPC_MMU_BOOKE206 cpu models? Again, I think we can easily ignore 440, so only care about booke206 and book3s for now. And I'm pretty sure we can't reuse any of the code in kvm_handle_debug() between the two targets, so you want to branch out early and have a specific booke206 / e500 handler. if (env->excp_model == EXCP_BOOKE206) return kvm_handle_debug_booke206(...); Alex