Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception

[Scott Wood <scottwood@freescale.com>]

On Fri, 2014-07-11 at 14:09 +0530, Bharat Bhushan wrote:
> This patch emulates debug registers and debug exception
> to support guest using debug resource. This enables running
> gdb/kgdb etc in guest.
> 
> On BOOKE architecture we cannot share debug resources between QEMU and
> guest because:
>     When QEMU is using debug resources then debug exception must
>     be always enabled. To achieve this we set MSR_DE and also set
>     MSRP_DEP so guest cannot change MSR_DE.
>
>     When emulating debug resource for guest we want guest
>     to control MSR_DE (enable/disable debug interrupt on need).
> 
>     So above mentioned two configuration cannot be supported
>     at the same time. So the result is that we cannot share
>     debug resources between QEMU and Guest on BOOKE architecture.
> 
> In the current design QEMU gets priority over guest, this means that if
> QEMU is using debug resources then guest cannot use them and if guest is
> using debug resource then QEMU can overwrite them.
> 
> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> ---
> Hi Alex,
> 
> I thought of having some print in register emulation if QEMU
> is using debug resource, Also when QEMU overwrites guest written
> values but that looks excessive. If I uses some variable which
> get set when guest starts using debug registers and check in
> debug set ioctl then that look ugly. Looking for suggestions
> 
>  arch/powerpc/include/asm/kvm_ppc.h |   3 +
>  arch/powerpc/kvm/booke.c           |  27 +++++++
>  arch/powerpc/kvm/booke_emulate.c   | 157 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 187 insertions(+)
> 
> diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
> index e2fd5a1..f3f7611 100644
> --- a/arch/powerpc/include/asm/kvm_ppc.h
> +++ b/arch/powerpc/include/asm/kvm_ppc.h
> @@ -173,6 +173,9 @@ extern int kvmppc_xics_get_xive(struct kvm *kvm, u32 irq, u32 *server,
>  extern int kvmppc_xics_int_on(struct kvm *kvm, u32 irq);
>  extern int kvmppc_xics_int_off(struct kvm *kvm, u32 irq);
>  
> +void kvmppc_core_dequeue_debug(struct kvm_vcpu *vcpu);
> +void kvmppc_core_queue_debug(struct kvm_vcpu *vcpu);
> +
>  union kvmppc_one_reg {
>  	u32	wval;
>  	u64	dval;
> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> index fadfe76..c2471ed 100644
> --- a/arch/powerpc/kvm/booke.c
> +++ b/arch/powerpc/kvm/booke.c
> @@ -264,6 +264,16 @@ static void kvmppc_core_dequeue_watchdog(struct kvm_vcpu *vcpu)
>  	clear_bit(BOOKE_IRQPRIO_WATCHDOG, &vcpu->arch.pending_exceptions);
>  }
>  
> +void kvmppc_core_queue_debug(struct kvm_vcpu *vcpu)
> +{
> +	kvmppc_booke_queue_irqprio(vcpu, BOOKE_IRQPRIO_DEBUG);
> +}
> +
> +void kvmppc_core_dequeue_debug(struct kvm_vcpu *vcpu)
> +{
> +	clear_bit(BOOKE_IRQPRIO_DEBUG, &vcpu->arch.pending_exceptions);
> +}
> +
>  static void set_guest_srr(struct kvm_vcpu *vcpu, unsigned long srr0, u32 srr1)
>  {
>  #ifdef CONFIG_KVM_BOOKE_HV
> @@ -783,6 +793,23 @@ static int kvmppc_handle_debug(struct kvm_run *run, struct kvm_vcpu *vcpu)
>  	struct debug_reg *dbg_reg = &(vcpu->arch.shadow_dbg_reg);
>  	u32 dbsr = vcpu->arch.dbsr;
>  
> +	if (vcpu->guest_debug == 0) {
> +		/* Debug resources belong to Guest */
> +		if (dbsr && (vcpu->arch.shared->msr & MSR_DE))
> +			kvmppc_core_queue_debug(vcpu);

Should also check for DBCR0[IDM].

> +
> +		/* Inject a program interrupt if trap debug is not allowed */
> +		if ((dbsr & DBSR_TIE) && !(vcpu->arch.shared->msr & MSR_DE))
> +			kvmppc_core_queue_program(vcpu, ESR_PTR);
> +
> +		return RESUME_GUEST;
> +	}
> +
> +	/*
> +	 * Prepare for userspace exit as debug resources
> +	 * are owned by userspace.
> +	 */
> +	vcpu->arch.dbsr = 0;
>  	run->debug.arch.status = 0;
>  	run->debug.arch.address = vcpu->arch.pc;

Why are you clearing vcpu->arch.dbsr?  Userspace might be interested in
the raw value, plus it's a change from the current API semantics.

Where is this run->debug.arch.<foo> stuff and KVM_EXIT_DEBUG documented?


> diff --git a/arch/powerpc/kvm/booke_emulate.c b/arch/powerpc/kvm/booke_emulate.c
> index 2a20194..3d143fe 100644
> --- a/arch/powerpc/kvm/booke_emulate.c
> +++ b/arch/powerpc/kvm/booke_emulate.c
> @@ -139,6 +139,7 @@ int kvmppc_booke_emulate_op(struct kvm_run *run, struct kvm_vcpu *vcpu,
>  int kvmppc_booke_emulate_mtspr(struct kvm_vcpu *vcpu, int sprn, ulong spr_val)
>  {
>  	int emulated = EMULATE_DONE;
> +	bool debug_inst = false;
>  
>  	switch (sprn) {
>  	case SPRN_DEAR:
> @@ -153,14 +154,137 @@ int kvmppc_booke_emulate_mtspr(struct kvm_vcpu *vcpu, int sprn, ulong spr_val)
>  	case SPRN_CSRR1:
>  		vcpu->arch.csrr1 = spr_val;
>  		break;
> +	case SPRN_DSRR0:
> +		vcpu->arch.dsrr0 = spr_val;
> +		break;
> +	case SPRN_DSRR1:
> +		vcpu->arch.dsrr1 = spr_val;
> +		break;
> +	case SPRN_IAC1:
> +		/*
> +		 * If userspace is debugging guest then guest
> +		 * can not access debug registers.
> +		 */
> +		if (vcpu->guest_debug)
> +			break;
> +
> +		debug_inst = true;
> +		vcpu->arch.dbg_reg.iac1 = spr_val;
> +		vcpu->arch.shadow_dbg_reg.iac1 = spr_val;
> +		break;
> +	case SPRN_IAC2:
> +		/*
> +		 * If userspace is debugging guest then guest
> +		 * can not access debug registers.
> +		 */
> +		if (vcpu->guest_debug)
> +			break;
> +
> +		debug_inst = true;
> +		vcpu->arch.dbg_reg.iac2 = spr_val;
> +		vcpu->arch.shadow_dbg_reg.iac2 = spr_val;
> +		break;
> +#if CONFIG_PPC_ADV_DEBUG_IACS > 2
> +	case SPRN_IAC3:
> +		/*
> +		 * If userspace is debugging guest then guest
> +		 * can not access debug registers.
> +		 */
> +		if (vcpu->guest_debug)
> +			break;
> +
> +		debug_inst = true;
> +		vcpu->arch.dbg_reg.iac3 = spr_val;
> +		vcpu->arch.shadow_dbg_reg.iac3 = spr_val;
> +		break;
> +	case SPRN_IAC4:
> +		/*
> +		 * If userspace is debugging guest then guest
> +		 * can not access debug registers.
> +		 */
> +		if (vcpu->guest_debug)
> +			break;
> +
> +		debug_inst = true;
> +		vcpu->arch.dbg_reg.iac4 = spr_val;
> +		vcpu->arch.shadow_dbg_reg.iac4 = spr_val;
> +		break;
> +#endif
> +	case SPRN_DAC1:
> +		/*
> +		 * If userspace is debugging guest then guest
> +		 * can not access debug registers.
> +		 */
> +		if (vcpu->guest_debug)
> +			break;
> +
> +		debug_inst = true;
> +		vcpu->arch.dbg_reg.dac1 = spr_val;
> +		vcpu->arch.shadow_dbg_reg.dac1 = spr_val;
> +		break;
> +	case SPRN_DAC2:
> +		/*
> +		 * If userspace is debugging guest then guest
> +		 * can not access debug registers.
> +		 */
> +		if (vcpu->guest_debug)
> +			break;
> +
> +		debug_inst = true;
> +		vcpu->arch.dbg_reg.dac2 = spr_val;
> +		vcpu->arch.shadow_dbg_reg.dac2 = spr_val;
> +		break;
>  	case SPRN_DBCR0:
> +		/*
> +		 * If userspace is debugging guest then guest
> +		 * can not access debug registers.
> +		 */
> +		if (vcpu->guest_debug)
> +			break;
> +
> +		debug_inst = true;
> +		spr_val &= (DBCR0_IDM | DBCR0_IC | DBCR0_BT | DBCR0_TIE |
> +			DBCR0_IAC1 | DBCR0_IAC2 | DBCR0_IAC3 | DBCR0_IAC4  |
> +			DBCR0_DAC1R | DBCR0_DAC1W | DBCR0_DAC2R | DBCR0_DAC2W);
> +
>  		vcpu->arch.dbg_reg.dbcr0 = spr_val;
> +		vcpu->arch.shadow_dbg_reg.dbcr0 = spr_val;
>  		break;
>  	case SPRN_DBCR1:
> +		/*
> +		 * If userspace is debugging guest then guest
> +		 * can not access debug registers.
> +		 */
> +		if (vcpu->guest_debug)
> +			break;
> +
> +		debug_inst = true;
>  		vcpu->arch.dbg_reg.dbcr1 = spr_val;
> +		vcpu->arch.shadow_dbg_reg.dbcr1 = spr_val;
> +		break;
> +	case SPRN_DBCR2:
> +		/*
> +		 * If userspace is debugging guest then guest
> +		 * can not access debug registers.
> +		 */
> +		if (vcpu->guest_debug)
> +			break;
> +
> +		debug_inst = true;
> +		vcpu->arch.dbg_reg.dbcr2 = spr_val;
> +		vcpu->arch.shadow_dbg_reg.dbcr2 = spr_val;
>  		break;

In what circumstances can the architected and shadow registers differ?

>  	case SPRN_DBSR:
> +		/*
> +		 * If userspace is debugging guest then guest
> +		 * can not access debug registers.
> +		 */
> +		if (vcpu->guest_debug)
> +			break;
> +
>  		vcpu->arch.dbsr &= ~spr_val;
> +		if (vcpu->arch.dbsr == 0)
> +			kvmppc_core_dequeue_debug(vcpu);
>  		break;

Not all DBSR bits cause an exception, e.g. IDE and MRR.

>  	case SPRN_TSR:
>  		kvmppc_clr_tsr_bits(vcpu, spr_val);
> @@ -273,6 +397,10 @@ int kvmppc_booke_emulate_mtspr(struct kvm_vcpu *vcpu, int sprn, ulong spr_val)
>  		emulated = EMULATE_FAIL;
>  	}
>  
> +	if (debug_inst) {
> +		switch_booke_debug_regs(&vcpu->arch.shadow_dbg_reg);
> +		current->thread.debug = vcpu->arch.shadow_dbg_reg;
> +	}

Could you explain what's going on with regard to copying the registers
into current->thread.debug?  Why is it done after loading the registers
into the hardware (is there a race if we get preempted in the middle)?

-Scott
;