| Message ID | 1410107494-25556-3-git-send-email-maddy@linux.vnet.ibm.com |
|---|---|
| State | New, archived |
| Headers | show |
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); > -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); >> > -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 */ + 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)) { + /* + * 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); + 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);
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(-)