This commit defines the API headers for guest debugging. There are two
architecture specific debug structures:
- kvm_guest_debug_arch, allows us to pass in HW debug registers
- kvm_debug_exit_arch, signals the exact debug exit and address
The type of debugging being used is control by the architecture specific
control bits of the kvm_guest_debug->control flags in the ioctl
structure.
Signed-off-by: Alex Bennée <[email protected]>
diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index 8e38878..de2450c 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -93,10 +93,30 @@ struct kvm_sregs {
struct kvm_fpu {
};
+/* See ARM ARM D7.3: Debug Registers
+ *
+ * The control registers are stored as 64bit values as the setup code
+ * is shared with the normal cpu context restore code in hyp.S which
+ * is 64 bit only.
+ */
+#define KVM_ARM_NDBG_REGS 16
struct kvm_guest_debug_arch {
+ __u64 dbg_bcr[KVM_ARM_NDBG_REGS];
+ __u64 dbg_bvr[KVM_ARM_NDBG_REGS];
+ __u64 dbg_wcr[KVM_ARM_NDBG_REGS];
+ __u64 dbg_wvr[KVM_ARM_NDBG_REGS];
};
+/* Exit types which define why we did a debug exit */
+#define KVM_DEBUG_EXIT_ERROR 0x0
+#define KVM_DEBUG_EXIT_SINGLE_STEP 0x1
+#define KVM_DEBUG_EXIT_SW_BKPT 0x2
+#define KVM_DEBUG_EXIT_HW_BKPT 0x3
+#define KVM_DEBUG_EXIT_HW_WTPT 0x4
+
struct kvm_debug_exit_arch {
+ __u64 address;
+ __u32 exit_type;
};
struct kvm_sync_regs {
@@ -198,4 +218,12 @@ struct kvm_arch_memory_slot {
#endif
+/* Architecture related debug defines - upper 16 bits of
+ * kvm_guest_debug->control
+ */
+#define KVM_GUESTDBG_USE_SW_BP_SHIFT 16
+#define KVM_GUESTDBG_USE_SW_BP (1 << KVM_GUESTDBG_USE_SW_BP_SHIFT)
+#define KVM_GUESTDBG_USE_HW_BP_SHIFT 17
+#define KVM_GUESTDBG_USE_HW_BP (1 << KVM_GUESTDBG_USE_HW_BP_SHIFT)
+
#endif /* __ARM_KVM_H__ */
--
2.1.3
On 25 November 2014 at 16:10, Alex Bennée <[email protected]> wrote:
> +/* Exit types which define why we did a debug exit */
> +#define KVM_DEBUG_EXIT_ERROR 0x0
> +#define KVM_DEBUG_EXIT_SINGLE_STEP 0x1
> +#define KVM_DEBUG_EXIT_SW_BKPT 0x2
> +#define KVM_DEBUG_EXIT_HW_BKPT 0x3
> +#define KVM_DEBUG_EXIT_HW_WTPT 0x4
The names of these imply that they're generic, but they're
defined in an arch-specific header file...
-- PMM
On 25/11/2014 17:10, Alex Bennée wrote:
> +/* Exit types which define why we did a debug exit */
> +#define KVM_DEBUG_EXIT_ERROR 0x0
> +#define KVM_DEBUG_EXIT_SINGLE_STEP 0x1
> +#define KVM_DEBUG_EXIT_SW_BKPT 0x2
> +#define KVM_DEBUG_EXIT_HW_BKPT 0x3
> +#define KVM_DEBUG_EXIT_HW_WTPT 0x4
> +
> struct kvm_debug_exit_arch {
> + __u64 address;
> + __u32 exit_type;
> };
>
So there is no register that says "this breakpoint has triggered" or
"this watchpoint has triggered"?
Paolo
On 25 November 2014 at 17:05, Paolo Bonzini <[email protected]> wrote:
> So there is no register that says "this breakpoint has triggered" or
> "this watchpoint has triggered"?
Nope. You take a debug exception; the syndrome register tells
you if it was a bp or a wp, and if it was a wp the fault address
register tells you the address being accessed (if it was a bp
you know the program counter, obviously). The debugger is expected
to be able to figure it out from there, if it cares.
-- PMM
On 25/11/2014 18:13, Peter Maydell wrote:
> On 25 November 2014 at 17:05, Paolo Bonzini <[email protected]> wrote:
>> > So there is no register that says "this breakpoint has triggered" or
>> > "this watchpoint has triggered"?
> Nope. You take a debug exception; the syndrome register tells
> you if it was a bp or a wp, and if it was a wp the fault address
> register tells you the address being accessed (if it was a bp
> you know the program counter, obviously). The debugger is expected
> to be able to figure it out from there, if it cares.
That's already good enough---do the KVM_DEBUG_EXIT_* constants match the
syndrome register, or if not why?
Paolo
Paolo Bonzini <[email protected]> writes:
> On 25/11/2014 18:13, Peter Maydell wrote:
>> On 25 November 2014 at 17:05, Paolo Bonzini <[email protected]> wrote:
>>> > So there is no register that says "this breakpoint has triggered" or
>>> > "this watchpoint has triggered"?
>> Nope. You take a debug exception; the syndrome register tells
>> you if it was a bp or a wp, and if it was a wp the fault address
>> register tells you the address being accessed (if it was a bp
>> you know the program counter, obviously). The debugger is expected
>> to be able to figure it out from there, if it cares.
>
> That's already good enough---do the KVM_DEBUG_EXIT_* constants match the
> syndrome register, or if not why?
No they don't. I did consider it at the time but I was wary of pulling
too much over into the uapi headers wholesale. If your happy to do that
I'll include the change in my next version.
I could also rationalise the exit handlers as they all pretty much do
the same thing (save for the exit/syndrome related info). Again I was
keeping things nicely separated in case any particular exception needed
excessive special case handling.
Would you like those changes?
>
> Paolo
--
Alex Bennée
On 26/11/2014 14:13, Alex Bennée wrote:
>
> Paolo Bonzini <[email protected]> writes:
>
>> On 25/11/2014 18:13, Peter Maydell wrote:
>>> On 25 November 2014 at 17:05, Paolo Bonzini <[email protected]> wrote:
>>>>> So there is no register that says "this breakpoint has triggered" or
>>>>> "this watchpoint has triggered"?
>>> Nope. You take a debug exception; the syndrome register tells
>>> you if it was a bp or a wp, and if it was a wp the fault address
>>> register tells you the address being accessed (if it was a bp
>>> you know the program counter, obviously). The debugger is expected
>>> to be able to figure it out from there, if it cares.
>>
>> That's already good enough---do the KVM_DEBUG_EXIT_* constants match the
>> syndrome register, or if not why?
>
> No they don't. I did consider it at the time but I was wary of pulling
> too much over into the uapi headers wholesale. If your happy to do that
> I'll include the change in my next version.
>
> I could also rationalise the exit handlers as they all pretty much do
> the same thing (save for the exit/syndrome related info). Again I was
> keeping things nicely separated in case any particular exception needed
> excessive special case handling.
>
> Would you like those changes?
Yes, please.
Paolo
On Tue, Nov 25, 2014 at 04:10:00PM +0000, Alex Benn?e wrote:
> This commit defines the API headers for guest debugging. There are two
> architecture specific debug structures:
>
> - kvm_guest_debug_arch, allows us to pass in HW debug registers
> - kvm_debug_exit_arch, signals the exact debug exit and address
>
> The type of debugging being used is control by the architecture specific
> control bits of the kvm_guest_debug->control flags in the ioctl
> structure.
>
> Signed-off-by: Alex Benn?e <[email protected]>
>
> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index 8e38878..de2450c 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -93,10 +93,30 @@ struct kvm_sregs {
> struct kvm_fpu {
> };
>
> +/* See ARM ARM D7.3: Debug Registers
> + *
> + * The control registers are stored as 64bit values as the setup code
> + * is shared with the normal cpu context restore code in hyp.S which
> + * is 64 bit only.
> + */
> +#define KVM_ARM_NDBG_REGS 16
> struct kvm_guest_debug_arch {
> + __u64 dbg_bcr[KVM_ARM_NDBG_REGS];
> + __u64 dbg_bvr[KVM_ARM_NDBG_REGS];
> + __u64 dbg_wcr[KVM_ARM_NDBG_REGS];
> + __u64 dbg_wvr[KVM_ARM_NDBG_REGS];
> };
>
> +/* Exit types which define why we did a debug exit */
> +#define KVM_DEBUG_EXIT_ERROR 0x0
> +#define KVM_DEBUG_EXIT_SINGLE_STEP 0x1
> +#define KVM_DEBUG_EXIT_SW_BKPT 0x2
> +#define KVM_DEBUG_EXIT_HW_BKPT 0x3
> +#define KVM_DEBUG_EXIT_HW_WTPT 0x4
> +
> struct kvm_debug_exit_arch {
> + __u64 address;
> + __u32 exit_type;
> };
>
> struct kvm_sync_regs {
> @@ -198,4 +218,12 @@ struct kvm_arch_memory_slot {
>
> #endif
>
> +/* Architecture related debug defines - upper 16 bits of
> + * kvm_guest_debug->control
> + */
> +#define KVM_GUESTDBG_USE_SW_BP_SHIFT 16
> +#define KVM_GUESTDBG_USE_SW_BP (1 << KVM_GUESTDBG_USE_SW_BP_SHIFT)
> +#define KVM_GUESTDBG_USE_HW_BP_SHIFT 17
> +#define KVM_GUESTDBG_USE_HW_BP (1 << KVM_GUESTDBG_USE_HW_BP_SHIFT)
> +
I see this are defined in arch/x86/include/uapi/asm/kvm.h,
so you needed to reproduce them here, but shouldn't they
be promoted to include/uapi/linux/kvm.h instead?
> #endif /* __ARM_KVM_H__ */
> --
> 2.1.3
>
> _______________________________________________
> kvmarm mailing list
> [email protected]
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Andrew Jones <[email protected]> writes:
> On Tue, Nov 25, 2014 at 04:10:00PM +0000, Alex Bennée wrote:
>> This commit defines the API headers for guest debugging. There are two
>> architecture specific debug structures:
<snip>
>> +/* Architecture related debug defines - upper 16 bits of
>> + * kvm_guest_debug->control
>> + */
>> +#define KVM_GUESTDBG_USE_SW_BP_SHIFT 16
>> +#define KVM_GUESTDBG_USE_SW_BP (1 << KVM_GUESTDBG_USE_SW_BP_SHIFT)
>> +#define KVM_GUESTDBG_USE_HW_BP_SHIFT 17
>> +#define KVM_GUESTDBG_USE_HW_BP (1 << KVM_GUESTDBG_USE_HW_BP_SHIFT)
>> +
>
> I see this are defined in arch/x86/include/uapi/asm/kvm.h,
> so you needed to reproduce them here, but shouldn't they
> be promoted to include/uapi/linux/kvm.h instead?
Well if we move them to common uapi we either restrict the $ARCH
specific options that don't have SW/HW BKPTS (would be weird but...) or
make them generic in the lower 16 bits (breaks API).
But in principle I have no objection if other don't.
--
Alex Bennée
Peter Maydell <[email protected]> writes:
> On 25 November 2014 at 16:10, Alex Bennée <[email protected]> wrote:
>> +/* Exit types which define why we did a debug exit */
>> +#define KVM_DEBUG_EXIT_ERROR 0x0
>> +#define KVM_DEBUG_EXIT_SINGLE_STEP 0x1
>> +#define KVM_DEBUG_EXIT_SW_BKPT 0x2
>> +#define KVM_DEBUG_EXIT_HW_BKPT 0x3
>> +#define KVM_DEBUG_EXIT_HW_WTPT 0x4
>
> The names of these imply that they're generic, but they're
> defined in an arch-specific header file...
Yeah, I think these will die and I'll just export the syndrome
information directly to QEMU.
--
Alex Bennée
On 26/11/2014 15:58, Alex Bennée wrote:
>
> Andrew Jones <[email protected]> writes:
>
>> On Tue, Nov 25, 2014 at 04:10:00PM +0000, Alex Bennée wrote:
>>> This commit defines the API headers for guest debugging. There are two
>>> architecture specific debug structures:
> <snip>
>>> +/* Architecture related debug defines - upper 16 bits of
>>> + * kvm_guest_debug->control
>>> + */
>>> +#define KVM_GUESTDBG_USE_SW_BP_SHIFT 16
>>> +#define KVM_GUESTDBG_USE_SW_BP (1 << KVM_GUESTDBG_USE_SW_BP_SHIFT)
>>> +#define KVM_GUESTDBG_USE_HW_BP_SHIFT 17
>>> +#define KVM_GUESTDBG_USE_HW_BP (1 << KVM_GUESTDBG_USE_HW_BP_SHIFT)
>>> +
>>
>> I see this are defined in arch/x86/include/uapi/asm/kvm.h,
>> so you needed to reproduce them here, but shouldn't they
>> be promoted to include/uapi/linux/kvm.h instead?
>
> Well if we move them to common uapi we either restrict the $ARCH
> specific options that don't have SW/HW BKPTS (would be weird but...) or
> make them generic in the lower 16 bits (breaks API).
>
> But in principle I have no objection if other don't.
I think it's a matter of personal taste. "Architecture-specific" means
"not all architectures may support it", but it's certainly a good idea
to reuse the same value if multiple architectures do support a #define.
What you did is fine, another possibility is to do
#define __KVM_GUESTDBG_USE_SW_BP (1 << 16)
in include/uapi/linux/kvm.h, and
#define KVM_GUESTDBG_USE_SW_BP __KVM_GUESTDBG_USE_SW_BP
in the arch-specific file. Andrew, is this closer to what you intended?
Paolo
On Wed, Nov 26, 2014 at 05:46:05PM +0100, Paolo Bonzini wrote:
>
>
> On 26/11/2014 15:58, Alex Benn?e wrote:
> >
> > Andrew Jones <[email protected]> writes:
> >
> >> On Tue, Nov 25, 2014 at 04:10:00PM +0000, Alex Benn?e wrote:
> >>> This commit defines the API headers for guest debugging. There are two
> >>> architecture specific debug structures:
> > <snip>
> >>> +/* Architecture related debug defines - upper 16 bits of
> >>> + * kvm_guest_debug->control
> >>> + */
> >>> +#define KVM_GUESTDBG_USE_SW_BP_SHIFT 16
> >>> +#define KVM_GUESTDBG_USE_SW_BP (1 << KVM_GUESTDBG_USE_SW_BP_SHIFT)
> >>> +#define KVM_GUESTDBG_USE_HW_BP_SHIFT 17
> >>> +#define KVM_GUESTDBG_USE_HW_BP (1 << KVM_GUESTDBG_USE_HW_BP_SHIFT)
> >>> +
> >>
> >> I see this are defined in arch/x86/include/uapi/asm/kvm.h,
> >> so you needed to reproduce them here, but shouldn't they
> >> be promoted to include/uapi/linux/kvm.h instead?
> >
> > Well if we move them to common uapi we either restrict the $ARCH
> > specific options that don't have SW/HW BKPTS (would be weird but...) or
> > make them generic in the lower 16 bits (breaks API).
> >
> > But in principle I have no objection if other don't.
>
> I think it's a matter of personal taste. "Architecture-specific" means
> "not all architectures may support it", but it's certainly a good idea
> to reuse the same value if multiple architectures do support a #define.
>
> What you did is fine, another possibility is to do
>
> #define __KVM_GUESTDBG_USE_SW_BP (1 << 16)
>
> in include/uapi/linux/kvm.h, and
>
> #define KVM_GUESTDBG_USE_SW_BP __KVM_GUESTDBG_USE_SW_BP
>
> in the arch-specific file. Andrew, is this closer to what you intended?
>
I just reread Documentation/virtual/kvm/api.txt a bit more closely and
see that these fall in the "architecture specific control" region of the
field. So forget what I said. But your suggestion of __KVM_GUESTDBG_USE_SW_BP
looks like a good idea to me.
drew
On Tue, Nov 25, 2014 at 04:10:00PM +0000, Alex Benn?e wrote:
> This commit defines the API headers for guest debugging. There are two
> architecture specific debug structures:
>
> - kvm_guest_debug_arch, allows us to pass in HW debug registers
> - kvm_debug_exit_arch, signals the exact debug exit and address
>
> The type of debugging being used is control by the architecture specific
s/control/controlled/ ?
> control bits of the kvm_guest_debug->control flags in the ioctl
> structure.
>
> Signed-off-by: Alex Benn?e <[email protected]>
>
> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index 8e38878..de2450c 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -93,10 +93,30 @@ struct kvm_sregs {
> struct kvm_fpu {
> };
>
> +/* See ARM ARM D7.3: Debug Registers
/* needs to go on a separate line.
> + *
> + * The control registers are stored as 64bit values as the setup code
> + * is shared with the normal cpu context restore code in hyp.S which
> + * is 64 bit only.
is this comment here because the architecture defines them as 32-bits?
If so, adding that would make this comment make more sense.
> + */
> +#define KVM_ARM_NDBG_REGS 16
> struct kvm_guest_debug_arch {
> + __u64 dbg_bcr[KVM_ARM_NDBG_REGS];
> + __u64 dbg_bvr[KVM_ARM_NDBG_REGS];
> + __u64 dbg_wcr[KVM_ARM_NDBG_REGS];
> + __u64 dbg_wvr[KVM_ARM_NDBG_REGS];
> };
>
> +/* Exit types which define why we did a debug exit */
> +#define KVM_DEBUG_EXIT_ERROR 0x0
> +#define KVM_DEBUG_EXIT_SINGLE_STEP 0x1
> +#define KVM_DEBUG_EXIT_SW_BKPT 0x2
> +#define KVM_DEBUG_EXIT_HW_BKPT 0x3
> +#define KVM_DEBUG_EXIT_HW_WTPT 0x4
> +
> struct kvm_debug_exit_arch {
> + __u64 address;
> + __u32 exit_type;
> };
>
> struct kvm_sync_regs {
> @@ -198,4 +218,12 @@ struct kvm_arch_memory_slot {
>
> #endif
>
> +/* Architecture related debug defines - upper 16 bits of
same not with commenting style here
> + * kvm_guest_debug->control
> + */
> +#define KVM_GUESTDBG_USE_SW_BP_SHIFT 16
> +#define KVM_GUESTDBG_USE_SW_BP (1 << KVM_GUESTDBG_USE_SW_BP_SHIFT)
> +#define KVM_GUESTDBG_USE_HW_BP_SHIFT 17
> +#define KVM_GUESTDBG_USE_HW_BP (1 << KVM_GUESTDBG_USE_HW_BP_SHIFT)
> +
> #endif /* __ARM_KVM_H__ */
> --
> 2.1.3
>
On Wed, Nov 26, 2014 at 03:04:10PM +0000, Alex Benn?e wrote:
>
> Peter Maydell <[email protected]> writes:
>
> > On 25 November 2014 at 16:10, Alex Benn?e <[email protected]> wrote:
> >> +/* Exit types which define why we did a debug exit */
> >> +#define KVM_DEBUG_EXIT_ERROR 0x0
> >> +#define KVM_DEBUG_EXIT_SINGLE_STEP 0x1
> >> +#define KVM_DEBUG_EXIT_SW_BKPT 0x2
> >> +#define KVM_DEBUG_EXIT_HW_BKPT 0x3
> >> +#define KVM_DEBUG_EXIT_HW_WTPT 0x4
> >
> > The names of these imply that they're generic, but they're
> > defined in an arch-specific header file...
>
> Yeah, I think these will die and I'll just export the syndrome
> information directly to QEMU.
huh?
-Christoffer
Christoffer Dall <[email protected]> writes:
> On Wed, Nov 26, 2014 at 03:04:10PM +0000, Alex Bennée wrote:
>>
>> Peter Maydell <[email protected]> writes:
>>
>> > On 25 November 2014 at 16:10, Alex Bennée <[email protected]> wrote:
>> >> +/* Exit types which define why we did a debug exit */
>> >> +#define KVM_DEBUG_EXIT_ERROR 0x0
>> >> +#define KVM_DEBUG_EXIT_SINGLE_STEP 0x1
>> >> +#define KVM_DEBUG_EXIT_SW_BKPT 0x2
>> >> +#define KVM_DEBUG_EXIT_HW_BKPT 0x3
>> >> +#define KVM_DEBUG_EXIT_HW_WTPT 0x4
>> >
>> > The names of these imply that they're generic, but they're
>> > defined in an arch-specific header file...
>>
>> Yeah, I think these will die and I'll just export the syndrome
>> information directly to QEMU.
>
> huh?
Rather than mapping syndrome to a specific exit value we might as well
export syndrome information to QEMU and let it define the reason.
>
> -Christoffer
--
Alex Bennée