diff mbox

[2/7] KVM: arm: guest debug, define API headers

Message ID 1416931805-23223-3-git-send-email-alex.bennee@linaro.org
State Superseded, archived
Headers show

Commit Message

Alex Bennée Nov. 25, 2014, 4:10 p.m. UTC
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 <alex.bennee@linaro.org>

Comments

Peter Maydell Nov. 25, 2014, 4:19 p.m. UTC | #1
On 25 November 2014 at 16:10, Alex Bennée <alex.bennee@linaro.org> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Peter Maydell Nov. 25, 2014, 5:13 p.m. UTC | #2
On 25 November 2014 at 17:05, Paolo Bonzini <pbonzini@redhat.com> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Alex Bennée Nov. 26, 2014, 1:13 p.m. UTC | #3
Paolo Bonzini <pbonzini@redhat.com> writes:

> On 25/11/2014 18:13, Peter Maydell wrote:
>> On 25 November 2014 at 17:05, Paolo Bonzini <pbonzini@redhat.com> 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 Nov. 26, 2014, 2:58 p.m. UTC | #4
Andrew Jones <drjones@redhat.com> 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 Nov. 26, 2014, 3:04 p.m. UTC | #5
Peter Maydell <peter.maydell@linaro.org> writes:

> On 25 November 2014 at 16:10, Alex Bennée <alex.bennee@linaro.org> 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.
Christoffer Dall Nov. 29, 2014, 4:20 p.m. UTC | #6
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 <alex.bennee@linaro.org>
> 
> 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
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Christoffer Dall Nov. 29, 2014, 4:20 p.m. UTC | #7
On Wed, Nov 26, 2014 at 03:04:10PM +0000, Alex Bennée wrote:
> 
> Peter Maydell <peter.maydell@linaro.org> writes:
> 
> > On 25 November 2014 at 16:10, Alex Bennée <alex.bennee@linaro.org> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Alex Bennée Dec. 1, 2014, 11:30 a.m. UTC | #8
Christoffer Dall <christoffer.dall@linaro.org> writes:

> On Wed, Nov 26, 2014 at 03:04:10PM +0000, Alex Bennée wrote:
>> 
>> Peter Maydell <peter.maydell@linaro.org> writes:
>> 
>> > On 25 November 2014 at 16:10, Alex Bennée <alex.bennee@linaro.org> 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
diff mbox

Patch

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__ */