On 02.05.2013, at 11:46, Bhushan Bharat-R65777 wrote:
>
>
>> -----Original Message-----
>> From: Alexander Graf [mailto:[email protected]]
>> Sent: Friday, April 26, 2013 4:46 PM
>> To: Bhushan Bharat-R65777
>> Cc: [email protected]; [email protected]; Wood Scott-B07421; Bhushan
>> Bharat-R65777
>> Subject: Re: [PATCH 7/7 v3] KVM: PPC: Add userspace debug stub support
>>
>>
>> On 08.04.2013, at 12:32, Bharat Bhushan wrote:
>>
>>> From: Bharat Bhushan <[email protected]>
>>>
>>> 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.
>>>
>>> Debug registers are saved/restored on vcpu_put()/vcpu_get().
>>> Also the debug registers are saved restored only if guest
>>> is using debug resources.
>>>
>>> Currently we do not support debug resource emulation to guest,
>>> so always exit to user space irrespective of user space is expecting
>>> the debug exception or not. This is unexpected event and let us
>>> leave the action on user space. This is similar to what it was before,
>>> only thing is that now we have proper exit state available to user space.
>>>
>>> Signed-off-by: Bharat Bhushan <[email protected]>
>>> ---
>>> arch/powerpc/include/asm/kvm_host.h | 8 +
>>> arch/powerpc/include/uapi/asm/kvm.h | 22 +++-
>>> arch/powerpc/kvm/booke.c | 242
>>> ++++++++++++++++++++++++++++++++---
>>> arch/powerpc/kvm/booke.h | 5 +
>>> 4 files changed, 255 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/arch/powerpc/include/asm/kvm_host.h
>> b/arch/powerpc/include/asm/kvm_host.h
>>> index e34f8fe..b9ad20f 100644
>>> --- a/arch/powerpc/include/asm/kvm_host.h
>>> +++ b/arch/powerpc/include/asm/kvm_host.h
>>> @@ -505,7 +505,15 @@ struct kvm_vcpu_arch {
>>> u32 mmucfg;
>>> u32 epr;
>>> u32 crit_save;
>>> +
>>> + /* Flag indicating that debug registers are used by guest */
>>> + bool debug_active;
>>> + /* for save/restore thread->dbcr0 on vcpu run/heavyweight_exit */
>>> + u32 saved_dbcr0;
>>> + /* guest debug registers*/
>>> struct kvmppc_booke_debug_reg dbg_reg;
>>> + /* shadow debug registers */
>>> + struct kvmppc_booke_debug_reg shadow_dbg_reg;
>>> #endif
>>> gpa_t paddr_accessed;
>>> gva_t vaddr_accessed;
>>> diff --git a/arch/powerpc/include/uapi/asm/kvm.h
>> b/arch/powerpc/include/uapi/asm/kvm.h
>>> index c0c38ed..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_NONE 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 97ae158..0e93416 100644
>>> --- a/arch/powerpc/kvm/booke.c
>>> +++ b/arch/powerpc/kvm/booke.c
>>> @@ -133,6 +133,29 @@ 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.
>>> + */
>>> + vcpu->arch.shared->msr |= MSR_DE;
>>> +#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 +173,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,
>>> @@ -646,6 +670,46 @@ int kvmppc_core_check_requests(struct kvm_vcpu *vcpu)
>>> return r;
>>> }
>>>
>>> +static void kvmppc_load_usespace_gebug(void)
>>
>> ...
>
> Please tell What does "..." mean. Does that mean next comment is about the
> above function?
It means "this one is so obvious I don't even have to write anything here".
Usespace? Gebug? Seriously? :)
>
>>
>>> +{
>>> + switch_booke_debug_regs(¤t->thread);
>>> +}
>>> +
>>> +static void kvmppc_booke_vcpu_load_debug_regs(struct kvm_vcpu *vcpu)
>>> +{
>>> + if (!vcpu->arch.debug_active)
>>> + return;
>>> +
>>> + /* Disable all debug events and clead pending debug events */
>>> + mtspr(SPRN_DBCR0, 0x0);
>>> + kvmppc_clear_dbsr();
>>> +
>>> + /*
>>> + * Check whether guest still need debug resource, if not then there
>>> + * is no need to restore guest context.
>>> + */
>>> + if (!vcpu->arch.shadow_dbg_reg.dbcr0)
>>> + return;
>>> +
>>> + /* Load Guest Context */
>>> + mtspr(SPRN_DBCR1, vcpu->arch.shadow_dbg_reg.dbcr1);
>>> + mtspr(SPRN_DBCR2, vcpu->arch.shadow_dbg_reg.dbcr2);
>>> +#ifdef CONFIG_KVM_E500MC
>>> + mtspr(SPRN_DBCR4, vcpu->arch.shadow_dbg_reg.dbcr4);
>>
>> You need to make sure DBCR4 is 0 when you leave things back to normal user
>> space. Otherwise guest debug can interfere with host debug.
>
>
> ok
>
>>
>>> +#endif
>>> + mtspr(SPRN_IAC1, vcpu->arch.shadow_dbg_reg.iac[0]);
>>> + mtspr(SPRN_IAC2, vcpu->arch.shadow_dbg_reg.iac[1]);
>>> +#if CONFIG_PPC_ADV_DEBUG_IACS > 2
>>> + mtspr(SPRN_IAC3, vcpu->arch.shadow_dbg_reg.iac[2]);
>>> + mtspr(SPRN_IAC4, vcpu->arch.shadow_dbg_reg.iac[3]);
>>> +#endif
>>> + mtspr(SPRN_DAC1, vcpu->arch.shadow_dbg_reg.dac[0]);
>>> + mtspr(SPRN_DAC2, vcpu->arch.shadow_dbg_reg.dac[1]);
>>> +
>>> + /* Enable debug events after other debug registers restored */
>>> + mtspr(SPRN_DBCR0, vcpu->arch.shadow_dbg_reg.dbcr0);
>>> +}
>>
>> All of the code above looks suspiciously similar to prime_debug_regs();.
>> Can't
>> we somehow reuse that?
>
> I think we can if
> - Save thread->debug_regs in local data structure
Yes, it can even be on the stack.
> - Load vcpu->arch->debug_regs in thread->debug_regs
> - Call prime_debug_regs();
> - Restore thread->debug_regs from local save values in first step
On heavyweight exit, based on the values on stack, yes.
>
>>
>>> +
>>> int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
>>> {
>>> int ret, s;
>>> @@ -693,11 +757,25 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct
>> kvm_vcpu *vcpu)
>>> kvmppc_load_guest_fp(vcpu);
>>> #endif
>>>
>>> + /*
>>> + * Clear current->thread.dbcr0 so that kernel does not
>>> + * restore h/w registers on context switch in vcpu running state.
>>> + */
>>> + vcpu->arch.debug_active = 1;
>>
>> = true;
>
> Ok
>
>>
>>> + vcpu->arch.saved_dbcr0 = current->thread.dbcr0;
>>> + current->thread.dbcr0 = 0;
>>> + kvmppc_booke_vcpu_load_debug_regs(vcpu);
>>
>> static void switch_booke_debug_regs(struct thread_struct *new_thread)
>> {
>> if ((current->thread.dbcr0 & DBCR0_IDM)
>> || (new_thread->dbcr0 & DBCR0_IDM))
>> prime_debug_regs(new_thread);
>> }
>>
>> The kernel will also restore debug state if the process we come from has
>> debugging enabled. Please adjust the comment accordingly.
>
> kvmppc_booke_vcpu_load_debug_regs(vcpu); cleares DBSR and DBCR0, even if
> previous process have used debug registers. Is not that sufficient? I do not
> think we should load all other registers.
It's sufficient, but the comment is wrong.
>
>>
>>> +
>>> ret = __kvmppc_vcpu_run(kvm_run, vcpu);
>>>
>>> /* No need for kvm_guest_exit. It's done in handle_exit.
>>> We also get here with interrupts enabled. */
>>>
>>> + /* Restore thread->dbcr0 */
>>> + vcpu->arch.debug_active = 0;
>>> + current->thread.dbcr0 = vcpu->arch.saved_dbcr0;
>>> + kvmppc_load_usespace_gebug();
>>> +
>>> #ifdef CONFIG_PPC_FPU
>>> kvmppc_save_guest_fp(vcpu);
>>>
>>> @@ -753,6 +831,36 @@ static int emulation_exit(struct kvm_run *run, struct
>> kvm_vcpu *vcpu)
>>> }
>>> }
>>>
>>> +/*
>>> + * Currently we do not support debug resource emulation to guest,
>>> + * so always exit to user space irrespective of user space is
>>> + * expecting the debug exception or not. This is unexpected event
>>> + * and let us leave the action on user space.
>>> + */
>>> +static int kvmppc_handle_debug(struct kvm_run *run, struct kvm_vcpu *vcpu)
>>> +{
>>> + u32 dbsr = mfspr(SPRN_DBSR);
>>> +
>>> + kvmppc_clear_dbsr();
>>> + run->debug.arch.status = 0;
>>> + run->debug.arch.address = vcpu->arch.pc;
>>> +
>>> + 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;
>>> @@ -1112,18 +1220,10 @@ 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;
>>> }
>>>
>>> @@ -1174,7 +1274,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
>>> @@ -1529,12 +1629,6 @@ int kvm_vcpu_ioctl_set_one_reg(struct kvm_vcpu *vcpu,
>> struct kvm_one_reg *reg)
>>> return r;
>>> }
>>>
>>> -int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>>> - struct kvm_guest_debug *dbg)
>>> -{
>>> - return -EINVAL;
>>> -}
>>> -
>>> int kvm_arch_vcpu_ioctl_get_fpu(struct kvm_vcpu *vcpu, struct kvm_fpu *fpu)
>>> {
>>> return -ENOTSUPP;
>>> @@ -1640,16 +1734,128 @@ void kvmppc_decrementer_func(unsigned long data)
>>> kvmppc_set_tsr_bits(vcpu, TSR_DIS);
>>> }
>>>
>>> +static void kvmppc_booke_vcpu_put_debug_regs(struct kvm_vcpu *vcpu)
>>> +{
>>> + /* Disable all debug events First */
>>
>> first
>>
>>> + mtspr(SPRN_DBCR0, 0x0);
>>> + /* Disable pending debug event by clearing DBSR */
>>> + kvmppc_clear_dbsr();
>>
>> kvmppc_handle_debug() happens with preemption enabled, no?
>
> Want to clarify, preemption will be enabled on calling local_irq_enable();
> in kvmppc_handle_exit()?
Yes. Implicitly :).
>
>> So we can have a
>> debug event that gets cleared on preempt by this.
>
> Should we read the DBSR in before local_irq_enable() in kvmppc_handle_exit()?
We have to, yes. Otherwise we could get preempted in between and get bogus data
I presume.
>
>>
>>> +}
>>> +
>>> +int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>>> + struct kvm_guest_debug *dbg)
>>> +{
>>> + struct kvmppc_booke_debug_reg *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
>>> + };
>>> +
>>> + if (!(dbg->control & KVM_GUESTDBG_ENABLE)) {
>>> + /* Clear All debug events */
>>> + vcpu->arch.shadow_dbg_reg.dbcr0 = 0;
>>> + vcpu->guest_debug = 0;
>>> +#ifdef CONFIG_KVM_BOOKE_HV
>>> + /*
>>> + * When user space is not using the debug resources
>>> + * then allow guest to change the MSR.DE.
>>> + */
>>> + vcpu->arch.shadow_msrp &= ~MSRP_DEP;
>>> +#endif
>>> + return 0;
>>> + }
>>> +
>>> +#ifdef CONFIG_KVM_BOOKE_HV
>>> + /*
>>> + * When user space is using the debug resource then
>>> + * do not allow guest to change the MSR.DE.
>>> + */
>>> + vcpu->arch.shadow_msrp &= ~MSRP_DEP;
>>
>> This is supposed to be |= right?
>
> Yes :-)
>
>>
>>> +#endif
>>> + 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))
>>> + /* Code below handles only HW breakpoints */
>>
>> Please move the comment out of the one-lined branch. Just put it below the
>> return.
>
> Ok
>
>>
>>> + return 0;
>>> +
>>> + dbg_reg = &(vcpu->arch.shadow_dbg_reg);
>>> +
>>> + /*
>>> + * On BOOKE (e500v2); Set DBCR1 and DBCR2 to allow debug events
>>> + * to occur when MSR.PR is set.
>>> + * On BOOKE-HV (e500mc+); MSR.PR = 0 when guest is running. So we
>>> + * should clear DBCR1 and DBCR2. And EPCR.DUVD is used to control
>>> + * that debug events will not come in hypervisor (GS = 0).
>>> + */
>>> +#ifdef CONFIG_KVM_BOOKE_HV
>>> + dbg_reg->dbcr1 = 0;
>>> + dbg_reg->dbcr2 = 0;
>>> +#else
>>> + dbg_reg->dbcr1 = DBCR1_IAC1US | DBCR1_IAC2US | DBCR1_IAC3US |
>>> + DBCR1_IAC4US;
>>> + dbg_reg->dbcr2 = DBCR2_DAC1US | DBCR2_DAC2US;
>>> +#endif
>>> +
>>> + for (n = 0; n < (KVMPPC_BOOKE_IAC_NUM + KVMPPC_BOOKE_DAC_NUM); n++) {
>>> + u32 type = dbg->arch.bp[n].type;
>>> +
>>> + if (!type)
>>> + continue;
>>
>> Scott's comment on why a zero-type is different from any other invalid type
>> is
>> still outstanding I think.
>
> Userspce does following
> - dbg->arch.bp[] is first zero initialized. So dbg->arch.bp[n].type is 0
> (KVMPPC_DEBUG_NONE)
> - Then set following for valid breakpoints/watchpoints:
> -- dbg->arch.bp[n].type (KVMPPC_DEBUG_BREAKPOINT or
> KVMPPC_DEBUG_WATCH_WRITE or KVMPPC_DEBUG_WATCH_READ)
> -- dbg->arch.bp[n].addr
>
> I tried to avoid loop when type is 0 (probably saying type ==
> KVMPPC_DEBUG_NONE should be more clear).
That works, yes.
> if type is invalid then should we return -EINVAL.
Yes.
Alex
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
