Re: [PATCH 1/2 v5] powerpc/kvm: support to handle sw breakpoint

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

On Monday 08 September 2014 06:35 PM, Alexander Graf wrote:
> 
> 
> On 07.09.14 18:31, Madhavan Srinivasan wrote:
>> This patch adds kernel side support for software breakpoint.
>> Design is that, by using an illegal instruction, we trap to hypervisor
>> via Emulation Assistance interrupt, where we check for the illegal instruction
>> and accordingly we return to Host or Guest. Patch also adds support for
>> software breakpoint in PR KVM.
>>
>> Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
>> ---
>>  arch/powerpc/include/asm/kvm_ppc.h |  6 ++++++
>>  arch/powerpc/kvm/book3s.c          |  3 ++-
>>  arch/powerpc/kvm/book3s_hv.c       | 41 ++++++++++++++++++++++++++++++++++----
>>  arch/powerpc/kvm/book3s_pr.c       |  3 +++
>>  arch/powerpc/kvm/emulate.c         | 18 +++++++++++++++++
>>  5 files changed, 66 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
>> index fb86a22..dd83c9a 100644
>> --- a/arch/powerpc/include/asm/kvm_ppc.h
>> +++ b/arch/powerpc/include/asm/kvm_ppc.h
>> @@ -38,6 +38,12 @@
>>  #include <asm/paca.h>
>>  #endif
>>  
>> +/*
>> + * KVMPPC_INST_SW_BREAKPOINT is debug Instruction
>> + * for supporting software breakpoint.
>> + */
>> +#define KVMPPC_INST_SW_BREAKPOINT	0x00dddd00
>> +
>>  enum emulation_result {
>>  	EMULATE_DONE,         /* no further processing */
>>  	EMULATE_DO_MMIO,      /* kvm_run filled with MMIO request */
>> diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
>> index dd03f6b..00e9c9f 100644
>> --- a/arch/powerpc/kvm/book3s.c
>> +++ b/arch/powerpc/kvm/book3s.c
>> @@ -778,7 +778,8 @@ int kvm_arch_vcpu_ioctl_translate(struct kvm_vcpu *vcpu,
>>  int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>>  					struct kvm_guest_debug *dbg)
>>  {
>> -	return -EINVAL;
>> +	vcpu->guest_debug = dbg->control;
>> +	return 0;
>>  }
>>  
>>  void kvmppc_decrementer_func(unsigned long data)
>> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
>> index 27cced9..3a2414c 100644
>> --- a/arch/powerpc/kvm/book3s_hv.c
>> +++ b/arch/powerpc/kvm/book3s_hv.c
>> @@ -725,6 +725,30 @@ static int kvmppc_hcall_impl_hv(unsigned long cmd)
>>  	return kvmppc_hcall_impl_hv_realmode(cmd);
>>  }
>>  
>> +static int kvmppc_emulate_debug_inst(struct kvm_run *run,
>> +					struct kvm_vcpu *vcpu)
>> +{
>> +	u32 last_inst;
>> +
>> +	if(kvmppc_get_last_inst(vcpu, INST_GENERIC, &last_inst) !=
>> +					EMULATE_DONE) {
>> +		/*
>> +		 * Fetch failed, so return to guest and
>> +		 * try executing it again.
>> +		 */
>> +		return RESUME_GUEST;
> 
> Please end the if() here. In the kernel we usually treat abort
> situations as separate.
> 

OK. Will change it.

>> +	} else {
>> +		if (last_inst == KVMPPC_INST_SW_BREAKPOINT) {
>> +			run->exit_reason = KVM_EXIT_DEBUG;
>> +			run->debug.arch.address = kvmppc_get_pc(vcpu);
>> +			return RESUME_HOST;
>> +		} else {
>> +			kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
>> +			return RESUME_GUEST;
>> +		}
>> +	}
>> +}
>> +
>>  static int kvmppc_handle_exit_hv(struct kvm_run *run, struct kvm_vcpu *vcpu,
>>  				 struct task_struct *tsk)
>>  {
>> @@ -807,12 +831,18 @@ static int kvmppc_handle_exit_hv(struct kvm_run *run, struct kvm_vcpu *vcpu,
>>  		break;
>>  	/*
>>  	 * This occurs if the guest executes an illegal instruction.
>> -	 * We just generate a program interrupt to the guest, since
>> -	 * we don't emulate any guest instructions at this stage.
>> +	 * If the guest debug is disabled, generate a program interrupt
>> +	 * to the guest. If guest debug is enabled, we need to check
>> +	 * whether the instruction is a software breakpoint instruction.
>> +	 * Accordingly return to Guest or Host.
>>  	 */
>>  	case BOOK3S_INTERRUPT_H_EMUL_ASSIST:
>> -		kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
>> -		r = RESUME_GUEST;
>> +		if(vcpu->guest_debug & KVM_GUESTDBG_USE_SW_BP) {
>> +			r = kvmppc_emulate_debug_inst(run, vcpu);
>> +		} else {
>> +			kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
>> +			r = RESUME_GUEST;
>> +		}
>>  		break;
>>  	/*
>>  	 * This occurs if the guest (kernel or userspace), does something that
>> @@ -922,6 +952,9 @@ static int kvmppc_get_one_reg_hv(struct kvm_vcpu *vcpu, u64 id,
>>  	long int i;
>>  
>>  	switch (id) {
>> +	case KVM_REG_PPC_DEBUG_INST:
>> +		*val = get_reg_val(id, KVMPPC_INST_SW_BREAKPOINT);
>> +		break;
>>  	case KVM_REG_PPC_HIOR:
>>  		*val = get_reg_val(id, 0);
>>  		break;
>> diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
>> index faffb27..6d73708 100644
>> --- a/arch/powerpc/kvm/book3s_pr.c
>> +++ b/arch/powerpc/kvm/book3s_pr.c
>> @@ -1319,6 +1319,9 @@ static int kvmppc_get_one_reg_pr(struct kvm_vcpu *vcpu, u64 id,
>>  	int r = 0;
>>  
>>  	switch (id) {
>> +	case KVM_REG_PPC_DEBUG_INST:
>> +		*val = get_reg_val(id, KVMPPC_INST_SW_BREAKPOINT);
>> +		break;
>>  	case KVM_REG_PPC_HIOR:
>>  		*val = get_reg_val(id, to_book3s(vcpu)->hior);
>>  		break;
>> diff --git a/arch/powerpc/kvm/emulate.c b/arch/powerpc/kvm/emulate.c
>> index e96b50d..30f326b 100644
>> --- a/arch/powerpc/kvm/emulate.c
>> +++ b/arch/powerpc/kvm/emulate.c
>> @@ -274,6 +274,24 @@ int kvmppc_emulate_instruction(struct kvm_run *run, struct kvm_vcpu *vcpu)
>>  		}
>>  		break;
>>  
>> +	case 0:
>> +		/*
>> +		 * Instruction with primary opcode 0. Based on PowerISA
>> +		 * these are illegal instructions.
>> +		 */
>> +		if(inst == KVMPPC_INST_SW_BREAKPOINT) {
> 
> Here you're missing a space between if and (. Are you sure that you ran
> checkpatch.pl?
> 

No. my bad. Did not run checkpatch.pl. Will fix it.

> 
>> +			run->exit_reason = KVM_EXIT_DEBUG;
>> +			run->debug.arch.address = kvmppc_get_pc(vcpu);
>> +			emulated = EMULATE_EXIT_USER;
>> +			advance = 0;
>> +			printk(KERN_INFO "pr_emulate: debug instr \n");
>> +		} else {
>> +			kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
>> +			emulated = EMULATE_DONE;
>> +			advance = 0;
> 
> Just set emulated = EMULATE_FAIL here, the same as default:.
> 
> 

Ok sure will do.

Thanks for review.
Maddy

> Alex
> 
>> +		}
>> +		break;	
>> +
>>  	default:
>>  		emulated = EMULATE_FAIL;
>>  	}
>>
>