Re: [PATCH 1/4] kvmppc: guest debug definitions

[Alexander Graf <agraf@suse.de>]

Liu Yu-B13201 wrote:
>  
>
>   
>> -----Original Message-----
>> From: kvm-ppc-owner@vger.kernel.org 
>> [mailto:kvm-ppc-owner@vger.kernel.org] On Behalf Of Alexander Graf
>> Sent: Wednesday, February 03, 2010 4:57 PM
>> To: Liu Yu-B13201
>> Cc: hollis@penguinppc.org; kvm-ppc@vger.kernel.org; 
>> kvm@vger.kernel.org; Liu Yu-B13201
>> Subject: Re: [PATCH 1/4] kvmppc: guest debug definitions
>>
>>
>> Am 03.02.2010 um 08:53 schrieb Liu Yu <yu.liu@freescale.com>:
>>
>>     
>>> Signed-off-by: Liu Yu <yu.liu@freescale.com>
>>> ---
>>> arch/powerpc/include/asm/kvm.h      |   20 ++++++++++++++++++++
>>> arch/powerpc/include/asm/kvm_host.h |   16 ++++++++++++++++
>>> 2 files changed, 36 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/arch/powerpc/include/asm/kvm.h b/arch/powerpc/include/ 
>>> asm/kvm.h
>>> index 81f3b0b..b7f7861 100644
>>> --- a/arch/powerpc/include/asm/kvm.h
>>> +++ b/arch/powerpc/include/asm/kvm.h
>>> @@ -22,6 +22,9 @@
>>>
>>> #include <linux/types.h>
>>>
>>> +/* Select powerpc specific features in <linux/kvm.h> */
>>> +#define __KVM_HAVE_GUEST_DEBUG
>>> +
>>> struct kvm_regs {
>>>    __u64 pc;
>>>    __u64 cr;
>>> @@ -71,10 +74,27 @@ struct kvm_fpu {
>>> };
>>>
>>> struct kvm_debug_exit_arch {
>>> +    __u32 exception;
>>> +    __u32 pc;
>>> +    __u32 status;
>>> };
>>>
>>> +#define KVM_INST_GUESTGDB               0x44000022
>>>       
>> What instruction is this again? :) Is it something reserved for  
>> purposes like this?
>>
>>     
>
> Just an invalid instruction which can generate program interrupt...
> I'm open to it's value btw.
>   

Well this definitely doesn't generate a program interrupt. Or at least
it shouldn't :-).
I just remembered where I've seen an opcode like this before. This is a
part of a dump of arch/powerpc/boot/ps3-hvcall.o

00000000 <lv1_get_logical_ppe_id>:
   0:   7c 08 02 a6     mflr    r0
   4:   90 01 00 04     stw     r0,4(r1)
   8:   94 21 ff f0     stwu    r1,-16(r1)
   c:   90 61 00 08     stw     r3,8(r1)
  10:   39 60 00 45     li      r11,69
  14:   44 00 00 22     sc      1

So as you can see, this is the hypercall instruction for lv1. IIRC beat
uses the same. I don't think we want to reuse that opcode for ourselves.
Maybe one day someone figures it's a good idea to implement a beat-style
ABI in KVM.

But IIRC sc can take a lot of values, so we can just take sc 0x1234 or
so :-).

>>> +
>>> +#define KVM_GUESTDBG_USE_SW_BP          0x00010000
>>> +#define KVM_GUESTDBG_USE_HW_BP          0x00020000
>>> +
>>> +#define KVMPPC_DEBUG_NOTYPE             0x0
>>> +#define KVMPPC_DEBUG_BREAKPOINT         (1UL << 1)
>>> +#define KVMPPC_DEBUG_WATCH_WRITE        (1UL << 2)
>>> +#define KVMPPC_DEBUG_WATCH_READ         (1UL << 3)
>>> +
>>> /* for KVM_SET_GUEST_DEBUG */
>>> struct kvm_guest_debug_arch {
>>> +    struct {
>>> +        __u32 addr;
>>> +        __u32 type;
>>> +    } bp[6];
>>>       
>> I can't look up the sources right now. Is this a struct that 
>> 1:1 maps  
>> to an ioctl struct? If so, we should add padding for a 
>> possible future  
>> extension of debug registers.
>>     
>
> Yes it's used by ioctl.
> What's the usually pad size?
>   

I don't think there's a default. I just tend to pad it to something
reasonable. I guess in this case we can even just extend bp to 128
entries, add a reasonable amount of churn to the debug info and be good:

struct kvm_guest_debug_arch {
    struct {
        __u64 addr;
        __u32 type;
        __u32 pad1;
        __u64 pad2;
    } bp[128];
}

This should be enough to even leverage performance monitoring stuff later on that would be able to check if r1 == 0x1234 and then stop :-).


Alex