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

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

On Tuesday 17 June 2014 04:38 PM, Alexander Graf wrote:
> 
> On 17.06.14 13:07, Madhavan Srinivasan wrote:
>> 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?
> 
> You're replacing the readable SRR1_PROGILL with the unreadable 0x80000.
> That's bad.
> 

Oops. My bad. Will undo that. I guess I messed up when was re basing it.

> 
> Alex
> 
Thanks for review
Regards
Maddy