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

[Madhavan Srinivasan <maddy@linux.vnet.ibm.com>]

On Monday 08 September 2014 06:39 PM, Alexander Graf wrote:
> 
> 
> 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? ;)
> 

Facepalm. Will fix it.

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

Will change it.

>> +		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
> 
This is what we pretty much do for the server side. Will changes it.

> 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.
> 
OK. Will do.


Thanks for review
Maddy

> 
> 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);
>>
>