On 04.10.2012, at 16:22, Bhushan Bharat-R65777 wrote:
>
>
>> -----Original Message-----
>> From: Alexander Graf [mailto:ag...@suse.de]
>> Sent: Thursday, October 04, 2012 4:56 PM
>> To: Bhushan Bharat-R65777
>> Cc: kvm-...@vger.kernel.org; kvm@vger.kernel.org
>> Subject: Re: [PATCH 6/6] KVM: booke/bookehv: Add debug stub support
>>
>>
>> On 04.10.2012, at 13:06, Bhushan Bharat-R65777 wrote:
>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: Alexander Graf [mailto:ag...@suse.de]
>>>> Sent: Monday, September 24, 2012 9:50 PM
>>>> To: Bhushan Bharat-R65777
>>>> Cc: kvm-...@vger.kernel.org; kvm@vger.kernel.org; Bhushan
>>>> Bharat-R65777
>>>> Subject: Re: [PATCH 6/6] KVM: booke/bookehv: Add debug stub support
>>>>
>>>>
>>>> On 21.08.2012, at 15:52, 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/asm/kvm.h | 29 ++++++-
>>>>> arch/powerpc/include/asm/kvm_host.h | 5 +
>>>>> arch/powerpc/kernel/asm-offsets.c | 26 ++++++
>>>>> arch/powerpc/kvm/booke.c | 144
>>>>> +++++++++++++++++++++++++++++--
>> --
>>>>> arch/powerpc/kvm/booke_interrupts.S | 110 +++++++++++++++++++++++++
>>>>> arch/powerpc/kvm/bookehv_interrupts.S | 141
>> +++++++++++++++++++++++++++++++-
>>>>> arch/powerpc/kvm/e500mc.c | 3 +-
>>>>> 7 files changed, 435 insertions(+), 23 deletions(-)
>>>>>
>>>>> diff --git a/arch/powerpc/include/asm/kvm.h
>>>>> b/arch/powerpc/include/asm/kvm.h index 61b197e..53479ea 100644
>>>>> --- a/arch/powerpc/include/asm/kvm.h
>>>>> +++ b/arch/powerpc/include/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;
>>>>> @@ -264,7 +265,31 @@ 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 pc;
>>>>> + /*
>>>>> + * exception -> returns the exception number. If the KVM_DEBUG_EXIT
>>>>> + * exit is not handled (say not h/w breakpoint or software breakpoint
>>>>> + * set for this address) by qemu then it is supposed to inject this
>>>>> + * exception to guest.
>>>>> + */
>>>>> + __u32 exception;
>>>>> + /*
>>>>> + * exiting to userspace because of h/w breakpoint, watchpoint
>>>>> + * (read, write or both) and software breakpoint.
>>>>> + */
>>>>> + __u32 status;
>>>>> };
>>>>>
>>>>> /* for KVM_SET_GUEST_DEBUG */
>>>>> @@ -276,10 +301,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 pad1;
>>>>> __u64 pad2;
>>>>> diff --git a/arch/powerpc/include/asm/kvm_host.h
>>>> b/arch/powerpc/include/asm/kvm_host.h
>>>>> index c7219c1..3ba465a 100644
>>>>> --- a/arch/powerpc/include/asm/kvm_host.h
>>>>> +++ b/arch/powerpc/include/asm/kvm_host.h
>>>>> @@ -496,7 +496,12 @@ struct kvm_vcpu_arch {
>>>>> u32 mmucfg;
>>>>> u32 epr;
>>>>> u32 crit_save;
>>>>> + /* guest debug registers*/
>>>>> struct kvmppc_booke_debug_reg dbg_reg;
>>>>> + /* shadow debug registers */
>>>>> + struct kvmppc_booke_debug_reg shadow_dbg_reg;
>>>>> + /* host debug registers*/
>>>>> + struct kvmppc_booke_debug_reg host_dbg_reg;
>>>>> #endif
>>>>> gpa_t paddr_accessed;
>>>>> gva_t vaddr_accessed;
>>>>> diff --git a/arch/powerpc/kernel/asm-offsets.c
>>>>> b/arch/powerpc/kernel/asm-
>>>> offsets.c
>>>>> index 555448e..6987821 100644
>>>>> --- a/arch/powerpc/kernel/asm-offsets.c
>>>>> +++ b/arch/powerpc/kernel/asm-offsets.c
>>>>> @@ -564,6 +564,32 @@ int main(void)
>>>>> DEFINE(VCPU_FAULT_DEAR, offsetof(struct kvm_vcpu, arch.fault_dear));
>>>>> DEFINE(VCPU_FAULT_ESR, offsetof(struct kvm_vcpu, arch.fault_esr));
>>>>> DEFINE(VCPU_CRIT_SAVE, offsetof(struct kvm_vcpu, arch.crit_save));
>>>>> + DEFINE(VCPU_DBSR, offsetof(struct kvm_vcpu, arch.dbsr));
>>>>> + DEFINE(VCPU_SHADOW_DBG, offsetof(struct kvm_vcpu, arch.shadow_dbg_reg));
>>>>> + DEFINE(VCPU_HOST_DBG, offsetof(struct kvm_vcpu, arch.host_dbg_reg));
>>>>> + DEFINE(KVMPPC_DBG_DBCR0, offsetof(struct kvmppc_booke_debug_reg,
>>>>> + dbcr0));
>>>>> + DEFINE(KVMPPC_DBG_DBCR1, offsetof(struct kvmppc_booke_debug_reg,
>>>>> + dbcr1));
>>>>> + DEFINE(KVMPPC_DBG_DBCR2, offsetof(struct kvmppc_booke_debug_reg,
>>>>> + dbcr2));
>>>>> +#ifdef CONFIG_KVM_E500MC
>>>>> + DEFINE(KVMPPC_DBG_DBCR4, offsetof(struct kvmppc_booke_debug_reg,
>>>>> + dbcr4));
>>>>> +#endif
>>>>> + DEFINE(KVMPPC_DBG_IAC1, offsetof(struct kvmppc_booke_debug_reg,
>>>>> + iac[0]));
>>>>> + DEFINE(KVMPPC_DBG_IAC2, offsetof(struct kvmppc_booke_debug_reg,
>>>>> + iac[1]));
>>>>> + DEFINE(KVMPPC_DBG_IAC3, offsetof(struct kvmppc_booke_debug_reg,
>>>>> + iac[2]));
>>>>> + DEFINE(KVMPPC_DBG_IAC4, offsetof(struct kvmppc_booke_debug_reg,
>>>>> + iac[3]));
>>>>> + DEFINE(KVMPPC_DBG_DAC1, offsetof(struct kvmppc_booke_debug_reg,
>>>>> + dac[0]));
>>>>> + DEFINE(KVMPPC_DBG_DAC2, offsetof(struct kvmppc_booke_debug_reg,
>>>>> + dac[1]));
>>>>> + DEFINE(VCPU_GUEST_DEBUG, offsetof(struct kvm_vcpu, guest_debug));
>>>>> #endif /* CONFIG_PPC_BOOK3S */
>>>>> #endif /* CONFIG_KVM */
>>>>>
>>>>> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
>>>>> index dd0e5b8..4d82e34 100644
>>>>> --- a/arch/powerpc/kvm/booke.c
>>>>> +++ b/arch/powerpc/kvm/booke.c
>>>>> @@ -132,6 +132,9 @@ void kvmppc_set_msr(struct kvm_vcpu *vcpu, u32
>>>>> new_msr)
>>>>>
>>>>> #ifdef CONFIG_KVM_BOOKE_HV
>>>>> new_msr |= MSR_GS;
>>>>> +
>>>>> + if (vcpu->guest_debug)
>>>>> + new_msr |= MSR_DE;
>>>>> #endif
>>>>>
>>>>> vcpu->arch.shared->msr = new_msr;
>>>>> @@ -667,10 +670,21 @@ out:
>>>>> return ret;
>>>>> }
>>>>>
>>>>> -static int emulation_exit(struct kvm_run *run, struct kvm_vcpu
>>>>> *vcpu)
>>>>> +static int emulation_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
>>>>> + int exit_nr)
>>>>> {
>>>>> enum emulation_result er;
>>>>>
>>>>> + if (unlikely(vcpu->guest_debug & KVM_GUESTDBG_USE_SW_BP) &&
>>>>> + vcpu->arch.last_inst == KVMPPC_INST_GUEST_GDB) {
>>>>
>>>> This belongs into the normal emulation code path, behind the same
>>>> switch() that everything else goes through.
>>>
>>> I am not sure I understood correctly. Below is the reason why I placed this
>> code here.
>>> Instruction where software breakpoint is to be set is replaced by "ehpriv"
>> instruction. On e500v2, this is not a valid instruction can causes program
>> interrupt. On e500mc, "ehpriv" is a valid instruction. Both the exit path
>> calls
>> emulation_exit(), so we have placed the code in this function.
>>> Do you want this code to be moved in program interrupt exit path for e500v2
>> and BOOKE_INTERRUPT_HV_PRIV for e500mc?
>>
>> Ok, in this patch you do (basically):
>>
>> int emulation_exit()
>> {
>> if (inst == DEBUG_INST) {
>> debug_stuff();
>> return;
>> }
>>
>> switch (inst) {
>> case INST_A:
>> foo();
>> ....
>> }
>> }
>
> Are not we doing something like this:
> int emulation_exit()
> {
> if (inst == DEBUG_INST) {
> debug_stuff();
> return;
> }
>
> status = kvmppc_emulate_instruction()
> switch (status) {
> case FAIL:
> foo();
> case DONE:
> foo1();
> ....
> }
> }
>
> Do you want something like this:
>
> int emulation_exit()
> {
>
> status = kvmppc_emulate_instruction()
> switch (status) {
> case FAIL:
> if (inst == DEBUG_INST) {
> debug_stuff();
> return;
> }
> foo();
>
> case DONE:
> foo1();
> ....
> }
> }
No, I want the DEBUG_INST be handled the same as any other instruction we
emulate.
>>
>> what I want is:
>>
>> int emulation_exit()
>> {
>> switch (inst) {
>> case INST_A:
>> foo(); break;
>> case DEBUG_INST:
>> debug_stuff(); break;
>> ....
>> }
>> }
>>
>>>
>>>
>>>>
>>>>> + run->exit_reason = KVM_EXIT_DEBUG;
>>>>> + run->debug.arch.pc = vcpu->arch.pc;
>>>>> + run->debug.arch.exception = exit_nr;
>>>>> + run->debug.arch.status = 0;
>>>>> + kvmppc_account_exit(vcpu, DEBUG_EXITS);
>>>>> + return RESUME_HOST;
>>>>> + }
>>>>> +
>>>>> er = kvmppc_emulate_instruction(run, vcpu);
>>>>> switch (er) {
>>>>> case EMULATE_DONE:
>>>>> @@ -697,6 +711,44 @@ static int emulation_exit(struct kvm_run *run,
>>>>> struct
>>>> kvm_vcpu *vcpu)
>>>>> default:
>>>>> BUG();
>>>>> }
>>>>> +
>>>>> + if (unlikely(vcpu->guest_debug & KVM_GUESTDBG_ENABLE) &&
>>>>> + (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)) {
>>>>
>>>> I don't understand how this is supposed to work. When we enable
>>>> singlestep, why would we end up in emulation_exit()?
>>>
>>> When singlestep is enabled then we set DBCR0[ICMP] and the debug handler
>> should be able to handle this. I think you are right.
>>>
>>>>
>>>>> + run->exit_reason = KVM_EXIT_DEBUG;
>>>>> + return RESUME_HOST;
>>>>> + }
>>>>> +}
>>>>> +
>>>>> +static int kvmppc_handle_debug(struct kvm_run *run, struct kvm_vcpu
>>>>> +*vcpu) {
>>>>> + u32 dbsr;
>>>>> +
>>>>> +#ifndef CONFIG_KVM_BOOKE_HV
>>>>> + if (cpu_has_feature(CPU_FTR_DEBUG_LVL_EXC))
>>>>> + vcpu->arch.pc = mfspr(SPRN_DSRR0);
>>>>> + else
>>>>> + vcpu->arch.pc = mfspr(SPRN_CSRR0); #endif
>>>>
>>>> Why doesn't this get handled in the asm code that recovers from the
>>>> respective exceptions?
>>>
>>> Yes. I will remove this.
>>>
>>>>
>>>>> + dbsr = vcpu->arch.dbsr;
>>>>> +
>>>>> + run->debug.arch.pc = vcpu->arch.pc;
>>>>> + run->debug.arch.status = 0;
>>>>> + vcpu->arch.dbsr = 0;
>>>>> +
>>>>> + 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.pc = vcpu->arch.shadow_dbg_reg.dac[0];
>>>>> + else if (dbsr & (DBSR_DAC2R | DBSR_DAC2W))
>>>>> + run->debug.arch.pc = vcpu->arch.shadow_dbg_reg.dac[1];
>>>>> + }
>>>>
>>>> Mind to explain the guest register sharing mechanism you are
>>>> envisioning? Which registers are used by the guest? Which are used by
>>>> the host? Who decides what gets used by whom?
>>>
>>> struct kvm_vcpu_arch {
>>> .
>>> .
>>>
>>> /* guest debug registers*/
>>> struct kvmppc_booke_debug_reg dbg_reg;
>>> /* shadow debug registers */
>>> struct kvmppc_booke_debug_reg shadow_dbg_reg;
>>> /* host debug registers*/
>>> struct kvmppc_booke_debug_reg host_dbg_reg;
>>>
>>> .
>>> .
>>> }
>>>
>>> dbg_reg: contains the values written by guest.
>>> shadow_dbg_reg: This contains the actual values to be written on behalf of
>> guest/qemu.
>>> host_dbg_reg: the debug register value of host (needed for store/retore
>> purpose).
>>>
>>> According to current design both, the qemu debug stub and guest, cannot
>>> share
>> the debug resources. QEMU debug stub is given priority.
>>> But host and guest can share all debug registers and host cannot debug guest
>> if it is using the debug resource (which probably does not look like a good
>> case).
>>
>> Ok. Assume that in guest context, we only ever want guest debugging enabled.
>> If
>> QEMU wants to inject a QEMU breakpoint, it will fiddle with the guest debug
>> registers itself to inject its breakpoint into them.
>
> I would like to understand the context you are describing.
> QEMU would like to inject debug interrupt to guest only if a debug interrupt
> happened in guest context and QEMU is not able to handle interrupt (because
> qemu debug have not set). Right?
Yes
> In that case the only thing that needed to be updated is guest->DBSR?
I think you lost me here :)
>
>>
>> With that scheme, would we still need shadow debug registers? Or could we
>> remove
>> that one level of indirection?
>>
>>>
>>>>
>>>>> +
>>>>> + return RESUME_HOST;
>>>>> }
>>>>>
>>>>> static void kvmppc_fill_pt_regs(struct pt_regs *regs) @@ -843,7
>>>>> +895,7 @@ int kvmppc_handle_exit(struct kvm_run *run, struct
>>>> kvm_vcpu *vcpu,
>>>>> break;
>>>>>
>>>>> case BOOKE_INTERRUPT_HV_PRIV:
>>>>> - r = emulation_exit(run, vcpu);
>>>>> + r = emulation_exit(run, vcpu, exit_nr);
>>>>> break;
>>>>>
>>>>> case BOOKE_INTERRUPT_PROGRAM:
>>>>> @@ -862,7 +914,7 @@ int kvmppc_handle_exit(struct kvm_run *run,
>>>>> struct
>>>> kvm_vcpu *vcpu,
>>>>> break;
>>>>> }
>>>>>
>>>>> - r = emulation_exit(run, vcpu);
>>>>> + r = emulation_exit(run, vcpu, exit_nr);
>>>>> break;
>>>>>
>>>>> case BOOKE_INTERRUPT_FP_UNAVAIL:
>>>>> @@ -1052,18 +1104,12 @@ 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->debug.arch.exception = exit_nr;
>>>>> + run->exit_reason = KVM_EXIT_DEBUG;
>>>>> + }
>>>>> kvmppc_account_exit(vcpu, DEBUG_EXITS);
>>>>> - r = RESUME_HOST;
>>>>> break;
>>>>> }
>>>>>
>>>>> @@ -1403,10 +1449,78 @@ 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;
>>>>> +
>>>>> + 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) {
>>>>> + 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
>>>>> + 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/booke_interrupts.S
>>>> b/arch/powerpc/kvm/booke_interrupts.S
>>>>> index dd9c5d4..2202936 100644
>>>>> --- a/arch/powerpc/kvm/booke_interrupts.S
>>>>> +++ b/arch/powerpc/kvm/booke_interrupts.S
>>>>> @@ -39,6 +39,8 @@
>>>>> #define HOST_MIN_STACK_SIZE (HOST_NV_GPR(R31) + 4) #define
>>>>> HOST_STACK_SIZE (((HOST_MIN_STACK_SIZE + 15) / 16) * 16) /* Align. */
>>>>> #define HOST_STACK_LR (HOST_STACK_SIZE + 4) /* In caller stack frame. */
>>>>> +#define DBCR0_AC_BITS (DBCR0_IAC1 | DBCR0_IAC2 | DBCR0_IAC3 |
>>>>> DBCR0_IAC4 | \
>>>>> + DBCR0_DAC1R | DBCR0_DAC1W | DBCR0_DAC2R | DBCR0_DAC2W)
>>>>>
>>>>> #define NEED_INST_MASK ((1<<BOOKE_INTERRUPT_PROGRAM) | \
>>>>> (1<<BOOKE_INTERRUPT_DTLB_MISS) | \ @@ -52,6
>>>>> +54,8 @@
>>>>> (1<<BOOKE_INTERRUPT_PROGRAM) | \
>>>>> (1<<BOOKE_INTERRUPT_DTLB_MISS))
>>>>>
>>>>> +#define NEED_DEBUG_SAVE (1<<BOOKE_INTERRUPT_DEBUG)
>>>>> +
>>>>> .macro __KVM_HANDLER ivor_nr scratch srr0
>>>>> stw r3, VCPU_GPR(R3)(r4)
>>>>> stw r5, VCPU_GPR(R5)(r4)
>>>>> @@ -212,6 +216,61 @@ _GLOBAL(kvmppc_resume_host)
>>>>> stw r9, VCPU_FAULT_ESR(r4)
>>>>> ..skip_esr:
>>>>>
>>>>> + addi r7, r4, VCPU_HOST_DBG
>>>>> + mfspr r9, SPRN_DBCR0
>>>>> + lwz r8, KVMPPC_DBG_DBCR0(r7)
>>>>> + andis. r9, r9, DBCR0_AC_BITS@h
>>>>> + beq skip_load_host_debug
>>>>> + li r9, 0
>>>>> + mtspr SPRN_DBCR0, r9 /* disable all debug event */
>>>>> + lwz r9, KVMPPC_DBG_DBCR1(r7)
>>>>> + mtspr SPRN_DBCR1, r9
>>>>> + lwz r9, KVMPPC_DBG_DBCR2(r7)
>>>>> + mtspr SPRN_DBCR2, r9
>>>>> + lwz r9, KVMPPC_DBG_IAC1+4(r7)
>>>>> + mtspr SPRN_IAC1, r9
>>>>> + lwz r9, KVMPPC_DBG_IAC2+4(r7)
>>>>> + mtspr SPRN_IAC2, r9
>>>>> +#if CONFIG_PPC_ADV_DEBUG_IACS > 2
>>>>> + lwz r9, KVMPPC_DBG_IAC3+4(r7)
>>>>> + mtspr SPRN_IAC3, r9
>>>>> + lwz r9, KVMPPC_DBG_IAC4+4(r7)
>>>>> + mtspr SPRN_IAC4, r9
>>>>> +#endif
>>>>> + lwz r9, KVMPPC_DBG_DAC1+4(r7)
>>>>> + mtspr SPRN_DAC1, r9
>>>>> + lwz r9, KVMPPC_DBG_DAC2+4(r7)
>>>>
>>>> Yikes. Please move this into a struct, similar to how the MSR
>>>> save/restore happens on x86.
>>>>
>>>> Also, these are 64-bit loads and stores, no?
>>>
>>> Ok, you mean PPC_LD/STD?
>>
>> Yeah :).
>>
>>>
>>>>
>>>>> + mtspr SPRN_DAC2, r9
>>>>> +skip_load_host_debug:
>>>>> + /* Clear h/w DBSR and save current(guest) DBSR */
>>>>> + mfspr r9, SPRN_DBSR
>>>>> + mtspr SPRN_DBSR, r9
>>>>> + isync
>>>>> + andi. r7, r6, NEED_DEBUG_SAVE
>>>>> + beq skip_dbsr_save
>>>>> + /*
>>>>> + * If vcpu->guest_debug flag is set then do not check for
>>>>> + * shared->msr.DE as this debugging (say by QEMU) does not
>>>>> + * depends on shared->msr.de. In these scanerios MSR.DE is
>>>>> + * always set using shared_msr and should be handled always.
>>>>> + */
>>>>> + lwz r7, VCPU_GUEST_DEBUG(r4)
>>>>> + cmpwi r7, 0
>>>>> + bne skip_save_trap_event
>>>>> + PPC_LL r3, VCPU_SHARED(r4)
>>>>> +#ifndef CONFIG_64BIT
>>>>> + lwz r3, (VCPU_SHARED_MSR + 4)(r3)
>>>>> +#else
>>>>> + ld r3, (VCPU_SHARED_MSR)(r3)
>>>>> +#endif
>>>>
>>>> Didn't we have 64-bit access helpers for these?
>>>
>>> I will use PPC_LD.
>>>
>>>>
>>>> Also this shouldn't happen in the entry/exit code. We have shadow_msr
>>>> for these purposes.
>>>
>>> Based on shared_msr.MSR_DE, we set make DBSR (set DBSR_TIE).
>>
>> Hrm. Yeah, must've misread something here.
>>
>> It would be awesome if we could make the debug register save/restore a bit
>> more
>> relaxed though. For example during preemption.
>
> If we delay this save/restore then host cannot do debugging of KVM code.
> Right?
You mean using kdb? Does anyone actually do that?
But let's leave this for later as an optimization.
Alex
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
