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

[Alexander Graf <agraf@suse.de>]

On 14.06.14 23:08, 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 mandates use of "abs" instruction
> (primary opcode 31 and extended opcode 360) as sw breakpoint instruction.
> Based on PowerISA v2.01, ABS instruction has been dropped from the architecture
> and treated an illegal instruction.
>
> Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
> ---
>   arch/powerpc/kvm/book3s.c    |  3 ++-
>   arch/powerpc/kvm/book3s_hv.c | 23 +++++++++++++++++++----
>   2 files changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
> index c254c27..b40fe5d 100644
> --- a/arch/powerpc/kvm/book3s.c
> +++ b/arch/powerpc/kvm/book3s.c
> @@ -789,7 +789,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 7a12edb..688421d 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -67,6 +67,14 @@
>   /* Used as a "null" value for timebase values */
>   #define TB_NIL	(~(u64)0)
>   
> +/*
> + * SW_BRK_DBG_INT is debug Instruction for supporting Software Breakpoint.
> + * Instruction mnemonic is ABS, primary opcode is 31 and extended opcode is 360.
> + * Based on PowerISA v2.01, ABS instruction has been dropped from the architecture
> + * and treated an illegal instruction.
> + */
> +#define SW_BRK_DBG_INT 0x7c0002d0

The instruction we use to trap needs to get exposed to user space via a 
ONE_REG property.

Also, why don't we use twi always or something else that actually is 
defined as illegal instruction? I would like to see this shared with 
book3s_32 PR.

> +
>   static void kvmppc_end_cede(struct kvm_vcpu *vcpu);
>   static int kvmppc_hv_setup_htab_rma(struct kvm_vcpu *vcpu);
>   
> @@ -721,12 +729,19 @@ 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.
> +	 * To support software breakpoint, we check for the sw breakpoint
> +	 * instruction to return back to host, if not we just generate a
> +	 * program interrupt to the guest.
>   	 */
>   	case BOOK3S_INTERRUPT_H_EMUL_ASSIST:
> -		kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
> -		r = RESUME_GUEST;
> +		if (vcpu->arch.last_inst == SW_BRK_DBG_INT) {

Don't access last_inst directly. Instead use the provided helpers.

> +			run->exit_reason = KVM_EXIT_DEBUG;
> +			run->debug.arch.address = vcpu->arch.pc;
> +			r = RESUME_HOST;
> +		} else {
> +			kvmppc_core_queue_program(vcpu, 0x80000);

magic numbers

> +			r = RESUME_GUEST;
> +		}
>   		break;
>   	/*
>   	 * This occurs if the guest (kernel or userspace), does something that

Please enable PR KVM as well while you're at it.


Alex