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 <ma...@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" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
