* Re: [PATCH v4] powerpc/kvm: support to handle sw breakpoint
@ 2014-08-21 9:10 ` Alexander Graf
0 siblings, 0 replies; 12+ messages in thread
From: Alexander Graf @ 2014-08-21 9:10 UTC (permalink / raw)
To: Madhavan Srinivasan, benh, paulus, mpe; +Cc: linuxppc-dev, kvm, kvm-ppc
On 20.08.14 07:52, 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.
>
> Changes v3->v4:
> Made changes to code comments and removed #define of zero opcode
> Added a new function to handle the debug instruction emulation in book3s_hv
> Rebased the code to latest upstream source.
>
> Changes v2->v3:
> Changed the debug instructions. Using the all zero opcode in the instruction word
> as illegal instruction as mentioned in Power ISA instead of ABS
> Removed reg updated in emulation assist and added a call to
> kvmppc_emulate_instruction for reg update.
>
> Changes v1->v2:
>
> Moved the debug instruction #def to kvm_book3s.h. This way PR_KVM can also share it.
> Added code to use KVM get one reg infrastructure to get debug opcode.
> Updated emulate.c to include emulation of debug instruction incase of PR_KVM.
> Made changes to commit message.
>
> Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
> ---
> arch/powerpc/include/asm/kvm_book3s.h | 7 +++++++
> arch/powerpc/kvm/book3s.c | 3 ++-
> arch/powerpc/kvm/book3s_hv.c | 32 ++++++++++++++++++++++++++++++--
> arch/powerpc/kvm/book3s_pr.c | 3 +++
> arch/powerpc/kvm/emulate.c | 11 +++++++++++
> 5 files changed, 53 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/kvm_book3s.h b/arch/powerpc/include/asm/kvm_book3s.h
> index 6acf0c2..a1944f8 100644
> --- a/arch/powerpc/include/asm/kvm_book3s.h
> +++ b/arch/powerpc/include/asm/kvm_book3s.h
> @@ -24,6 +24,13 @@
> #include <linux/kvm_host.h>
> #include <asm/kvm_book3s_asm.h>
>
> +/*
> + * KVMPPC_INST_BOOK3S_DEBUG is debug Instruction for supporting Software Breakpoint.
> + * Based on PowerISA v2.07, Instruction with primary opcode 0 will be treated as illegal
> + * instruction.
> + */
> +#define KVMPPC_INST_BOOK3S_DEBUG 0x00dddd00
Please change the BookE version of this as well, put the define in a
common header and use a non book specific name.
> +
> struct kvmppc_bat {
> u64 raw;
> u32 bepi;
> 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..0a92e45 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -725,6 +725,14 @@ static int kvmppc_hcall_impl_hv(unsigned long cmd)
> return kvmppc_hcall_impl_hv_realmode(cmd);
> }
>
> +static int kvmppc_emulate_debug_instruction_hv(struct kvm_run *run,
> + struct kvm_vcpu *vcpu)
> +{
> + run->exit_reason = KVM_EXIT_DEBUG;
> + run->debug.arch.address = kvmppc_get_pc(vcpu);
> + return 0;
> +}
> +
> static int kvmppc_handle_exit_hv(struct kvm_run *run, struct kvm_vcpu *vcpu,
> struct task_struct *tsk)
> {
> @@ -811,9 +819,26 @@ static int kvmppc_handle_exit_hv(struct kvm_run *run, struct kvm_vcpu *vcpu,
> * we don't emulate any guest instructions at this stage.
This comment is no longer true, it should get changed.
> */
> case BOOK3S_INTERRUPT_H_EMUL_ASSIST:
> - kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
> - r = RESUME_GUEST;
> + {
> + u32 last_inst;
> + if(kvmppc_get_last_inst(vcpu, INST_GENERIC, &last_inst) !=
> + EMULATE_DONE) {
Please only do this if debugging is active.
> + /*
> + * Fetch failed, so return to guest and
> + * try executing it again.
> + */
> + r = RESUME_GUEST;
> + } else {
> + if (last_inst == KVMPPC_INST_BOOK3S_DEBUG) {
> + kvmppc_emulate_debug_instruction_hv(run, vcpu);
> + r = RESUME_HOST;
> + } else {
> + kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
> + r = RESUME_GUEST;
> + }
> + }
> break;
> + }
> /*
> * This occurs if the guest (kernel or userspace), does something that
> * is prohibited by HFSCR. We just generate a program interrupt to
> @@ -922,6 +947,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_BOOK3S_DEBUG);
> + 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..cd2a39d 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_BOOK3S_DEBUG);
> + 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..68f38b2 100644
> --- a/arch/powerpc/kvm/emulate.c
> +++ b/arch/powerpc/kvm/emulate.c
> @@ -274,6 +274,17 @@ 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.
> + */
> + run->exit_reason = KVM_EXIT_DEBUG;
This is wrong. We need to check that the instruction is 0x00dddd00. Any
other op0 instruction should trigger an invalid instruction trap.
Alex
> + run->debug.arch.address = kvmppc_get_pc(vcpu);
> + emulated = EMULATE_EXIT_USER;
> + advance = 0;
> + break;
> +
> default:
> emulated = EMULATE_FAIL;
> }
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v4] powerpc/kvm: support to handle sw breakpoint
@ 2014-08-21 9:10 ` Alexander Graf
0 siblings, 0 replies; 12+ messages in thread
From: Alexander Graf @ 2014-08-21 9:10 UTC (permalink / raw)
To: Madhavan Srinivasan, benh, paulus, mpe; +Cc: linuxppc-dev, kvm-ppc, kvm
On 20.08.14 07:52, 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.
>
> Changes v3->v4:
> Made changes to code comments and removed #define of zero opcode
> Added a new function to handle the debug instruction emulation in book3s_hv
> Rebased the code to latest upstream source.
>
> Changes v2->v3:
> Changed the debug instructions. Using the all zero opcode in the instruction word
> as illegal instruction as mentioned in Power ISA instead of ABS
> Removed reg updated in emulation assist and added a call to
> kvmppc_emulate_instruction for reg update.
>
> Changes v1->v2:
>
> Moved the debug instruction #def to kvm_book3s.h. This way PR_KVM can also share it.
> Added code to use KVM get one reg infrastructure to get debug opcode.
> Updated emulate.c to include emulation of debug instruction incase of PR_KVM.
> Made changes to commit message.
>
> Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
> ---
> arch/powerpc/include/asm/kvm_book3s.h | 7 +++++++
> arch/powerpc/kvm/book3s.c | 3 ++-
> arch/powerpc/kvm/book3s_hv.c | 32 ++++++++++++++++++++++++++++++--
> arch/powerpc/kvm/book3s_pr.c | 3 +++
> arch/powerpc/kvm/emulate.c | 11 +++++++++++
> 5 files changed, 53 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/kvm_book3s.h b/arch/powerpc/include/asm/kvm_book3s.h
> index 6acf0c2..a1944f8 100644
> --- a/arch/powerpc/include/asm/kvm_book3s.h
> +++ b/arch/powerpc/include/asm/kvm_book3s.h
> @@ -24,6 +24,13 @@
> #include <linux/kvm_host.h>
> #include <asm/kvm_book3s_asm.h>
>
> +/*
> + * KVMPPC_INST_BOOK3S_DEBUG is debug Instruction for supporting Software Breakpoint.
> + * Based on PowerISA v2.07, Instruction with primary opcode 0 will be treated as illegal
> + * instruction.
> + */
> +#define KVMPPC_INST_BOOK3S_DEBUG 0x00dddd00
Please change the BookE version of this as well, put the define in a
common header and use a non book specific name.
> +
> struct kvmppc_bat {
> u64 raw;
> u32 bepi;
> 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..0a92e45 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -725,6 +725,14 @@ static int kvmppc_hcall_impl_hv(unsigned long cmd)
> return kvmppc_hcall_impl_hv_realmode(cmd);
> }
>
> +static int kvmppc_emulate_debug_instruction_hv(struct kvm_run *run,
> + struct kvm_vcpu *vcpu)
> +{
> + run->exit_reason = KVM_EXIT_DEBUG;
> + run->debug.arch.address = kvmppc_get_pc(vcpu);
> + return 0;
> +}
> +
> static int kvmppc_handle_exit_hv(struct kvm_run *run, struct kvm_vcpu *vcpu,
> struct task_struct *tsk)
> {
> @@ -811,9 +819,26 @@ static int kvmppc_handle_exit_hv(struct kvm_run *run, struct kvm_vcpu *vcpu,
> * we don't emulate any guest instructions at this stage.
This comment is no longer true, it should get changed.
> */
> case BOOK3S_INTERRUPT_H_EMUL_ASSIST:
> - kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
> - r = RESUME_GUEST;
> + {
> + u32 last_inst;
> + if(kvmppc_get_last_inst(vcpu, INST_GENERIC, &last_inst) !=
> + EMULATE_DONE) {
Please only do this if debugging is active.
> + /*
> + * Fetch failed, so return to guest and
> + * try executing it again.
> + */
> + r = RESUME_GUEST;
> + } else {
> + if (last_inst == KVMPPC_INST_BOOK3S_DEBUG) {
> + kvmppc_emulate_debug_instruction_hv(run, vcpu);
> + r = RESUME_HOST;
> + } else {
> + kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
> + r = RESUME_GUEST;
> + }
> + }
> break;
> + }
> /*
> * This occurs if the guest (kernel or userspace), does something that
> * is prohibited by HFSCR. We just generate a program interrupt to
> @@ -922,6 +947,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_BOOK3S_DEBUG);
> + 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..cd2a39d 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_BOOK3S_DEBUG);
> + 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..68f38b2 100644
> --- a/arch/powerpc/kvm/emulate.c
> +++ b/arch/powerpc/kvm/emulate.c
> @@ -274,6 +274,17 @@ 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.
> + */
> + run->exit_reason = KVM_EXIT_DEBUG;
This is wrong. We need to check that the instruction is 0x00dddd00. Any
other op0 instruction should trigger an invalid instruction trap.
Alex
> + run->debug.arch.address = kvmppc_get_pc(vcpu);
> + emulated = EMULATE_EXIT_USER;
> + advance = 0;
> + break;
> +
> default:
> emulated = EMULATE_FAIL;
> }
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v4] powerpc/kvm: support to handle sw breakpoint
2014-08-21 9:10 ` Alexander Graf
(?)
@ 2014-08-23 10:17 ` Madhavan Srinivasan
-1 siblings, 0 replies; 12+ messages in thread
From: Madhavan Srinivasan @ 2014-08-23 10:17 UTC (permalink / raw)
To: Alexander Graf, benh, paulus, mpe; +Cc: linuxppc-dev, kvm-ppc, kvm
On Thursday 21 August 2014 02:40 PM, Alexander Graf wrote:
>
>
> On 20.08.14 07:52, 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.
>>
>> Changes v3->v4:
>> Made changes to code comments and removed #define of zero opcode
>> Added a new function to handle the debug instruction emulation in book3s_hv
>> Rebased the code to latest upstream source.
>>
>> Changes v2->v3:
>> Changed the debug instructions. Using the all zero opcode in the instruction word
>> as illegal instruction as mentioned in Power ISA instead of ABS
>> Removed reg updated in emulation assist and added a call to
>> kvmppc_emulate_instruction for reg update.
>>
>> Changes v1->v2:
>>
>> Moved the debug instruction #def to kvm_book3s.h. This way PR_KVM can also share it.
>> Added code to use KVM get one reg infrastructure to get debug opcode.
>> Updated emulate.c to include emulation of debug instruction incase of PR_KVM.
>> Made changes to commit message.
>>
>> Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
>> ---
>> arch/powerpc/include/asm/kvm_book3s.h | 7 +++++++
>> arch/powerpc/kvm/book3s.c | 3 ++-
>> arch/powerpc/kvm/book3s_hv.c | 32 ++++++++++++++++++++++++++++++--
>> arch/powerpc/kvm/book3s_pr.c | 3 +++
>> arch/powerpc/kvm/emulate.c | 11 +++++++++++
>> 5 files changed, 53 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/kvm_book3s.h b/arch/powerpc/include/asm/kvm_book3s.h
>> index 6acf0c2..a1944f8 100644
>> --- a/arch/powerpc/include/asm/kvm_book3s.h
>> +++ b/arch/powerpc/include/asm/kvm_book3s.h
>> @@ -24,6 +24,13 @@
>> #include <linux/kvm_host.h>
>> #include <asm/kvm_book3s_asm.h>
>>
>> +/*
>> + * KVMPPC_INST_BOOK3S_DEBUG is debug Instruction for supporting Software Breakpoint.
>> + * Based on PowerISA v2.07, Instruction with primary opcode 0 will be treated as illegal
>> + * instruction.
>> + */
>> +#define KVMPPC_INST_BOOK3S_DEBUG 0x00dddd00
>
> Please change the BookE version of this as well, put the define in a
> common header and use a non book specific name.
>
I first wanted to get the server side in and then take up this, but i
can do it with this, just concerned incase of booke testing :(
>> +
>> struct kvmppc_bat {
>> u64 raw;
>> u32 bepi;
>> 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..0a92e45 100644
>> --- a/arch/powerpc/kvm/book3s_hv.c
>> +++ b/arch/powerpc/kvm/book3s_hv.c
>> @@ -725,6 +725,14 @@ static int kvmppc_hcall_impl_hv(unsigned long cmd)
>> return kvmppc_hcall_impl_hv_realmode(cmd);
>> }
>>
>> +static int kvmppc_emulate_debug_instruction_hv(struct kvm_run *run,
>> + struct kvm_vcpu *vcpu)
>> +{
>> + run->exit_reason = KVM_EXIT_DEBUG;
>> + run->debug.arch.address = kvmppc_get_pc(vcpu);
>> + return 0;
>> +}
>> +
>> static int kvmppc_handle_exit_hv(struct kvm_run *run, struct kvm_vcpu *vcpu,
>> struct task_struct *tsk)
>> {
>> @@ -811,9 +819,26 @@ static int kvmppc_handle_exit_hv(struct kvm_run *run, struct kvm_vcpu *vcpu,
>> * we don't emulate any guest instructions at this stage.
>
> This comment is no longer true, it should get changed.
>
Will change it.
>> */
>> case BOOK3S_INTERRUPT_H_EMUL_ASSIST:
>> - kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
>> - r = RESUME_GUEST;
>> + {
>> + u32 last_inst;
>> + if(kvmppc_get_last_inst(vcpu, INST_GENERIC, &last_inst) !=
>> + EMULATE_DONE) {
>
> Please only do this if debugging is active.
>
Ok sure. But i will add in the else part of the check. Since we do the
instruction check there.
>> + /*
>> + * Fetch failed, so return to guest and
>> + * try executing it again.
>> + */
>> + r = RESUME_GUEST;
>> + } else {
>> + if (last_inst == KVMPPC_INST_BOOK3S_DEBUG) {
>> + kvmppc_emulate_debug_instruction_hv(run, vcpu);
>> + r = RESUME_HOST;
>> + } else {
>> + kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
>> + r = RESUME_GUEST;
>> + }
>> + }
>> break;
>> + }
>> /*
>> * This occurs if the guest (kernel or userspace), does something that
>> * is prohibited by HFSCR. We just generate a program interrupt to
>> @@ -922,6 +947,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_BOOK3S_DEBUG);
>> + 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..cd2a39d 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_BOOK3S_DEBUG);
>> + 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..68f38b2 100644
>> --- a/arch/powerpc/kvm/emulate.c
>> +++ b/arch/powerpc/kvm/emulate.c
>> @@ -274,6 +274,17 @@ 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.
>> + */
>> + run->exit_reason = KVM_EXIT_DEBUG;
>
> This is wrong. We need to check that the instruction is 0x00dddd00. Any
> other op0 instruction should trigger an invalid instruction trap.
>
Ok.
Thanks for review
Regards
Maddy
>
> Alex
>
>> + run->debug.arch.address = kvmppc_get_pc(vcpu);
>> + emulated = EMULATE_EXIT_USER;
>> + advance = 0;
>> + break;
>> +
>> default:
>> emulated = EMULATE_FAIL;
>> }
>>
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v4] powerpc/kvm: support to handle sw breakpoint
@ 2014-08-23 10:17 ` Madhavan Srinivasan
0 siblings, 0 replies; 12+ messages in thread
From: Madhavan Srinivasan @ 2014-08-23 10:29 UTC (permalink / raw)
To: Alexander Graf, benh, paulus, mpe; +Cc: linuxppc-dev, kvm, kvm-ppc
On Thursday 21 August 2014 02:40 PM, Alexander Graf wrote:
>
>
> On 20.08.14 07:52, 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.
>>
>> Changes v3->v4:
>> Made changes to code comments and removed #define of zero opcode
>> Added a new function to handle the debug instruction emulation in book3s_hv
>> Rebased the code to latest upstream source.
>>
>> Changes v2->v3:
>> Changed the debug instructions. Using the all zero opcode in the instruction word
>> as illegal instruction as mentioned in Power ISA instead of ABS
>> Removed reg updated in emulation assist and added a call to
>> kvmppc_emulate_instruction for reg update.
>>
>> Changes v1->v2:
>>
>> Moved the debug instruction #def to kvm_book3s.h. This way PR_KVM can also share it.
>> Added code to use KVM get one reg infrastructure to get debug opcode.
>> Updated emulate.c to include emulation of debug instruction incase of PR_KVM.
>> Made changes to commit message.
>>
>> Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
>> ---
>> arch/powerpc/include/asm/kvm_book3s.h | 7 +++++++
>> arch/powerpc/kvm/book3s.c | 3 ++-
>> arch/powerpc/kvm/book3s_hv.c | 32 ++++++++++++++++++++++++++++++--
>> arch/powerpc/kvm/book3s_pr.c | 3 +++
>> arch/powerpc/kvm/emulate.c | 11 +++++++++++
>> 5 files changed, 53 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/kvm_book3s.h b/arch/powerpc/include/asm/kvm_book3s.h
>> index 6acf0c2..a1944f8 100644
>> --- a/arch/powerpc/include/asm/kvm_book3s.h
>> +++ b/arch/powerpc/include/asm/kvm_book3s.h
>> @@ -24,6 +24,13 @@
>> #include <linux/kvm_host.h>
>> #include <asm/kvm_book3s_asm.h>
>>
>> +/*
>> + * KVMPPC_INST_BOOK3S_DEBUG is debug Instruction for supporting Software Breakpoint.
>> + * Based on PowerISA v2.07, Instruction with primary opcode 0 will be treated as illegal
>> + * instruction.
>> + */
>> +#define KVMPPC_INST_BOOK3S_DEBUG 0x00dddd00
>
> Please change the BookE version of this as well, put the define in a
> common header and use a non book specific name.
>
I first wanted to get the server side in and then take up this, but i
can do it with this, just concerned incase of booke testing :(
>> +
>> struct kvmppc_bat {
>> u64 raw;
>> u32 bepi;
>> 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..0a92e45 100644
>> --- a/arch/powerpc/kvm/book3s_hv.c
>> +++ b/arch/powerpc/kvm/book3s_hv.c
>> @@ -725,6 +725,14 @@ static int kvmppc_hcall_impl_hv(unsigned long cmd)
>> return kvmppc_hcall_impl_hv_realmode(cmd);
>> }
>>
>> +static int kvmppc_emulate_debug_instruction_hv(struct kvm_run *run,
>> + struct kvm_vcpu *vcpu)
>> +{
>> + run->exit_reason = KVM_EXIT_DEBUG;
>> + run->debug.arch.address = kvmppc_get_pc(vcpu);
>> + return 0;
>> +}
>> +
>> static int kvmppc_handle_exit_hv(struct kvm_run *run, struct kvm_vcpu *vcpu,
>> struct task_struct *tsk)
>> {
>> @@ -811,9 +819,26 @@ static int kvmppc_handle_exit_hv(struct kvm_run *run, struct kvm_vcpu *vcpu,
>> * we don't emulate any guest instructions at this stage.
>
> This comment is no longer true, it should get changed.
>
Will change it.
>> */
>> case BOOK3S_INTERRUPT_H_EMUL_ASSIST:
>> - kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
>> - r = RESUME_GUEST;
>> + {
>> + u32 last_inst;
>> + if(kvmppc_get_last_inst(vcpu, INST_GENERIC, &last_inst) !>> + EMULATE_DONE) {
>
> Please only do this if debugging is active.
>
Ok sure. But i will add in the else part of the check. Since we do the
instruction check there.
>> + /*
>> + * Fetch failed, so return to guest and
>> + * try executing it again.
>> + */
>> + r = RESUME_GUEST;
>> + } else {
>> + if (last_inst = KVMPPC_INST_BOOK3S_DEBUG) {
>> + kvmppc_emulate_debug_instruction_hv(run, vcpu);
>> + r = RESUME_HOST;
>> + } else {
>> + kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
>> + r = RESUME_GUEST;
>> + }
>> + }
>> break;
>> + }
>> /*
>> * This occurs if the guest (kernel or userspace), does something that
>> * is prohibited by HFSCR. We just generate a program interrupt to
>> @@ -922,6 +947,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_BOOK3S_DEBUG);
>> + 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..cd2a39d 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_BOOK3S_DEBUG);
>> + 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..68f38b2 100644
>> --- a/arch/powerpc/kvm/emulate.c
>> +++ b/arch/powerpc/kvm/emulate.c
>> @@ -274,6 +274,17 @@ 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.
>> + */
>> + run->exit_reason = KVM_EXIT_DEBUG;
>
> This is wrong. We need to check that the instruction is 0x00dddd00. Any
> other op0 instruction should trigger an invalid instruction trap.
>
Ok.
Thanks for review
Regards
Maddy
>
> Alex
>
>> + run->debug.arch.address = kvmppc_get_pc(vcpu);
>> + emulated = EMULATE_EXIT_USER;
>> + advance = 0;
>> + break;
>> +
>> default:
>> emulated = EMULATE_FAIL;
>> }
>>
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v4] powerpc/kvm: support to handle sw breakpoint
@ 2014-08-23 10:17 ` Madhavan Srinivasan
0 siblings, 0 replies; 12+ messages in thread
From: Madhavan Srinivasan @ 2014-08-23 10:17 UTC (permalink / raw)
To: Alexander Graf, benh, paulus, mpe; +Cc: linuxppc-dev, kvm, kvm-ppc
On Thursday 21 August 2014 02:40 PM, Alexander Graf wrote:
>
>
> On 20.08.14 07:52, 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.
>>
>> Changes v3->v4:
>> Made changes to code comments and removed #define of zero opcode
>> Added a new function to handle the debug instruction emulation in book3s_hv
>> Rebased the code to latest upstream source.
>>
>> Changes v2->v3:
>> Changed the debug instructions. Using the all zero opcode in the instruction word
>> as illegal instruction as mentioned in Power ISA instead of ABS
>> Removed reg updated in emulation assist and added a call to
>> kvmppc_emulate_instruction for reg update.
>>
>> Changes v1->v2:
>>
>> Moved the debug instruction #def to kvm_book3s.h. This way PR_KVM can also share it.
>> Added code to use KVM get one reg infrastructure to get debug opcode.
>> Updated emulate.c to include emulation of debug instruction incase of PR_KVM.
>> Made changes to commit message.
>>
>> Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
>> ---
>> arch/powerpc/include/asm/kvm_book3s.h | 7 +++++++
>> arch/powerpc/kvm/book3s.c | 3 ++-
>> arch/powerpc/kvm/book3s_hv.c | 32 ++++++++++++++++++++++++++++++--
>> arch/powerpc/kvm/book3s_pr.c | 3 +++
>> arch/powerpc/kvm/emulate.c | 11 +++++++++++
>> 5 files changed, 53 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/kvm_book3s.h b/arch/powerpc/include/asm/kvm_book3s.h
>> index 6acf0c2..a1944f8 100644
>> --- a/arch/powerpc/include/asm/kvm_book3s.h
>> +++ b/arch/powerpc/include/asm/kvm_book3s.h
>> @@ -24,6 +24,13 @@
>> #include <linux/kvm_host.h>
>> #include <asm/kvm_book3s_asm.h>
>>
>> +/*
>> + * KVMPPC_INST_BOOK3S_DEBUG is debug Instruction for supporting Software Breakpoint.
>> + * Based on PowerISA v2.07, Instruction with primary opcode 0 will be treated as illegal
>> + * instruction.
>> + */
>> +#define KVMPPC_INST_BOOK3S_DEBUG 0x00dddd00
>
> Please change the BookE version of this as well, put the define in a
> common header and use a non book specific name.
>
I first wanted to get the server side in and then take up this, but i
can do it with this, just concerned incase of booke testing :(
>> +
>> struct kvmppc_bat {
>> u64 raw;
>> u32 bepi;
>> 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..0a92e45 100644
>> --- a/arch/powerpc/kvm/book3s_hv.c
>> +++ b/arch/powerpc/kvm/book3s_hv.c
>> @@ -725,6 +725,14 @@ static int kvmppc_hcall_impl_hv(unsigned long cmd)
>> return kvmppc_hcall_impl_hv_realmode(cmd);
>> }
>>
>> +static int kvmppc_emulate_debug_instruction_hv(struct kvm_run *run,
>> + struct kvm_vcpu *vcpu)
>> +{
>> + run->exit_reason = KVM_EXIT_DEBUG;
>> + run->debug.arch.address = kvmppc_get_pc(vcpu);
>> + return 0;
>> +}
>> +
>> static int kvmppc_handle_exit_hv(struct kvm_run *run, struct kvm_vcpu *vcpu,
>> struct task_struct *tsk)
>> {
>> @@ -811,9 +819,26 @@ static int kvmppc_handle_exit_hv(struct kvm_run *run, struct kvm_vcpu *vcpu,
>> * we don't emulate any guest instructions at this stage.
>
> This comment is no longer true, it should get changed.
>
Will change it.
>> */
>> case BOOK3S_INTERRUPT_H_EMUL_ASSIST:
>> - kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
>> - r = RESUME_GUEST;
>> + {
>> + u32 last_inst;
>> + if(kvmppc_get_last_inst(vcpu, INST_GENERIC, &last_inst) !=
>> + EMULATE_DONE) {
>
> Please only do this if debugging is active.
>
Ok sure. But i will add in the else part of the check. Since we do the
instruction check there.
>> + /*
>> + * Fetch failed, so return to guest and
>> + * try executing it again.
>> + */
>> + r = RESUME_GUEST;
>> + } else {
>> + if (last_inst == KVMPPC_INST_BOOK3S_DEBUG) {
>> + kvmppc_emulate_debug_instruction_hv(run, vcpu);
>> + r = RESUME_HOST;
>> + } else {
>> + kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
>> + r = RESUME_GUEST;
>> + }
>> + }
>> break;
>> + }
>> /*
>> * This occurs if the guest (kernel or userspace), does something that
>> * is prohibited by HFSCR. We just generate a program interrupt to
>> @@ -922,6 +947,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_BOOK3S_DEBUG);
>> + 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..cd2a39d 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_BOOK3S_DEBUG);
>> + 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..68f38b2 100644
>> --- a/arch/powerpc/kvm/emulate.c
>> +++ b/arch/powerpc/kvm/emulate.c
>> @@ -274,6 +274,17 @@ 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.
>> + */
>> + run->exit_reason = KVM_EXIT_DEBUG;
>
> This is wrong. We need to check that the instruction is 0x00dddd00. Any
> other op0 instruction should trigger an invalid instruction trap.
>
Ok.
Thanks for review
Regards
Maddy
>
> Alex
>
>> + run->debug.arch.address = kvmppc_get_pc(vcpu);
>> + emulated = EMULATE_EXIT_USER;
>> + advance = 0;
>> + break;
>> +
>> default:
>> emulated = EMULATE_FAIL;
>> }
>>
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v4] powerpc/kvm: support to handle sw breakpoint
2014-08-23 10:17 ` Madhavan Srinivasan
(?)
@ 2014-08-23 12:24 ` Alexander Graf
-1 siblings, 0 replies; 12+ messages in thread
From: Alexander Graf @ 2014-08-23 12:24 UTC (permalink / raw)
To: Madhavan Srinivasan
Cc: benh@kernel.crashing.org, paulus@samba.org, mpe@ellerman.id.au,
linuxppc-dev@lists.ozlabs.org, kvm@vger.kernel.org,
kvm-ppc@vger.kernel.org
> Am 23.08.2014 um 12:17 schrieb Madhavan Srinivasan <maddy@linux.vnet.ibm.com>:
>
>> On Thursday 21 August 2014 02:40 PM, Alexander Graf wrote:
>>
>>
>>> On 20.08.14 07:52, 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.
>>>
>>> Changes v3->v4:
>>> Made changes to code comments and removed #define of zero opcode
>>> Added a new function to handle the debug instruction emulation in book3s_hv
>>> Rebased the code to latest upstream source.
>>>
>>> Changes v2->v3:
>>> Changed the debug instructions. Using the all zero opcode in the instruction word
>>> as illegal instruction as mentioned in Power ISA instead of ABS
>>> Removed reg updated in emulation assist and added a call to
>>> kvmppc_emulate_instruction for reg update.
>>>
>>> Changes v1->v2:
>>>
>>> Moved the debug instruction #def to kvm_book3s.h. This way PR_KVM can also share it.
>>> Added code to use KVM get one reg infrastructure to get debug opcode.
>>> Updated emulate.c to include emulation of debug instruction incase of PR_KVM.
>>> Made changes to commit message.
>>>
>>> Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
>>> ---
>>> arch/powerpc/include/asm/kvm_book3s.h | 7 +++++++
>>> arch/powerpc/kvm/book3s.c | 3 ++-
>>> arch/powerpc/kvm/book3s_hv.c | 32 ++++++++++++++++++++++++++++++--
>>> arch/powerpc/kvm/book3s_pr.c | 3 +++
>>> arch/powerpc/kvm/emulate.c | 11 +++++++++++
>>> 5 files changed, 53 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/powerpc/include/asm/kvm_book3s.h b/arch/powerpc/include/asm/kvm_book3s.h
>>> index 6acf0c2..a1944f8 100644
>>> --- a/arch/powerpc/include/asm/kvm_book3s.h
>>> +++ b/arch/powerpc/include/asm/kvm_book3s.h
>>> @@ -24,6 +24,13 @@
>>> #include <linux/kvm_host.h>
>>> #include <asm/kvm_book3s_asm.h>
>>>
>>> +/*
>>> + * KVMPPC_INST_BOOK3S_DEBUG is debug Instruction for supporting Software Breakpoint.
>>> + * Based on PowerISA v2.07, Instruction with primary opcode 0 will be treated as illegal
>>> + * instruction.
>>> + */
>>> +#define KVMPPC_INST_BOOK3S_DEBUG 0x00dddd00
>>
>> Please change the BookE version of this as well, put the define in a
>> common header and use a non book specific name.
>
> I first wanted to get the server side in and then take up this, but i
> can do it with this, just concerned incase of booke testing :(
I can test, no problem. You can at least do compile tests on your side already ;)
>
>>> +
>>> struct kvmppc_bat {
>>> u64 raw;
>>> u32 bepi;
>>> 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..0a92e45 100644
>>> --- a/arch/powerpc/kvm/book3s_hv.c
>>> +++ b/arch/powerpc/kvm/book3s_hv.c
>>> @@ -725,6 +725,14 @@ static int kvmppc_hcall_impl_hv(unsigned long cmd)
>>> return kvmppc_hcall_impl_hv_realmode(cmd);
>>> }
>>>
>>> +static int kvmppc_emulate_debug_instruction_hv(struct kvm_run *run,
>>> + struct kvm_vcpu *vcpu)
>>> +{
>>> + run->exit_reason = KVM_EXIT_DEBUG;
>>> + run->debug.arch.address = kvmppc_get_pc(vcpu);
>>> + return 0;
>>> +}
>>> +
>>> static int kvmppc_handle_exit_hv(struct kvm_run *run, struct kvm_vcpu *vcpu,
>>> struct task_struct *tsk)
>>> {
>>> @@ -811,9 +819,26 @@ static int kvmppc_handle_exit_hv(struct kvm_run *run, struct kvm_vcpu *vcpu,
>>> * we don't emulate any guest instructions at this stage.
>>
>> This comment is no longer true, it should get changed.
>
> Will change it.
>
>>> */
>>> case BOOK3S_INTERRUPT_H_EMUL_ASSIST:
>>> - kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
>>> - r = RESUME_GUEST;
>>> + {
>>> + u32 last_inst;
>>> + if(kvmppc_get_last_inst(vcpu, INST_GENERIC, &last_inst) !>>> + EMULATE_DONE) {
>>
>> Please only do this if debugging is active.
>
> Ok sure. But i will add in the else part of the check. Since we do the
> instruction check there.
We should only fetch the instruction if sw breakpoint debugging is active.
Alex
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v4] powerpc/kvm: support to handle sw breakpoint
@ 2014-08-23 12:24 ` Alexander Graf
0 siblings, 0 replies; 12+ messages in thread
From: Alexander Graf @ 2014-08-23 12:24 UTC (permalink / raw)
To: Madhavan Srinivasan
Cc: benh@kernel.crashing.org, paulus@samba.org, mpe@ellerman.id.au,
linuxppc-dev@lists.ozlabs.org, kvm@vger.kernel.org,
kvm-ppc@vger.kernel.org
> Am 23.08.2014 um 12:17 schrieb Madhavan Srinivasan <maddy@linux.vnet.ibm.com>:
>
>> On Thursday 21 August 2014 02:40 PM, Alexander Graf wrote:
>>
>>
>>> On 20.08.14 07:52, 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.
>>>
>>> Changes v3->v4:
>>> Made changes to code comments and removed #define of zero opcode
>>> Added a new function to handle the debug instruction emulation in book3s_hv
>>> Rebased the code to latest upstream source.
>>>
>>> Changes v2->v3:
>>> Changed the debug instructions. Using the all zero opcode in the instruction word
>>> as illegal instruction as mentioned in Power ISA instead of ABS
>>> Removed reg updated in emulation assist and added a call to
>>> kvmppc_emulate_instruction for reg update.
>>>
>>> Changes v1->v2:
>>>
>>> Moved the debug instruction #def to kvm_book3s.h. This way PR_KVM can also share it.
>>> Added code to use KVM get one reg infrastructure to get debug opcode.
>>> Updated emulate.c to include emulation of debug instruction incase of PR_KVM.
>>> Made changes to commit message.
>>>
>>> Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
>>> ---
>>> arch/powerpc/include/asm/kvm_book3s.h | 7 +++++++
>>> arch/powerpc/kvm/book3s.c | 3 ++-
>>> arch/powerpc/kvm/book3s_hv.c | 32 ++++++++++++++++++++++++++++++--
>>> arch/powerpc/kvm/book3s_pr.c | 3 +++
>>> arch/powerpc/kvm/emulate.c | 11 +++++++++++
>>> 5 files changed, 53 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/powerpc/include/asm/kvm_book3s.h b/arch/powerpc/include/asm/kvm_book3s.h
>>> index 6acf0c2..a1944f8 100644
>>> --- a/arch/powerpc/include/asm/kvm_book3s.h
>>> +++ b/arch/powerpc/include/asm/kvm_book3s.h
>>> @@ -24,6 +24,13 @@
>>> #include <linux/kvm_host.h>
>>> #include <asm/kvm_book3s_asm.h>
>>>
>>> +/*
>>> + * KVMPPC_INST_BOOK3S_DEBUG is debug Instruction for supporting Software Breakpoint.
>>> + * Based on PowerISA v2.07, Instruction with primary opcode 0 will be treated as illegal
>>> + * instruction.
>>> + */
>>> +#define KVMPPC_INST_BOOK3S_DEBUG 0x00dddd00
>>
>> Please change the BookE version of this as well, put the define in a
>> common header and use a non book specific name.
>
> I first wanted to get the server side in and then take up this, but i
> can do it with this, just concerned incase of booke testing :(
I can test, no problem. You can at least do compile tests on your side already ;)
>
>>> +
>>> struct kvmppc_bat {
>>> u64 raw;
>>> u32 bepi;
>>> 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..0a92e45 100644
>>> --- a/arch/powerpc/kvm/book3s_hv.c
>>> +++ b/arch/powerpc/kvm/book3s_hv.c
>>> @@ -725,6 +725,14 @@ static int kvmppc_hcall_impl_hv(unsigned long cmd)
>>> return kvmppc_hcall_impl_hv_realmode(cmd);
>>> }
>>>
>>> +static int kvmppc_emulate_debug_instruction_hv(struct kvm_run *run,
>>> + struct kvm_vcpu *vcpu)
>>> +{
>>> + run->exit_reason = KVM_EXIT_DEBUG;
>>> + run->debug.arch.address = kvmppc_get_pc(vcpu);
>>> + return 0;
>>> +}
>>> +
>>> static int kvmppc_handle_exit_hv(struct kvm_run *run, struct kvm_vcpu *vcpu,
>>> struct task_struct *tsk)
>>> {
>>> @@ -811,9 +819,26 @@ static int kvmppc_handle_exit_hv(struct kvm_run *run, struct kvm_vcpu *vcpu,
>>> * we don't emulate any guest instructions at this stage.
>>
>> This comment is no longer true, it should get changed.
>
> Will change it.
>
>>> */
>>> case BOOK3S_INTERRUPT_H_EMUL_ASSIST:
>>> - kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
>>> - r = RESUME_GUEST;
>>> + {
>>> + u32 last_inst;
>>> + if(kvmppc_get_last_inst(vcpu, INST_GENERIC, &last_inst) !=
>>> + EMULATE_DONE) {
>>
>> Please only do this if debugging is active.
>
> Ok sure. But i will add in the else part of the check. Since we do the
> instruction check there.
We should only fetch the instruction if sw breakpoint debugging is active.
Alex
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v4] powerpc/kvm: support to handle sw breakpoint
@ 2014-08-23 12:24 ` Alexander Graf
0 siblings, 0 replies; 12+ messages in thread
From: Alexander Graf @ 2014-08-23 12:24 UTC (permalink / raw)
To: Madhavan Srinivasan
Cc: kvm@vger.kernel.org, kvm-ppc@vger.kernel.org, paulus@samba.org,
linuxppc-dev@lists.ozlabs.org
> Am 23.08.2014 um 12:17 schrieb Madhavan Srinivasan <maddy@linux.vnet.ibm.c=
om>:
>=20
>> On Thursday 21 August 2014 02:40 PM, Alexander Graf wrote:
>>=20
>>=20
>>> On 20.08.14 07:52, 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 instr=
uction
>>> and accordingly we return to Host or Guest. Patch also adds support for
>>> software breakpoint in PR KVM.
>>>=20
>>> Changes v3->v4:
>>> Made changes to code comments and removed #define of zero opcode
>>> Added a new function to handle the debug instruction emulation in book3s=
_hv
>>> Rebased the code to latest upstream source.
>>>=20
>>> Changes v2->v3:
>>> Changed the debug instructions. Using the all zero opcode in the instruc=
tion word
>>> as illegal instruction as mentioned in Power ISA instead of ABS
>>> Removed reg updated in emulation assist and added a call to
>>> kvmppc_emulate_instruction for reg update.
>>>=20
>>> Changes v1->v2:
>>>=20
>>> Moved the debug instruction #def to kvm_book3s.h. This way PR_KVM can al=
so share it.
>>> Added code to use KVM get one reg infrastructure to get debug opcode.
>>> Updated emulate.c to include emulation of debug instruction incase of PR=
_KVM.
>>> Made changes to commit message.
>>>=20
>>> Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
>>> ---
>>> arch/powerpc/include/asm/kvm_book3s.h | 7 +++++++
>>> arch/powerpc/kvm/book3s.c | 3 ++-
>>> arch/powerpc/kvm/book3s_hv.c | 32 +++++++++++++++++++++++++++++=
+--
>>> arch/powerpc/kvm/book3s_pr.c | 3 +++
>>> arch/powerpc/kvm/emulate.c | 11 +++++++++++
>>> 5 files changed, 53 insertions(+), 3 deletions(-)
>>>=20
>>> diff --git a/arch/powerpc/include/asm/kvm_book3s.h b/arch/powerpc/includ=
e/asm/kvm_book3s.h
>>> index 6acf0c2..a1944f8 100644
>>> --- a/arch/powerpc/include/asm/kvm_book3s.h
>>> +++ b/arch/powerpc/include/asm/kvm_book3s.h
>>> @@ -24,6 +24,13 @@
>>> #include <linux/kvm_host.h>
>>> #include <asm/kvm_book3s_asm.h>
>>>=20
>>> +/*
>>> + * KVMPPC_INST_BOOK3S_DEBUG is debug Instruction for supporting Softwar=
e Breakpoint.
>>> + * Based on PowerISA v2.07, Instruction with primary opcode 0 will be t=
reated as illegal
>>> + * instruction.
>>> + */
>>> +#define KVMPPC_INST_BOOK3S_DEBUG 0x00dddd00
>>=20
>> Please change the BookE version of this as well, put the define in a
>> common header and use a non book specific name.
>=20
> I first wanted to get the server side in and then take up this, but i
> can do it with this, just concerned incase of booke testing :(
I can test, no problem. You can at least do compile tests on your side alrea=
dy ;)
>=20
>>> +
>>> struct kvmppc_bat {
>>> u64 raw;
>>> u32 bepi;
>>> 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 *v=
cpu,
>>> int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>>> struct kvm_guest_debug *dbg)
>>> {
>>> - return -EINVAL;
>>> + vcpu->guest_debug =3D dbg->control;
>>> + return 0;
>>> }
>>>=20
>>> 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..0a92e45 100644
>>> --- a/arch/powerpc/kvm/book3s_hv.c
>>> +++ b/arch/powerpc/kvm/book3s_hv.c
>>> @@ -725,6 +725,14 @@ static int kvmppc_hcall_impl_hv(unsigned long cmd)
>>> return kvmppc_hcall_impl_hv_realmode(cmd);
>>> }
>>>=20
>>> +static int kvmppc_emulate_debug_instruction_hv(struct kvm_run *run,
>>> + struct kvm_vcpu *vcpu)
>>> +{
>>> + run->exit_reason =3D KVM_EXIT_DEBUG;
>>> + run->debug.arch.address =3D kvmppc_get_pc(vcpu);
>>> + return 0;
>>> +}
>>> +
>>> static int kvmppc_handle_exit_hv(struct kvm_run *run, struct kvm_vcpu *v=
cpu,
>>> struct task_struct *tsk)
>>> {
>>> @@ -811,9 +819,26 @@ static int kvmppc_handle_exit_hv(struct kvm_run *ru=
n, struct kvm_vcpu *vcpu,
>>> * we don't emulate any guest instructions at this stage.
>>=20
>> This comment is no longer true, it should get changed.
>=20
> Will change it.
>=20
>>> */
>>> case BOOK3S_INTERRUPT_H_EMUL_ASSIST:
>>> - kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
>>> - r =3D RESUME_GUEST;
>>> + {
>>> + u32 last_inst;
>>> + if(kvmppc_get_last_inst(vcpu, INST_GENERIC, &last_inst) !=3D
>>> + EMULATE_DONE) {
>>=20
>> Please only do this if debugging is active.
>=20
> Ok sure. But i will add in the else part of the check. Since we do the
> instruction check there.
We should only fetch the instruction if sw breakpoint debugging is active.
Alex
^ permalink raw reply [flat|nested] 12+ messages in thread