Re: [PATCH 3/4] kvmppc/booke: guest debug support

[Jan Kiszka <jan.kiszka@siemens.com>]

Liu Yu wrote:
> According to user's gdb command,
> we set the corresponding debug control bits in shadow.
> 
> Signed-off-by: Liu Yu <yu.liu-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> ---
>  arch/powerpc/include/asm/kvm_ppc.h |    3 +
>  arch/powerpc/kvm/booke.c           |   93 ++++++++++++++++++++++++++++++++++--
>  arch/powerpc/kvm/e500.c            |    8 ---
>  arch/powerpc/kvm/powerpc.c         |    2 +-
>  4 files changed, 93 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
> index e264282..8918aac 100644
> --- a/arch/powerpc/include/asm/kvm_ppc.h
> +++ b/arch/powerpc/include/asm/kvm_ppc.h
> @@ -94,6 +94,9 @@ extern int kvmppc_core_emulate_op(struct kvm_run *run, struct kvm_vcpu *vcpu,
>  extern int kvmppc_core_emulate_mtspr(struct kvm_vcpu *vcpu, int sprn, int rs);
>  extern int kvmppc_core_emulate_mfspr(struct kvm_vcpu *vcpu, int sprn, int rt);
>  
> +extern int kvmppc_core_set_guest_debug(struct kvm_vcpu *vcpu,
> +                                       struct kvm_guest_debug *dbg);
> +
>  extern int kvmppc_booke_init(void);
>  extern void kvmppc_booke_exit(void);
>  
> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> index 4d686cc..ec2722d 100644
> --- a/arch/powerpc/kvm/booke.c
> +++ b/arch/powerpc/kvm/booke.c
> @@ -267,6 +267,16 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
>  			break;
>  		}
>  
> +		if (unlikely(vcpu->guest_debug & KVM_GUESTDBG_ENABLE) &&

This should better check for KVM_GUESTDBG_USE_SW_BP.

> +		             (vcpu->arch.last_inst == KVM_INST_GUESTGDB)) {
> +			run->exit_reason = KVM_EXIT_DEBUG;
> +			run->debug.arch.pc = vcpu->arch.pc;
> +			run->debug.arch.exception = exit_nr;
> +			kvmppc_account_exit(vcpu, DEBUG_EXITS);
> +			r = RESUME_HOST;
> +			break;
> +		}
> +
>  		er = kvmppc_emulate_instruction(run, vcpu);
>  		switch (er) {
>  		case EMULATE_DONE:
> @@ -293,6 +303,12 @@ int kvmppc_handle_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)) {

Checking for KVM_GUESTDBG_ENABLE is redundant as you enforce guest_debug
= 0 in kvmppc_core_set_guest_debug if KVM_GUESTDBG_ENABLE is not set.

> +			run->exit_reason = KVM_EXIT_DEBUG;
> +			r = RESUME_HOST;
> +		}
>  		break;
>  
>  	case BOOKE_INTERRUPT_FP_UNAVAIL:
> @@ -421,12 +437,27 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
>  		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->debug.arch.pc = vcpu->arch.pc;
> +		run->debug.arch.status = 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.dac1;
> +			else if (dbsr & (DBSR_DAC2R | DBSR_DAC2W))
> +				run->debug.arch.pc = vcpu->arch.shadow_dbg_reg.dac2;
> +		}
>  
> +		/* clear events in DBSR register */
> +		mtspr(SPRN_DBSR, ~0);
> +
> +		run->debug.arch.exception = exit_nr;
>  		run->exit_reason = KVM_EXIT_DEBUG;
>  		kvmppc_account_exit(vcpu, DEBUG_EXITS);
>  		r = RESUME_HOST;
> @@ -560,6 +591,60 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm, struct kvm_dirty_log *log)
>  	return -ENOTSUPP;
>  }
>  
> +int kvmppc_core_set_guest_debug(struct kvm_vcpu *vcpu,
> +                                struct kvm_guest_debug *dbg)
> +{
> +	if (!(dbg->control & KVM_GUESTDBG_ENABLE)) {
> +		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_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
> +		};
> +
> +		for (n = 0; n < 6 && dbg->arch.bp[n].type; n++) {
> +			if (dbg->arch.bp[n].type & KVMPPC_DEBUG_BREAKPOINT)
> +				gdbgr->dbcr0 |= bp_code[b];
> +			if (dbg->arch.bp[n].type & KVMPPC_DEBUG_WATCH_READ)
> +				gdbgr->dbcr0 |= wp_code[w + 2];
> +			if (dbg->arch.bp[n].type & KVMPPC_DEBUG_WATCH_WRITE)
> +				gdbgr->dbcr0 |= wp_code[w];
> +
> +			if (b < 4 && (gdbgr->dbcr0 & (DBCR0_IAC1 | DBCR0_IAC2 |
> +			              DBCR0_IAC3 | DBCR0_IAC4))) {
> +				gdbgr->iac[b] = dbg->arch.bp[n].addr;
> +				b++;
> +			}
> +			if (w < 2 && (gdbgr->dbcr0 & (DBCR0_DAC1W | DBCR0_DAC1R
> +			              | DBCR0_DAC2W | DBCR0_DAC2R))) {
> +				gdbgr->dac[w] = dbg->arch.bp[n].addr;
> +				w++;
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  int __init kvmppc_booke_init(void)
>  {
>  	unsigned long ivor[16];
> diff --git a/arch/powerpc/kvm/e500.c b/arch/powerpc/kvm/e500.c
> index efa1198..65192eb 100644
> --- a/arch/powerpc/kvm/e500.c
> +++ b/arch/powerpc/kvm/e500.c
> @@ -24,14 +24,6 @@
>  #include "booke.h"
>  #include "e500_tlb.h"
>  
> -void kvmppc_core_load_host_debugstate(struct kvm_vcpu *vcpu)
> -{
> -}
> -
> -void kvmppc_core_load_guest_debugstate(struct kvm_vcpu *vcpu)
> -{
> -}
> -
>  void kvmppc_core_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>  {
>  	kvmppc_e500_tlb_load(vcpu, cpu);
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index 51aedd7..5bb17e5 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -265,7 +265,7 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>  int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>                                          struct kvm_guest_debug *dbg)
>  {
> -	return -EINVAL;
> +	return kvmppc_core_set_guest_debug(vcpu, dbg);
>  }
>  
>  static void kvmppc_complete_dcr_load(struct kvm_vcpu *vcpu,

Series looks good otherwise (from generic guest debugging POV).

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux