On 04.10.2012, at 17:19, Bhushan Bharat-R65777 wrote:
>
>
>>>>>>> -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.
>
> I would like to understand what you are thinking:
> What I derived is , add the instruction in kvmppc_emulate_instruction() (or
> its child function) which,
> 1) fill the relevant information in run-> , kvmppc_account_exit(vcpu,
> DEBUG_EXITS); and returns EMULATION_DONE
> And in emulation_exit()
> status = kvmppc_emulate_instruction()
> switch (status) {
> case EMULATION_DONE:
> if (inst == DEBUG)
> return RESUME_HOST;
> }
> Or
> 2) kvmppc_account_exit(vcpu, DEBUG_EXITS); returns EMULATION_DONE;
> And in emulation_exit()
> status = kvmppc_emulate_instruction()
> switch (status) {
> case EMULATION_DONE:
> if (inst == DEBUG) {
> fill run->
> return RESUME_HOST;
> }
> }
>
> Or
> 3) kvmppc_account_exit(vcpu, DEBUG_EXITS); returns a new status type
> (EMULATION_DEBUG_INST)
> And in emulation_exit()
> status = kvmppc_emulate_instruction()
> switch (status) {
> case EMULATION_DEBUG_INST:
> fill run->
> return RESUME_HOST;
> }
This one :).
>
>>
>>>>
>>>> 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 :)
>
> Really :) . May be I wrote something wrong.
>
>>
>>>
>>>>
>>>> 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.
>
> Do you mean that debug registers should be saved/restored in preemption? any
> specific reason? I think the current save/restore does not add much overhead
> and we can better it now only, is not it?
It definitely adds quite a bit of overhead in a very performance critical path
(instruction emulation for v2).
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
