diff mbox

[v3] powerpc/kvm: support to handle sw breakpoint

Message ID 53EA0572.1070806@suse.de (mailing list archive)
State New, archived
Headers show

Commit Message

Alexander Graf Aug. 12, 2014, 12:15 p.m. UTC
On 12.08.14 13:35, Madhavan Srinivasan wrote:
> On Tuesday 12 August 2014 04:49 PM, Alexander Graf wrote:
>> On 12.08.14 07:17, Madhavan Srinivasan wrote:
>>> On Monday 11 August 2014 02:45 PM, Alexander Graf wrote:
>>>> On 11.08.14 10:51, Benjamin Herrenschmidt wrote:
>>>>> On Mon, 2014-08-11 at 09:26 +0200, Alexander Graf wrote:
>>>>>>> diff --git a/arch/powerpc/kvm/emulate.c b/arch/powerpc/kvm/emulate.c
>>>>>>> index da86d9b..d95014e 100644
>>>>>>> --- a/arch/powerpc/kvm/emulate.c
>>>>>>> +++ b/arch/powerpc/kvm/emulate.c
>>>>>> This should be book3s_emulate.c.
>>>>> Any reason we can't make that 00dddd00 opcode as breakpoint common to
>>>>> all powerpc variants ?
>>>> I can't think of a good reason. We use a hypercall on booke (which traps
>>>> into an illegal instruction for pr) today, but I don't think it has to
>>>> be that way.
>>>>
>>>> Given that the user space API allows us to change it dynamically, there
>>>> should be nothing blocking us from going with 00dddd00 always.
>>>>
>>> Kindly correct me if i am wrong. So we can still have a common code in
>>> emulate.c to set the env for both HV and pr incase of illegal
>>> instruction (i will rebase latest src). But suggestion here to use
>>> 00dddd00, in that case current path in embed is kvmppc_handle_exit
>>> (booke.c) -> BOOKE_INTERRUPT_HV_PRIV -> emulation_exit ->
>>> kvmppc_emulate_instruction, will change to kvmppc_handle_exit (booke.c)
>>> -> BOOKE_INTERRUPT_PROGRAM -> if debug instr call emulation_exit else
>>> send to guest?
>> I can't follow your description above.
>>
> My bad.
>
>> With the latest git version HV KVM does not include emulate.c anymore.
>>
>> Also, it would make a lot of sense of have the same soft breakpoint
>> instruction across all ppc targets, so it would make sense to change it
>> to 0x00dddd00 for booke as well.
>>
> Got it. Was describing the current control flow with respect to booke
> and where changes needed (for same software breakpoint inst). This is
> for my understanding and wanted verify.
>
> kvmppc_handle_exit(booke.c)
> 	-> BOOKE_INTERRUPT_HV_PRIV
> 		-> emulation_exit
> 			->kvmppc_emulate_instruction
>
> Incase of using the same software breakpoint instruction (0x00dddd00),
> then we need to add code in booke something like this
>
> kvmppc_handle_exit (booke.c)
> 	-> BOOKE_INTERRUPT_PROGRAM
> 		->	if debug instr
> 				->emulation_exit
> 			else
> 				->send to guest?

Bleks. I see your point. I guess you need something like this for booke:

      }
