Re: [PATCH 2/2 v5] powerpc/kvm: common sw breakpoint instr across ppc

[Alexander Graf <agraf@suse.de>]

On 07.09.14 18:31, Madhavan Srinivasan wrote:
> This patch extends the use of illegal instruction as software
> breakpoint instruction across the ppc platform. Patch extends
> booke program interrupt code to support software breakpoint.
> 
> Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
> ---
> 
> Patch is only compile tested. Will really help if
> someone can try it out and let me know comments.
> 
>  arch/powerpc/kvm/booke.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> index b4c89fa..1b84853 100644
> --- a/arch/powerpc/kvm/booke.c
> +++ b/arch/powerpc/kvm/booke.c
> @@ -870,6 +870,10 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
>  	case BOOKE_INTERRUPT_HV_PRIV:
>  		emulated = kvmppc_get_last_inst(vcpu, false, &last_inst);
>  		break;
> +	case BOOKE_INTERRUPT_PROGRAM:
> +		/*SW breakpoints arrive as illegal instructions on HV */

Is it my email client or is there a space missing again? ;)

Also, please only fetch the last instruction if debugging is active.

> +		emulated = kvmppc_get_last_inst(vcpu, false, &last_inst);
> +		break;
>  	default:
>  		break;
>  	}
> @@ -947,7 +951,17 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
>  		break;
>  
>  	case BOOKE_INTERRUPT_PROGRAM:
> -		if (vcpu->arch.shared->msr & (MSR_PR | MSR_GS)) {
> +		if ((vcpu->arch.shared->msr & (MSR_PR | MSR_GS)) &&
> +			(last_inst == KVMPPC_INST_SW_BREAKPOINT)) {

I think this is changing the logic from "if the guest is in user mode or
we're in HV, deflect" to "if the guest is in user mode or an HV guest
and the instruction is a breakpoint, treat it as debug. Otherwise
deflect". So you're essentially breaking PR KVM here from what I can tell.

Why don't you just split the whole thing out to the beginning of
BOOKE_INTERRUPT_PROGRAM and check for

  a) debug is enabled
  b) instruction is sw breakpoint

instead?

> +			/*
> +			 * We are here because of an SW breakpoint instr,
> +			 * so lets return to host to handle.
> +			 */
> +			r = kvmppc_handle_debug(run, vcpu);
> +			run->exit_reason = KVM_EXIT_DEBUG;
> +			kvmppc_account_exit(vcpu, DEBUG_EXITS);
> +			break;
> +		} else {	
>  			/*
>  			 * Program traps generated by user-level software must
>  			 * be handled by the guest kernel.
> @@ -1505,7 +1519,7 @@ int kvm_vcpu_ioctl_get_one_reg(struct kvm_vcpu *vcpu, struct kvm_one_reg *reg)
>  		val = get_reg_val(reg->id, vcpu->arch.tsr);
>  		break;
>  	case KVM_REG_PPC_DEBUG_INST:
> -		val = get_reg_val(reg->id, KVMPPC_INST_EHPRIV_DEBUG);

Please also remove the definition of EHPRIV_DEBUG.


Alex

> +		val = get_reg_val(reg->id, KVMPPC_INST_SW_BREAKPOINT);
>  		break;
>  	case KVM_REG_PPC_VRSAVE:
>  		val = get_reg_val(reg->id, vcpu->arch.vrsave);
>