On 14.03.2013, at 13:22, Jan Kiszka wrote: > On 2013-03-14 13:19, Alexander Graf wrote: >> >> On 14.03.2013, at 13:13, Jan Kiszka wrote: >> >>> On 2013-03-14 13:09, Alexander Graf wrote: >>>> >>>> On 14.03.2013, at 12:57, Jan Kiszka wrote: >>>> >>>>> On 2013-03-14 12:54, Alexander Graf wrote: >>>>>> >>>>>> On 14.03.2013, at 05:42, Bhushan Bharat-R65777 wrote: >>>>>> >>>>>>> >>>>>>> >>>>>>>> -----Original Message----- >>>>>>>> From: Alexander Graf [mailto:agraf@xxxxxxx] >>>>>>>> Sent: Thursday, March 07, 2013 6:51 PM >>>>>>>> To: Bhushan Bharat-R65777 >>>>>>>> Cc: kvm-ppc@xxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx; Wood Scott-B07421; Bhushan >>>>>>>> Bharat-R65777 >>>>>>>> Subject: Re: [PATCH 3/7] KVM: PPC: debug stub interface parameter defined >>>>>>>> >>>>>>>> >>>>>>>> On 28.02.2013, at 05:13, Bharat Bhushan wrote: >>>>>>>> >>>>>>>>> This patch defines the interface parameter for KVM_SET_GUEST_DEBUG >>>>>>>>> ioctl support. Follow up patches will use this for setting up hardware >>>>>>>>> breakpoints, watchpoints and software breakpoints. >>>>>>>>> >>>>>>>>> Also kvm_arch_vcpu_ioctl_set_guest_debug() is brought one level below. >>>>>>>>> This is because I am not sure what is required for book3s. So this >>>>>>>>> ioctl behaviour will not change for book3s. >>>>>>>>> >>>>>>>>> Signed-off-by: Bharat Bhushan <bharat.bhushan@xxxxxxxxxxxxx> >>>>>>>>> --- >>>>>>>>> arch/powerpc/include/uapi/asm/kvm.h | 23 +++++++++++++++++++++++ >>>>>>>>> arch/powerpc/kvm/book3s.c | 6 ++++++ >>>>>>>>> arch/powerpc/kvm/booke.c | 6 ++++++ >>>>>>>>> arch/powerpc/kvm/powerpc.c | 6 ------ >>>>>>>>> 4 files changed, 35 insertions(+), 6 deletions(-) >>>>>>>>> >>>>>>>>> diff --git a/arch/powerpc/include/uapi/asm/kvm.h >>>>>>>>> b/arch/powerpc/include/uapi/asm/kvm.h >>>>>>>>> index c2ff99c..15f9a00 100644 >>>>>>>>> --- a/arch/powerpc/include/uapi/asm/kvm.h >>>>>>>>> +++ b/arch/powerpc/include/uapi/asm/kvm.h >>>>>>>>> @@ -272,8 +272,31 @@ struct kvm_debug_exit_arch { >>>>>>>>> >>>>>>>>> /* for KVM_SET_GUEST_DEBUG */ >>>>>>>>> struct kvm_guest_debug_arch { >>>>>>>>> + struct { >>>>>>>>> + /* H/W breakpoint/watchpoint address */ >>>>>>>>> + __u64 addr; >>>>>>>>> + /* >>>>>>>>> + * Type denotes h/w breakpoint, read watchpoint, write >>>>>>>>> + * watchpoint or watchpoint (both read and write). >>>>>>>>> + */ >>>>>>>>> +#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) >>>>>>>>> + __u32 type; >>>>>>>>> + __u32 reserved; >>>>>>>>> + } bp[16]; >>>>>>>>> }; >>>>>>>>> >>>>>>>>> +/* Debug related defines */ >>>>>>>>> +/* >>>>>>>>> + * kvm_guest_debug->control is a 32 bit field. The lower 16 bits are >>>>>>>>> +generic >>>>>>>>> + * and upper 16 bits are architecture specific. Architecture specific >>>>>>>>> +defines >>>>>>>>> + * that ioctl is for setting hardware breakpoint or software breakpoint. >>>>>>>>> + */ >>>>>>>>> +#define KVM_GUESTDBG_USE_SW_BP 0x00010000 >>>>>>>>> +#define KVM_GUESTDBG_USE_HW_BP 0x00020000 >>>>>>>> >>>>>>>> You only need >>>>>>>> >>>>>>>> #define KVM_GUESTDBG_HW_BP 0x00010000 >>>>>>>> >>>>>>>> In absence of the flag, it's a SW breakpoint. >>>>>>> >>>>>>> We kept this for 2 reasons; 1) Same logic is applied for i386, so trying to keep consistent 2) better clarity. >>>>>> >>>>>> Jan, was there any special reason to have 2 flags for HW/SW breakpoint on x86 rather than one bit that indicates which one is used? >>>>> >>>>> Different mechanics on x86: HW goes via debug registers and shows up as >>>>> INT1, SW is INT3 (plus guest patching done by user land). >>>> >>>> Well, the same thing goes for us. What I'm asking is whether there is a specific reason (extensibility, oversight, taste, ...) that you did >>>> >>>> #define KVM_GUESTDBG_USE_SW_BP 0x00010000 >>>> #define KVM_GUESTDBG_USE_HW_BP 0x00020000 >>>> >>>> rather than >>>> >>>> #define KVM_GUESTDBG_BP_TYPE 0x00010000 >>>> #define KVM_GUESTDBG_BP_TYPE_SW 0x00010000 >>>> #define KVM_GUESTDBG_BP_TYPE_HW 0x00000000 >>>> >>>> :) >>> >>> Those bits enable or disable the features separately. You may also leave >>> both off if you like (and just use single stepping). >> >> Ah, so these are global configuration bits, not per-breakpoint configuration? > > Yes, the are meant for kvm_guest_debug.control on x86. I see that this > is apparently different for ppc. Those bits you cited just control the > general enabling of hard or soft BPs, not the activation of individual > one. That is encoded into the BP registers on x86. I suppose the same thing applies for PPC and I simply didn't realize it :). So Bharat, if these bits are used for global configuration whether a specific debug type is routed to user space, having separate bits is the way to go. Alex -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
