On 14.03.2013, at 06:18, Bhushan Bharat-R65777 wrote:
>
>
>> -----Original Message-----
>> From: Alexander Graf [mailto:ag...@suse.de]
>> Sent: Thursday, March 07, 2013 7:09 PM
>> To: Bhushan Bharat-R65777
>> Cc: kvm-ppc@vger.kernel.org; k...@vger.kernel.org; Wood Scott-B07421; Bhushan
>> Bharat-R65777
>> Subject: Re: [PATCH 7/7] KVM: PPC: Add userspace debug stub support
>>
>>
>> On 28.02.2013, at 05:13, Bharat Bhushan wrote:
>>
>>> This patch adds the debug stub support on booke/bookehv.
>>> Now QEMU debug stub can use hw breakpoint, watchpoint and software
>>> breakpoint to debug guest.
>>>
>>> Signed-off-by: Bharat Bhushan <bharat.bhus...@freescale.com>
>>> ---
>>> arch/powerpc/include/uapi/asm/kvm.h | 22 +++++-
>>> arch/powerpc/kvm/booke.c | 143
>>> +++++++++++++++++++++++++++++++---
>>> arch/powerpc/kvm/e500_emulate.c | 6 ++
>>> arch/powerpc/kvm/e500mc.c | 3 +-
>>> 4 files changed, 155 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/arch/powerpc/include/uapi/asm/kvm.h
>>> b/arch/powerpc/include/uapi/asm/kvm.h
>>> index 15f9a00..d7ce449 100644
>>> --- a/arch/powerpc/include/uapi/asm/kvm.h
>>> +++ b/arch/powerpc/include/uapi/asm/kvm.h
>>> @@ -25,6 +25,7 @@
>>> /* Select powerpc specific features in <linux/kvm.h> */ #define
>>> __KVM_HAVE_SPAPR_TCE #define __KVM_HAVE_PPC_SMT
>>> +#define __KVM_HAVE_GUEST_DEBUG
>>>
>>> struct kvm_regs {
>>> __u64 pc;
>>> @@ -267,7 +268,24 @@ struct kvm_fpu {
>>> __u64 fpr[32];
>>> };
>>>
>>> +/*
>>> + * Defines for h/w breakpoint, watchpoint (read, write or both) and
>>> + * software breakpoint.
>>> + * These are used as "type" in KVM_SET_GUEST_DEBUG ioctl and "status"
>>> + * for KVM_DEBUG_EXIT.
>>> + */
>>> +#define KVMPPC_DEBUG_NONE 0x0
>>> +#define KVMPPC_DEBUG_BREAKPOINT (1UL << 1)
>>> +#define KVMPPC_DEBUG_WATCH_WRITE (1UL << 2)
>>> +#define KVMPPC_DEBUG_WATCH_READ (1UL << 3)
>>> struct kvm_debug_exit_arch {
>>> + __u64 address;
>>> + /*
>>> + * exiting to userspace because of h/w breakpoint, watchpoint
>>> + * (read, write or both) and software breakpoint.
>>> + */
>>> + __u32 status;
>>> + __u32 reserved;
>>> };
>>>
>>> /* for KVM_SET_GUEST_DEBUG */
>>> @@ -279,10 +297,6 @@ struct kvm_guest_debug_arch {
>>> * Type denotes h/w breakpoint, read watchpoint, write
>>> * watchpoint or watchpoint (both read and write).
>>> */
>>> -#define KVMPPC_DEBUG_NOTYPE 0x0
>>> -#define KVMPPC_DEBUG_BREAKPOINT (1UL << 1)
>>> -#define KVMPPC_DEBUG_WATCH_WRITE (1UL << 2)
>>> -#define KVMPPC_DEBUG_WATCH_READ (1UL << 3)
>>> __u32 type;
>>> __u32 reserved;
>>> } bp[16];
>>> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c index
>>> 1de93a8..21b0313 100644
>>> --- a/arch/powerpc/kvm/booke.c
>>> +++ b/arch/powerpc/kvm/booke.c
>>> @@ -133,6 +133,30 @@ static void kvmppc_vcpu_sync_fpu(struct kvm_vcpu
>>> *vcpu) #endif }
>>>
>>> +static void kvmppc_vcpu_sync_debug(struct kvm_vcpu *vcpu) {
>>> + /* Synchronize guest's desire to get debug interrupts into shadow
>>> +MSR */ #ifndef CONFIG_KVM_BOOKE_HV
>>> + vcpu->arch.shadow_msr &= ~MSR_DE;
>>> + vcpu->arch.shadow_msr |= vcpu->arch.shared->msr & MSR_DE; #endif
>>> +
>>> + /* Force enable debug interrupts when user space wants to debug */
>>> + if (vcpu->guest_debug) {
>>> +#ifdef CONFIG_KVM_BOOKE_HV
>>> + /*
>>> + * Since there is no shadow MSR, sync MSR_DE into the guest
>>> + * visible MSR. Do not allow guest to change MSR[DE].
>>> + */
>>> + vcpu->arch.shared->msr |= MSR_DE;
>>> + mtspr(SPRN_MSRP, mfspr(SPRN_MSRP) | MSRP_DEP); #else
>>> + vcpu->arch.shadow_msr |= MSR_DE;
>>> + vcpu->arch.shared->msr &= ~MSR_DE;
>>> +#endif
>>> + }
>>> +}
>>> +
>>> /*
>>> * Helper function for "full" MSR writes. No need to call this if
>>> only
>>> * EE/CE/ME/DE/RI are changing.
>>> @@ -150,6 +174,7 @@ void kvmppc_set_msr(struct kvm_vcpu *vcpu, u32 new_msr)
>>> kvmppc_mmu_msr_notify(vcpu, old_msr);
>>> kvmppc_vcpu_sync_spe(vcpu);
>>> kvmppc_vcpu_sync_fpu(vcpu);
>>> + kvmppc_vcpu_sync_debug(vcpu);
>>> }
>>>
>>> static void kvmppc_booke_queue_irqprio(struct kvm_vcpu *vcpu, @@
>>> -736,6 +761,13 @@ static int emulation_exit(struct kvm_run *run, struct
>> kvm_vcpu *vcpu)
>>> run->exit_reason = KVM_EXIT_DCR;
>>> return RESUME_HOST;
>>>
>>> + case EMULATE_EXIT_USER:
>>> + run->exit_reason = KVM_EXIT_DEBUG;
>>> + run->debug.arch.address = vcpu->arch.pc;
>>> + run->debug.arch.status = 0;
>>> + kvmppc_account_exit(vcpu, DEBUG_EXITS);
>>
>> As mentioned previously, this is wrong and needs to go into the instruction
>> emulation code for that opcode.
>
> ok
>
>>
>>> + return RESUME_HOST;
>>> +
>>> case EMULATE_FAIL:
>>> printk(KERN_CRIT "%s: emulation at %lx failed (%08x)\n",
>>> __func__, vcpu->arch.pc, vcpu->arch.last_inst); @@ -751,6
>>> +783,28 @@ static int emulation_exit(struct kvm_run *run, struct kvm_vcpu
>> *vcpu)
>>> }
>>> }
>>>
>>> +static int kvmppc_handle_debug(struct kvm_run *run, struct kvm_vcpu
>>> +*vcpu) {
>>> + u32 dbsr = vcpu->arch.dbsr;
>>> + run->debug.arch.status = 0;
>>> + run->debug.arch.address = vcpu->arch.pc;
>>
>> This should go into the if(breakpoint) branch.
>
> Can there be the case when do breakpoint and debug interrupt happen?
At least not according to the code below :). If that's a valid case, then quite
a bit of code would need to be remodeled. I'd say ignore the possibility for
now.
>
>>
>>> +
>>> + if (dbsr & (DBSR_IAC1 | DBSR_IAC2 | DBSR_IAC3 | DBSR_IAC4)) {
>>> + run->debug.arch.status |= KVMPPC_DEBUG_BREAKPOINT;
>>> + } else {
>>> + if (dbsr & (DBSR_DAC1W | DBSR_DAC2W))
>>> + run->debug.arch.status |= KVMPPC_DEBUG_WATCH_WRITE;
>>> + else if (dbsr & (DBSR_DAC1R | DBSR_DAC2R))
>>> + run->debug.arch.status |= KVMPPC_DEBUG_WATCH_READ;
>>> + if (dbsr & (DBSR_DAC1R | DBSR_DAC1W))
>>> + run->debug.arch.address =
>>> vcpu->arch.shadow_dbg_reg.dac[0];
>>> + else if (dbsr & (DBSR_DAC2R | DBSR_DAC2W))
>>> + run->debug.arch.address =
>>> vcpu->arch.shadow_dbg_reg.dac[1];
>>> + }
>>> +
>>> + return RESUME_HOST;
>>> +}
>>> +
>>> static void kvmppc_fill_pt_regs(struct pt_regs *regs) {
>>> ulong r1, ip, msr, lr;
>>> @@ -1110,18 +1164,11 @@ int kvmppc_handle_exit(struct kvm_run *run, struct
>> kvm_vcpu *vcpu,
>>> }
>>>
>>> case BOOKE_INTERRUPT_DEBUG: {
>>> - u32 dbsr;
>>> -
>>> - vcpu->arch.pc = mfspr(SPRN_CSRR0);
>>> -
>>> - /* clear IAC events in DBSR register */
>>> - dbsr = mfspr(SPRN_DBSR);
>>> - dbsr &= DBSR_IAC1 | DBSR_IAC2 | DBSR_IAC3 | DBSR_IAC4;
>>> - mtspr(SPRN_DBSR, dbsr);
>>> -
>>> - run->exit_reason = KVM_EXIT_DEBUG;
>>> + r = kvmppc_handle_debug(run, vcpu);
>>> + if (r == RESUME_HOST) {
>>> + run->exit_reason = KVM_EXIT_DEBUG;
>>> + }
>>> kvmppc_account_exit(vcpu, DEBUG_EXITS);
>>> - r = RESUME_HOST;
>>> break;
>>> }
>>>
>>> @@ -1172,7 +1219,7 @@ int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu)
>>> kvmppc_set_msr(vcpu, 0);
>>>
>>> #ifndef CONFIG_KVM_BOOKE_HV
>>> - vcpu->arch.shadow_msr = MSR_USER | MSR_DE | MSR_IS | MSR_DS;
>>> + vcpu->arch.shadow_msr = MSR_USER | MSR_IS | MSR_DS;
>>> vcpu->arch.shadow_pid = 1;
>>> vcpu->arch.shared->msr = 0;
>>> #endif
>>> @@ -1527,10 +1574,80 @@ int kvm_vcpu_ioctl_set_one_reg(struct kvm_vcpu
>>> *vcpu,
>> struct kvm_one_reg *reg)
>>> return r;
>>> }
>>>
>>> +#define BP_NUM KVMPPC_BOOKE_IAC_NUM
>>> +#define WP_NUM KVMPPC_BOOKE_DAC_NUM
>>> +
>>> int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>>> struct kvm_guest_debug *dbg)
>>> {
>>> - return -EINVAL;
>>> +
>>> + if (!(dbg->control & KVM_GUESTDBG_ENABLE)) {
>>> + /* Clear All debug events */
>>> + vcpu->arch.shadow_dbg_reg.dbcr0 = 0;
>>> + vcpu->guest_debug = 0;
>>> + return 0;
>>> + }
>>> +
>>> + vcpu->guest_debug = dbg->control;
>>> + vcpu->arch.shadow_dbg_reg.dbcr0 = 0;
>>> + /* Set DBCR0_EDM in guest visible DBCR0 register. */
>>> + vcpu->arch.dbg_reg.dbcr0 = DBCR0_EDM;
>>> +
>>> + if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)
>>> + vcpu->arch.shadow_dbg_reg.dbcr0 |= DBCR0_IDM | DBCR0_IC;
>>> +
>>> + if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP) {
>>
>> if (!(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP)) {
>> /* Code below handles only HW breakpoints */
>> return 0;
>> }
>
> ok
>
>>
>>> + struct kvmppc_booke_debug_reg *gdbgr =
>>> + &(vcpu->arch.shadow_dbg_reg);
>>> + int n, b = 0, w = 0;
>>> + const u32 bp_code[] = {
>>> + DBCR0_IAC1 | DBCR0_IDM,
>>> + DBCR0_IAC2 | DBCR0_IDM,
>>> + DBCR0_IAC3 | DBCR0_IDM,
>>> + DBCR0_IAC4 | DBCR0_IDM
>>> + };
>>> + const u32 wp_code[] = {
>>> + DBCR0_DAC1W | DBCR0_IDM,
>>> + DBCR0_DAC2W | DBCR0_IDM,
>>> + DBCR0_DAC1R | DBCR0_IDM,
>>> + DBCR0_DAC2R | DBCR0_IDM
>>> + };
>>> +
>>> +#ifndef CONFIG_KVM_BOOKE_HV
>>
>> Please no double negation.
> You mean we should use
> #ifdef CONFIG_KVM_BOOKE_HV
> gdbgr->dbcr1 = 0;
> gdbgr->dbcr2 = 0;
> #else
> gdbgr->dbcr1 = DBCR1_IAC1US | DBCR1_IAC2US |
> DBCR1_IAC3US | DBCR1_IAC4US;
> gdbgr->dbcr2 = DBCR2_DAC1US | DBCR2_DAC2US;
> #endif
>
>> Also, what is this about?
>
> This These bits says that IAC1-4 and DAC1-2 can happen when MSR.PR is set of
> not.
> On BOOKE (e500v2); MSR.PR = 1 when guest is running. So we need to set these
> bits
> On BOOKE-HV (e500mc+); MSR.PR = 0 when guest is running. So we do not need
> these bits to be set.
Ah, please add a comment explaining this here.
>
>>
>>> + gdbgr->dbcr1 = DBCR1_IAC1US | DBCR1_IAC2US |
>>> + DBCR1_IAC3US | DBCR1_IAC4US;
>>> + gdbgr->dbcr2 = DBCR2_DAC1US | DBCR2_DAC2US; #else
>>> + gdbgr->dbcr1 = 0;
>>> + gdbgr->dbcr2 = 0;
>>> +#endif
>>> +
>>> + for (n = 0; n < (BP_NUM + WP_NUM); n++) {
>>> + u32 type = dbg->arch.bp[n].type;
>>> +
>>> + if (!type)
>>> + break;
>>> +
>>> + if (type & (KVMPPC_DEBUG_WATCH_READ |
>>> + KVMPPC_DEBUG_WATCH_WRITE)) {
>>> + if (w < WP_NUM) {
>>> + if (type & KVMPPC_DEBUG_WATCH_READ)
>>> + gdbgr->dbcr0 |= wp_code[w + 2];
>>> + if (type & KVMPPC_DEBUG_WATCH_WRITE)
>>> + gdbgr->dbcr0 |= wp_code[w];
>>> + gdbgr->dac[w] = dbg->arch.bp[n].addr;
>>> + w++;
>>> + }
>>> + } else if (type & KVMPPC_DEBUG_BREAKPOINT) {
>>> + if (b < BP_NUM) {
>>> + gdbgr->dbcr0 |= bp_code[b];
>>> + gdbgr->iac[b] = dbg->arch.bp[n].addr;
>>> + b++;
>>> + }
>>> + }
>>> + }
>>> + }
>>> + return 0;
>>> }
>>>
>>> int kvm_arch_vcpu_ioctl_get_fpu(struct kvm_vcpu *vcpu, struct kvm_fpu
>>> *fpu) diff --git a/arch/powerpc/kvm/e500_emulate.c
>>> b/arch/powerpc/kvm/e500_emulate.c index e78f353..83ac877 100644
>>> --- a/arch/powerpc/kvm/e500_emulate.c
>>> +++ b/arch/powerpc/kvm/e500_emulate.c
>>> @@ -26,6 +26,7 @@
>>> #define XOP_TLBRE 946
>>> #define XOP_TLBWE 978
>>> #define XOP_TLBILX 18
>>> +#define XOP_EHPRIV 270
>>>
>>> #ifdef CONFIG_KVM_E500MC
>>> static int dbell2prio(ulong param)
>>> @@ -130,6 +131,11 @@ int kvmppc_core_emulate_op(struct kvm_run *run, struct
>> kvm_vcpu *vcpu,
>>> emulated = kvmppc_e500_emul_tlbivax(vcpu, ea);
>>> break;
>>>
>>> + case XOP_EHPRIV:
>>> + emulated = EMULATE_EXIT_USER;
>>> + *advance = 0;
>>> + break;
>>> +
>>> default:
>>> emulated = EMULATE_FAIL;
>>> }
>>> diff --git a/arch/powerpc/kvm/e500mc.c b/arch/powerpc/kvm/e500mc.c
>>> index 1f89d26..f5fc6f5 100644
>>> --- a/arch/powerpc/kvm/e500mc.c
>>> +++ b/arch/powerpc/kvm/e500mc.c
>>> @@ -182,8 +182,7 @@ int kvmppc_core_vcpu_setup(struct kvm_vcpu *vcpu)
>>> {
>>> struct kvmppc_vcpu_e500 *vcpu_e500 = to_e500(vcpu);
>>>
>>> - vcpu->arch.shadow_epcr = SPRN_EPCR_DSIGS | SPRN_EPCR_DGTMI | \
>>> - SPRN_EPCR_DUVD;
>>> + vcpu->arch.shadow_epcr = SPRN_EPCR_DSIGS | SPRN_EPCR_DGTMI;
>>
>> Doesn't this route all debug events through the host?
>
> No; This means that debug events can occur in hypervisor state or not.
>
> EPCR.DUVD = 0 ; Debug events can occur in the hypervisor state.
>
> EPCR.DUVD = 1 ; Debug events cannot occur in the hypervisor state.
>
> So we allow debug events to occur in hypervisor state.
Why do we care about debug events in our entry/exit code and didn't care about
them before? If anything, this is a completely separate patch, orthogonal to
this patch series, and requires a good bit of explanation.
Alex
> On lightweight exit we set ECPU.DUVD (if guest using debug facility) so debug
> events will not come during guest entry/exit code. On guest exit we clear
> this bit (after restoring host state) so hypervisor can use debug features.
>
> Thanks
> -Bharat
>>
>>
>> Alex
>>
>>> #ifdef CONFIG_64BIT
>>> vcpu->arch.shadow_epcr |= SPRN_EPCR_ICM; #endif
>>> --
>>> 1.7.0.4
>>>
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
>>> the body of a message to majord...@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
>
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