@@ -953,7 +958,8 @@ 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_SOFT_BREAKPOINT)) {
              /*
               * Program traps generated by user-level software must
               * be handled by the guest kernel.



> 				
>> Basically you would have handling code in emulate.c and book3s_hv.c at
>> the end of the day.
>>
> Yes. Will resend the patch with updated code.

Thanks,


Alex

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Madhavan Srinivasan Aug. 12, 2014, 12:21 p.m. UTC | #1
On Tuesday 12 August 2014 05:45 PM, Alexander Graf wrote:
> 
> On 12.08.14 13:35, Madhavan Srinivasan wrote:
>> On Tuesday 12 August 2014 04:49 PM, Alexander Graf wrote:
>>> On 12.08.14 07:17, Madhavan Srinivasan wrote:
>>>> On Monday 11 August 2014 02:45 PM, Alexander Graf wrote:
>>>>> On 11.08.14 10:51, Benjamin Herrenschmidt wrote:
>>>>>> On Mon, 2014-08-11 at 09:26 +0200, Alexander Graf wrote:
>>>>>>>> diff --git a/arch/powerpc/kvm/emulate.c
>>>>>>>> b/arch/powerpc/kvm/emulate.c
>>>>>>>> index da86d9b..d95014e 100644
>>>>>>>> --- a/arch/powerpc/kvm/emulate.c
>>>>>>>> +++ b/arch/powerpc/kvm/emulate.c
>>>>>>> This should be book3s_emulate.c.
>>>>>> Any reason we can't make that 00dddd00 opcode as breakpoint common to
>>>>>> all powerpc variants ?
>>>>> I can't think of a good reason. We use a hypercall on booke (which
>>>>> traps
>>>>> into an illegal instruction for pr) today, but I don't think it has to
>>>>> be that way.
>>>>>
>>>>> Given that the user space API allows us to change it dynamically,
>>>>> there
>>>>> should be nothing blocking us from going with 00dddd00 always.
>>>>>
>>>> Kindly correct me if i am wrong. So we can still have a common code in
>>>> emulate.c to set the env for both HV and pr incase of illegal
>>>> instruction (i will rebase latest src). But suggestion here to use
>>>> 00dddd00, in that case current path in embed is kvmppc_handle_exit
>>>> (booke.c) -> BOOKE_INTERRUPT_HV_PRIV -> emulation_exit ->
>>>> kvmppc_emulate_instruction, will change to kvmppc_handle_exit (booke.c)
>>>> -> BOOKE_INTERRUPT_PROGRAM -> if debug instr call emulation_exit else
>>>> send to guest?
>>> I can't follow your description above.
>>>
>> My bad.
>>
>>> With the latest git version HV KVM does not include emulate.c anymore.
>>>
>>> Also, it would make a lot of sense of have the same soft breakpoint
>>> instruction across all ppc targets, so it would make sense to change it
>>> to 0x00dddd00 for booke as well.
>>>
>> Got it. Was describing the current control flow with respect to booke
>> and where changes needed (for same software breakpoint inst). This is
>> for my understanding and wanted verify.
>>
>> kvmppc_handle_exit(booke.c)
>>     -> BOOKE_INTERRUPT_HV_PRIV
>>         -> emulation_exit
>>             ->kvmppc_emulate_instruction
>>
>> Incase of using the same software breakpoint instruction (0x00dddd00),
>> then we need to add code in booke something like this
>>
>> kvmppc_handle_exit (booke.c)
>>     -> BOOKE_INTERRUPT_PROGRAM
>>         ->    if debug instr
>>                 ->emulation_exit
>>             else
>>                 ->send to guest?
> 
> Bleks. I see your point. I guess you need something like this for booke:
> 
> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> index 074b7fc..1fdeee0 100644
> --- a/arch/powerpc/kvm/booke.c
> +++ b/arch/powerpc/kvm/booke.c
> @@ -876,6 +876,11 @@ 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 */
> +        if (vcpu->guest_debug & KVM_GUESTDBG_USE_SW_BP)
> +            emulated = kvmppc_get_last_inst(vcpu, false, &last_inst);
> +        break;
>      default:
>          break;
>      }
> @@ -953,7 +958,8 @@ 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_SOFT_BREAKPOINT)) {
>              /*
>               * Program traps generated by user-level software must
>               * be handled by the guest kernel.
> 
> 
> 

Ok make sense.

Regards
Maddy

>>                
>>> Basically you would have handling code in emulate.c and book3s_hv.c at
>>> the end of the day.
>>>
>> Yes. Will resend the patch with updated code.
> 
> Thanks,
> 
> 
> Alex
> 

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index 074b7fc..1fdeee0 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -876,6 +876,11 @@  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 */
+        if (vcpu->guest_debug & KVM_GUESTDBG_USE_SW_BP)
+            emulated = kvmppc_get_last_inst(vcpu, false, &last_inst);
+        break;
      default:
          break;