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

[Madhavan Srinivasan <maddy@linux.vnet.ibm.com>]

On Tuesday 17 June 2014 02:24 PM, Alexander Graf wrote:
> 
> 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.
> 

Yes. I got to know about that from Bharat (patchset "ppc debug: Add
debug stub support"). I will change it.

> 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.
> 

Ok. Will look and replace it.

>> +            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
^^^^^
I did not understand this?

>> +            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.
> 
My bad, I did not try the PR KVM. I will try it out.

> 
> Alex
> 
Thanks for review
Regards
Maddy