[PATCH 1/7] KVM: add commentary for kvm_debug_exit_arch struct

[PATCH 1/7] KVM: add commentary for kvm_debug_exit_arch struct

[Alex Bennée]

Bring into line with the commentary for the other structures and their
KVM_EXIT_* cases.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 6076882..523f476 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -226,6 +226,7 @@ struct kvm_run {
 			__u32 count;
 			__u64 data_offset; /* relative to kvm_run start */
 		} io;
+		/* KVM_EXIT_DEBUG */
 		struct {
 			struct kvm_debug_exit_arch arch;
 		} debug;
-- 
2.1.3

Re: [PATCH 1/7] KVM: add commentary for kvm_debug_exit_arch struct

[Andrew Jones]

On Tue, Nov 25, 2014 at 04:09:59PM +0000, Alex Bennée wrote:
> Bring into line with the commentary for the other structures and their
> KVM_EXIT_* cases.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 6076882..523f476 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -226,6 +226,7 @@ struct kvm_run {
>  			__u32 count;
>  			__u64 data_offset; /* relative to kvm_run start */
>  		} io;
> +		/* KVM_EXIT_DEBUG */
>  		struct {
>  			struct kvm_debug_exit_arch arch;
>  		} debug;
> -- 
> 2.1.3

Can we change this to a 'KVM: add commentary for kvm exit type data'
patch? We're also missing

/* KVM_EXIT_INTERNAL_ERROR */

and

/* KVM_EXIT_PAPR_HCALL */

> 
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

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

[Alex Bennée]

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>

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

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

[Peter Maydell]

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

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

[Alex Bennée]

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.

-- 
Alex Bennée

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

[Christoffer Dall]

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

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

[Alex Bennée]

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

-- 
Alex Bennée

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

[Paolo Bonzini]

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

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

[Peter Maydell]

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

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

[Paolo Bonzini]

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?

Paolo

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

[Alex Bennée]

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

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

[Paolo Bonzini]

On 26/11/2014 14:13, Alex Bennée wrote:
> 
> 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?

Yes, please.

Paolo

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

[Andrew Jones]

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 <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
> + *
> + * 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
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

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

[Alex Bennée]

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

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

[Paolo Bonzini]

On 26/11/2014 15:58, Alex Bennée wrote:
> 
> 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.

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

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

[Andrew Jones]

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 <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.
> 
> 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

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

[Christoffer Dall]

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
> 

[PATCH 3/7] KVM: arm: guest debug, add stub KVM_SET_GUEST_DEBUG ioctl

[Alex Bennée]

This commit adds a stub function to support the KVM_SET_GUEST_DEBUG
ioctl. Currently any operation flag will return EINVAL. Actual
functionality will be added with further patches.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>.

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index 7610eaa..2c6386e 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -2570,7 +2570,7 @@ handled.
 4.87 KVM_SET_GUEST_DEBUG
 
 Capability: KVM_CAP_SET_GUEST_DEBUG
-Architectures: x86, s390, ppc
+Architectures: x86, s390, ppc, arm64
 Type: vcpu ioctl
 Parameters: struct kvm_guest_debug (in)
 Returns: 0 on success; -1 on error
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 9e193c8..a0ff410 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -180,6 +180,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 	case KVM_CAP_ARM_PSCI:
 	case KVM_CAP_ARM_PSCI_0_2:
 	case KVM_CAP_READONLY_MEM:
+	case KVM_CAP_SET_GUEST_DEBUG:
 		r = 1;
 		break;
 	case KVM_CAP_COALESCED_MMIO:
@@ -302,7 +303,34 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
 int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
 					struct kvm_guest_debug *dbg)
 {
-	return -EINVAL;
+	/* If it's not enabled clear all flags */
+	if (!(dbg->control & KVM_GUESTDBG_ENABLE)) {
+		vcpu->guest_debug = 0;
+		return 0;
+	}
+
+	vcpu->guest_debug = dbg->control;
+	kvm_info("%s: guest_debug is 0x%lx\n", __func__, vcpu->guest_debug);
+
+	/* Single Step */
+	if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
+		kvm_info("SS requested, not yet implemented\n");
+		return -EINVAL;
+	}
+
+	/* Software Break Points */
+	if (vcpu->guest_debug & KVM_GUESTDBG_USE_SW_BP) {
+		kvm_info("SW BP support requested, not yet implemented\n");
+		return -EINVAL;
+	}
+
+	/* Hardware assisted Break and Watch points */
+	if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP) {
+		kvm_info("HW BP support requested, not yet implemented\n");
+		return -EINVAL;
+	}
+
+	return 0;
 }
 
 
-- 
2.1.3

Re: [PATCH 3/7] KVM: arm: guest debug, add stub KVM_SET_GUEST_DEBUG ioctl

[Andrew Jones]

On Tue, Nov 25, 2014 at 04:10:01PM +0000, Alex Bennée wrote:
> This commit adds a stub function to support the KVM_SET_GUEST_DEBUG
> ioctl. Currently any operation flag will return EINVAL. Actual
> functionality will be added with further patches.

Technically the stub is already there, and you're extending it to
start looking at control flags, but still not doing anything yet.

> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>.
> 
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index 7610eaa..2c6386e 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -2570,7 +2570,7 @@ handled.
>  4.87 KVM_SET_GUEST_DEBUG
>  
>  Capability: KVM_CAP_SET_GUEST_DEBUG
> -Architectures: x86, s390, ppc
> +Architectures: x86, s390, ppc, arm64
>  Type: vcpu ioctl
>  Parameters: struct kvm_guest_debug (in)
>  Returns: 0 on success; -1 on error
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 9e193c8..a0ff410 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -180,6 +180,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>  	case KVM_CAP_ARM_PSCI:
>  	case KVM_CAP_ARM_PSCI_0_2:
>  	case KVM_CAP_READONLY_MEM:
> +	case KVM_CAP_SET_GUEST_DEBUG:
>  		r = 1;
>  		break;
>  	case KVM_CAP_COALESCED_MMIO:
> @@ -302,7 +303,34 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>  int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>  					struct kvm_guest_debug *dbg)
>  {
> -	return -EINVAL;
> +	/* If it's not enabled clear all flags */
> +	if (!(dbg->control & KVM_GUESTDBG_ENABLE)) {
> +		vcpu->guest_debug = 0;
> +		return 0;
> +	}
> +
> +	vcpu->guest_debug = dbg->control;
> +	kvm_info("%s: guest_debug is 0x%lx\n", __func__, vcpu->guest_debug);
> +
> +	/* Single Step */
> +	if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
> +		kvm_info("SS requested, not yet implemented\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Software Break Points */
> +	if (vcpu->guest_debug & KVM_GUESTDBG_USE_SW_BP) {
> +		kvm_info("SW BP support requested, not yet implemented\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Hardware assisted Break and Watch points */
> +	if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP) {
> +		kvm_info("HW BP support requested, not yet implemented\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
>  }

I guess all the kvm_info's were useful for developing this patch series,
but do we still need them?

>  
>  
> -- 
> 2.1.3
> 
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 3/7] KVM: arm: guest debug, add stub KVM_SET_GUEST_DEBUG ioctl

[Alex Bennée]

Andrew Jones <drjones@redhat.com> writes:

> On Tue, Nov 25, 2014 at 04:10:01PM +0000, Alex Bennée wrote:
>> This commit adds a stub function to support the KVM_SET_GUEST_DEBUG
>> ioctl. Currently any operation flag will return EINVAL. Actual
>> functionality will be added with further patches.
>
> Technically the stub is already there, and you're extending it to
> start looking at control flags, but still not doing anything yet.

Sure we do:

>>  int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>>  					struct kvm_guest_debug *dbg)
>>  {
>> -	return -EINVAL;
>> +	/* If it's not enabled clear all flags */
>> +	if (!(dbg->control & KVM_GUESTDBG_ENABLE)) {
>> +		vcpu->guest_debug = 0;
>> +		return 0;
>> +	}

That's some class non-functionality right there ;-)

>> +	vcpu->guest_debug = dbg->control;
>> +	kvm_info("%s: guest_debug is 0x%lx\n", __func__, vcpu->guest_debug);
>> +
>> +	/* Single Step */
>> +	if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
>> +		kvm_info("SS requested, not yet implemented\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* Software Break Points */
>> +	if (vcpu->guest_debug & KVM_GUESTDBG_USE_SW_BP) {
>> +		kvm_info("SW BP support requested, not yet implemented\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* Hardware assisted Break and Watch points */
>> +	if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP) {
>> +		kvm_info("HW BP support requested, not yet implemented\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>>  }
>
> I guess all the kvm_info's were useful for developing this patch series,
> but do we still need them?

They also served the very useful roll of stopping checkpatch.pl bitching
about my reluctance to remove braces from the if () { } clauses. However
I take your point. I can certainly remove the kvm_info() statements as
each bit of functionality is added while leaving this one to help when
someone is bisecting and confused.

-- 
Alex Bennée

Re: [PATCH 3/7] KVM: arm: guest debug, add stub KVM_SET_GUEST_DEBUG ioctl

[Christoffer Dall]

On Tue, Nov 25, 2014 at 04:10:01PM +0000, Alex Bennée wrote:
> This commit adds a stub function to support the KVM_SET_GUEST_DEBUG
> ioctl. Currently any operation flag will return EINVAL. Actual
> functionality will be added with further patches.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>.
> 
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index 7610eaa..2c6386e 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -2570,7 +2570,7 @@ handled.
>  4.87 KVM_SET_GUEST_DEBUG
>  
>  Capability: KVM_CAP_SET_GUEST_DEBUG
> -Architectures: x86, s390, ppc
> +Architectures: x86, s390, ppc, arm64
>  Type: vcpu ioctl
>  Parameters: struct kvm_guest_debug (in)
>  Returns: 0 on success; -1 on error
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 9e193c8..a0ff410 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -180,6 +180,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>  	case KVM_CAP_ARM_PSCI:
>  	case KVM_CAP_ARM_PSCI_0_2:
>  	case KVM_CAP_READONLY_MEM:
> +	case KVM_CAP_SET_GUEST_DEBUG:

Don't we usually wait with introducing visibility to user space until
you actually do support the ioctl?

>  		r = 1;
>  		break;
>  	case KVM_CAP_COALESCED_MMIO:
> @@ -302,7 +303,34 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>  int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>  					struct kvm_guest_debug *dbg)
>  {
> -	return -EINVAL;
> +	/* If it's not enabled clear all flags */
> +	if (!(dbg->control & KVM_GUESTDBG_ENABLE)) {
> +		vcpu->guest_debug = 0;
> +		return 0;
> +	}
> +
> +	vcpu->guest_debug = dbg->control;
> +	kvm_info("%s: guest_debug is 0x%lx\n", __func__, vcpu->guest_debug);
> +
> +	/* Single Step */
> +	if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
> +		kvm_info("SS requested, not yet implemented\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Software Break Points */
> +	if (vcpu->guest_debug & KVM_GUESTDBG_USE_SW_BP) {
> +		kvm_info("SW BP support requested, not yet implemented\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Hardware assisted Break and Watch points */
> +	if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP) {
> +		kvm_info("HW BP support requested, not yet implemented\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
>  }
>  
>  
> -- 
> 2.1.3
> 

[PATCH 4/7] KVM: arm64: guest debug, add SW break point support

[Alex Bennée]

This adds support for SW breakpoints inserted by userspace.

First we need to trap all BKPT exceptions in the hypervisor (ELS). This
in controlled through the MDCR_EL2 register. I've added a new field to
the vcpu structure to hold this value. There should be scope to
rationlise this with the VCPU_DEBUG_FLAGS/KVM_ARM64_DEBUG_DIRTY_SHIFT
manipulation in later patches.

Once the exception arrives we triggers an exit from the main hypervisor
loop to the hypervisor controller. The kvm_debug_exit_arch carries the
address of the exception. If the controller doesn't know of breakpoint
then we have a guest inserted breakpoint and the hypervisor needs to
start again and deliver the exception to guest.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index 2c6386e..9383359 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -2592,7 +2592,7 @@ when running. Common control bits are:
 The top 16 bits of the control field are architecture specific control
 flags which can include the following:
 
-  - KVM_GUESTDBG_USE_SW_BP:     using software breakpoints [x86]
+  - KVM_GUESTDBG_USE_SW_BP:     using software breakpoints [x86, arm64]
   - KVM_GUESTDBG_USE_HW_BP:     using hardware breakpoints [x86, s390]
   - KVM_GUESTDBG_INJECT_DB:     inject DB type exception [x86]
   - KVM_GUESTDBG_INJECT_BP:     inject BP type exception [x86]
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index a0ff410..48d26bb 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -303,6 +303,9 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
 int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
 					struct kvm_guest_debug *dbg)
 {
+	/* Route debug traps to el2? */
+	bool route_el2 = false;
+
 	/* If it's not enabled clear all flags */
 	if (!(dbg->control & KVM_GUESTDBG_ENABLE)) {
 		vcpu->guest_debug = 0;
@@ -320,8 +323,8 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
 
 	/* Software Break Points */
 	if (vcpu->guest_debug & KVM_GUESTDBG_USE_SW_BP) {
-		kvm_info("SW BP support requested, not yet implemented\n");
-		return -EINVAL;
+		kvm_info("SW BP support requested\n");
+		route_el2 = true;
 	}
 
 	/* Hardware assisted Break and Watch points */
@@ -330,6 +333,20 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
 		return -EINVAL;
 	}
 
+	/* If we are going to handle any debug exceptions we need to
+	 * set MDCE_EL2.TDE to ensure they are routed to the
+	 * hypervisor. If userspace determines the exception was
+	 * actually internal to the guest it needs to handle
+	 * re-injecting the exception.
+	 */
+	if (route_el2) {
+		kvm_info("routing debug exceptions");
+		vcpu->arch.mdcr_el2 = MDCR_EL2_TDE;
+	} else {
+		kvm_info("not routing debug exceptions");
+		vcpu->arch.mdcr_el2 = 0;
+	}
+
 	return 0;
 }
 
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 2012c4b..38b0f07 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -98,6 +98,7 @@ struct kvm_vcpu_arch {
 
 	/* HYP configuration */
 	u64 hcr_el2;
+	u32 mdcr_el2;
 
 	/* Exception Information */
 	struct kvm_vcpu_fault_info fault;
diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
index 9a9fce0..8da1043 100644
--- a/arch/arm64/kernel/asm-offsets.c
+++ b/arch/arm64/kernel/asm-offsets.c
@@ -122,6 +122,7 @@ int main(void)
   DEFINE(VCPU_HPFAR_EL2,	offsetof(struct kvm_vcpu, arch.fault.hpfar_el2));
   DEFINE(VCPU_DEBUG_FLAGS,	offsetof(struct kvm_vcpu, arch.debug_flags));
   DEFINE(VCPU_HCR_EL2,		offsetof(struct kvm_vcpu, arch.hcr_el2));
+  DEFINE(VCPU_MDCR_EL2,	offsetof(struct kvm_vcpu, arch.mdcr_el2));
   DEFINE(VCPU_IRQ_LINES,	offsetof(struct kvm_vcpu, arch.irq_lines));
   DEFINE(VCPU_HOST_CONTEXT,	offsetof(struct kvm_vcpu, arch.host_cpu_context));
   DEFINE(VCPU_TIMER_CNTV_CTL,	offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_ctl));
diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
index 34b8bd0..28dc92b 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -71,6 +71,26 @@ static int kvm_handle_wfx(struct kvm_vcpu *vcpu, struct kvm_run *run)
 	return 1;
 }
 
+/**
+ * kvm_handle_bkpt - handle a break-point instruction
+ *
+ * @vcpu:	the vcpu pointer
+ *
+ * By definition if we arrive here it's because we are routing
+ * all SW breakpoints to the hyper-visor as some may be a result of
+ * guest debugging. If user-space decides this particular break-point
+ * isn't for the host to handle it can re-feed the exception to the
+ * guest.
+ */
+static int kvm_handle_bkpt(struct kvm_vcpu *vcpu, struct kvm_run *run)
+{
+	run->exit_reason = KVM_EXIT_DEBUG;
+	run->debug.arch.exit_type = KVM_DEBUG_EXIT_SW_BKPT;
+	run->debug.arch.address = *vcpu_pc(vcpu);
+	kvm_info("exiting from %llx\n", run->debug.arch.address);
+	return 0;
+}
+
 static exit_handle_fn arm_exit_handlers[] = {
 	[ESR_EL2_EC_WFI]	= kvm_handle_wfx,
 	[ESR_EL2_EC_CP15_32]	= kvm_handle_cp15_32,
@@ -85,6 +105,8 @@ static exit_handle_fn arm_exit_handlers[] = {
 	[ESR_EL2_EC_SYS64]	= kvm_handle_sys_reg,
 	[ESR_EL2_EC_IABT]	= kvm_handle_guest_abort,
 	[ESR_EL2_EC_DABT]	= kvm_handle_guest_abort,
+	[ESR_EL2_EC_BKPT32]	= kvm_handle_bkpt,
+	[ESR_EL2_EC_BRK64]	= kvm_handle_bkpt,
 };
 
 static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu)
diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
index b72aa9f..3c733ea 100644
--- a/arch/arm64/kvm/hyp.S
+++ b/arch/arm64/kvm/hyp.S
@@ -772,6 +772,10 @@
 	orr	x2, x2, #(MDCR_EL2_TPM | MDCR_EL2_TPMCR)
 	orr	x2, x2, #(MDCR_EL2_TDRA | MDCR_EL2_TDOSA)
 
+	// Any other bits (currently TDE)
+	ldr	x3, [x0, #VCPU_MDCR_EL2]
+	orr	x2, x2, x3
+
 	// Check for KVM_ARM64_DEBUG_DIRTY, and set debug to trap
 	// if not dirty.
 	ldr	x3, [x0, #VCPU_DEBUG_FLAGS]
-- 
2.1.3

Re: [PATCH 4/7] KVM: arm64: guest debug, add SW break point support

[Andrew Jones]

On Tue, Nov 25, 2014 at 04:10:02PM +0000, Alex Bennée wrote:
> This adds support for SW breakpoints inserted by userspace.
> 
> First we need to trap all BKPT exceptions in the hypervisor (ELS). This
> in controlled through the MDCR_EL2 register. I've added a new field to
> the vcpu structure to hold this value. There should be scope to
> rationlise this with the VCPU_DEBUG_FLAGS/KVM_ARM64_DEBUG_DIRTY_SHIFT
> manipulation in later patches.

I think we should start using the new mdcr_el2 field everywhere we plan
to within the same series that it is introduced. Otherwise it's hard
to tell if we need an mdcr_el2 field, or if a more generic field would
suffice. We can always translate bits of a more generic field to
mdcr_el2 bits when necessary, but not the reverse.

> 
> Once the exception arrives we triggers an exit from the main hypervisor
s/triggers/trigger/
 
> loop to the hypervisor controller. The kvm_debug_exit_arch carries the
> address of the exception. If the controller doesn't know of breakpoint
                                                             ^ a
> then we have a guest inserted breakpoint and the hypervisor needs to
> start again and deliver the exception to guest.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index 2c6386e..9383359 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -2592,7 +2592,7 @@ when running. Common control bits are:
>  The top 16 bits of the control field are architecture specific control
>  flags which can include the following:
>  
> -  - KVM_GUESTDBG_USE_SW_BP:     using software breakpoints [x86]
> +  - KVM_GUESTDBG_USE_SW_BP:     using software breakpoints [x86, arm64]
>    - KVM_GUESTDBG_USE_HW_BP:     using hardware breakpoints [x86, s390]
>    - KVM_GUESTDBG_INJECT_DB:     inject DB type exception [x86]
>    - KVM_GUESTDBG_INJECT_BP:     inject BP type exception [x86]
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index a0ff410..48d26bb 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -303,6 +303,9 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>  int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>  					struct kvm_guest_debug *dbg)
>  {
> +	/* Route debug traps to el2? */
> +	bool route_el2 = false;
> +
>  	/* If it's not enabled clear all flags */
>  	if (!(dbg->control & KVM_GUESTDBG_ENABLE)) {
>  		vcpu->guest_debug = 0;
> @@ -320,8 +323,8 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>  
>  	/* Software Break Points */
>  	if (vcpu->guest_debug & KVM_GUESTDBG_USE_SW_BP) {
> -		kvm_info("SW BP support requested, not yet implemented\n");
> -		return -EINVAL;
> +		kvm_info("SW BP support requested\n");
> +		route_el2 = true;
>  	}
>  
>  	/* Hardware assisted Break and Watch points */
> @@ -330,6 +333,20 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>  		return -EINVAL;
>  	}
>  
> +	/* If we are going to handle any debug exceptions we need to
> +	 * set MDCE_EL2.TDE to ensure they are routed to the
> +	 * hypervisor. If userspace determines the exception was
> +	 * actually internal to the guest it needs to handle
> +	 * re-injecting the exception.
> +	 */

kernel comment blocks typically start with an empty line, e.g.
/*
 * comment block
 */

> +	if (route_el2) {
> +		kvm_info("routing debug exceptions");
> +		vcpu->arch.mdcr_el2 = MDCR_EL2_TDE;
> +	} else {
> +		kvm_info("not routing debug exceptions");
> +		vcpu->arch.mdcr_el2 = 0;
> +	}

This looks weird because we're only managing some of the mdcr_el2 bits
with the mdcr_el2 field. If we were managing all of them then these
would need to be |= MDCR_EL2_TDE and &= ~SOME_MASK instead. If we never
plan to manage all the bits, then I think that points more towards the
need for a more generic field instead.

> +
>  	return 0;
>  }
>  
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 2012c4b..38b0f07 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -98,6 +98,7 @@ struct kvm_vcpu_arch {
>  
>  	/* HYP configuration */
>  	u64 hcr_el2;
> +	u32 mdcr_el2;
>  
>  	/* Exception Information */
>  	struct kvm_vcpu_fault_info fault;
> diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
> index 9a9fce0..8da1043 100644
> --- a/arch/arm64/kernel/asm-offsets.c
> +++ b/arch/arm64/kernel/asm-offsets.c
> @@ -122,6 +122,7 @@ int main(void)
>    DEFINE(VCPU_HPFAR_EL2,	offsetof(struct kvm_vcpu, arch.fault.hpfar_el2));
>    DEFINE(VCPU_DEBUG_FLAGS,	offsetof(struct kvm_vcpu, arch.debug_flags));
>    DEFINE(VCPU_HCR_EL2,		offsetof(struct kvm_vcpu, arch.hcr_el2));
> +  DEFINE(VCPU_MDCR_EL2,	offsetof(struct kvm_vcpu, arch.mdcr_el2));
>    DEFINE(VCPU_IRQ_LINES,	offsetof(struct kvm_vcpu, arch.irq_lines));
>    DEFINE(VCPU_HOST_CONTEXT,	offsetof(struct kvm_vcpu, arch.host_cpu_context));
>    DEFINE(VCPU_TIMER_CNTV_CTL,	offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_ctl));
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index 34b8bd0..28dc92b 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -71,6 +71,26 @@ static int kvm_handle_wfx(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  	return 1;
>  }
>  
> +/**
> + * kvm_handle_bkpt - handle a break-point instruction
> + *
> + * @vcpu:	the vcpu pointer

I see you inherited this header format from kvm_handle_wfx, which
probably left @run off the input list because it doesn't use it.
We do use it in this handler though, so we should probably list it.

> + *
> + * By definition if we arrive here it's because we are routing
> + * all SW breakpoints to the hyper-visor as some may be a result of
> + * guest debugging. If user-space decides this particular break-point
> + * isn't for the host to handle it can re-feed the exception to the
> + * guest.
> + */
> +static int kvm_handle_bkpt(struct kvm_vcpu *vcpu, struct kvm_run *run)
> +{
> +	run->exit_reason = KVM_EXIT_DEBUG;
> +	run->debug.arch.exit_type = KVM_DEBUG_EXIT_SW_BKPT;
> +	run->debug.arch.address = *vcpu_pc(vcpu);
> +	kvm_info("exiting from %llx\n", run->debug.arch.address);

*Must* get rid of this kvm_info, else log explosion shall occur.

> +	return 0;
> +}
> +
>  static exit_handle_fn arm_exit_handlers[] = {
>  	[ESR_EL2_EC_WFI]	= kvm_handle_wfx,
>  	[ESR_EL2_EC_CP15_32]	= kvm_handle_cp15_32,
> @@ -85,6 +105,8 @@ static exit_handle_fn arm_exit_handlers[] = {
>  	[ESR_EL2_EC_SYS64]	= kvm_handle_sys_reg,
>  	[ESR_EL2_EC_IABT]	= kvm_handle_guest_abort,
>  	[ESR_EL2_EC_DABT]	= kvm_handle_guest_abort,
> +	[ESR_EL2_EC_BKPT32]	= kvm_handle_bkpt,
> +	[ESR_EL2_EC_BRK64]	= kvm_handle_bkpt,
>  };

There appears to be a typo in the ARM ARM. Subsection "Software
Breakpoint Instruction exception" of D1.10.4 says BRK (ESR_EL2_EC_BRK64)
is 0x39, but the table above that has it correctly as 0x3c. (This
comment doesn't really have anything to do with your patch, but I
thought I'd call it out here as I just noticed it while reading that
section for this review.)

>  
>  static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu)
> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
> index b72aa9f..3c733ea 100644
> --- a/arch/arm64/kvm/hyp.S
> +++ b/arch/arm64/kvm/hyp.S
> @@ -772,6 +772,10 @@
>  	orr	x2, x2, #(MDCR_EL2_TPM | MDCR_EL2_TPMCR)
>  	orr	x2, x2, #(MDCR_EL2_TDRA | MDCR_EL2_TDOSA)
>  
> +	// Any other bits (currently TDE)
> +	ldr	x3, [x0, #VCPU_MDCR_EL2]
> +	orr	x2, x2, x3

I've already commented on my opinions on only partially managing
mdcr_el2 bits with the new field.

> +
>  	// Check for KVM_ARM64_DEBUG_DIRTY, and set debug to trap
>  	// if not dirty.
>  	ldr	x3, [x0, #VCPU_DEBUG_FLAGS]
> -- 
> 2.1.3
> 
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 4/7] KVM: arm64: guest debug, add SW break point support

[Peter Maydell]

On 26 November 2014 at 16:07, Andrew Jones <drjones@redhat.com> wrote:
> There appears to be a typo in the ARM ARM. Subsection "Software
> Breakpoint Instruction exception" of D1.10.4 says BRK (ESR_EL2_EC_BRK64)
> is 0x39, but the table above that has it correctly as 0x3c.

Thanks for pointing out this typo -- I've reported it to the
appropriate documentation people.

-- PMM

Re: [PATCH 4/7] KVM: arm64: guest debug, add SW break point support

[Christoffer Dall]

On Wed, Nov 26, 2014 at 05:07:20PM +0100, Andrew Jones wrote:
> On Tue, Nov 25, 2014 at 04:10:02PM +0000, Alex Bennée wrote:
> > This adds support for SW breakpoints inserted by userspace.
> > 
> > First we need to trap all BKPT exceptions in the hypervisor (ELS). This
> > in controlled through the MDCR_EL2 register. I've added a new field to
> > the vcpu structure to hold this value. There should be scope to
> > rationlise this with the VCPU_DEBUG_FLAGS/KVM_ARM64_DEBUG_DIRTY_SHIFT
> > manipulation in later patches.
> 
> I think we should start using the new mdcr_el2 field everywhere we plan
> to within the same series that it is introduced. Otherwise it's hard
> to tell if we need an mdcr_el2 field, or if a more generic field would
> suffice. We can always translate bits of a more generic field to
> mdcr_el2 bits when necessary, but not the reverse.
> 
Agreed, this is getting messy already with this patch.

-Christoffer

Re: [PATCH 4/7] KVM: arm64: guest debug, add SW break point support

[Christoffer Dall]

On Tue, Nov 25, 2014 at 04:10:02PM +0000, Alex Bennée wrote:
> This adds support for SW breakpoints inserted by userspace.
> 
> First we need to trap all BKPT exceptions in the hypervisor (ELS). This
> in controlled through the MDCR_EL2 register. I've added a new field to

this is ?

> the vcpu structure to hold this value. There should be scope to
> rationlise this with the VCPU_DEBUG_FLAGS/KVM_ARM64_DEBUG_DIRTY_SHIFT
> manipulation in later patches.

I don't understand what you mean with rationalising something in later
patches.

> 
> Once the exception arrives we triggers an exit from the main hypervisor
> loop to the hypervisor controller. The kvm_debug_exit_arch carries the

hypervisor controller is userspace?

> address of the exception. If the controller doesn't know of breakpoint

the breakint ?

> then we have a guest inserted breakpoint and the hypervisor needs to
> start again and deliver the exception to guest.

to the guest

> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index 2c6386e..9383359 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -2592,7 +2592,7 @@ when running. Common control bits are:
>  The top 16 bits of the control field are architecture specific control
>  flags which can include the following:
>  
> -  - KVM_GUESTDBG_USE_SW_BP:     using software breakpoints [x86]
> +  - KVM_GUESTDBG_USE_SW_BP:     using software breakpoints [x86, arm64]
>    - KVM_GUESTDBG_USE_HW_BP:     using hardware breakpoints [x86, s390]
>    - KVM_GUESTDBG_INJECT_DB:     inject DB type exception [x86]
>    - KVM_GUESTDBG_INJECT_BP:     inject BP type exception [x86]
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index a0ff410..48d26bb 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -303,6 +303,9 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>  int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>  					struct kvm_guest_debug *dbg)
>  {
> +	/* Route debug traps to el2? */
> +	bool route_el2 = false;
> +
>  	/* If it's not enabled clear all flags */
>  	if (!(dbg->control & KVM_GUESTDBG_ENABLE)) {
>  		vcpu->guest_debug = 0;
> @@ -320,8 +323,8 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>  
>  	/* Software Break Points */
>  	if (vcpu->guest_debug & KVM_GUESTDBG_USE_SW_BP) {
> -		kvm_info("SW BP support requested, not yet implemented\n");
> -		return -EINVAL;
> +		kvm_info("SW BP support requested\n");
> +		route_el2 = true;
>  	}
>  
>  	/* Hardware assisted Break and Watch points */
> @@ -330,6 +333,20 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>  		return -EINVAL;
>  	}
>  
> +	/* If we are going to handle any debug exceptions we need to

wings on the comments

> +	 * set MDCE_EL2.TDE to ensure they are routed to the
> +	 * hypervisor. If userspace determines the exception was
> +	 * actually internal to the guest it needs to handle
> +	 * re-injecting the exception.
> +	 */
> +	if (route_el2) {
> +		kvm_info("routing debug exceptions");
> +		vcpu->arch.mdcr_el2 = MDCR_EL2_TDE;
> +	} else {
> +		kvm_info("not routing debug exceptions");
> +		vcpu->arch.mdcr_el2 = 0;
> +	}
> +
>  	return 0;
>  }
>  
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 2012c4b..38b0f07 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -98,6 +98,7 @@ struct kvm_vcpu_arch {
>  
>  	/* HYP configuration */
>  	u64 hcr_el2;
> +	u32 mdcr_el2;
>  
>  	/* Exception Information */
>  	struct kvm_vcpu_fault_info fault;
> diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
> index 9a9fce0..8da1043 100644
> --- a/arch/arm64/kernel/asm-offsets.c
> +++ b/arch/arm64/kernel/asm-offsets.c
> @@ -122,6 +122,7 @@ int main(void)
>    DEFINE(VCPU_HPFAR_EL2,	offsetof(struct kvm_vcpu, arch.fault.hpfar_el2));
>    DEFINE(VCPU_DEBUG_FLAGS,	offsetof(struct kvm_vcpu, arch.debug_flags));
>    DEFINE(VCPU_HCR_EL2,		offsetof(struct kvm_vcpu, arch.hcr_el2));
> +  DEFINE(VCPU_MDCR_EL2,	offsetof(struct kvm_vcpu, arch.mdcr_el2));
>    DEFINE(VCPU_IRQ_LINES,	offsetof(struct kvm_vcpu, arch.irq_lines));
>    DEFINE(VCPU_HOST_CONTEXT,	offsetof(struct kvm_vcpu, arch.host_cpu_context));
>    DEFINE(VCPU_TIMER_CNTV_CTL,	offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_ctl));
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index 34b8bd0..28dc92b 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -71,6 +71,26 @@ static int kvm_handle_wfx(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  	return 1;
>  }
>  
> +/**
> + * kvm_handle_bkpt - handle a break-point instruction
> + *
> + * @vcpu:	the vcpu pointer
> + *
> + * By definition if we arrive here it's because we are routing
> + * all SW breakpoints to the hyper-visor as some may be a result of

s/hyper-visor/hypervisor/

this sentence is weird. I think what you want is a full stop after
hypervisor and then a new sentence: Userspace may decide that this
particular breakpoint should be routed to the guest (if the breakpoint
does not come form userspace but by someone debugging a process inside
the guest), and in that case inject a software breakpoint instruction by
<insert the method here>.



> + * guest debugging. If user-space decides this particular break-point
> + * isn't for the host to handle it can re-feed the exception to the
> + * guest.
> + */
> +static int kvm_handle_bkpt(struct kvm_vcpu *vcpu, struct kvm_run *run)
> +{
> +	run->exit_reason = KVM_EXIT_DEBUG;
> +	run->debug.arch.exit_type = KVM_DEBUG_EXIT_SW_BKPT;
> +	run->debug.arch.address = *vcpu_pc(vcpu);
> +	kvm_info("exiting from %llx\n", run->debug.arch.address);
> +	return 0;
> +}
> +
>  static exit_handle_fn arm_exit_handlers[] = {
>  	[ESR_EL2_EC_WFI]	= kvm_handle_wfx,
>  	[ESR_EL2_EC_CP15_32]	= kvm_handle_cp15_32,
> @@ -85,6 +105,8 @@ static exit_handle_fn arm_exit_handlers[] = {
>  	[ESR_EL2_EC_SYS64]	= kvm_handle_sys_reg,
>  	[ESR_EL2_EC_IABT]	= kvm_handle_guest_abort,
>  	[ESR_EL2_EC_DABT]	= kvm_handle_guest_abort,
> +	[ESR_EL2_EC_BKPT32]	= kvm_handle_bkpt,
> +	[ESR_EL2_EC_BRK64]	= kvm_handle_bkpt,
>  };
>  
>  static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu)
> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
> index b72aa9f..3c733ea 100644
> --- a/arch/arm64/kvm/hyp.S
> +++ b/arch/arm64/kvm/hyp.S
> @@ -772,6 +772,10 @@
>  	orr	x2, x2, #(MDCR_EL2_TPM | MDCR_EL2_TPMCR)
>  	orr	x2, x2, #(MDCR_EL2_TDRA | MDCR_EL2_TDOSA)
>  
> +	// Any other bits (currently TDE)
> +	ldr	x3, [x0, #VCPU_MDCR_EL2]
> +	orr	x2, x2, x3
> +
>  	// Check for KVM_ARM64_DEBUG_DIRTY, and set debug to trap
>  	// if not dirty.
>  	ldr	x3, [x0, #VCPU_DEBUG_FLAGS]
> -- 
> 2.1.3
> 

It may be simpler to just load/restore the MDCR_EL2 and do all
bit-manipulations for a specific VCPU in C-code instead of this.

-Christoffer

Re: [PATCH 4/7] KVM: arm64: guest debug, add SW break point support

[Alex Bennée]

Christoffer Dall <christoffer.dall@linaro.org> writes:

> On Tue, Nov 25, 2014 at 04:10:02PM +0000, Alex Bennée wrote:
>> This adds support for SW breakpoints inserted by userspace.
>> 
>> First we need to trap all BKPT exceptions in the hypervisor (ELS). This
>> in controlled through the MDCR_EL2 register. I've added a new field to
>
> this is ?
>
>> the vcpu structure to hold this value. There should be scope to
>> rationlise this with the VCPU_DEBUG_FLAGS/KVM_ARM64_DEBUG_DIRTY_SHIFT
>> manipulation in later patches.
>
> I don't understand what you mean with rationalising something in later
> patches.

I was pointing to potential future improvements. I'm wary of touching
the lazy register syncing code in this patch set as it greatly increases
the scope of testing. However I guess I'll have to bite the bullet and
merge it in a sensible way.

>
>> 
>> Once the exception arrives we triggers an exit from the main hypervisor
>> loop to the hypervisor controller. The kvm_debug_exit_arch carries the
>
> hypervisor controller is userspace?

Yeah sorry that one sneaked through.

>
>> address of the exception. If the controller doesn't know of breakpoint
>
> the breakint ?
>
>> then we have a guest inserted breakpoint and the hypervisor needs to
>> start again and deliver the exception to guest.
>
> to the guest
>
>> 
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> 
>> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
>> index 2c6386e..9383359 100644
>> --- a/Documentation/virtual/kvm/api.txt
>> +++ b/Documentation/virtual/kvm/api.txt
>> @@ -2592,7 +2592,7 @@ when running. Common control bits are:
>>  The top 16 bits of the control field are architecture specific control
>>  flags which can include the following:
>>  
>> -  - KVM_GUESTDBG_USE_SW_BP:     using software breakpoints [x86]
>> +  - KVM_GUESTDBG_USE_SW_BP:     using software breakpoints [x86, arm64]
>>    - KVM_GUESTDBG_USE_HW_BP:     using hardware breakpoints [x86, s390]
>>    - KVM_GUESTDBG_INJECT_DB:     inject DB type exception [x86]
>>    - KVM_GUESTDBG_INJECT_BP:     inject BP type exception [x86]
>> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
>> index a0ff410..48d26bb 100644
>> --- a/arch/arm/kvm/arm.c
>> +++ b/arch/arm/kvm/arm.c
>> @@ -303,6 +303,9 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>>  int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>>  					struct kvm_guest_debug *dbg)
>>  {
>> +	/* Route debug traps to el2? */
>> +	bool route_el2 = false;
>> +
>>  	/* If it's not enabled clear all flags */
>>  	if (!(dbg->control & KVM_GUESTDBG_ENABLE)) {
>>  		vcpu->guest_debug = 0;
>> @@ -320,8 +323,8 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>>  
>>  	/* Software Break Points */
>>  	if (vcpu->guest_debug & KVM_GUESTDBG_USE_SW_BP) {
>> -		kvm_info("SW BP support requested, not yet implemented\n");
>> -		return -EINVAL;
>> +		kvm_info("SW BP support requested\n");
>> +		route_el2 = true;
>>  	}
>>  
>>  	/* Hardware assisted Break and Watch points */
>> @@ -330,6 +333,20 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>>  		return -EINVAL;
>>  	}
>>  
>> +	/* If we are going to handle any debug exceptions we need to
>
> wings on the comments
>
>> +	 * set MDCE_EL2.TDE to ensure they are routed to the
>> +	 * hypervisor. If userspace determines the exception was
>> +	 * actually internal to the guest it needs to handle
>> +	 * re-injecting the exception.
>> +	 */
>> +	if (route_el2) {
>> +		kvm_info("routing debug exceptions");
>> +		vcpu->arch.mdcr_el2 = MDCR_EL2_TDE;
>> +	} else {
>> +		kvm_info("not routing debug exceptions");
>> +		vcpu->arch.mdcr_el2 = 0;
>> +	}
>> +
>>  	return 0;
>>  }
>>  
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index 2012c4b..38b0f07 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -98,6 +98,7 @@ struct kvm_vcpu_arch {
>>  
>>  	/* HYP configuration */
>>  	u64 hcr_el2;
>> +	u32 mdcr_el2;
>>  
>>  	/* Exception Information */
>>  	struct kvm_vcpu_fault_info fault;
>> diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
>> index 9a9fce0..8da1043 100644
>> --- a/arch/arm64/kernel/asm-offsets.c
>> +++ b/arch/arm64/kernel/asm-offsets.c
>> @@ -122,6 +122,7 @@ int main(void)
>>    DEFINE(VCPU_HPFAR_EL2,	offsetof(struct kvm_vcpu, arch.fault.hpfar_el2));
>>    DEFINE(VCPU_DEBUG_FLAGS,	offsetof(struct kvm_vcpu, arch.debug_flags));
>>    DEFINE(VCPU_HCR_EL2,		offsetof(struct kvm_vcpu, arch.hcr_el2));
>> +  DEFINE(VCPU_MDCR_EL2,	offsetof(struct kvm_vcpu, arch.mdcr_el2));
>>    DEFINE(VCPU_IRQ_LINES,	offsetof(struct kvm_vcpu, arch.irq_lines));
>>    DEFINE(VCPU_HOST_CONTEXT,	offsetof(struct kvm_vcpu, arch.host_cpu_context));
>>    DEFINE(VCPU_TIMER_CNTV_CTL,	offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_ctl));
>> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
>> index 34b8bd0..28dc92b 100644
>> --- a/arch/arm64/kvm/handle_exit.c
>> +++ b/arch/arm64/kvm/handle_exit.c
>> @@ -71,6 +71,26 @@ static int kvm_handle_wfx(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>  	return 1;
>>  }
>>  
>> +/**
>> + * kvm_handle_bkpt - handle a break-point instruction
>> + *
>> + * @vcpu:	the vcpu pointer
>> + *
>> + * By definition if we arrive here it's because we are routing
>> + * all SW breakpoints to the hyper-visor as some may be a result of
>
> s/hyper-visor/hypervisor/
>
> this sentence is weird. I think what you want is a full stop after
> hypervisor and then a new sentence: Userspace may decide that this
> particular breakpoint should be routed to the guest (if the breakpoint
> does not come form userspace but by someone debugging a process inside
> the guest), and in that case inject a software breakpoint instruction by
> <insert the method here>.
>
>
>
>> + * guest debugging. If user-space decides this particular break-point
>> + * isn't for the host to handle it can re-feed the exception to the
>> + * guest.
>> + */
>> +static int kvm_handle_bkpt(struct kvm_vcpu *vcpu, struct kvm_run *run)
>> +{
>> +	run->exit_reason = KVM_EXIT_DEBUG;
>> +	run->debug.arch.exit_type = KVM_DEBUG_EXIT_SW_BKPT;
>> +	run->debug.arch.address = *vcpu_pc(vcpu);
>> +	kvm_info("exiting from %llx\n", run->debug.arch.address);
>> +	return 0;
>> +}
>> +
>>  static exit_handle_fn arm_exit_handlers[] = {
>>  	[ESR_EL2_EC_WFI]	= kvm_handle_wfx,
>>  	[ESR_EL2_EC_CP15_32]	= kvm_handle_cp15_32,
>> @@ -85,6 +105,8 @@ static exit_handle_fn arm_exit_handlers[] = {
>>  	[ESR_EL2_EC_SYS64]	= kvm_handle_sys_reg,
>>  	[ESR_EL2_EC_IABT]	= kvm_handle_guest_abort,
>>  	[ESR_EL2_EC_DABT]	= kvm_handle_guest_abort,
>> +	[ESR_EL2_EC_BKPT32]	= kvm_handle_bkpt,
>> +	[ESR_EL2_EC_BRK64]	= kvm_handle_bkpt,
>>  };
>>  
>>  static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu)
>> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
>> index b72aa9f..3c733ea 100644
>> --- a/arch/arm64/kvm/hyp.S
>> +++ b/arch/arm64/kvm/hyp.S
>> @@ -772,6 +772,10 @@
>>  	orr	x2, x2, #(MDCR_EL2_TPM | MDCR_EL2_TPMCR)
>>  	orr	x2, x2, #(MDCR_EL2_TDRA | MDCR_EL2_TDOSA)
>>  
>> +	// Any other bits (currently TDE)
>> +	ldr	x3, [x0, #VCPU_MDCR_EL2]
>> +	orr	x2, x2, x3
>> +
>>  	// Check for KVM_ARM64_DEBUG_DIRTY, and set debug to trap
>>  	// if not dirty.
>>  	ldr	x3, [x0, #VCPU_DEBUG_FLAGS]
>> -- 
>> 2.1.3
>> 
>
> It may be simpler to just load/restore the MDCR_EL2 and do all
> bit-manipulations for a specific VCPU in C-code instead of this.
>
> -Christoffer

-- 
Alex Bennée

[PATCH 5/7] KVM: arm64: guest debug, add support for single-step

[Alex Bennée]

This adds support for single-stepping the guest. As userspace can and
will manipulate guest registers before restarting any tweaking of the
registers has to occur just before control is passed back to the guest.
Furthermore while guest debugging is in effect we need to squash the
ability of the guest to single-step itself as we have no easy way of
re-entering the guest after the exception has been delivered to the
hypervisor.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 48d26bb..a76daae 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -38,6 +38,7 @@
 #include <asm/tlbflush.h>
 #include <asm/cacheflush.h>
 #include <asm/virt.h>
+#include <asm/debug-monitors.h>
 #include <asm/kvm_arm.h>
 #include <asm/kvm_asm.h>
 #include <asm/kvm_mmu.h>
@@ -300,6 +301,17 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
 	kvm_arm_set_running_vcpu(NULL);
 }
 
+/**
+ * kvm_arch_vcpu_ioctl_set_guest_debug - Setup guest debugging
+ * @kvm:	pointer to the KVM struct
+ * @kvm_guest_debug: the ioctl data buffer
+ *
+ * This sets up the VM for guest debugging. Care has to be taken when
+ * manipulating guest registers as these will be set/cleared by the
+ * hyper-visor controller, typically before each kvm_run event. As a
+ * result modification of the guest registers needs to take place
+ * after they have been restored in the hyp.S trampoline code.
+ */
 int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
 					struct kvm_guest_debug *dbg)
 {
@@ -317,8 +329,8 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
 
 	/* Single Step */
 	if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
-		kvm_info("SS requested, not yet implemented\n");
-		return -EINVAL;
+		kvm_info("SS requested\n");
+		route_el2 = true;
 	}
 
 	/* Software Break Points */
diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
index 8da1043..78e5ae1 100644
--- a/arch/arm64/kernel/asm-offsets.c
+++ b/arch/arm64/kernel/asm-offsets.c
@@ -121,6 +121,7 @@ int main(void)
   DEFINE(VCPU_FAR_EL2,		offsetof(struct kvm_vcpu, arch.fault.far_el2));
   DEFINE(VCPU_HPFAR_EL2,	offsetof(struct kvm_vcpu, arch.fault.hpfar_el2));
   DEFINE(VCPU_DEBUG_FLAGS,	offsetof(struct kvm_vcpu, arch.debug_flags));
+  DEFINE(GUEST_DEBUG,		offsetof(struct kvm_vcpu, guest_debug));
   DEFINE(VCPU_HCR_EL2,		offsetof(struct kvm_vcpu, arch.hcr_el2));
   DEFINE(VCPU_MDCR_EL2,	offsetof(struct kvm_vcpu, arch.mdcr_el2));
   DEFINE(VCPU_IRQ_LINES,	offsetof(struct kvm_vcpu, arch.irq_lines));
diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
index 28dc92b..6def054 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -91,6 +91,25 @@ static int kvm_handle_bkpt(struct kvm_vcpu *vcpu, struct kvm_run *run)
 	return 0;
 }
 
+/**
+ * kvm_handle_ss - handle single step exceptions
+ *
+ * @vcpu:	the vcpu pointer
+ *
+ * See: ARM ARM D2.12 for the details. While the host is routing debug
+ * exceptions to it's handlers we have to suppress the ability of the
+ * guest to trigger exceptions.
+ */
+static int kvm_handle_ss(struct kvm_vcpu *vcpu, struct kvm_run *run)
+{
+	WARN_ON(!(vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP));
+
+	run->exit_reason = KVM_EXIT_DEBUG;
+	run->debug.arch.exit_type = KVM_DEBUG_EXIT_SINGLE_STEP;
+	run->debug.arch.address = *vcpu_pc(vcpu);
+	return 0;
+}
+
 static exit_handle_fn arm_exit_handlers[] = {
 	[ESR_EL2_EC_WFI]	= kvm_handle_wfx,
 	[ESR_EL2_EC_CP15_32]	= kvm_handle_cp15_32,
@@ -105,6 +124,7 @@ static exit_handle_fn arm_exit_handlers[] = {
 	[ESR_EL2_EC_SYS64]	= kvm_handle_sys_reg,
 	[ESR_EL2_EC_IABT]	= kvm_handle_guest_abort,
 	[ESR_EL2_EC_DABT]	= kvm_handle_guest_abort,
+	[ESR_EL2_EC_SOFTSTP]    = kvm_handle_ss,
 	[ESR_EL2_EC_BKPT32]	= kvm_handle_bkpt,
 	[ESR_EL2_EC_BRK64]	= kvm_handle_bkpt,
 };
diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
index 3c733ea..c0bc218 100644
--- a/arch/arm64/kvm/hyp.S
+++ b/arch/arm64/kvm/hyp.S
@@ -16,6 +16,7 @@
  */
 
 #include <linux/linkage.h>
+#include <linux/kvm.h>
 
 #include <asm/assembler.h>
 #include <asm/memory.h>
@@ -168,6 +169,31 @@
 	// x19-x29, lr, sp*, elr*, spsr*
 	restore_common_regs
 
+	// After restoring the guest registers but before we return to the guest
+	// we may want to make some final tweaks to support guest debugging.
+	ldr	x3, [x0, #GUEST_DEBUG]
+	tbz	x3, #KVM_GUESTDBG_ENABLE_SHIFT, 2f	// No guest debug
+
+	// x0 - preserved as VCPU ptr
+	// x1 - spsr
+	// x2 - mdscr
+	mrs	x1, spsr_el2
+	mrs 	x2, mdscr_el1
+
+	// See ARM ARM D2.12.3 The software step state machine
+	// If we are doing Single Step - set MDSCR_EL1.SS and PSTATE.SS
+	orr	x1, x1, #DBG_SPSR_SS
+	orr	x2, x2, #DBG_MDSCR_SS
+	tbnz	x3, #KVM_GUESTDBG_SINGLESTEP_SHIFT, 1f
+	// If we are not doing Single Step we want to prevent the guest doing so
+	// as otherwise we will have to deal with the re-routed exceptions as we
+	// are doing other guest debug related things
+	eor	x1, x1, #DBG_SPSR_SS
+	eor	x2, x2, #DBG_MDSCR_SS
+1:
+	msr	spsr_el2, x1
+	msr	mdscr_el1, x2
+2:
 	// Last bits of the 64bit state
 	pop	x2, x3
 	pop	x0, x1
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 523f476..347e5b0 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -7,6 +7,8 @@
  * Note: you must update KVM_API_VERSION if you change this interface.
  */
 
+#ifndef __ASSEMBLY__
+
 #include <linux/types.h>
 #include <linux/compiler.h>
 #include <linux/ioctl.h>
@@ -515,11 +517,6 @@ struct kvm_s390_irq {
 	} u;
 };
 
-/* for KVM_SET_GUEST_DEBUG */
-
-#define KVM_GUESTDBG_ENABLE		0x00000001
-#define KVM_GUESTDBG_SINGLESTEP		0x00000002
-
 struct kvm_guest_debug {
 	__u32 control;
 	__u32 pad;
@@ -1189,4 +1186,15 @@ struct kvm_assigned_msix_entry {
 	__u16 padding[3];
 };
 
+#endif /* __ASSEMBLY__ */
+
+/* for KVM_SET_GUEST_DEBUG */
+
+#define KVM_GUESTDBG_ENABLE_SHIFT	0
+#define KVM_GUESTDBG_ENABLE		(1 << KVM_GUESTDBG_ENABLE_SHIFT)
+#define KVM_GUESTDBG_SINGLESTEP_SHIFT	1
+#define KVM_GUESTDBG_SINGLESTEP	(1 << KVM_GUESTDBG_SINGLESTEP_SHIFT)
+
+
+
 #endif /* __LINUX_KVM_H */
-- 
2.1.3

Re: [PATCH 5/7] KVM: arm64: guest debug, add support for single-step

[Andrew Jones]

On Tue, Nov 25, 2014 at 04:10:03PM +0000, Alex Bennée wrote:
> This adds support for single-stepping the guest. As userspace can and
> will manipulate guest registers before restarting any tweaking of the
> registers has to occur just before control is passed back to the guest.
> Furthermore while guest debugging is in effect we need to squash the
> ability of the guest to single-step itself as we have no easy way of
> re-entering the guest after the exception has been delivered to the
> hypervisor.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 48d26bb..a76daae 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -38,6 +38,7 @@
>  #include <asm/tlbflush.h>
>  #include <asm/cacheflush.h>
>  #include <asm/virt.h>
> +#include <asm/debug-monitors.h>
>  #include <asm/kvm_arm.h>
>  #include <asm/kvm_asm.h>
>  #include <asm/kvm_mmu.h>
> @@ -300,6 +301,17 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>  	kvm_arm_set_running_vcpu(NULL);
>  }
>  
> +/**
> + * kvm_arch_vcpu_ioctl_set_guest_debug - Setup guest debugging
> + * @kvm:	pointer to the KVM struct
> + * @kvm_guest_debug: the ioctl data buffer
> + *
> + * This sets up the VM for guest debugging. Care has to be taken when
> + * manipulating guest registers as these will be set/cleared by the
> + * hyper-visor controller, typically before each kvm_run event. As a
> + * result modification of the guest registers needs to take place
> + * after they have been restored in the hyp.S trampoline code.
> + */
>  int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>  					struct kvm_guest_debug *dbg)
>  {
> @@ -317,8 +329,8 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>  
>  	/* Single Step */
>  	if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
> -		kvm_info("SS requested, not yet implemented\n");
> -		return -EINVAL;
> +		kvm_info("SS requested\n");
> +		route_el2 = true;
>  	}
>  
>  	/* Software Break Points */
> diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
> index 8da1043..78e5ae1 100644
> --- a/arch/arm64/kernel/asm-offsets.c
> +++ b/arch/arm64/kernel/asm-offsets.c
> @@ -121,6 +121,7 @@ int main(void)
>    DEFINE(VCPU_FAR_EL2,		offsetof(struct kvm_vcpu, arch.fault.far_el2));
>    DEFINE(VCPU_HPFAR_EL2,	offsetof(struct kvm_vcpu, arch.fault.hpfar_el2));
>    DEFINE(VCPU_DEBUG_FLAGS,	offsetof(struct kvm_vcpu, arch.debug_flags));
> +  DEFINE(GUEST_DEBUG,		offsetof(struct kvm_vcpu, guest_debug));
>    DEFINE(VCPU_HCR_EL2,		offsetof(struct kvm_vcpu, arch.hcr_el2));
>    DEFINE(VCPU_MDCR_EL2,	offsetof(struct kvm_vcpu, arch.mdcr_el2));
>    DEFINE(VCPU_IRQ_LINES,	offsetof(struct kvm_vcpu, arch.irq_lines));
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index 28dc92b..6def054 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -91,6 +91,25 @@ static int kvm_handle_bkpt(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  	return 0;
>  }
>  
> +/**
> + * kvm_handle_ss - handle single step exceptions
> + *
> + * @vcpu:	the vcpu pointer

same @run comment as other handler header in previous patch

> + *
> + * See: ARM ARM D2.12 for the details. While the host is routing debug
> + * exceptions to it's handlers we have to suppress the ability of the
> + * guest to trigger exceptions.
> + */
> +static int kvm_handle_ss(struct kvm_vcpu *vcpu, struct kvm_run *run)
> +{
> +	WARN_ON(!(vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP));

I'm not sure about this WARN_ON. Is there some scenario you were
thinking of when you put it here? Is there some scenario where this
could trigger so frequently we kill the log buffer?

> +
> +	run->exit_reason = KVM_EXIT_DEBUG;
> +	run->debug.arch.exit_type = KVM_DEBUG_EXIT_SINGLE_STEP;
> +	run->debug.arch.address = *vcpu_pc(vcpu);
> +	return 0;
> +}
> +
>  static exit_handle_fn arm_exit_handlers[] = {
>  	[ESR_EL2_EC_WFI]	= kvm_handle_wfx,
>  	[ESR_EL2_EC_CP15_32]	= kvm_handle_cp15_32,
> @@ -105,6 +124,7 @@ static exit_handle_fn arm_exit_handlers[] = {
>  	[ESR_EL2_EC_SYS64]	= kvm_handle_sys_reg,
>  	[ESR_EL2_EC_IABT]	= kvm_handle_guest_abort,
>  	[ESR_EL2_EC_DABT]	= kvm_handle_guest_abort,
> +	[ESR_EL2_EC_SOFTSTP]    = kvm_handle_ss,
>  	[ESR_EL2_EC_BKPT32]	= kvm_handle_bkpt,
>  	[ESR_EL2_EC_BRK64]	= kvm_handle_bkpt,
>  };
> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
> index 3c733ea..c0bc218 100644
> --- a/arch/arm64/kvm/hyp.S
> +++ b/arch/arm64/kvm/hyp.S
> @@ -16,6 +16,7 @@
>   */
>  
>  #include <linux/linkage.h>
> +#include <linux/kvm.h>
>  
>  #include <asm/assembler.h>
>  #include <asm/memory.h>
> @@ -168,6 +169,31 @@
>  	// x19-x29, lr, sp*, elr*, spsr*
>  	restore_common_regs
>  
> +	// After restoring the guest registers but before we return to the guest
> +	// we may want to make some final tweaks to support guest debugging.
> +	ldr	x3, [x0, #GUEST_DEBUG]
> +	tbz	x3, #KVM_GUESTDBG_ENABLE_SHIFT, 2f	// No guest debug
> +
> +	// x0 - preserved as VCPU ptr
> +	// x1 - spsr
> +	// x2 - mdscr
> +	mrs	x1, spsr_el2
> +	mrs 	x2, mdscr_el1
> +
> +	// See ARM ARM D2.12.3 The software step state machine
> +	// If we are doing Single Step - set MDSCR_EL1.SS and PSTATE.SS
> +	orr	x1, x1, #DBG_SPSR_SS
> +	orr	x2, x2, #DBG_MDSCR_SS
> +	tbnz	x3, #KVM_GUESTDBG_SINGLESTEP_SHIFT, 1f
> +	// If we are not doing Single Step we want to prevent the guest doing so
> +	// as otherwise we will have to deal with the re-routed exceptions as we
> +	// are doing other guest debug related things
> +	eor	x1, x1, #DBG_SPSR_SS
> +	eor	x2, x2, #DBG_MDSCR_SS
> +1:
> +	msr	spsr_el2, x1
> +	msr	mdscr_el1, x2
> +2:
>  	// Last bits of the 64bit state
>  	pop	x2, x3
>  	pop	x0, x1
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 523f476..347e5b0 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -7,6 +7,8 @@
>   * Note: you must update KVM_API_VERSION if you change this interface.
>   */
>  
> +#ifndef __ASSEMBLY__
> +
>  #include <linux/types.h>
>  #include <linux/compiler.h>
>  #include <linux/ioctl.h>
> @@ -515,11 +517,6 @@ struct kvm_s390_irq {
>  	} u;
>  };
>  
> -/* for KVM_SET_GUEST_DEBUG */
> -
> -#define KVM_GUESTDBG_ENABLE		0x00000001
> -#define KVM_GUESTDBG_SINGLESTEP		0x00000002
> -
>  struct kvm_guest_debug {
>  	__u32 control;
>  	__u32 pad;
> @@ -1189,4 +1186,15 @@ struct kvm_assigned_msix_entry {
>  	__u16 padding[3];
>  };
>  
> +#endif /* __ASSEMBLY__ */
> +
> +/* for KVM_SET_GUEST_DEBUG */
> +
> +#define KVM_GUESTDBG_ENABLE_SHIFT	0
> +#define KVM_GUESTDBG_ENABLE		(1 << KVM_GUESTDBG_ENABLE_SHIFT)
> +#define KVM_GUESTDBG_SINGLESTEP_SHIFT	1
> +#define KVM_GUESTDBG_SINGLESTEP	(1 << KVM_GUESTDBG_SINGLESTEP_SHIFT)

EALIGN: we can tab these defines up better

> +
> +
> +
>  #endif /* __LINUX_KVM_H */
> -- 
> 2.1.3
> 
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 5/7] KVM: arm64: guest debug, add support for single-step

[Alex Bennée]

Andrew Jones <drjones@redhat.com> writes:

> On Tue, Nov 25, 2014 at 04:10:03PM +0000, Alex Bennée wrote:
>> This adds support for single-stepping the guest. As userspace can and
>> will manipulate guest registers before restarting any tweaking of the
>> registers has to occur just before control is passed back to the guest.
>> Furthermore while guest debugging is in effect we need to squash the
>> ability of the guest to single-step itself as we have no easy way of
>> re-entering the guest after the exception has been delivered to the
>> hypervisor.
>> 
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> 
>> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
>> index 48d26bb..a76daae 100644
>> --- a/arch/arm/kvm/arm.c
>> +++ b/arch/arm/kvm/arm.c
>> @@ -38,6 +38,7 @@
>>  #include <asm/tlbflush.h>
>>  #include <asm/cacheflush.h>
>>  #include <asm/virt.h>
>> +#include <asm/debug-monitors.h>
>>  #include <asm/kvm_arm.h>
>>  #include <asm/kvm_asm.h>
>>  #include <asm/kvm_mmu.h>
>> @@ -300,6 +301,17 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>>  	kvm_arm_set_running_vcpu(NULL);
>>  }
>>  
>> +/**
>> + * kvm_arch_vcpu_ioctl_set_guest_debug - Setup guest debugging
>> + * @kvm:	pointer to the KVM struct
>> + * @kvm_guest_debug: the ioctl data buffer
>> + *
>> + * This sets up the VM for guest debugging. Care has to be taken when
>> + * manipulating guest registers as these will be set/cleared by the
>> + * hyper-visor controller, typically before each kvm_run event. As a
>> + * result modification of the guest registers needs to take place
>> + * after they have been restored in the hyp.S trampoline code.
>> + */
>>  int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>>  					struct kvm_guest_debug *dbg)
>>  {
>> @@ -317,8 +329,8 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>>  
>>  	/* Single Step */
>>  	if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
>> -		kvm_info("SS requested, not yet implemented\n");
>> -		return -EINVAL;
>> +		kvm_info("SS requested\n");
>> +		route_el2 = true;
>>  	}
>>  
>>  	/* Software Break Points */
>> diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
>> index 8da1043..78e5ae1 100644
>> --- a/arch/arm64/kernel/asm-offsets.c
>> +++ b/arch/arm64/kernel/asm-offsets.c
>> @@ -121,6 +121,7 @@ int main(void)
>>    DEFINE(VCPU_FAR_EL2,		offsetof(struct kvm_vcpu, arch.fault.far_el2));
>>    DEFINE(VCPU_HPFAR_EL2,	offsetof(struct kvm_vcpu, arch.fault.hpfar_el2));
>>    DEFINE(VCPU_DEBUG_FLAGS,	offsetof(struct kvm_vcpu, arch.debug_flags));
>> +  DEFINE(GUEST_DEBUG,		offsetof(struct kvm_vcpu, guest_debug));
>>    DEFINE(VCPU_HCR_EL2,		offsetof(struct kvm_vcpu, arch.hcr_el2));
>>    DEFINE(VCPU_MDCR_EL2,	offsetof(struct kvm_vcpu, arch.mdcr_el2));
>>    DEFINE(VCPU_IRQ_LINES,	offsetof(struct kvm_vcpu, arch.irq_lines));
>> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
>> index 28dc92b..6def054 100644
>> --- a/arch/arm64/kvm/handle_exit.c
>> +++ b/arch/arm64/kvm/handle_exit.c
>> @@ -91,6 +91,25 @@ static int kvm_handle_bkpt(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>  	return 0;
>>  }
>>  
>> +/**
>> + * kvm_handle_ss - handle single step exceptions
>> + *
>> + * @vcpu:	the vcpu pointer
>
> same @run comment as other handler header in previous patch

Yeah I think I'll be merging them all together given the comments about
passing syndrome info directly.

>> + *
>> + * See: ARM ARM D2.12 for the details. While the host is routing debug
>> + * exceptions to it's handlers we have to suppress the ability of the
>> + * guest to trigger exceptions.
>> + */
>> +static int kvm_handle_ss(struct kvm_vcpu *vcpu, struct kvm_run *run)
>> +{
>> +	WARN_ON(!(vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP));
>
> I'm not sure about this WARN_ON. Is there some scenario you were
> thinking of when you put it here? Is there some scenario where this
> could trigger so frequently we kill the log buffer?

The main one I had in mind was not suppressing the guest's attempt to
step while guest debugging was running.

<snip>
>>  
>> -/* for KVM_SET_GUEST_DEBUG */
>> -
>> -#define KVM_GUESTDBG_ENABLE		0x00000001
>> -#define KVM_GUESTDBG_SINGLESTEP		0x00000002
>> -
>>  struct kvm_guest_debug {
>>  	__u32 control;
>>  	__u32 pad;
>> @@ -1189,4 +1186,15 @@ struct kvm_assigned_msix_entry {
>>  	__u16 padding[3];
>>  };
>>  
>> +#endif /* __ASSEMBLY__ */
>> +
>> +/* for KVM_SET_GUEST_DEBUG */
>> +
>> +#define KVM_GUESTDBG_ENABLE_SHIFT	0
>> +#define KVM_GUESTDBG_ENABLE		(1 << KVM_GUESTDBG_ENABLE_SHIFT)
>> +#define KVM_GUESTDBG_SINGLESTEP_SHIFT	1
>> +#define KVM_GUESTDBG_SINGLESTEP	(1 << KVM_GUESTDBG_SINGLESTEP_SHIFT)
>
> EALIGN: we can tab these defines up better

Sure, I'll clean those up.

-- 
Alex Bennée

Re: [PATCH 5/7] KVM: arm64: guest debug, add support for single-step

[Peter Maydell]

On 25 November 2014 at 16:10, Alex Bennée <alex.bennee@linaro.org> wrote:
> This adds support for single-stepping the guest. As userspace can and
> will manipulate guest registers before restarting any tweaking of the
> registers has to occur just before control is passed back to the guest.
> Furthermore while guest debugging is in effect we need to squash the
> ability of the guest to single-step itself as we have no easy way of
> re-entering the guest after the exception has been delivered to the
> hypervisor.

A corner case I don't think this patch handles: if the debugger
tries to single step an insn which is emulated by the
hypervisor (because it's a load/store which is trapped and
handled as emulated mmio in userspace) then we won't
correctly update the single-step state machine (and so we'll end
up incorrectly stopping after the following insn rather than
before, I think).

You should be able to achieve this effect by simply always clearing
the guest's PSTATE.SS when you advance the PC to skip the emulated
instruction (cf the comment in the pseudocode SSAdvance() function).

I think we should also be doing this PC advance on return from
userspace's handling of the mmio rather than before we drop back
to userspace as we do now, but I can't remember why I think that.
Christoffer, I don't suppose you recall, do you? I think it was
you I had this conversation with on IRC a month or so back...

-- PMM

Re: [PATCH 5/7] KVM: arm64: guest debug, add support for single-step

[Christoffer Dall]

On Wed, Nov 26, 2014 at 07:27:06PM +0000, Peter Maydell wrote:
> On 25 November 2014 at 16:10, Alex Bennée <alex.bennee@linaro.org> wrote:
> > This adds support for single-stepping the guest. As userspace can and
> > will manipulate guest registers before restarting any tweaking of the
> > registers has to occur just before control is passed back to the guest.
> > Furthermore while guest debugging is in effect we need to squash the
> > ability of the guest to single-step itself as we have no easy way of
> > re-entering the guest after the exception has been delivered to the
> > hypervisor.
> 
> A corner case I don't think this patch handles: if the debugger
> tries to single step an insn which is emulated by the
> hypervisor (because it's a load/store which is trapped and
> handled as emulated mmio in userspace) then we won't
> correctly update the single-step state machine (and so we'll end
> up incorrectly stopping after the following insn rather than
> before, I think).
> 
> You should be able to achieve this effect by simply always clearing
> the guest's PSTATE.SS when you advance the PC to skip the emulated
> instruction (cf the comment in the pseudocode SSAdvance() function).
> 
> I think we should also be doing this PC advance on return from
> userspace's handling of the mmio rather than before we drop back
> to userspace as we do now, but I can't remember why I think that.
> Christoffer, I don't suppose you recall, do you? I think it was
> you I had this conversation with on IRC a month or so back...
> 
I don't remember clearly, no.  Was it not during lunch at LCU we had
this conversation?

In any case, I think it was related to how userspace observes the state
of the CPU, because when you do the MMIO operation emulation in
userspace, currently if you observe the PC though GET_ONE_REG, you'll
see a PC pointing to the next instruction, not the one you're emulating
which is strange.

Not sure what the relation to a guest single-stepping itself was.

-Christoffer

Re: [PATCH 5/7] KVM: arm64: guest debug, add support for single-step

[Peter Maydell]

On 30 November 2014 at 10:10, Christoffer Dall
<christoffer.dall@linaro.org> wrote:
> In any case, I think it was related to how userspace observes the state
> of the CPU, because when you do the MMIO operation emulation in
> userspace, currently if you observe the PC though GET_ONE_REG, you'll
> see a PC pointing to the next instruction, not the one you're emulating
> which is strange.

Also if we ever add support for userspace to say "this MMIO should
fault" then we definitely need the PC-advance to happen afterwards,
not before.

> Not sure what the relation to a guest single-stepping itself was.

I think it just came up in the course of that discussion, because
single-step handling also needs to perform an action (clear PSTATE.SS)
as part of the "advance over this insn" operation. But I think that
you're right that doing the advance before dropping out to userspace
is no worse for singlestep than it is for any other case.

-- PMM

Re: [PATCH 5/7] KVM: arm64: guest debug, add support for single-step

[Christoffer Dall]

On Tue, Nov 25, 2014 at 04:10:03PM +0000, Alex Bennée wrote:
> This adds support for single-stepping the guest. As userspace can and
> will manipulate guest registers before restarting any tweaking of the
> registers has to occur just before control is passed back to the guest.
> Furthermore while guest debugging is in effect we need to squash the
> ability of the guest to single-step itself as we have no easy way of
> re-entering the guest after the exception has been delivered to the
> hypervisor.

Admittedly this is a corner case, but wouldn't the only really nasty bit
of this be to emulate the guest debug exception?

> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 48d26bb..a76daae 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -38,6 +38,7 @@
>  #include <asm/tlbflush.h>
>  #include <asm/cacheflush.h>
>  #include <asm/virt.h>
> +#include <asm/debug-monitors.h>
>  #include <asm/kvm_arm.h>
>  #include <asm/kvm_asm.h>
>  #include <asm/kvm_mmu.h>
> @@ -300,6 +301,17 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>  	kvm_arm_set_running_vcpu(NULL);
>  }
>  
> +/**
> + * kvm_arch_vcpu_ioctl_set_guest_debug - Setup guest debugging
> + * @kvm:	pointer to the KVM struct
> + * @kvm_guest_debug: the ioctl data buffer
> + *
> + * This sets up the VM for guest debugging. Care has to be taken when
> + * manipulating guest registers as these will be set/cleared by the
> + * hyper-visor controller, typically before each kvm_run event. As a

hypervisor

> + * result modification of the guest registers needs to take place
> + * after they have been restored in the hyp.S trampoline code.

I don't understand this??

> + */
>  int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>  					struct kvm_guest_debug *dbg)
>  {
> @@ -317,8 +329,8 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>  
>  	/* Single Step */
>  	if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
> -		kvm_info("SS requested, not yet implemented\n");
> -		return -EINVAL;
> +		kvm_info("SS requested\n");
> +		route_el2 = true;
>  	}
>  
>  	/* Software Break Points */
> diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
> index 8da1043..78e5ae1 100644
> --- a/arch/arm64/kernel/asm-offsets.c
> +++ b/arch/arm64/kernel/asm-offsets.c
> @@ -121,6 +121,7 @@ int main(void)
>    DEFINE(VCPU_FAR_EL2,		offsetof(struct kvm_vcpu, arch.fault.far_el2));
>    DEFINE(VCPU_HPFAR_EL2,	offsetof(struct kvm_vcpu, arch.fault.hpfar_el2));
>    DEFINE(VCPU_DEBUG_FLAGS,	offsetof(struct kvm_vcpu, arch.debug_flags));
> +  DEFINE(GUEST_DEBUG,		offsetof(struct kvm_vcpu, guest_debug));
>    DEFINE(VCPU_HCR_EL2,		offsetof(struct kvm_vcpu, arch.hcr_el2));
>    DEFINE(VCPU_MDCR_EL2,	offsetof(struct kvm_vcpu, arch.mdcr_el2));
>    DEFINE(VCPU_IRQ_LINES,	offsetof(struct kvm_vcpu, arch.irq_lines));
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index 28dc92b..6def054 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -91,6 +91,25 @@ static int kvm_handle_bkpt(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  	return 0;
>  }
>  
> +/**
> + * kvm_handle_ss - handle single step exceptions
> + *
> + * @vcpu:	the vcpu pointer
> + *
> + * See: ARM ARM D2.12 for the details. While the host is routing debug
> + * exceptions to it's handlers we have to suppress the ability of the

its handlers

> + * guest to trigger exceptions.

not really sure why this comment is here?  Does it really help anyone
reading this specific function or does it just confuse people more?

> + */
> +static int kvm_handle_ss(struct kvm_vcpu *vcpu, struct kvm_run *run)
> +{
> +	WARN_ON(!(vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP));

is this something that can actually happen or should it be a BUG_ON() -
which may even go away once you're doing hacking on this?

> +
> +	run->exit_reason = KVM_EXIT_DEBUG;
> +	run->debug.arch.exit_type = KVM_DEBUG_EXIT_SINGLE_STEP;
> +	run->debug.arch.address = *vcpu_pc(vcpu);
> +	return 0;
> +}
> +
>  static exit_handle_fn arm_exit_handlers[] = {
>  	[ESR_EL2_EC_WFI]	= kvm_handle_wfx,
>  	[ESR_EL2_EC_CP15_32]	= kvm_handle_cp15_32,
> @@ -105,6 +124,7 @@ static exit_handle_fn arm_exit_handlers[] = {
>  	[ESR_EL2_EC_SYS64]	= kvm_handle_sys_reg,
>  	[ESR_EL2_EC_IABT]	= kvm_handle_guest_abort,
>  	[ESR_EL2_EC_DABT]	= kvm_handle_guest_abort,
> +	[ESR_EL2_EC_SOFTSTP]    = kvm_handle_ss,
>  	[ESR_EL2_EC_BKPT32]	= kvm_handle_bkpt,
>  	[ESR_EL2_EC_BRK64]	= kvm_handle_bkpt,
>  };
> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
> index 3c733ea..c0bc218 100644
> --- a/arch/arm64/kvm/hyp.S
> +++ b/arch/arm64/kvm/hyp.S
> @@ -16,6 +16,7 @@
>   */
>  
>  #include <linux/linkage.h>
> +#include <linux/kvm.h>
>  
>  #include <asm/assembler.h>
>  #include <asm/memory.h>
> @@ -168,6 +169,31 @@
>  	// x19-x29, lr, sp*, elr*, spsr*
>  	restore_common_regs
>  
> +	// After restoring the guest registers but before we return to the guest
> +	// we may want to make some final tweaks to support guest debugging.

"we may want" sounds like we're not sure what we'll be doing here.  We
probably want to write something like "If the guest is being debugged we
need to set blah blah blah".

> +	ldr	x3, [x0, #GUEST_DEBUG]
> +	tbz	x3, #KVM_GUESTDBG_ENABLE_SHIFT, 2f	// No guest debug
> +
> +	// x0 - preserved as VCPU ptr
> +	// x1 - spsr
> +	// x2 - mdscr

not sure we need this comment

> +	mrs	x1, spsr_el2
> +	mrs 	x2, mdscr_el1
> +
> +	// See ARM ARM D2.12.3 The software step state machine
> +	// If we are doing Single Step - set MDSCR_EL1.SS and PSTATE.SS
> +	orr	x1, x1, #DBG_SPSR_SS
> +	orr	x2, x2, #DBG_MDSCR_SS
> +	tbnz	x3, #KVM_GUESTDBG_SINGLESTEP_SHIFT, 1f
> +	// If we are not doing Single Step we want to prevent the guest doing so
> +	// as otherwise we will have to deal with the re-routed exceptions as we
> +	// are doing other guest debug related things
> +	eor	x1, x1, #DBG_SPSR_SS
> +	eor	x2, x2, #DBG_MDSCR_SS

this really confuses me: so you're setting the SS bits in both
registers, and then if we're not single-stepping the guest, you clear
both bits again?

Wouldn't it be much simper to mask off the bits with a 'bic' and then
setting the bits when needed?

Alternatively, we could manage all these registers from C code and just
save/restore them off the VCPU struct.


> +1:
> +	msr	spsr_el2, x1
> +	msr	mdscr_el1, x2
> +2:
>  	// Last bits of the 64bit state
>  	pop	x2, x3
>  	pop	x0, x1
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 523f476..347e5b0 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -7,6 +7,8 @@
>   * Note: you must update KVM_API_VERSION if you change this interface.
>   */
>  
> +#ifndef __ASSEMBLY__
> +
>  #include <linux/types.h>
>  #include <linux/compiler.h>
>  #include <linux/ioctl.h>
> @@ -515,11 +517,6 @@ struct kvm_s390_irq {
>  	} u;
>  };
>  
> -/* for KVM_SET_GUEST_DEBUG */
> -
> -#define KVM_GUESTDBG_ENABLE		0x00000001
> -#define KVM_GUESTDBG_SINGLESTEP		0x00000002
> -
>  struct kvm_guest_debug {
>  	__u32 control;
>  	__u32 pad;
> @@ -1189,4 +1186,15 @@ struct kvm_assigned_msix_entry {
>  	__u16 padding[3];
>  };
>  
> +#endif /* __ASSEMBLY__ */
> +
> +/* for KVM_SET_GUEST_DEBUG */
> +
> +#define KVM_GUESTDBG_ENABLE_SHIFT	0
> +#define KVM_GUESTDBG_ENABLE		(1 << KVM_GUESTDBG_ENABLE_SHIFT)
> +#define KVM_GUESTDBG_SINGLESTEP_SHIFT	1
> +#define KVM_GUESTDBG_SINGLESTEP	(1 << KVM_GUESTDBG_SINGLESTEP_SHIFT)
> +
> +
> +
>  #endif /* __LINUX_KVM_H */
> -- 
> 2.1.3
> 

Re: [PATCH 5/7] KVM: arm64: guest debug, add support for single-step

[Alex Bennée]

Christoffer Dall <christoffer.dall@linaro.org> writes:

> On Tue, Nov 25, 2014 at 04:10:03PM +0000, Alex Bennée wrote:
>> This adds support for single-stepping the guest. As userspace can and
>> will manipulate guest registers before restarting any tweaking of the
>> registers has to occur just before control is passed back to the guest.
>> Furthermore while guest debugging is in effect we need to squash the
>> ability of the guest to single-step itself as we have no easy way of
>> re-entering the guest after the exception has been delivered to the
>> hypervisor.
>
> Admittedly this is a corner case, but wouldn't the only really nasty bit
> of this be to emulate the guest debug exception?

Well yes - currently this is all squashed by ignoring the guest's wishes
while we are debugging (save for SW breakpoints).

>
>> 
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> 
>> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
>> index 48d26bb..a76daae 100644
>> --- a/arch/arm/kvm/arm.c
>> +++ b/arch/arm/kvm/arm.c
>> @@ -38,6 +38,7 @@
>>  #include <asm/tlbflush.h>
>>  #include <asm/cacheflush.h>
>>  #include <asm/virt.h>
>> +#include <asm/debug-monitors.h>
>>  #include <asm/kvm_arm.h>
>>  #include <asm/kvm_asm.h>
>>  #include <asm/kvm_mmu.h>
>> @@ -300,6 +301,17 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>>  	kvm_arm_set_running_vcpu(NULL);
>>  }
>>  
>> +/**
>> + * kvm_arch_vcpu_ioctl_set_guest_debug - Setup guest debugging
>> + * @kvm:	pointer to the KVM struct
>> + * @kvm_guest_debug: the ioctl data buffer
>> + *
>> + * This sets up the VM for guest debugging. Care has to be taken when
>> + * manipulating guest registers as these will be set/cleared by the
>> + * hyper-visor controller, typically before each kvm_run event. As a
>
> hypervisor
>
>> + * result modification of the guest registers needs to take place
>> + * after they have been restored in the hyp.S trampoline code.
>
> I don't understand this??

We can't use GET/SET one reg to manipulate the registers we want as
these are the guest visible versions and subject to modification by
userspace. This is why the debugging code makes it's changes after the
guest state has been restored.

>
>> + */
>>  int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>>  					struct kvm_guest_debug *dbg)
>>  {
>> @@ -317,8 +329,8 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>>  
>>  	/* Single Step */
>>  	if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
>> -		kvm_info("SS requested, not yet implemented\n");
>> -		return -EINVAL;
>> +		kvm_info("SS requested\n");
>> +		route_el2 = true;
>>  	}
>>  
>>  	/* Software Break Points */
>> diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
>> index 8da1043..78e5ae1 100644
>> --- a/arch/arm64/kernel/asm-offsets.c
>> +++ b/arch/arm64/kernel/asm-offsets.c
>> @@ -121,6 +121,7 @@ int main(void)
>>    DEFINE(VCPU_FAR_EL2,		offsetof(struct kvm_vcpu, arch.fault.far_el2));
>>    DEFINE(VCPU_HPFAR_EL2,	offsetof(struct kvm_vcpu, arch.fault.hpfar_el2));
>>    DEFINE(VCPU_DEBUG_FLAGS,	offsetof(struct kvm_vcpu, arch.debug_flags));
>> +  DEFINE(GUEST_DEBUG,		offsetof(struct kvm_vcpu, guest_debug));
>>    DEFINE(VCPU_HCR_EL2,		offsetof(struct kvm_vcpu, arch.hcr_el2));
>>    DEFINE(VCPU_MDCR_EL2,	offsetof(struct kvm_vcpu, arch.mdcr_el2));
>>    DEFINE(VCPU_IRQ_LINES,	offsetof(struct kvm_vcpu, arch.irq_lines));
>> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
>> index 28dc92b..6def054 100644
>> --- a/arch/arm64/kvm/handle_exit.c
>> +++ b/arch/arm64/kvm/handle_exit.c
>> @@ -91,6 +91,25 @@ static int kvm_handle_bkpt(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>  	return 0;
>>  }
>>  
>> +/**
>> + * kvm_handle_ss - handle single step exceptions
>> + *
>> + * @vcpu:	the vcpu pointer
>> + *
>> + * See: ARM ARM D2.12 for the details. While the host is routing debug
>> + * exceptions to it's handlers we have to suppress the ability of the
>
> its handlers
>
>> + * guest to trigger exceptions.
>
> not really sure why this comment is here?  Does it really help anyone
> reading this specific function or does it just confuse people more?
>
>> + */
>> +static int kvm_handle_ss(struct kvm_vcpu *vcpu, struct kvm_run *run)
>> +{
>> +	WARN_ON(!(vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP));
>
> is this something that can actually happen or should it be a BUG_ON() -
> which may even go away once you're doing hacking on this?

It shouldn't happen. I was treating more like an assert, failure of
which would indicate something has gone wrong somewhere although
generally not worth bringing the kernel down for.

>
>> +
>> +	run->exit_reason = KVM_EXIT_DEBUG;
>> +	run->debug.arch.exit_type = KVM_DEBUG_EXIT_SINGLE_STEP;
>> +	run->debug.arch.address = *vcpu_pc(vcpu);
>> +	return 0;
>> +}
>> +
>>  static exit_handle_fn arm_exit_handlers[] = {
>>  	[ESR_EL2_EC_WFI]	= kvm_handle_wfx,
>>  	[ESR_EL2_EC_CP15_32]	= kvm_handle_cp15_32,
>> @@ -105,6 +124,7 @@ static exit_handle_fn arm_exit_handlers[] = {
>>  	[ESR_EL2_EC_SYS64]	= kvm_handle_sys_reg,
>>  	[ESR_EL2_EC_IABT]	= kvm_handle_guest_abort,
>>  	[ESR_EL2_EC_DABT]	= kvm_handle_guest_abort,
>> +	[ESR_EL2_EC_SOFTSTP]    = kvm_handle_ss,
>>  	[ESR_EL2_EC_BKPT32]	= kvm_handle_bkpt,
>>  	[ESR_EL2_EC_BRK64]	= kvm_handle_bkpt,
>>  };
>> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
>> index 3c733ea..c0bc218 100644
>> --- a/arch/arm64/kvm/hyp.S
>> +++ b/arch/arm64/kvm/hyp.S
>> @@ -16,6 +16,7 @@
>>   */
>>  
>>  #include <linux/linkage.h>
>> +#include <linux/kvm.h>
>>  
>>  #include <asm/assembler.h>
>>  #include <asm/memory.h>
>> @@ -168,6 +169,31 @@
>>  	// x19-x29, lr, sp*, elr*, spsr*
>>  	restore_common_regs
>>  
>> +	// After restoring the guest registers but before we return to the guest
>> +	// we may want to make some final tweaks to support guest debugging.
>
> "we may want" sounds like we're not sure what we'll be doing here.  We
> probably want to write something like "If the guest is being debugged we
> need to set blah blah blah".
>
>> +	ldr	x3, [x0, #GUEST_DEBUG]
>> +	tbz	x3, #KVM_GUESTDBG_ENABLE_SHIFT, 2f	// No guest debug
>> +
>> +	// x0 - preserved as VCPU ptr
>> +	// x1 - spsr
>> +	// x2 - mdscr
>
> not sure we need this comment
>
>> +	mrs	x1, spsr_el2
>> +	mrs 	x2, mdscr_el1
>> +
>> +	// See ARM ARM D2.12.3 The software step state machine
>> +	// If we are doing Single Step - set MDSCR_EL1.SS and PSTATE.SS
>> +	orr	x1, x1, #DBG_SPSR_SS
>> +	orr	x2, x2, #DBG_MDSCR_SS
>> +	tbnz	x3, #KVM_GUESTDBG_SINGLESTEP_SHIFT, 1f
>> +	// If we are not doing Single Step we want to prevent the guest doing so
>> +	// as otherwise we will have to deal with the re-routed exceptions as we
>> +	// are doing other guest debug related things
>> +	eor	x1, x1, #DBG_SPSR_SS
>> +	eor	x2, x2, #DBG_MDSCR_SS
>
> this really confuses me: so you're setting the SS bits in both
> registers, and then if we're not single-stepping the guest, you clear
> both bits again?
>
> Wouldn't it be much simper to mask off the bits with a 'bic' and then
> setting the bits when needed?

Is there a non-vector BIC #imm? I was being frugal with register usage
at this point. The orr/eor steps where just to avoid having too many
branch cases.

> Alternatively, we could manage all these registers from C code and just
> save/restore them off the VCPU struct.

Yes but this has to be done as we run into the hyp.S code after all
guest registers are confirmed as the changes are on-top of whatever the
guest view is (for the _el1 regs).

Where would you suggest that goes?

>> +1:
>> +	msr	spsr_el2, x1
>> +	msr	mdscr_el1, x2
>> +2:
>>  	// Last bits of the 64bit state
>>  	pop	x2, x3
>>  	pop	x0, x1
>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>> index 523f476..347e5b0 100644
>> --- a/include/uapi/linux/kvm.h
>> +++ b/include/uapi/linux/kvm.h
>> @@ -7,6 +7,8 @@
>>   * Note: you must update KVM_API_VERSION if you change this interface.
>>   */
>>  
>> +#ifndef __ASSEMBLY__
>> +
>>  #include <linux/types.h>
>>  #include <linux/compiler.h>
>>  #include <linux/ioctl.h>
>> @@ -515,11 +517,6 @@ struct kvm_s390_irq {
>>  	} u;
>>  };
>>  
>> -/* for KVM_SET_GUEST_DEBUG */
>> -
>> -#define KVM_GUESTDBG_ENABLE		0x00000001
>> -#define KVM_GUESTDBG_SINGLESTEP		0x00000002
>> -
>>  struct kvm_guest_debug {
>>  	__u32 control;
>>  	__u32 pad;
>> @@ -1189,4 +1186,15 @@ struct kvm_assigned_msix_entry {
>>  	__u16 padding[3];
>>  };
>>  
>> +#endif /* __ASSEMBLY__ */
>> +
>> +/* for KVM_SET_GUEST_DEBUG */
>> +
>> +#define KVM_GUESTDBG_ENABLE_SHIFT	0
>> +#define KVM_GUESTDBG_ENABLE		(1 << KVM_GUESTDBG_ENABLE_SHIFT)
>> +#define KVM_GUESTDBG_SINGLESTEP_SHIFT	1
>> +#define KVM_GUESTDBG_SINGLESTEP	(1 << KVM_GUESTDBG_SINGLESTEP_SHIFT)
>> +
>> +
>> +
>>  #endif /* __LINUX_KVM_H */
>> -- 
>> 2.1.3
>> 

-- 
Alex Bennée

Re: [PATCH 5/7] KVM: arm64: guest debug, add support for single-step

[Christoffer Dall]

On Mon, Dec 01, 2014 at 11:50:14AM +0000, Alex Bennée wrote:
> 
> Christoffer Dall <christoffer.dall@linaro.org> writes:
> 
> > On Tue, Nov 25, 2014 at 04:10:03PM +0000, Alex Bennée wrote:
> >> This adds support for single-stepping the guest. As userspace can and
> >> will manipulate guest registers before restarting any tweaking of the
> >> registers has to occur just before control is passed back to the guest.
> >> Furthermore while guest debugging is in effect we need to squash the
> >> ability of the guest to single-step itself as we have no easy way of
> >> re-entering the guest after the exception has been delivered to the
> >> hypervisor.
> >
> > Admittedly this is a corner case, but wouldn't the only really nasty bit
> > of this be to emulate the guest debug exception?
> 
> Well yes - currently this is all squashed by ignoring the guest's wishes
> while we are debugging (save for SW breakpoints).
> 
> >
> >> 
> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> >> 
> >> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> >> index 48d26bb..a76daae 100644
> >> --- a/arch/arm/kvm/arm.c
> >> +++ b/arch/arm/kvm/arm.c
> >> @@ -38,6 +38,7 @@
> >>  #include <asm/tlbflush.h>
> >>  #include <asm/cacheflush.h>
> >>  #include <asm/virt.h>
> >> +#include <asm/debug-monitors.h>
> >>  #include <asm/kvm_arm.h>
> >>  #include <asm/kvm_asm.h>
> >>  #include <asm/kvm_mmu.h>
> >> @@ -300,6 +301,17 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
> >>  	kvm_arm_set_running_vcpu(NULL);
> >>  }
> >>  
> >> +/**
> >> + * kvm_arch_vcpu_ioctl_set_guest_debug - Setup guest debugging
> >> + * @kvm:	pointer to the KVM struct
> >> + * @kvm_guest_debug: the ioctl data buffer
> >> + *
> >> + * This sets up the VM for guest debugging. Care has to be taken when
> >> + * manipulating guest registers as these will be set/cleared by the
> >> + * hyper-visor controller, typically before each kvm_run event. As a
> >
> > hypervisor
> >
> >> + * result modification of the guest registers needs to take place
> >> + * after they have been restored in the hyp.S trampoline code.
> >
> > I don't understand this??
> 
> We can't use GET/SET one reg to manipulate the registers we want as
> these are the guest visible versions and subject to modification by
> userspace. This is why the debugging code makes it's changes after the
> guest state has been restored.
> 

eh, once you're in the KVM_RUN ioctl, user space can't fiddle your VCPU
regs because you're holding the vcpu mutex, so doing stuff in some
callout from kvm_arch_vcpu_ioctl_run() seems every bid as valid for this
case as doing it in EL2.  In fact, the only reason why we're doing
anything in EL2 is when you're accessing state only accessible in EL2,
when you need to write the whole thing in assembly (like the context
switch of GP registers) etc.

If it doesn't have huge performance costs, we should use C-code in EL1
to the furthest extent possible.

> >
> >> + */
> >>  int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
> >>  					struct kvm_guest_debug *dbg)
> >>  {
> >> @@ -317,8 +329,8 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
> >>  
> >>  	/* Single Step */
> >>  	if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
> >> -		kvm_info("SS requested, not yet implemented\n");
> >> -		return -EINVAL;
> >> +		kvm_info("SS requested\n");
> >> +		route_el2 = true;
> >>  	}
> >>  
> >>  	/* Software Break Points */
> >> diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
> >> index 8da1043..78e5ae1 100644
> >> --- a/arch/arm64/kernel/asm-offsets.c
> >> +++ b/arch/arm64/kernel/asm-offsets.c
> >> @@ -121,6 +121,7 @@ int main(void)
> >>    DEFINE(VCPU_FAR_EL2,		offsetof(struct kvm_vcpu, arch.fault.far_el2));
> >>    DEFINE(VCPU_HPFAR_EL2,	offsetof(struct kvm_vcpu, arch.fault.hpfar_el2));
> >>    DEFINE(VCPU_DEBUG_FLAGS,	offsetof(struct kvm_vcpu, arch.debug_flags));
> >> +  DEFINE(GUEST_DEBUG,		offsetof(struct kvm_vcpu, guest_debug));
> >>    DEFINE(VCPU_HCR_EL2,		offsetof(struct kvm_vcpu, arch.hcr_el2));
> >>    DEFINE(VCPU_MDCR_EL2,	offsetof(struct kvm_vcpu, arch.mdcr_el2));
> >>    DEFINE(VCPU_IRQ_LINES,	offsetof(struct kvm_vcpu, arch.irq_lines));
> >> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> >> index 28dc92b..6def054 100644
> >> --- a/arch/arm64/kvm/handle_exit.c
> >> +++ b/arch/arm64/kvm/handle_exit.c
> >> @@ -91,6 +91,25 @@ static int kvm_handle_bkpt(struct kvm_vcpu *vcpu, struct kvm_run *run)
> >>  	return 0;
> >>  }
> >>  
> >> +/**
> >> + * kvm_handle_ss - handle single step exceptions
> >> + *
> >> + * @vcpu:	the vcpu pointer
> >> + *
> >> + * See: ARM ARM D2.12 for the details. While the host is routing debug
> >> + * exceptions to it's handlers we have to suppress the ability of the
> >
> > its handlers
> >
> >> + * guest to trigger exceptions.
> >
> > not really sure why this comment is here?  Does it really help anyone
> > reading this specific function or does it just confuse people more?
> >
> >> + */
> >> +static int kvm_handle_ss(struct kvm_vcpu *vcpu, struct kvm_run *run)
> >> +{
> >> +	WARN_ON(!(vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP));
> >
> > is this something that can actually happen or should it be a BUG_ON() -
> > which may even go away once you're doing hacking on this?
> 
> It shouldn't happen. I was treating more like an assert, failure of
> which would indicate something has gone wrong somewhere although
> generally not worth bringing the kernel down for.
> 

huh, I guess that's fair enough, but somewhat unconventional for kernel
code.  Typically I read WARN_ON (I could be wrong here) as something
that may happen under extreme circumstances (debugging turned on, crazy
low-memory situations etc.), but not as something that catches a bug.

I've seen the argument before that if something that sholdn't ever
happen in the kernel, indeed does happen in the kernel, then that is a
bug, and then you should panic().

So I do feel that this is either a kvm_info()/kvm_err() situation or a
BUG_ON() situation, or nothing at all.

> >
> >> +
> >> +	run->exit_reason = KVM_EXIT_DEBUG;
> >> +	run->debug.arch.exit_type = KVM_DEBUG_EXIT_SINGLE_STEP;
> >> +	run->debug.arch.address = *vcpu_pc(vcpu);
> >> +	return 0;
> >> +}
> >> +
> >>  static exit_handle_fn arm_exit_handlers[] = {
> >>  	[ESR_EL2_EC_WFI]	= kvm_handle_wfx,
> >>  	[ESR_EL2_EC_CP15_32]	= kvm_handle_cp15_32,
> >> @@ -105,6 +124,7 @@ static exit_handle_fn arm_exit_handlers[] = {
> >>  	[ESR_EL2_EC_SYS64]	= kvm_handle_sys_reg,
> >>  	[ESR_EL2_EC_IABT]	= kvm_handle_guest_abort,
> >>  	[ESR_EL2_EC_DABT]	= kvm_handle_guest_abort,
> >> +	[ESR_EL2_EC_SOFTSTP]    = kvm_handle_ss,
> >>  	[ESR_EL2_EC_BKPT32]	= kvm_handle_bkpt,
> >>  	[ESR_EL2_EC_BRK64]	= kvm_handle_bkpt,
> >>  };
> >> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
> >> index 3c733ea..c0bc218 100644
> >> --- a/arch/arm64/kvm/hyp.S
> >> +++ b/arch/arm64/kvm/hyp.S
> >> @@ -16,6 +16,7 @@
> >>   */
> >>  
> >>  #include <linux/linkage.h>
> >> +#include <linux/kvm.h>
> >>  
> >>  #include <asm/assembler.h>
> >>  #include <asm/memory.h>
> >> @@ -168,6 +169,31 @@
> >>  	// x19-x29, lr, sp*, elr*, spsr*
> >>  	restore_common_regs
> >>  
> >> +	// After restoring the guest registers but before we return to the guest
> >> +	// we may want to make some final tweaks to support guest debugging.
> >
> > "we may want" sounds like we're not sure what we'll be doing here.  We
> > probably want to write something like "If the guest is being debugged we
> > need to set blah blah blah".
> >
> >> +	ldr	x3, [x0, #GUEST_DEBUG]
> >> +	tbz	x3, #KVM_GUESTDBG_ENABLE_SHIFT, 2f	// No guest debug
> >> +
> >> +	// x0 - preserved as VCPU ptr
> >> +	// x1 - spsr
> >> +	// x2 - mdscr
> >
> > not sure we need this comment
> >
> >> +	mrs	x1, spsr_el2
> >> +	mrs 	x2, mdscr_el1
> >> +
> >> +	// See ARM ARM D2.12.3 The software step state machine
> >> +	// If we are doing Single Step - set MDSCR_EL1.SS and PSTATE.SS
> >> +	orr	x1, x1, #DBG_SPSR_SS
> >> +	orr	x2, x2, #DBG_MDSCR_SS
> >> +	tbnz	x3, #KVM_GUESTDBG_SINGLESTEP_SHIFT, 1f
> >> +	// If we are not doing Single Step we want to prevent the guest doing so
> >> +	// as otherwise we will have to deal with the re-routed exceptions as we
> >> +	// are doing other guest debug related things
> >> +	eor	x1, x1, #DBG_SPSR_SS
> >> +	eor	x2, x2, #DBG_MDSCR_SS
> >
> > this really confuses me: so you're setting the SS bits in both
> > registers, and then if we're not single-stepping the guest, you clear
> > both bits again?
> >
> > Wouldn't it be much simper to mask off the bits with a 'bic' and then
> > setting the bits when needed?
> 
> Is there a non-vector BIC #imm? I was being frugal with register usage
> at this point. The orr/eor steps where just to avoid having too many
> branch cases.
> 

there are "bic x3, x3, #1" and such in this very file, so I would guess,
yes.

> > Alternatively, we could manage all these registers from C code and just
> > save/restore them off the VCPU struct.
> 
> Yes but this has to be done as we run into the hyp.S code after all
> guest registers are confirmed as the changes are on-top of whatever the
> guest view is (for the _el1 regs).
> 
> Where would you suggest that goes?
> 

As a call-out from the arch-specific KVM_RUN ioctl, call
kvm_setup_guest_debug() or something like that, just like we do to check
if our vmid is valid and to setup the vgic state and so on.  Does that
work?

-Christoffer

[PATCH 6/7] KVM: arm64: re-factor hyp.S debug register code

[Alex Bennée]

This is a pre-cursor to sharing the code with the guest debug support.
This replaces the big macro that fishes data out of a fixed location
with a more general helper macro to restore a set of debug registers. It
uses macro substitution so it can be re-used for debug control and value
registers. It does however rely on the debug registers being 64 bit
aligned (as they happen to be in the hyp ABI).

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
index c0bc218..b38ce3d 100644
--- a/arch/arm64/kvm/hyp.S
+++ b/arch/arm64/kvm/hyp.S
@@ -490,65 +490,12 @@
 	msr	mdscr_el1,	x25
 .endm
 
-.macro restore_debug
-	// x2: base address for cpu context
-	// x3: tmp register
-
-	mrs	x26, id_aa64dfr0_el1
-	ubfx	x24, x26, #12, #4	// Extract BRPs
-	ubfx	x25, x26, #20, #4	// Extract WRPs
-	mov	w26, #15
-	sub	w24, w26, w24		// How many BPs to skip
-	sub	w25, w26, w25		// How many WPs to skip
-
-	add	x3, x2, #CPU_SYSREG_OFFSET(DBGBCR0_EL1)
-
-	adr	x26, 1f
-	add	x26, x26, x24, lsl #2
-	br	x26
-1:
-	ldr	x20, [x3, #(15 * 8)]
-	ldr	x19, [x3, #(14 * 8)]
-	ldr	x18, [x3, #(13 * 8)]
-	ldr	x17, [x3, #(12 * 8)]
-	ldr	x16, [x3, #(11 * 8)]
-	ldr	x15, [x3, #(10 * 8)]
-	ldr	x14, [x3, #(9 * 8)]
-	ldr	x13, [x3, #(8 * 8)]
-	ldr	x12, [x3, #(7 * 8)]
-	ldr	x11, [x3, #(6 * 8)]
-	ldr	x10, [x3, #(5 * 8)]
-	ldr	x9, [x3, #(4 * 8)]
-	ldr	x8, [x3, #(3 * 8)]
-	ldr	x7, [x3, #(2 * 8)]
-	ldr	x6, [x3, #(1 * 8)]
-	ldr	x5, [x3, #(0 * 8)]
-
+.macro setup_debug_registers type
+        // x3: pointer to register set
+        // x4: number of registers to copy
+        // x5..x20/x26 trashed
 	adr	x26, 1f
-	add	x26, x26, x24, lsl #2
-	br	x26
-1:
-	msr	dbgbcr15_el1, x20
-	msr	dbgbcr14_el1, x19
-	msr	dbgbcr13_el1, x18
-	msr	dbgbcr12_el1, x17
-	msr	dbgbcr11_el1, x16
-	msr	dbgbcr10_el1, x15
-	msr	dbgbcr9_el1, x14
-	msr	dbgbcr8_el1, x13
-	msr	dbgbcr7_el1, x12
-	msr	dbgbcr6_el1, x11
-	msr	dbgbcr5_el1, x10
-	msr	dbgbcr4_el1, x9
-	msr	dbgbcr3_el1, x8
-	msr	dbgbcr2_el1, x7
-	msr	dbgbcr1_el1, x6
-	msr	dbgbcr0_el1, x5
-
-	add	x3, x2, #CPU_SYSREG_OFFSET(DBGBVR0_EL1)
-
-	adr	x26, 1f
-	add	x26, x26, x24, lsl #2
+	add	x26, x26, x4, lsl #2
 	br	x26
 1:
 	ldr	x20, [x3, #(15 * 8)]
@@ -569,116 +516,25 @@
 	ldr	x5, [x3, #(0 * 8)]
 
 	adr	x26, 1f
-	add	x26, x26, x24, lsl #2
-	br	x26
-1:
-	msr	dbgbvr15_el1, x20
-	msr	dbgbvr14_el1, x19
-	msr	dbgbvr13_el1, x18
-	msr	dbgbvr12_el1, x17
-	msr	dbgbvr11_el1, x16
-	msr	dbgbvr10_el1, x15
-	msr	dbgbvr9_el1, x14
-	msr	dbgbvr8_el1, x13
-	msr	dbgbvr7_el1, x12
-	msr	dbgbvr6_el1, x11
-	msr	dbgbvr5_el1, x10
-	msr	dbgbvr4_el1, x9
-	msr	dbgbvr3_el1, x8
-	msr	dbgbvr2_el1, x7
-	msr	dbgbvr1_el1, x6
-	msr	dbgbvr0_el1, x5
-
-	add	x3, x2, #CPU_SYSREG_OFFSET(DBGWCR0_EL1)
-
-	adr	x26, 1f
-	add	x26, x26, x25, lsl #2
+	add	x26, x26, x4, lsl #2
 	br	x26
 1:
-	ldr	x20, [x3, #(15 * 8)]
-	ldr	x19, [x3, #(14 * 8)]
-	ldr	x18, [x3, #(13 * 8)]
-	ldr	x17, [x3, #(12 * 8)]
-	ldr	x16, [x3, #(11 * 8)]
-	ldr	x15, [x3, #(10 * 8)]
-	ldr	x14, [x3, #(9 * 8)]
-	ldr	x13, [x3, #(8 * 8)]
-	ldr	x12, [x3, #(7 * 8)]
-	ldr	x11, [x3, #(6 * 8)]
-	ldr	x10, [x3, #(5 * 8)]
-	ldr	x9, [x3, #(4 * 8)]
-	ldr	x8, [x3, #(3 * 8)]
-	ldr	x7, [x3, #(2 * 8)]
-	ldr	x6, [x3, #(1 * 8)]
-	ldr	x5, [x3, #(0 * 8)]
-
-	adr	x26, 1f
-	add	x26, x26, x25, lsl #2
-	br	x26
-1:
-	msr	dbgwcr15_el1, x20
-	msr	dbgwcr14_el1, x19
-	msr	dbgwcr13_el1, x18
-	msr	dbgwcr12_el1, x17
-	msr	dbgwcr11_el1, x16
-	msr	dbgwcr10_el1, x15
-	msr	dbgwcr9_el1, x14
-	msr	dbgwcr8_el1, x13
-	msr	dbgwcr7_el1, x12
-	msr	dbgwcr6_el1, x11
-	msr	dbgwcr5_el1, x10
-	msr	dbgwcr4_el1, x9
-	msr	dbgwcr3_el1, x8
-	msr	dbgwcr2_el1, x7
-	msr	dbgwcr1_el1, x6
-	msr	dbgwcr0_el1, x5
-
-	add	x3, x2, #CPU_SYSREG_OFFSET(DBGWVR0_EL1)
-
-	adr	x26, 1f
-	add	x26, x26, x25, lsl #2
-	br	x26
-1:
-	ldr	x20, [x3, #(15 * 8)]
-	ldr	x19, [x3, #(14 * 8)]
-	ldr	x18, [x3, #(13 * 8)]
-	ldr	x17, [x3, #(12 * 8)]
-	ldr	x16, [x3, #(11 * 8)]
-	ldr	x15, [x3, #(10 * 8)]
-	ldr	x14, [x3, #(9 * 8)]
-	ldr	x13, [x3, #(8 * 8)]
-	ldr	x12, [x3, #(7 * 8)]
-	ldr	x11, [x3, #(6 * 8)]
-	ldr	x10, [x3, #(5 * 8)]
-	ldr	x9, [x3, #(4 * 8)]
-	ldr	x8, [x3, #(3 * 8)]
-	ldr	x7, [x3, #(2 * 8)]
-	ldr	x6, [x3, #(1 * 8)]
-	ldr	x5, [x3, #(0 * 8)]
-
-	adr	x26, 1f
-	add	x26, x26, x25, lsl #2
-	br	x26
-1:
-	msr	dbgwvr15_el1, x20
-	msr	dbgwvr14_el1, x19
-	msr	dbgwvr13_el1, x18
-	msr	dbgwvr12_el1, x17
-	msr	dbgwvr11_el1, x16
-	msr	dbgwvr10_el1, x15
-	msr	dbgwvr9_el1, x14
-	msr	dbgwvr8_el1, x13
-	msr	dbgwvr7_el1, x12
-	msr	dbgwvr6_el1, x11
-	msr	dbgwvr5_el1, x10
-	msr	dbgwvr4_el1, x9
-	msr	dbgwvr3_el1, x8
-	msr	dbgwvr2_el1, x7
-	msr	dbgwvr1_el1, x6
-	msr	dbgwvr0_el1, x5
-
-	ldr	x21, [x2, #CPU_SYSREG_OFFSET(MDCCINT_EL1)]
-	msr	mdccint_el1, x21
+	msr	\type\()15_el1, x20
+	msr	\type\()14_el1, x19
+	msr	\type\()13_el1, x18
+	msr	\type\()12_el1, x17
+	msr	\type\()11_el1, x16
+	msr	\type\()10_el1, x15
+	msr	\type\()9_el1, x14
+	msr	\type\()8_el1, x13
+	msr	\type\()7_el1, x12
+	msr	\type\()6_el1, x11
+	msr	\type\()5_el1, x10
+	msr	\type\()4_el1, x9
+	msr	\type\()3_el1, x8
+	msr	\type\()2_el1, x7
+	msr	\type\()1_el1, x6
+	msr	\type\()0_el1, x5
 .endm
 
 .macro skip_32bit_state tmp, target
@@ -929,8 +785,34 @@ __save_debug:
 	save_debug
 	ret
 
+/* Restore saved guest debug state */
 __restore_debug:
-	restore_debug
+	// x2: base address for cpu context
+	// x3: ptr to guest registers passed to setup_debug_registers
+	// x5..x20/x26: trashed
+
+	mrs	x26, id_aa64dfr0_el1
+	ubfx	x24, x26, #12, #4	// Extract BRPs
+	ubfx	x25, x26, #20, #4	// Extract WRPs
+	mov	w26, #15
+	sub	w24, w26, w24		// How many BPs to skip
+	sub	w25, w26, w25		// How many WPs to skip
+
+	mov     x4, x24
+	add	x3, x2, #CPU_SYSREG_OFFSET(DBGBCR0_EL1)
+	setup_debug_registers dbgbcr
+	add	x3, x2, #CPU_SYSREG_OFFSET(DBGBVR0_EL1)
+	setup_debug_registers dbgbvr
+
+	mov     x4, x25
+	add	x3, x2, #CPU_SYSREG_OFFSET(DBGWCR0_EL1)
+	setup_debug_registers dbgwcr
+	add	x3, x2, #CPU_SYSREG_OFFSET(DBGWVR0_EL1)
+	setup_debug_registers dbgwvr
+
+	ldr	x21, [x2, #CPU_SYSREG_OFFSET(MDCCINT_EL1)]
+	msr	mdccint_el1, x21
+
 	ret
 
 __save_fpsimd:
-- 
2.1.3

Re: [PATCH 6/7] KVM: arm64: re-factor hyp.S debug register code

[Andrew Jones]

On Tue, Nov 25, 2014 at 04:10:04PM +0000, Alex Bennée wrote:
> This is a pre-cursor to sharing the code with the guest debug support.
> This replaces the big macro that fishes data out of a fixed location
> with a more general helper macro to restore a set of debug registers. It
> uses macro substitution so it can be re-used for debug control and value
> registers. It does however rely on the debug registers being 64 bit
> aligned (as they happen to be in the hyp ABI).
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
> index c0bc218..b38ce3d 100644
> --- a/arch/arm64/kvm/hyp.S
> +++ b/arch/arm64/kvm/hyp.S
> @@ -490,65 +490,12 @@
>  	msr	mdscr_el1,	x25
>  .endm
>  
> -.macro restore_debug
> -	// x2: base address for cpu context
> -	// x3: tmp register
> -
> -	mrs	x26, id_aa64dfr0_el1
> -	ubfx	x24, x26, #12, #4	// Extract BRPs
> -	ubfx	x25, x26, #20, #4	// Extract WRPs
> -	mov	w26, #15
> -	sub	w24, w26, w24		// How many BPs to skip
> -	sub	w25, w26, w25		// How many WPs to skip
> -
> -	add	x3, x2, #CPU_SYSREG_OFFSET(DBGBCR0_EL1)
> -
> -	adr	x26, 1f
> -	add	x26, x26, x24, lsl #2
> -	br	x26
> -1:
> -	ldr	x20, [x3, #(15 * 8)]
> -	ldr	x19, [x3, #(14 * 8)]
> -	ldr	x18, [x3, #(13 * 8)]
> -	ldr	x17, [x3, #(12 * 8)]
> -	ldr	x16, [x3, #(11 * 8)]
> -	ldr	x15, [x3, #(10 * 8)]
> -	ldr	x14, [x3, #(9 * 8)]
> -	ldr	x13, [x3, #(8 * 8)]
> -	ldr	x12, [x3, #(7 * 8)]
> -	ldr	x11, [x3, #(6 * 8)]
> -	ldr	x10, [x3, #(5 * 8)]
> -	ldr	x9, [x3, #(4 * 8)]
> -	ldr	x8, [x3, #(3 * 8)]
> -	ldr	x7, [x3, #(2 * 8)]
> -	ldr	x6, [x3, #(1 * 8)]
> -	ldr	x5, [x3, #(0 * 8)]
> -
> +.macro setup_debug_registers type
> +        // x3: pointer to register set
> +        // x4: number of registers to copy
> +        // x5..x20/x26 trashed
                     ^ comma instead?
>  	adr	x26, 1f
> -	add	x26, x26, x24, lsl #2
> -	br	x26
> -1:
> -	msr	dbgbcr15_el1, x20
> -	msr	dbgbcr14_el1, x19
> -	msr	dbgbcr13_el1, x18
> -	msr	dbgbcr12_el1, x17
> -	msr	dbgbcr11_el1, x16
> -	msr	dbgbcr10_el1, x15
> -	msr	dbgbcr9_el1, x14
> -	msr	dbgbcr8_el1, x13
> -	msr	dbgbcr7_el1, x12
> -	msr	dbgbcr6_el1, x11
> -	msr	dbgbcr5_el1, x10
> -	msr	dbgbcr4_el1, x9
> -	msr	dbgbcr3_el1, x8
> -	msr	dbgbcr2_el1, x7
> -	msr	dbgbcr1_el1, x6
> -	msr	dbgbcr0_el1, x5
> -
> -	add	x3, x2, #CPU_SYSREG_OFFSET(DBGBVR0_EL1)
> -
> -	adr	x26, 1f
> -	add	x26, x26, x24, lsl #2
> +	add	x26, x26, x4, lsl #2
>  	br	x26
>  1:
>  	ldr	x20, [x3, #(15 * 8)]
> @@ -569,116 +516,25 @@
>  	ldr	x5, [x3, #(0 * 8)]
>  
>  	adr	x26, 1f
> -	add	x26, x26, x24, lsl #2
> -	br	x26
> -1:
> -	msr	dbgbvr15_el1, x20
> -	msr	dbgbvr14_el1, x19
> -	msr	dbgbvr13_el1, x18
> -	msr	dbgbvr12_el1, x17
> -	msr	dbgbvr11_el1, x16
> -	msr	dbgbvr10_el1, x15
> -	msr	dbgbvr9_el1, x14
> -	msr	dbgbvr8_el1, x13
> -	msr	dbgbvr7_el1, x12
> -	msr	dbgbvr6_el1, x11
> -	msr	dbgbvr5_el1, x10
> -	msr	dbgbvr4_el1, x9
> -	msr	dbgbvr3_el1, x8
> -	msr	dbgbvr2_el1, x7
> -	msr	dbgbvr1_el1, x6
> -	msr	dbgbvr0_el1, x5
> -
> -	add	x3, x2, #CPU_SYSREG_OFFSET(DBGWCR0_EL1)
> -
> -	adr	x26, 1f
> -	add	x26, x26, x25, lsl #2
> +	add	x26, x26, x4, lsl #2
>  	br	x26
>  1:
> -	ldr	x20, [x3, #(15 * 8)]
> -	ldr	x19, [x3, #(14 * 8)]
> -	ldr	x18, [x3, #(13 * 8)]
> -	ldr	x17, [x3, #(12 * 8)]
> -	ldr	x16, [x3, #(11 * 8)]
> -	ldr	x15, [x3, #(10 * 8)]
> -	ldr	x14, [x3, #(9 * 8)]
> -	ldr	x13, [x3, #(8 * 8)]
> -	ldr	x12, [x3, #(7 * 8)]
> -	ldr	x11, [x3, #(6 * 8)]
> -	ldr	x10, [x3, #(5 * 8)]
> -	ldr	x9, [x3, #(4 * 8)]
> -	ldr	x8, [x3, #(3 * 8)]
> -	ldr	x7, [x3, #(2 * 8)]
> -	ldr	x6, [x3, #(1 * 8)]
> -	ldr	x5, [x3, #(0 * 8)]
> -
> -	adr	x26, 1f
> -	add	x26, x26, x25, lsl #2
> -	br	x26
> -1:
> -	msr	dbgwcr15_el1, x20
> -	msr	dbgwcr14_el1, x19
> -	msr	dbgwcr13_el1, x18
> -	msr	dbgwcr12_el1, x17
> -	msr	dbgwcr11_el1, x16
> -	msr	dbgwcr10_el1, x15
> -	msr	dbgwcr9_el1, x14
> -	msr	dbgwcr8_el1, x13
> -	msr	dbgwcr7_el1, x12
> -	msr	dbgwcr6_el1, x11
> -	msr	dbgwcr5_el1, x10
> -	msr	dbgwcr4_el1, x9
> -	msr	dbgwcr3_el1, x8
> -	msr	dbgwcr2_el1, x7
> -	msr	dbgwcr1_el1, x6
> -	msr	dbgwcr0_el1, x5
> -
> -	add	x3, x2, #CPU_SYSREG_OFFSET(DBGWVR0_EL1)
> -
> -	adr	x26, 1f
> -	add	x26, x26, x25, lsl #2
> -	br	x26
> -1:
> -	ldr	x20, [x3, #(15 * 8)]
> -	ldr	x19, [x3, #(14 * 8)]
> -	ldr	x18, [x3, #(13 * 8)]
> -	ldr	x17, [x3, #(12 * 8)]
> -	ldr	x16, [x3, #(11 * 8)]
> -	ldr	x15, [x3, #(10 * 8)]
> -	ldr	x14, [x3, #(9 * 8)]
> -	ldr	x13, [x3, #(8 * 8)]
> -	ldr	x12, [x3, #(7 * 8)]
> -	ldr	x11, [x3, #(6 * 8)]
> -	ldr	x10, [x3, #(5 * 8)]
> -	ldr	x9, [x3, #(4 * 8)]
> -	ldr	x8, [x3, #(3 * 8)]
> -	ldr	x7, [x3, #(2 * 8)]
> -	ldr	x6, [x3, #(1 * 8)]
> -	ldr	x5, [x3, #(0 * 8)]
> -
> -	adr	x26, 1f
> -	add	x26, x26, x25, lsl #2
> -	br	x26
> -1:
> -	msr	dbgwvr15_el1, x20
> -	msr	dbgwvr14_el1, x19
> -	msr	dbgwvr13_el1, x18
> -	msr	dbgwvr12_el1, x17
> -	msr	dbgwvr11_el1, x16
> -	msr	dbgwvr10_el1, x15
> -	msr	dbgwvr9_el1, x14
> -	msr	dbgwvr8_el1, x13
> -	msr	dbgwvr7_el1, x12
> -	msr	dbgwvr6_el1, x11
> -	msr	dbgwvr5_el1, x10
> -	msr	dbgwvr4_el1, x9
> -	msr	dbgwvr3_el1, x8
> -	msr	dbgwvr2_el1, x7
> -	msr	dbgwvr1_el1, x6
> -	msr	dbgwvr0_el1, x5
> -
> -	ldr	x21, [x2, #CPU_SYSREG_OFFSET(MDCCINT_EL1)]
> -	msr	mdccint_el1, x21
> +	msr	\type\()15_el1, x20
> +	msr	\type\()14_el1, x19
> +	msr	\type\()13_el1, x18
> +	msr	\type\()12_el1, x17
> +	msr	\type\()11_el1, x16
> +	msr	\type\()10_el1, x15
> +	msr	\type\()9_el1, x14
> +	msr	\type\()8_el1, x13
> +	msr	\type\()7_el1, x12
> +	msr	\type\()6_el1, x11
> +	msr	\type\()5_el1, x10
> +	msr	\type\()4_el1, x9
> +	msr	\type\()3_el1, x8
> +	msr	\type\()2_el1, x7
> +	msr	\type\()1_el1, x6
> +	msr	\type\()0_el1, x5
>  .endm
>  
>  .macro skip_32bit_state tmp, target
> @@ -929,8 +785,34 @@ __save_debug:
>  	save_debug
>  	ret
>  
> +/* Restore saved guest debug state */
>  __restore_debug:
> -	restore_debug
> +	// x2: base address for cpu context
> +	// x3: ptr to guest registers passed to setup_debug_registers

I'm not sure we need to describe x3 here, as it's input for
setup_debug_registers, and thus described there. If it's preferred
though, then we should also describe x4.

> +	// x5..x20/x26: trashed

And x3 and x4, and x21,x24,x25

> +
> +	mrs	x26, id_aa64dfr0_el1
> +	ubfx	x24, x26, #12, #4	// Extract BRPs
> +	ubfx	x25, x26, #20, #4	// Extract WRPs
> +	mov	w26, #15
> +	sub	w24, w26, w24		// How many BPs to skip
> +	sub	w25, w26, w25		// How many WPs to skip
> +
> +	mov     x4, x24
> +	add	x3, x2, #CPU_SYSREG_OFFSET(DBGBCR0_EL1)
> +	setup_debug_registers dbgbcr
> +	add	x3, x2, #CPU_SYSREG_OFFSET(DBGBVR0_EL1)
> +	setup_debug_registers dbgbvr
> +
> +	mov     x4, x25
> +	add	x3, x2, #CPU_SYSREG_OFFSET(DBGWCR0_EL1)
> +	setup_debug_registers dbgwcr
> +	add	x3, x2, #CPU_SYSREG_OFFSET(DBGWVR0_EL1)
> +	setup_debug_registers dbgwvr
> +
> +	ldr	x21, [x2, #CPU_SYSREG_OFFSET(MDCCINT_EL1)]
> +	msr	mdccint_el1, x21
> +
>  	ret


How about doing the same refactoring on save_debug just for
consistency and code shrinkage?

>  
>  __save_fpsimd:
> -- 
> 2.1.3
> 
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 6/7] KVM: arm64: re-factor hyp.S debug register code

[Christoffer Dall]

On Tue, Nov 25, 2014 at 04:10:04PM +0000, Alex Bennée wrote:
> This is a pre-cursor to sharing the code with the guest debug support.
> This replaces the big macro that fishes data out of a fixed location
> with a more general helper macro to restore a set of debug registers. It
> uses macro substitution so it can be re-used for debug control and value
> registers. It does however rely on the debug registers being 64 bit
> aligned (as they happen to be in the hyp ABI).

can you enforce that somewhere?

> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
> index c0bc218..b38ce3d 100644
> --- a/arch/arm64/kvm/hyp.S
> +++ b/arch/arm64/kvm/hyp.S
> @@ -490,65 +490,12 @@
>  	msr	mdscr_el1,	x25
>  .endm
>  
> -.macro restore_debug
> -	// x2: base address for cpu context
> -	// x3: tmp register
> -
> -	mrs	x26, id_aa64dfr0_el1
> -	ubfx	x24, x26, #12, #4	// Extract BRPs
> -	ubfx	x25, x26, #20, #4	// Extract WRPs
> -	mov	w26, #15
> -	sub	w24, w26, w24		// How many BPs to skip
> -	sub	w25, w26, w25		// How many WPs to skip
> -
> -	add	x3, x2, #CPU_SYSREG_OFFSET(DBGBCR0_EL1)
> -
> -	adr	x26, 1f
> -	add	x26, x26, x24, lsl #2
> -	br	x26
> -1:
> -	ldr	x20, [x3, #(15 * 8)]
> -	ldr	x19, [x3, #(14 * 8)]
> -	ldr	x18, [x3, #(13 * 8)]
> -	ldr	x17, [x3, #(12 * 8)]
> -	ldr	x16, [x3, #(11 * 8)]
> -	ldr	x15, [x3, #(10 * 8)]
> -	ldr	x14, [x3, #(9 * 8)]
> -	ldr	x13, [x3, #(8 * 8)]
> -	ldr	x12, [x3, #(7 * 8)]
> -	ldr	x11, [x3, #(6 * 8)]
> -	ldr	x10, [x3, #(5 * 8)]
> -	ldr	x9, [x3, #(4 * 8)]
> -	ldr	x8, [x3, #(3 * 8)]
> -	ldr	x7, [x3, #(2 * 8)]
> -	ldr	x6, [x3, #(1 * 8)]
> -	ldr	x5, [x3, #(0 * 8)]
> -
> +.macro setup_debug_registers type
> +        // x3: pointer to register set
> +        // x4: number of registers to copy
> +        // x5..x20/x26 trashed

does this mean x5 through x20, and x26, get trashed or that potentially
x5 thgough x26 get trashed?  I'm assuming the first, so replace the
slash with 'and' would be more clear.

>  	adr	x26, 1f
> -	add	x26, x26, x24, lsl #2
> -	br	x26
> -1:
> -	msr	dbgbcr15_el1, x20
> -	msr	dbgbcr14_el1, x19
> -	msr	dbgbcr13_el1, x18
> -	msr	dbgbcr12_el1, x17
> -	msr	dbgbcr11_el1, x16
> -	msr	dbgbcr10_el1, x15
> -	msr	dbgbcr9_el1, x14
> -	msr	dbgbcr8_el1, x13
> -	msr	dbgbcr7_el1, x12
> -	msr	dbgbcr6_el1, x11
> -	msr	dbgbcr5_el1, x10
> -	msr	dbgbcr4_el1, x9
> -	msr	dbgbcr3_el1, x8
> -	msr	dbgbcr2_el1, x7
> -	msr	dbgbcr1_el1, x6
> -	msr	dbgbcr0_el1, x5
> -
> -	add	x3, x2, #CPU_SYSREG_OFFSET(DBGBVR0_EL1)
> -
> -	adr	x26, 1f
> -	add	x26, x26, x24, lsl #2
> +	add	x26, x26, x4, lsl #2
>  	br	x26
>  1:
>  	ldr	x20, [x3, #(15 * 8)]
> @@ -569,116 +516,25 @@
>  	ldr	x5, [x3, #(0 * 8)]
>  
>  	adr	x26, 1f
> -	add	x26, x26, x24, lsl #2
> -	br	x26
> -1:
> -	msr	dbgbvr15_el1, x20
> -	msr	dbgbvr14_el1, x19
> -	msr	dbgbvr13_el1, x18
> -	msr	dbgbvr12_el1, x17
> -	msr	dbgbvr11_el1, x16
> -	msr	dbgbvr10_el1, x15
> -	msr	dbgbvr9_el1, x14
> -	msr	dbgbvr8_el1, x13
> -	msr	dbgbvr7_el1, x12
> -	msr	dbgbvr6_el1, x11
> -	msr	dbgbvr5_el1, x10
> -	msr	dbgbvr4_el1, x9
> -	msr	dbgbvr3_el1, x8
> -	msr	dbgbvr2_el1, x7
> -	msr	dbgbvr1_el1, x6
> -	msr	dbgbvr0_el1, x5
> -
> -	add	x3, x2, #CPU_SYSREG_OFFSET(DBGWCR0_EL1)
> -
> -	adr	x26, 1f
> -	add	x26, x26, x25, lsl #2
> +	add	x26, x26, x4, lsl #2
>  	br	x26
>  1:
> -	ldr	x20, [x3, #(15 * 8)]
> -	ldr	x19, [x3, #(14 * 8)]
> -	ldr	x18, [x3, #(13 * 8)]
> -	ldr	x17, [x3, #(12 * 8)]
> -	ldr	x16, [x3, #(11 * 8)]
> -	ldr	x15, [x3, #(10 * 8)]
> -	ldr	x14, [x3, #(9 * 8)]
> -	ldr	x13, [x3, #(8 * 8)]
> -	ldr	x12, [x3, #(7 * 8)]
> -	ldr	x11, [x3, #(6 * 8)]
> -	ldr	x10, [x3, #(5 * 8)]
> -	ldr	x9, [x3, #(4 * 8)]
> -	ldr	x8, [x3, #(3 * 8)]
> -	ldr	x7, [x3, #(2 * 8)]
> -	ldr	x6, [x3, #(1 * 8)]
> -	ldr	x5, [x3, #(0 * 8)]
> -
> -	adr	x26, 1f
> -	add	x26, x26, x25, lsl #2
> -	br	x26
> -1:
> -	msr	dbgwcr15_el1, x20
> -	msr	dbgwcr14_el1, x19
> -	msr	dbgwcr13_el1, x18
> -	msr	dbgwcr12_el1, x17
> -	msr	dbgwcr11_el1, x16
> -	msr	dbgwcr10_el1, x15
> -	msr	dbgwcr9_el1, x14
> -	msr	dbgwcr8_el1, x13
> -	msr	dbgwcr7_el1, x12
> -	msr	dbgwcr6_el1, x11
> -	msr	dbgwcr5_el1, x10
> -	msr	dbgwcr4_el1, x9
> -	msr	dbgwcr3_el1, x8
> -	msr	dbgwcr2_el1, x7
> -	msr	dbgwcr1_el1, x6
> -	msr	dbgwcr0_el1, x5
> -
> -	add	x3, x2, #CPU_SYSREG_OFFSET(DBGWVR0_EL1)
> -
> -	adr	x26, 1f
> -	add	x26, x26, x25, lsl #2
> -	br	x26
> -1:
> -	ldr	x20, [x3, #(15 * 8)]
> -	ldr	x19, [x3, #(14 * 8)]
> -	ldr	x18, [x3, #(13 * 8)]
> -	ldr	x17, [x3, #(12 * 8)]
> -	ldr	x16, [x3, #(11 * 8)]
> -	ldr	x15, [x3, #(10 * 8)]
> -	ldr	x14, [x3, #(9 * 8)]
> -	ldr	x13, [x3, #(8 * 8)]
> -	ldr	x12, [x3, #(7 * 8)]
> -	ldr	x11, [x3, #(6 * 8)]
> -	ldr	x10, [x3, #(5 * 8)]
> -	ldr	x9, [x3, #(4 * 8)]
> -	ldr	x8, [x3, #(3 * 8)]
> -	ldr	x7, [x3, #(2 * 8)]
> -	ldr	x6, [x3, #(1 * 8)]
> -	ldr	x5, [x3, #(0 * 8)]
> -
> -	adr	x26, 1f
> -	add	x26, x26, x25, lsl #2
> -	br	x26
> -1:
> -	msr	dbgwvr15_el1, x20
> -	msr	dbgwvr14_el1, x19
> -	msr	dbgwvr13_el1, x18
> -	msr	dbgwvr12_el1, x17
> -	msr	dbgwvr11_el1, x16
> -	msr	dbgwvr10_el1, x15
> -	msr	dbgwvr9_el1, x14
> -	msr	dbgwvr8_el1, x13
> -	msr	dbgwvr7_el1, x12
> -	msr	dbgwvr6_el1, x11
> -	msr	dbgwvr5_el1, x10
> -	msr	dbgwvr4_el1, x9
> -	msr	dbgwvr3_el1, x8
> -	msr	dbgwvr2_el1, x7
> -	msr	dbgwvr1_el1, x6
> -	msr	dbgwvr0_el1, x5
> -
> -	ldr	x21, [x2, #CPU_SYSREG_OFFSET(MDCCINT_EL1)]
> -	msr	mdccint_el1, x21
> +	msr	\type\()15_el1, x20
> +	msr	\type\()14_el1, x19
> +	msr	\type\()13_el1, x18
> +	msr	\type\()12_el1, x17
> +	msr	\type\()11_el1, x16
> +	msr	\type\()10_el1, x15
> +	msr	\type\()9_el1, x14
> +	msr	\type\()8_el1, x13
> +	msr	\type\()7_el1, x12
> +	msr	\type\()6_el1, x11
> +	msr	\type\()5_el1, x10
> +	msr	\type\()4_el1, x9
> +	msr	\type\()3_el1, x8
> +	msr	\type\()2_el1, x7
> +	msr	\type\()1_el1, x6
> +	msr	\type\()0_el1, x5
>  .endm
>  
>  .macro skip_32bit_state tmp, target
> @@ -929,8 +785,34 @@ __save_debug:
>  	save_debug
>  	ret
>  
> +/* Restore saved guest debug state */
>  __restore_debug:
> -	restore_debug
> +	// x2: base address for cpu context
> +	// x3: ptr to guest registers passed to setup_debug_registers
> +	// x5..x20/x26: trashed
> +
> +	mrs	x26, id_aa64dfr0_el1
> +	ubfx	x24, x26, #12, #4	// Extract BRPs
> +	ubfx	x25, x26, #20, #4	// Extract WRPs
> +	mov	w26, #15
> +	sub	w24, w26, w24		// How many BPs to skip
> +	sub	w25, w26, w25		// How many WPs to skip
> +
> +	mov     x4, x24
> +	add	x3, x2, #CPU_SYSREG_OFFSET(DBGBCR0_EL1)
> +	setup_debug_registers dbgbcr
> +	add	x3, x2, #CPU_SYSREG_OFFSET(DBGBVR0_EL1)
> +	setup_debug_registers dbgbvr
> +
> +	mov     x4, x25
> +	add	x3, x2, #CPU_SYSREG_OFFSET(DBGWCR0_EL1)
> +	setup_debug_registers dbgwcr
> +	add	x3, x2, #CPU_SYSREG_OFFSET(DBGWVR0_EL1)
> +	setup_debug_registers dbgwvr
> +
> +	ldr	x21, [x2, #CPU_SYSREG_OFFSET(MDCCINT_EL1)]
> +	msr	mdccint_el1, x21
> +
>  	ret
>  
>  __save_fpsimd:
> -- 
> 2.1.3
> 

Re: [PATCH 6/7] KVM: arm64: re-factor hyp.S debug register code

[Alex Bennée]

Christoffer Dall <christoffer.dall@linaro.org> writes:

> On Tue, Nov 25, 2014 at 04:10:04PM +0000, Alex Bennée wrote:
>> This is a pre-cursor to sharing the code with the guest debug support.
>> This replaces the big macro that fishes data out of a fixed location
>> with a more general helper macro to restore a set of debug registers. It
>> uses macro substitution so it can be re-used for debug control and value
>> registers. It does however rely on the debug registers being 64 bit
>> aligned (as they happen to be in the hyp ABI).
>
> can you enforce that somewhere?

There is a comment in kvm_asm.h:

/*
 * 0 is reserved as an invalid value.
 * Order *must* be kept in sync with the hyp switch code.
 */

But I'm not sure how to enforce it in assembly. Is there a #pragma or
something I can use?

>
>> 
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> 
>> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
>> index c0bc218..b38ce3d 100644
>> --- a/arch/arm64/kvm/hyp.S
>> +++ b/arch/arm64/kvm/hyp.S
>> @@ -490,65 +490,12 @@
>>  	msr	mdscr_el1,	x25
>>  .endm
>>  
>> -.macro restore_debug
>> -	// x2: base address for cpu context
>> -	// x3: tmp register
>> -
>> -	mrs	x26, id_aa64dfr0_el1
>> -	ubfx	x24, x26, #12, #4	// Extract BRPs
>> -	ubfx	x25, x26, #20, #4	// Extract WRPs
>> -	mov	w26, #15
>> -	sub	w24, w26, w24		// How many BPs to skip
>> -	sub	w25, w26, w25		// How many WPs to skip
>> -
>> -	add	x3, x2, #CPU_SYSREG_OFFSET(DBGBCR0_EL1)
>> -
>> -	adr	x26, 1f
>> -	add	x26, x26, x24, lsl #2
>> -	br	x26
>> -1:
>> -	ldr	x20, [x3, #(15 * 8)]
>> -	ldr	x19, [x3, #(14 * 8)]
>> -	ldr	x18, [x3, #(13 * 8)]
>> -	ldr	x17, [x3, #(12 * 8)]
>> -	ldr	x16, [x3, #(11 * 8)]
>> -	ldr	x15, [x3, #(10 * 8)]
>> -	ldr	x14, [x3, #(9 * 8)]
>> -	ldr	x13, [x3, #(8 * 8)]
>> -	ldr	x12, [x3, #(7 * 8)]
>> -	ldr	x11, [x3, #(6 * 8)]
>> -	ldr	x10, [x3, #(5 * 8)]
>> -	ldr	x9, [x3, #(4 * 8)]
>> -	ldr	x8, [x3, #(3 * 8)]
>> -	ldr	x7, [x3, #(2 * 8)]
>> -	ldr	x6, [x3, #(1 * 8)]
>> -	ldr	x5, [x3, #(0 * 8)]
>> -
>> +.macro setup_debug_registers type
>> +        // x3: pointer to register set
>> +        // x4: number of registers to copy
>> +        // x5..x20/x26 trashed
>
> does this mean x5 through x20, and x26, get trashed or that potentially
> x5 thgough x26 get trashed?  I'm assuming the first, so replace the
> slash with 'and' would be more clear.
>
>>  	adr	x26, 1f
>> -	add	x26, x26, x24, lsl #2
>> -	br	x26
>> -1:
>> -	msr	dbgbcr15_el1, x20
>> -	msr	dbgbcr14_el1, x19
>> -	msr	dbgbcr13_el1, x18
>> -	msr	dbgbcr12_el1, x17
>> -	msr	dbgbcr11_el1, x16
>> -	msr	dbgbcr10_el1, x15
>> -	msr	dbgbcr9_el1, x14
>> -	msr	dbgbcr8_el1, x13
>> -	msr	dbgbcr7_el1, x12
>> -	msr	dbgbcr6_el1, x11
>> -	msr	dbgbcr5_el1, x10
>> -	msr	dbgbcr4_el1, x9
>> -	msr	dbgbcr3_el1, x8
>> -	msr	dbgbcr2_el1, x7
>> -	msr	dbgbcr1_el1, x6
>> -	msr	dbgbcr0_el1, x5
>> -
>> -	add	x3, x2, #CPU_SYSREG_OFFSET(DBGBVR0_EL1)
>> -
>> -	adr	x26, 1f
>> -	add	x26, x26, x24, lsl #2
>> +	add	x26, x26, x4, lsl #2
>>  	br	x26
>>  1:
>>  	ldr	x20, [x3, #(15 * 8)]
>> @@ -569,116 +516,25 @@
>>  	ldr	x5, [x3, #(0 * 8)]
>>  
>>  	adr	x26, 1f
>> -	add	x26, x26, x24, lsl #2
>> -	br	x26
>> -1:
>> -	msr	dbgbvr15_el1, x20
>> -	msr	dbgbvr14_el1, x19
>> -	msr	dbgbvr13_el1, x18
>> -	msr	dbgbvr12_el1, x17
>> -	msr	dbgbvr11_el1, x16
>> -	msr	dbgbvr10_el1, x15
>> -	msr	dbgbvr9_el1, x14
>> -	msr	dbgbvr8_el1, x13
>> -	msr	dbgbvr7_el1, x12
>> -	msr	dbgbvr6_el1, x11
>> -	msr	dbgbvr5_el1, x10
>> -	msr	dbgbvr4_el1, x9
>> -	msr	dbgbvr3_el1, x8
>> -	msr	dbgbvr2_el1, x7
>> -	msr	dbgbvr1_el1, x6
>> -	msr	dbgbvr0_el1, x5
>> -
>> -	add	x3, x2, #CPU_SYSREG_OFFSET(DBGWCR0_EL1)
>> -
>> -	adr	x26, 1f
>> -	add	x26, x26, x25, lsl #2
>> +	add	x26, x26, x4, lsl #2
>>  	br	x26
>>  1:
>> -	ldr	x20, [x3, #(15 * 8)]
>> -	ldr	x19, [x3, #(14 * 8)]
>> -	ldr	x18, [x3, #(13 * 8)]
>> -	ldr	x17, [x3, #(12 * 8)]
>> -	ldr	x16, [x3, #(11 * 8)]
>> -	ldr	x15, [x3, #(10 * 8)]
>> -	ldr	x14, [x3, #(9 * 8)]
>> -	ldr	x13, [x3, #(8 * 8)]
>> -	ldr	x12, [x3, #(7 * 8)]
>> -	ldr	x11, [x3, #(6 * 8)]
>> -	ldr	x10, [x3, #(5 * 8)]
>> -	ldr	x9, [x3, #(4 * 8)]
>> -	ldr	x8, [x3, #(3 * 8)]
>> -	ldr	x7, [x3, #(2 * 8)]
>> -	ldr	x6, [x3, #(1 * 8)]
>> -	ldr	x5, [x3, #(0 * 8)]
>> -
>> -	adr	x26, 1f
>> -	add	x26, x26, x25, lsl #2
>> -	br	x26
>> -1:
>> -	msr	dbgwcr15_el1, x20
>> -	msr	dbgwcr14_el1, x19
>> -	msr	dbgwcr13_el1, x18
>> -	msr	dbgwcr12_el1, x17
>> -	msr	dbgwcr11_el1, x16
>> -	msr	dbgwcr10_el1, x15
>> -	msr	dbgwcr9_el1, x14
>> -	msr	dbgwcr8_el1, x13
>> -	msr	dbgwcr7_el1, x12
>> -	msr	dbgwcr6_el1, x11
>> -	msr	dbgwcr5_el1, x10
>> -	msr	dbgwcr4_el1, x9
>> -	msr	dbgwcr3_el1, x8
>> -	msr	dbgwcr2_el1, x7
>> -	msr	dbgwcr1_el1, x6
>> -	msr	dbgwcr0_el1, x5
>> -
>> -	add	x3, x2, #CPU_SYSREG_OFFSET(DBGWVR0_EL1)
>> -
>> -	adr	x26, 1f
>> -	add	x26, x26, x25, lsl #2
>> -	br	x26
>> -1:
>> -	ldr	x20, [x3, #(15 * 8)]
>> -	ldr	x19, [x3, #(14 * 8)]
>> -	ldr	x18, [x3, #(13 * 8)]
>> -	ldr	x17, [x3, #(12 * 8)]
>> -	ldr	x16, [x3, #(11 * 8)]
>> -	ldr	x15, [x3, #(10 * 8)]
>> -	ldr	x14, [x3, #(9 * 8)]
>> -	ldr	x13, [x3, #(8 * 8)]
>> -	ldr	x12, [x3, #(7 * 8)]
>> -	ldr	x11, [x3, #(6 * 8)]
>> -	ldr	x10, [x3, #(5 * 8)]
>> -	ldr	x9, [x3, #(4 * 8)]
>> -	ldr	x8, [x3, #(3 * 8)]
>> -	ldr	x7, [x3, #(2 * 8)]
>> -	ldr	x6, [x3, #(1 * 8)]
>> -	ldr	x5, [x3, #(0 * 8)]
>> -
>> -	adr	x26, 1f
>> -	add	x26, x26, x25, lsl #2
>> -	br	x26
>> -1:
>> -	msr	dbgwvr15_el1, x20
>> -	msr	dbgwvr14_el1, x19
>> -	msr	dbgwvr13_el1, x18
>> -	msr	dbgwvr12_el1, x17
>> -	msr	dbgwvr11_el1, x16
>> -	msr	dbgwvr10_el1, x15
>> -	msr	dbgwvr9_el1, x14
>> -	msr	dbgwvr8_el1, x13
>> -	msr	dbgwvr7_el1, x12
>> -	msr	dbgwvr6_el1, x11
>> -	msr	dbgwvr5_el1, x10
>> -	msr	dbgwvr4_el1, x9
>> -	msr	dbgwvr3_el1, x8
>> -	msr	dbgwvr2_el1, x7
>> -	msr	dbgwvr1_el1, x6
>> -	msr	dbgwvr0_el1, x5
>> -
>> -	ldr	x21, [x2, #CPU_SYSREG_OFFSET(MDCCINT_EL1)]
>> -	msr	mdccint_el1, x21
>> +	msr	\type\()15_el1, x20
>> +	msr	\type\()14_el1, x19
>> +	msr	\type\()13_el1, x18
>> +	msr	\type\()12_el1, x17
>> +	msr	\type\()11_el1, x16
>> +	msr	\type\()10_el1, x15
>> +	msr	\type\()9_el1, x14
>> +	msr	\type\()8_el1, x13
>> +	msr	\type\()7_el1, x12
>> +	msr	\type\()6_el1, x11
>> +	msr	\type\()5_el1, x10
>> +	msr	\type\()4_el1, x9
>> +	msr	\type\()3_el1, x8
>> +	msr	\type\()2_el1, x7
>> +	msr	\type\()1_el1, x6
>> +	msr	\type\()0_el1, x5
>>  .endm
>>  
>>  .macro skip_32bit_state tmp, target
>> @@ -929,8 +785,34 @@ __save_debug:
>>  	save_debug
>>  	ret
>>  
>> +/* Restore saved guest debug state */
>>  __restore_debug:
>> -	restore_debug
>> +	// x2: base address for cpu context
>> +	// x3: ptr to guest registers passed to setup_debug_registers
>> +	// x5..x20/x26: trashed
>> +
>> +	mrs	x26, id_aa64dfr0_el1
>> +	ubfx	x24, x26, #12, #4	// Extract BRPs
>> +	ubfx	x25, x26, #20, #4	// Extract WRPs
>> +	mov	w26, #15
>> +	sub	w24, w26, w24		// How many BPs to skip
>> +	sub	w25, w26, w25		// How many WPs to skip
>> +
>> +	mov     x4, x24
>> +	add	x3, x2, #CPU_SYSREG_OFFSET(DBGBCR0_EL1)
>> +	setup_debug_registers dbgbcr
>> +	add	x3, x2, #CPU_SYSREG_OFFSET(DBGBVR0_EL1)
>> +	setup_debug_registers dbgbvr
>> +
>> +	mov     x4, x25
>> +	add	x3, x2, #CPU_SYSREG_OFFSET(DBGWCR0_EL1)
>> +	setup_debug_registers dbgwcr
>> +	add	x3, x2, #CPU_SYSREG_OFFSET(DBGWVR0_EL1)
>> +	setup_debug_registers dbgwvr
>> +
>> +	ldr	x21, [x2, #CPU_SYSREG_OFFSET(MDCCINT_EL1)]
>> +	msr	mdccint_el1, x21
>> +
>>  	ret
>>  
>>  __save_fpsimd:
>> -- 
>> 2.1.3
>> 

-- 
Alex Bennée

Re: [PATCH 6/7] KVM: arm64: re-factor hyp.S debug register code

[Christoffer Dall]

On Mon, Dec 01, 2014 at 11:52:44AM +0000, Alex Bennée wrote:
> 
> Christoffer Dall <christoffer.dall@linaro.org> writes:
> 
> > On Tue, Nov 25, 2014 at 04:10:04PM +0000, Alex Bennée wrote:
> >> This is a pre-cursor to sharing the code with the guest debug support.
> >> This replaces the big macro that fishes data out of a fixed location
> >> with a more general helper macro to restore a set of debug registers. It
> >> uses macro substitution so it can be re-used for debug control and value
> >> registers. It does however rely on the debug registers being 64 bit
> >> aligned (as they happen to be in the hyp ABI).
> >
> > can you enforce that somewhere?
> 
> There is a comment in kvm_asm.h:
> 
> /*
>  * 0 is reserved as an invalid value.
>  * Order *must* be kept in sync with the hyp switch code.
>  */
> 
> But I'm not sure how to enforce it in assembly. Is there a #pragma or
> something I can use?
> 

You can add a BUG_ON somewhere at runtime, but I wouldn't bother, you
can stick a note in that existing comment just so people don't change
the declaration of the registers to be 32-bit aligned or something else.

-Christoffer

[PATCH 7/7] KVM: arm64: guest debug, HW assisted debug support

[Alex Bennée]

This adds support for userspace to control the HW debug registers for
guest debug. We'll only copy the $ARCH defined number across as that's
all that hyp.S will use anyway. I've moved some helper functions into
the hw_breakpoint.h header for re-use.

As with single step we need to tweak the guest registers to enable the
exceptions but we don't want to overwrite the guest copy of these
registers so this is done close to the guest entry.

Two new capabilities have been added to the KVM_EXTENSION ioctl to allow
userspace to query the number of hardware break and watch points
available on the host hardware.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index 9383359..5e8c673 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -2593,7 +2593,7 @@ The top 16 bits of the control field are architecture specific control
 flags which can include the following:
 
   - KVM_GUESTDBG_USE_SW_BP:     using software breakpoints [x86, arm64]
-  - KVM_GUESTDBG_USE_HW_BP:     using hardware breakpoints [x86, s390]
+  - KVM_GUESTDBG_USE_HW_BP:     using hardware breakpoints [x86, s390, arm64]
   - KVM_GUESTDBG_INJECT_DB:     inject DB type exception [x86]
   - KVM_GUESTDBG_INJECT_BP:     inject BP type exception [x86]
   - KVM_GUESTDBG_EXIT_PENDING:  trigger an immediate guest exit [s390]
@@ -2606,7 +2606,10 @@ we need to ensure the guest vCPUs architecture specific registers are
 updated to the correct (supplied) values.
 
 The second part of the structure is architecture specific and
-typically contains a set of debug registers.
+typically contains a set of debug registers. For arm64 the number of
+debug registers is implementation defined and can be determined by
+querying the KVM_CAP_GUEST_DEBUG_HW_BPS and KVM_CAP_GUEST_DEBUG_HW_WPS
+capabilities.
 
 When debug events exit the main run loop with the reason
 KVM_EXIT_DEBUG with the kvm_debug_exit_arch part of the kvm_run
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index a76daae..c8ec23a 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -39,6 +39,7 @@
 #include <asm/cacheflush.h>
 #include <asm/virt.h>
 #include <asm/debug-monitors.h>
+#include <asm/hw_breakpoint.h>
 #include <asm/kvm_arm.h>
 #include <asm/kvm_asm.h>
 #include <asm/kvm_mmu.h>
@@ -341,8 +342,37 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
 
 	/* Hardware assisted Break and Watch points */
 	if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP) {
-		kvm_info("HW BP support requested, not yet implemented\n");
-		return -EINVAL;
+		int i;
+		int nb = get_num_brps();
+		int nw = get_num_wrps();
+
+		/* Copy across up to IMPDEF debug registers to our
+		 * shadow copy in the vcpu structure. The hyp.S code
+		 * will then set them up before we re-enter the guest.
+		 */
+		memcpy(vcpu->arch.guest_debug_regs.dbg_bcr,
+			dbg->arch.dbg_bcr, sizeof(__u64)*nb);
+		memcpy(vcpu->arch.guest_debug_regs.dbg_bvr,
+			dbg->arch.dbg_bvr, sizeof(__u64)*nb);
+		memcpy(vcpu->arch.guest_debug_regs.dbg_wcr,
+			dbg->arch.dbg_wcr, sizeof(__u64)*nw);
+		memcpy(vcpu->arch.guest_debug_regs.dbg_wvr,
+			dbg->arch.dbg_wvr, sizeof(__u64)*nw);
+
+		kvm_info("HW BP support requested\n");
+		for (i = 0; i < nb; i++) {
+			kvm_info("%d: dbg_bcr=0x%llx dbg_bvr=0x%llx\n",
+				 i,
+				vcpu->arch.guest_debug_regs.dbg_bcr[i],
+				vcpu->arch.guest_debug_regs.dbg_bvr[i]);
+		}
+		for (i = 0; i < nw; i++) {
+			kvm_info("%d: dbg_wcr=0x%llx dbg_wvr=0x%llx\n",
+				 i,
+				 vcpu->arch.guest_debug_regs.dbg_wcr[i],
+				 vcpu->arch.guest_debug_regs.dbg_wvr[i]);
+		}
+		route_el2 = true;
 	}
 
 	/* If we are going to handle any debug exceptions we need to
diff --git a/arch/arm64/include/asm/hw_breakpoint.h b/arch/arm64/include/asm/hw_breakpoint.h
index 52b484b..c450552 100644
--- a/arch/arm64/include/asm/hw_breakpoint.h
+++ b/arch/arm64/include/asm/hw_breakpoint.h
@@ -130,6 +130,18 @@ static inline void ptrace_hw_copy_thread(struct task_struct *task)
 }
 #endif
 
+/* Determine number of BRP registers available. */
+static inline int get_num_brps(void)
+{
+	return ((read_cpuid(ID_AA64DFR0_EL1) >> 12) & 0xf) + 1;
+}
+
+/* Determine number of WRP registers available. */
+static inline int get_num_wrps(void)
+{
+	return ((read_cpuid(ID_AA64DFR0_EL1) >> 20) & 0xf) + 1;
+}
+
 extern struct pmu perf_ops_bp;
 
 #endif	/* __KERNEL__ */
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 38b0f07..e386bf4 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -103,8 +103,9 @@ struct kvm_vcpu_arch {
 	/* Exception Information */
 	struct kvm_vcpu_fault_info fault;
 
-	/* Debug state */
+	/* Guest debug state */
 	u64 debug_flags;
+	struct kvm_guest_debug_arch guest_debug_regs;
 
 	/* Pointer to host CPU context */
 	kvm_cpu_context_t *host_cpu_context;
diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
index 78e5ae1..c9ecfd3 100644
--- a/arch/arm64/kernel/asm-offsets.c
+++ b/arch/arm64/kernel/asm-offsets.c
@@ -122,6 +122,10 @@ int main(void)
   DEFINE(VCPU_HPFAR_EL2,	offsetof(struct kvm_vcpu, arch.fault.hpfar_el2));
   DEFINE(VCPU_DEBUG_FLAGS,	offsetof(struct kvm_vcpu, arch.debug_flags));
   DEFINE(GUEST_DEBUG,		offsetof(struct kvm_vcpu, guest_debug));
+  DEFINE(GUEST_DEBUG_BCR,	offsetof(struct kvm_vcpu, arch.guest_debug_regs.dbg_bcr));
+  DEFINE(GUEST_DEBUG_BVR,	offsetof(struct kvm_vcpu, arch.guest_debug_regs.dbg_bvr));
+  DEFINE(GUEST_DEBUG_WCR,	offsetof(struct kvm_vcpu, arch.guest_debug_regs.dbg_wcr));
+  DEFINE(GUEST_DEBUG_WVR,	offsetof(struct kvm_vcpu, arch.guest_debug_regs.dbg_wvr));
   DEFINE(VCPU_HCR_EL2,		offsetof(struct kvm_vcpu, arch.hcr_el2));
   DEFINE(VCPU_MDCR_EL2,	offsetof(struct kvm_vcpu, arch.mdcr_el2));
   DEFINE(VCPU_IRQ_LINES,	offsetof(struct kvm_vcpu, arch.irq_lines));
diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
index df1cf15..45dcc6f 100644
--- a/arch/arm64/kernel/hw_breakpoint.c
+++ b/arch/arm64/kernel/hw_breakpoint.c
@@ -49,18 +49,6 @@ static DEFINE_PER_CPU(int, stepping_kernel_bp);
 static int core_num_brps;
 static int core_num_wrps;
 
-/* Determine number of BRP registers available. */
-static int get_num_brps(void)
-{
-	return ((read_cpuid(ID_AA64DFR0_EL1) >> 12) & 0xf) + 1;
-}
-
-/* Determine number of WRP registers available. */
-static int get_num_wrps(void)
-{
-	return ((read_cpuid(ID_AA64DFR0_EL1) >> 20) & 0xf) + 1;
-}
-
 int hw_breakpoint_slots(int type)
 {
 	/*
diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
index 6def054..d024e47 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -110,6 +110,42 @@ static int kvm_handle_ss(struct kvm_vcpu *vcpu, struct kvm_run *run)
 	return 0;
 }
 
+/**
+ * kvm_handle_hw_bp - handle HW assisted break point
+ *
+ * @vcpu:	the vcpu pointer
+ *
+ */
+static int kvm_handle_hw_bp(struct kvm_vcpu *vcpu, struct kvm_run *run)
+{
+	WARN_ON(!(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP));
+
+	run->exit_reason = KVM_EXIT_DEBUG;
+	run->debug.arch.exit_type = KVM_DEBUG_EXIT_HW_BKPT;
+	run->debug.arch.address = *vcpu_pc(vcpu);
+	return 0;
+}
+
+/**
+ * kvm_handle_watch - handle HW assisted watch point
+ *
+ * @vcpu:	the vcpu pointer
+ *
+ * These are basically the same as breakpoints (and indeed may use the
+ * breakpoint in a linked fashion). However they generate a specific
+ * exception so we trap it here for reporting to the guest.
+ *
+ */
+static int kvm_handle_watch(struct kvm_vcpu *vcpu, struct kvm_run *run)
+{
+	WARN_ON(!(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP));
+
+	run->exit_reason = KVM_EXIT_DEBUG;
+	run->debug.arch.exit_type = KVM_DEBUG_EXIT_HW_WTPT;
+	run->debug.arch.address = *vcpu_pc(vcpu);
+	return 0;
+}
+
 static exit_handle_fn arm_exit_handlers[] = {
 	[ESR_EL2_EC_WFI]	= kvm_handle_wfx,
 	[ESR_EL2_EC_CP15_32]	= kvm_handle_cp15_32,
@@ -125,6 +161,8 @@ static exit_handle_fn arm_exit_handlers[] = {
 	[ESR_EL2_EC_IABT]	= kvm_handle_guest_abort,
 	[ESR_EL2_EC_DABT]	= kvm_handle_guest_abort,
 	[ESR_EL2_EC_SOFTSTP]    = kvm_handle_ss,
+	[ESR_EL2_EC_WATCHPT]	= kvm_handle_watch,
+	[ESR_EL2_EC_BREAKPT]	= kvm_handle_hw_bp,
 	[ESR_EL2_EC_BKPT32]	= kvm_handle_bkpt,
 	[ESR_EL2_EC_BRK64]	= kvm_handle_bkpt,
 };
diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
index b38ce3d..96f71ab 100644
--- a/arch/arm64/kvm/hyp.S
+++ b/arch/arm64/kvm/hyp.S
@@ -18,6 +18,7 @@
 #include <linux/linkage.h>
 #include <linux/kvm.h>
 
+#include <uapi/asm/kvm.h>
 #include <asm/assembler.h>
 #include <asm/memory.h>
 #include <asm/asm-offsets.h>
@@ -174,6 +175,7 @@
 	ldr	x3, [x0, #GUEST_DEBUG]
 	tbz	x3, #KVM_GUESTDBG_ENABLE_SHIFT, 2f	// No guest debug
 
+	// Both Step and HW BP/WP ops need to modify spsr_el2 and mdscr_el1
 	// x0 - preserved as VCPU ptr
 	// x1 - spsr
 	// x2 - mdscr
@@ -191,6 +193,11 @@
 	eor	x1, x1, #DBG_SPSR_SS
 	eor	x2, x2, #DBG_MDSCR_SS
 1:
+	// If we are doing HW BP/WP - set MDSCR_EL1.KDE/MDE
+	tbz	x3, #KVM_GUESTDBG_USE_HW_BP_SHIFT, 3f
+	orr	x2, x2, #DBG_MDSCR_KDE
+	orr	x2, x2, #DBG_MDSCR_MDE
+3:
 	msr	spsr_el2, x1
 	msr	mdscr_el1, x2
 2:
@@ -815,6 +822,33 @@ __restore_debug:
 
 	ret
 
+/* Setup debug state for debug of guest */
+__setup_debug:
+	// x0: vcpu base address
+	// x3: ptr to guest registers passed to setup_debug_registers
+	// x5..x20/x26: trashed
+
+	mrs	x26, id_aa64dfr0_el1
+	ubfx	x24, x26, #12, #4	// Extract BRPs
+	ubfx	x25, x26, #20, #4	// Extract WRPs
+	mov	w26, #15
+	sub	w24, w26, w24		// How many BPs to skip
+	sub	w25, w26, w25		// How many WPs to skip
+
+	mov     x4, x24
+	add	x3, x0, #GUEST_DEBUG_BCR
+	setup_debug_registers dbgbcr
+	add	x3, x0, #GUEST_DEBUG_BVR
+	setup_debug_registers dbgbvr
+
+	mov     x4, x25
+	add	x3, x0, #GUEST_DEBUG_WCR
+	setup_debug_registers dbgwcr
+	add	x3, x0, #GUEST_DEBUG_WVR
+	setup_debug_registers dbgwvr
+
+	ret
+
 __save_fpsimd:
 	save_fpsimd
 	ret
@@ -861,6 +895,13 @@ ENTRY(__kvm_vcpu_run)
 	bl __restore_sysregs
 	bl __restore_fpsimd
 
+        // Now is the time to set-up the debug registers if we
+        // are debugging the guest
+	ldr	x3, [x0, #GUEST_DEBUG]
+	tbz	x3, #KVM_GUESTDBG_USE_HW_BP_SHIFT, 2f
+	bl	__setup_debug
+	b	1f
+2:
 	skip_debug_state x3, 1f
 	bl	__restore_debug
 1:
@@ -881,6 +922,11 @@ __kvm_vcpu_return:
 	bl __save_fpsimd
 	bl __save_sysregs
 
+	// If we are debugging the guest don't save debug registers
+	// otherwise we'll be trashing are only good copy we have.
+	ldr	x3, [x0, #GUEST_DEBUG]
+	tbnz	x3, #KVM_GUESTDBG_USE_HW_BP_SHIFT, 1f
+
 	skip_debug_state x3, 1f
 	bl	__save_debug
 1:
diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
index 70a7816..0de6caa 100644
--- a/arch/arm64/kvm/reset.c
+++ b/arch/arm64/kvm/reset.c
@@ -64,6 +64,12 @@ int kvm_arch_dev_ioctl_check_extension(long ext)
 	case KVM_CAP_ARM_EL1_32BIT:
 		r = cpu_has_32bit_el1();
 		break;
+	case KVM_CAP_GUEST_DEBUG_HW_BPS:
+		r = get_num_brps();
+		break;
+	case KVM_CAP_GUEST_DEBUG_HW_WPS:
+		r  = get_num_wrps();
+		break;
 	default:
 		r = 0;
 	}
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 347e5b0..49a5f97 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -759,6 +759,8 @@ struct kvm_ppc_smmu_info {
 #define KVM_CAP_PPC_FIXUP_HCALL 103
 #define KVM_CAP_PPC_ENABLE_HCALL 104
 #define KVM_CAP_CHECK_EXTENSION_VM 105
+#define KVM_CAP_GUEST_DEBUG_HW_BPS 106
+#define KVM_CAP_GUEST_DEBUG_HW_WPS 107
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
-- 
2.1.3

Re: [PATCH 7/7] KVM: arm64: guest debug, HW assisted debug support

[Andrew Jones]

On Tue, Nov 25, 2014 at 04:10:05PM +0000, Alex Bennée wrote:
> This adds support for userspace to control the HW debug registers for
> guest debug. We'll only copy the $ARCH defined number across as that's
> all that hyp.S will use anyway. I've moved some helper functions into
> the hw_breakpoint.h header for re-use.
> 
> As with single step we need to tweak the guest registers to enable the
> exceptions but we don't want to overwrite the guest copy of these
> registers so this is done close to the guest entry.
> 
> Two new capabilities have been added to the KVM_EXTENSION ioctl to allow
> userspace to query the number of hardware break and watch points
> available on the host hardware.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index 9383359..5e8c673 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -2593,7 +2593,7 @@ The top 16 bits of the control field are architecture specific control
>  flags which can include the following:
>  
>    - KVM_GUESTDBG_USE_SW_BP:     using software breakpoints [x86, arm64]
> -  - KVM_GUESTDBG_USE_HW_BP:     using hardware breakpoints [x86, s390]
> +  - KVM_GUESTDBG_USE_HW_BP:     using hardware breakpoints [x86, s390, arm64]
>    - KVM_GUESTDBG_INJECT_DB:     inject DB type exception [x86]
>    - KVM_GUESTDBG_INJECT_BP:     inject BP type exception [x86]
>    - KVM_GUESTDBG_EXIT_PENDING:  trigger an immediate guest exit [s390]
> @@ -2606,7 +2606,10 @@ we need to ensure the guest vCPUs architecture specific registers are
>  updated to the correct (supplied) values.
>  
>  The second part of the structure is architecture specific and
> -typically contains a set of debug registers.
> +typically contains a set of debug registers. For arm64 the number of
> +debug registers is implementation defined and can be determined by
> +querying the KVM_CAP_GUEST_DEBUG_HW_BPS and KVM_CAP_GUEST_DEBUG_HW_WPS
> +capabilities.
>  
>  When debug events exit the main run loop with the reason
>  KVM_EXIT_DEBUG with the kvm_debug_exit_arch part of the kvm_run
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index a76daae..c8ec23a 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -39,6 +39,7 @@
>  #include <asm/cacheflush.h>
>  #include <asm/virt.h>
>  #include <asm/debug-monitors.h>
> +#include <asm/hw_breakpoint.h>
>  #include <asm/kvm_arm.h>
>  #include <asm/kvm_asm.h>
>  #include <asm/kvm_mmu.h>
> @@ -341,8 +342,37 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>  
>  	/* Hardware assisted Break and Watch points */
>  	if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP) {
> -		kvm_info("HW BP support requested, not yet implemented\n");
> -		return -EINVAL;
> +		int i;
> +		int nb = get_num_brps();
> +		int nw = get_num_wrps();
> +
> +		/* Copy across up to IMPDEF debug registers to our
> +		 * shadow copy in the vcpu structure. The hyp.S code
> +		 * will then set them up before we re-enter the guest.
> +		 */
> +		memcpy(vcpu->arch.guest_debug_regs.dbg_bcr,
> +			dbg->arch.dbg_bcr, sizeof(__u64)*nb);
> +		memcpy(vcpu->arch.guest_debug_regs.dbg_bvr,
> +			dbg->arch.dbg_bvr, sizeof(__u64)*nb);
> +		memcpy(vcpu->arch.guest_debug_regs.dbg_wcr,
> +			dbg->arch.dbg_wcr, sizeof(__u64)*nw);
> +		memcpy(vcpu->arch.guest_debug_regs.dbg_wvr,
> +			dbg->arch.dbg_wvr, sizeof(__u64)*nw);
> +
> +		kvm_info("HW BP support requested\n");
> +		for (i = 0; i < nb; i++) {
> +			kvm_info("%d: dbg_bcr=0x%llx dbg_bvr=0x%llx\n",
> +				 i,
> +				vcpu->arch.guest_debug_regs.dbg_bcr[i],
> +				vcpu->arch.guest_debug_regs.dbg_bvr[i]);
> +		}
> +		for (i = 0; i < nw; i++) {
> +			kvm_info("%d: dbg_wcr=0x%llx dbg_wvr=0x%llx\n",
> +				 i,
> +				 vcpu->arch.guest_debug_regs.dbg_wcr[i],
> +				 vcpu->arch.guest_debug_regs.dbg_wvr[i]);
> +		}

I think we decided to drop the kvm_info's. Here's several more lines of
logging to drop too.

> +		route_el2 = true;
>  	}
>  
>  	/* If we are going to handle any debug exceptions we need to
> diff --git a/arch/arm64/include/asm/hw_breakpoint.h b/arch/arm64/include/asm/hw_breakpoint.h
> index 52b484b..c450552 100644
> --- a/arch/arm64/include/asm/hw_breakpoint.h
> +++ b/arch/arm64/include/asm/hw_breakpoint.h
> @@ -130,6 +130,18 @@ static inline void ptrace_hw_copy_thread(struct task_struct *task)
>  }
>  #endif
>  
> +/* Determine number of BRP registers available. */
> +static inline int get_num_brps(void)
> +{
> +	return ((read_cpuid(ID_AA64DFR0_EL1) >> 12) & 0xf) + 1;
> +}
> +
> +/* Determine number of WRP registers available. */
> +static inline int get_num_wrps(void)
> +{
> +	return ((read_cpuid(ID_AA64DFR0_EL1) >> 20) & 0xf) + 1;
> +}
> +
>  extern struct pmu perf_ops_bp;
>  
>  #endif	/* __KERNEL__ */
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 38b0f07..e386bf4 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -103,8 +103,9 @@ struct kvm_vcpu_arch {
>  	/* Exception Information */
>  	struct kvm_vcpu_fault_info fault;
>  
> -	/* Debug state */
> +	/* Guest debug state */
>  	u64 debug_flags;
> +	struct kvm_guest_debug_arch guest_debug_regs;
>  
>  	/* Pointer to host CPU context */
>  	kvm_cpu_context_t *host_cpu_context;
> diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
> index 78e5ae1..c9ecfd3 100644
> --- a/arch/arm64/kernel/asm-offsets.c
> +++ b/arch/arm64/kernel/asm-offsets.c
> @@ -122,6 +122,10 @@ int main(void)
>    DEFINE(VCPU_HPFAR_EL2,	offsetof(struct kvm_vcpu, arch.fault.hpfar_el2));
>    DEFINE(VCPU_DEBUG_FLAGS,	offsetof(struct kvm_vcpu, arch.debug_flags));
>    DEFINE(GUEST_DEBUG,		offsetof(struct kvm_vcpu, guest_debug));
> +  DEFINE(GUEST_DEBUG_BCR,	offsetof(struct kvm_vcpu, arch.guest_debug_regs.dbg_bcr));
> +  DEFINE(GUEST_DEBUG_BVR,	offsetof(struct kvm_vcpu, arch.guest_debug_regs.dbg_bvr));
> +  DEFINE(GUEST_DEBUG_WCR,	offsetof(struct kvm_vcpu, arch.guest_debug_regs.dbg_wcr));
> +  DEFINE(GUEST_DEBUG_WVR,	offsetof(struct kvm_vcpu, arch.guest_debug_regs.dbg_wvr));
>    DEFINE(VCPU_HCR_EL2,		offsetof(struct kvm_vcpu, arch.hcr_el2));
>    DEFINE(VCPU_MDCR_EL2,	offsetof(struct kvm_vcpu, arch.mdcr_el2));
>    DEFINE(VCPU_IRQ_LINES,	offsetof(struct kvm_vcpu, arch.irq_lines));
> diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
> index df1cf15..45dcc6f 100644
> --- a/arch/arm64/kernel/hw_breakpoint.c
> +++ b/arch/arm64/kernel/hw_breakpoint.c
> @@ -49,18 +49,6 @@ static DEFINE_PER_CPU(int, stepping_kernel_bp);
>  static int core_num_brps;
>  static int core_num_wrps;
>  
> -/* Determine number of BRP registers available. */
> -static int get_num_brps(void)
> -{
> -	return ((read_cpuid(ID_AA64DFR0_EL1) >> 12) & 0xf) + 1;
> -}
> -
> -/* Determine number of WRP registers available. */
> -static int get_num_wrps(void)
> -{
> -	return ((read_cpuid(ID_AA64DFR0_EL1) >> 20) & 0xf) + 1;
> -}
> -
>  int hw_breakpoint_slots(int type)
>  {
>  	/*
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index 6def054..d024e47 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -110,6 +110,42 @@ static int kvm_handle_ss(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  	return 0;
>  }
>  
> +/**
> + * kvm_handle_hw_bp - handle HW assisted break point
> + *
> + * @vcpu:	the vcpu pointer

@run

> + *
> + */
> +static int kvm_handle_hw_bp(struct kvm_vcpu *vcpu, struct kvm_run *run)
> +{
> +	WARN_ON(!(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP));

Another WARN_ON to consider dropping.

> +
> +	run->exit_reason = KVM_EXIT_DEBUG;
> +	run->debug.arch.exit_type = KVM_DEBUG_EXIT_HW_BKPT;
> +	run->debug.arch.address = *vcpu_pc(vcpu);
> +	return 0;
> +}
> +
> +/**
> + * kvm_handle_watch - handle HW assisted watch point
> + *
> + * @vcpu:	the vcpu pointer

@run

> + *
> + * These are basically the same as breakpoints (and indeed may use the
> + * breakpoint in a linked fashion). However they generate a specific
> + * exception so we trap it here for reporting to the guest.
> + *
> + */
> +static int kvm_handle_watch(struct kvm_vcpu *vcpu, struct kvm_run *run)
> +{
> +	WARN_ON(!(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP));
> +

drop WARN_ON?

> +	run->exit_reason = KVM_EXIT_DEBUG;
> +	run->debug.arch.exit_type = KVM_DEBUG_EXIT_HW_WTPT;
> +	run->debug.arch.address = *vcpu_pc(vcpu);
> +	return 0;
> +}
> +
>  static exit_handle_fn arm_exit_handlers[] = {
>  	[ESR_EL2_EC_WFI]	= kvm_handle_wfx,
>  	[ESR_EL2_EC_CP15_32]	= kvm_handle_cp15_32,
> @@ -125,6 +161,8 @@ static exit_handle_fn arm_exit_handlers[] = {
>  	[ESR_EL2_EC_IABT]	= kvm_handle_guest_abort,
>  	[ESR_EL2_EC_DABT]	= kvm_handle_guest_abort,
>  	[ESR_EL2_EC_SOFTSTP]    = kvm_handle_ss,
> +	[ESR_EL2_EC_WATCHPT]	= kvm_handle_watch,
> +	[ESR_EL2_EC_BREAKPT]	= kvm_handle_hw_bp,
>  	[ESR_EL2_EC_BKPT32]	= kvm_handle_bkpt,
>  	[ESR_EL2_EC_BRK64]	= kvm_handle_bkpt,
>  };
> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
> index b38ce3d..96f71ab 100644
> --- a/arch/arm64/kvm/hyp.S
> +++ b/arch/arm64/kvm/hyp.S
> @@ -18,6 +18,7 @@
>  #include <linux/linkage.h>
>  #include <linux/kvm.h>
>  
> +#include <uapi/asm/kvm.h>
>  #include <asm/assembler.h>
>  #include <asm/memory.h>
>  #include <asm/asm-offsets.h>
> @@ -174,6 +175,7 @@
>  	ldr	x3, [x0, #GUEST_DEBUG]
>  	tbz	x3, #KVM_GUESTDBG_ENABLE_SHIFT, 2f	// No guest debug
>  
> +	// Both Step and HW BP/WP ops need to modify spsr_el2 and mdscr_el1

Comment not quite right, since they both don't need to modify both.
Step modifies both. BP/WP modifies mdscr.

>  	// x0 - preserved as VCPU ptr
>  	// x1 - spsr
>  	// x2 - mdscr
> @@ -191,6 +193,11 @@
>  	eor	x1, x1, #DBG_SPSR_SS
>  	eor	x2, x2, #DBG_MDSCR_SS
>  1:
> +	// If we are doing HW BP/WP - set MDSCR_EL1.KDE/MDE
> +	tbz	x3, #KVM_GUESTDBG_USE_HW_BP_SHIFT, 3f
> +	orr	x2, x2, #DBG_MDSCR_KDE
> +	orr	x2, x2, #DBG_MDSCR_MDE

We don't need to make sure KDE/MDE are cleared when we're not doing
BP/WP, as we do with the SS bit?

> +3:
>  	msr	spsr_el2, x1
>  	msr	mdscr_el1, x2
>  2:

Not super pleasant to have the labels occur in 1,3,2 order, but
whatever.

> @@ -815,6 +822,33 @@ __restore_debug:
>  
>  	ret
>  
> +/* Setup debug state for debug of guest */
> +__setup_debug:
> +	// x0: vcpu base address
> +	// x3: ptr to guest registers passed to setup_debug_registers
> +	// x5..x20/x26: trashed

Same comment on this header as for __restore_debug's new header in
a previous patch.

> +
> +	mrs	x26, id_aa64dfr0_el1
> +	ubfx	x24, x26, #12, #4	// Extract BRPs
> +	ubfx	x25, x26, #20, #4	// Extract WRPs
> +	mov	w26, #15
> +	sub	w24, w26, w24		// How many BPs to skip
> +	sub	w25, w26, w25		// How many WPs to skip
> +
> +	mov     x4, x24
> +	add	x3, x0, #GUEST_DEBUG_BCR
> +	setup_debug_registers dbgbcr

Now I see why the name change of restore_debug to setup_debug_registers,
it fits better here in __setup_debug. Should we name is something that
fits both better?

> +	add	x3, x0, #GUEST_DEBUG_BVR
> +	setup_debug_registers dbgbvr
> +
> +	mov     x4, x25
> +	add	x3, x0, #GUEST_DEBUG_WCR
> +	setup_debug_registers dbgwcr
> +	add	x3, x0, #GUEST_DEBUG_WVR
> +	setup_debug_registers dbgwvr
> +
> +	ret
> +
>  __save_fpsimd:
>  	save_fpsimd
>  	ret
> @@ -861,6 +895,13 @@ ENTRY(__kvm_vcpu_run)
>  	bl __restore_sysregs
>  	bl __restore_fpsimd
>  
> +        // Now is the time to set-up the debug registers if we
> +        // are debugging the guest
    ^^^ whitespace

> +	ldr	x3, [x0, #GUEST_DEBUG]
> +	tbz	x3, #KVM_GUESTDBG_USE_HW_BP_SHIFT, 2f
> +	bl	__setup_debug
> +	b	1f
> +2:
>  	skip_debug_state x3, 1f
>  	bl	__restore_debug
>  1:
> @@ -881,6 +922,11 @@ __kvm_vcpu_return:
>  	bl __save_fpsimd
>  	bl __save_sysregs
>  
> +	// If we are debugging the guest don't save debug registers
> +	// otherwise we'll be trashing are only good copy we have.
                                       ^^ the
> +	ldr	x3, [x0, #GUEST_DEBUG]
> +	tbnz	x3, #KVM_GUESTDBG_USE_HW_BP_SHIFT, 1f
> +
>  	skip_debug_state x3, 1f
>  	bl	__save_debug

I feel like there should be a more elegant way to merge the selection of
__setup_debug vs. __restore_debug and skip_debug_state.

>  1:
> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> index 70a7816..0de6caa 100644
> --- a/arch/arm64/kvm/reset.c
> +++ b/arch/arm64/kvm/reset.c
> @@ -64,6 +64,12 @@ int kvm_arch_dev_ioctl_check_extension(long ext)
>  	case KVM_CAP_ARM_EL1_32BIT:
>  		r = cpu_has_32bit_el1();
>  		break;
> +	case KVM_CAP_GUEST_DEBUG_HW_BPS:
> +		r = get_num_brps();
> +		break;
> +	case KVM_CAP_GUEST_DEBUG_HW_WPS:
> +		r  = get_num_wrps();
                  ^ extra space here
> +		break;
>  	default:
>  		r = 0;
>  	}
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 347e5b0..49a5f97 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -759,6 +759,8 @@ struct kvm_ppc_smmu_info {
>  #define KVM_CAP_PPC_FIXUP_HCALL 103
>  #define KVM_CAP_PPC_ENABLE_HCALL 104
>  #define KVM_CAP_CHECK_EXTENSION_VM 105
> +#define KVM_CAP_GUEST_DEBUG_HW_BPS 106
> +#define KVM_CAP_GUEST_DEBUG_HW_WPS 107
>  
>  #ifdef KVM_CAP_IRQ_ROUTING
>  
> -- 
> 2.1.3
> 
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 7/7] KVM: arm64: guest debug, HW assisted debug support

[Christoffer Dall]

On Tue, Nov 25, 2014 at 04:10:05PM +0000, Alex Bennée wrote:
> This adds support for userspace to control the HW debug registers for
> guest debug. We'll only copy the $ARCH defined number across as that's
> all that hyp.S will use anyway. I've moved some helper functions into
> the hw_breakpoint.h header for re-use.
> 
> As with single step we need to tweak the guest registers to enable the
> exceptions but we don't want to overwrite the guest copy of these
> registers so this is done close to the guest entry.
> 
> Two new capabilities have been added to the KVM_EXTENSION ioctl to allow
> userspace to query the number of hardware break and watch points
> available on the host hardware.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index 9383359..5e8c673 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -2593,7 +2593,7 @@ The top 16 bits of the control field are architecture specific control
>  flags which can include the following:
>  
>    - KVM_GUESTDBG_USE_SW_BP:     using software breakpoints [x86, arm64]
> -  - KVM_GUESTDBG_USE_HW_BP:     using hardware breakpoints [x86, s390]
> +  - KVM_GUESTDBG_USE_HW_BP:     using hardware breakpoints [x86, s390, arm64]
>    - KVM_GUESTDBG_INJECT_DB:     inject DB type exception [x86]
>    - KVM_GUESTDBG_INJECT_BP:     inject BP type exception [x86]
>    - KVM_GUESTDBG_EXIT_PENDING:  trigger an immediate guest exit [s390]
> @@ -2606,7 +2606,10 @@ we need to ensure the guest vCPUs architecture specific registers are
>  updated to the correct (supplied) values.
>  
>  The second part of the structure is architecture specific and
> -typically contains a set of debug registers.
> +typically contains a set of debug registers. For arm64 the number of

new paragraph for the arch-specific stuff.

> +debug registers is implementation defined and can be determined by
> +querying the KVM_CAP_GUEST_DEBUG_HW_BPS and KVM_CAP_GUEST_DEBUG_HW_WPS
> +capabilities.
>  
>  When debug events exit the main run loop with the reason
>  KVM_EXIT_DEBUG with the kvm_debug_exit_arch part of the kvm_run
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index a76daae..c8ec23a 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -39,6 +39,7 @@
>  #include <asm/cacheflush.h>
>  #include <asm/virt.h>
>  #include <asm/debug-monitors.h>
> +#include <asm/hw_breakpoint.h>
>  #include <asm/kvm_arm.h>
>  #include <asm/kvm_asm.h>
>  #include <asm/kvm_mmu.h>
> @@ -341,8 +342,37 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>  
>  	/* Hardware assisted Break and Watch points */
>  	if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP) {
> -		kvm_info("HW BP support requested, not yet implemented\n");
> -		return -EINVAL;
> +		int i;
> +		int nb = get_num_brps();
> +		int nw = get_num_wrps();
> +
> +		/* Copy across up to IMPDEF debug registers to our

coding style

> +		 * shadow copy in the vcpu structure. The hyp.S code
> +		 * will then set them up before we re-enter the guest.
> +		 */
> +		memcpy(vcpu->arch.guest_debug_regs.dbg_bcr,
> +			dbg->arch.dbg_bcr, sizeof(__u64)*nb);
> +		memcpy(vcpu->arch.guest_debug_regs.dbg_bvr,
> +			dbg->arch.dbg_bvr, sizeof(__u64)*nb);
> +		memcpy(vcpu->arch.guest_debug_regs.dbg_wcr,
> +			dbg->arch.dbg_wcr, sizeof(__u64)*nw);
> +		memcpy(vcpu->arch.guest_debug_regs.dbg_wvr,
> +			dbg->arch.dbg_wvr, sizeof(__u64)*nw);
> +
> +		kvm_info("HW BP support requested\n");
> +		for (i = 0; i < nb; i++) {
> +			kvm_info("%d: dbg_bcr=0x%llx dbg_bvr=0x%llx\n",
> +				 i,
> +				vcpu->arch.guest_debug_regs.dbg_bcr[i],
> +				vcpu->arch.guest_debug_regs.dbg_bvr[i]);
> +		}
> +		for (i = 0; i < nw; i++) {
> +			kvm_info("%d: dbg_wcr=0x%llx dbg_wvr=0x%llx\n",
> +				 i,
> +				 vcpu->arch.guest_debug_regs.dbg_wcr[i],
> +				 vcpu->arch.guest_debug_regs.dbg_wvr[i]);
> +		}
> +		route_el2 = true;
>  	}
>  
>  	/* If we are going to handle any debug exceptions we need to
> diff --git a/arch/arm64/include/asm/hw_breakpoint.h b/arch/arm64/include/asm/hw_breakpoint.h
> index 52b484b..c450552 100644
> --- a/arch/arm64/include/asm/hw_breakpoint.h
> +++ b/arch/arm64/include/asm/hw_breakpoint.h
> @@ -130,6 +130,18 @@ static inline void ptrace_hw_copy_thread(struct task_struct *task)
>  }
>  #endif
>  
> +/* Determine number of BRP registers available. */
> +static inline int get_num_brps(void)
> +{
> +	return ((read_cpuid(ID_AA64DFR0_EL1) >> 12) & 0xf) + 1;
> +}
> +
> +/* Determine number of WRP registers available. */
> +static inline int get_num_wrps(void)
> +{
> +	return ((read_cpuid(ID_AA64DFR0_EL1) >> 20) & 0xf) + 1;
> +}
> +
>  extern struct pmu perf_ops_bp;
>  
>  #endif	/* __KERNEL__ */
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 38b0f07..e386bf4 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -103,8 +103,9 @@ struct kvm_vcpu_arch {
>  	/* Exception Information */
>  	struct kvm_vcpu_fault_info fault;
>  
> -	/* Debug state */
> +	/* Guest debug state */
>  	u64 debug_flags;
> +	struct kvm_guest_debug_arch guest_debug_regs;
>  
>  	/* Pointer to host CPU context */
>  	kvm_cpu_context_t *host_cpu_context;
> diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
> index 78e5ae1..c9ecfd3 100644
> --- a/arch/arm64/kernel/asm-offsets.c
> +++ b/arch/arm64/kernel/asm-offsets.c
> @@ -122,6 +122,10 @@ int main(void)
>    DEFINE(VCPU_HPFAR_EL2,	offsetof(struct kvm_vcpu, arch.fault.hpfar_el2));
>    DEFINE(VCPU_DEBUG_FLAGS,	offsetof(struct kvm_vcpu, arch.debug_flags));
>    DEFINE(GUEST_DEBUG,		offsetof(struct kvm_vcpu, guest_debug));
> +  DEFINE(GUEST_DEBUG_BCR,	offsetof(struct kvm_vcpu, arch.guest_debug_regs.dbg_bcr));
> +  DEFINE(GUEST_DEBUG_BVR,	offsetof(struct kvm_vcpu, arch.guest_debug_regs.dbg_bvr));
> +  DEFINE(GUEST_DEBUG_WCR,	offsetof(struct kvm_vcpu, arch.guest_debug_regs.dbg_wcr));
> +  DEFINE(GUEST_DEBUG_WVR,	offsetof(struct kvm_vcpu, arch.guest_debug_regs.dbg_wvr));
>    DEFINE(VCPU_HCR_EL2,		offsetof(struct kvm_vcpu, arch.hcr_el2));
>    DEFINE(VCPU_MDCR_EL2,	offsetof(struct kvm_vcpu, arch.mdcr_el2));
>    DEFINE(VCPU_IRQ_LINES,	offsetof(struct kvm_vcpu, arch.irq_lines));
> diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
> index df1cf15..45dcc6f 100644
> --- a/arch/arm64/kernel/hw_breakpoint.c
> +++ b/arch/arm64/kernel/hw_breakpoint.c
> @@ -49,18 +49,6 @@ static DEFINE_PER_CPU(int, stepping_kernel_bp);
>  static int core_num_brps;
>  static int core_num_wrps;
>  
> -/* Determine number of BRP registers available. */
> -static int get_num_brps(void)
> -{
> -	return ((read_cpuid(ID_AA64DFR0_EL1) >> 12) & 0xf) + 1;
> -}
> -
> -/* Determine number of WRP registers available. */
> -static int get_num_wrps(void)
> -{
> -	return ((read_cpuid(ID_AA64DFR0_EL1) >> 20) & 0xf) + 1;
> -}
> -
>  int hw_breakpoint_slots(int type)
>  {
>  	/*
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index 6def054..d024e47 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -110,6 +110,42 @@ static int kvm_handle_ss(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  	return 0;
>  }
>  
> +/**
> + * kvm_handle_hw_bp - handle HW assisted break point
> + *
> + * @vcpu:	the vcpu pointer
> + *
> + */
> +static int kvm_handle_hw_bp(struct kvm_vcpu *vcpu, struct kvm_run *run)
> +{
> +	WARN_ON(!(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP));
> +
> +	run->exit_reason = KVM_EXIT_DEBUG;
> +	run->debug.arch.exit_type = KVM_DEBUG_EXIT_HW_BKPT;
> +	run->debug.arch.address = *vcpu_pc(vcpu);
> +	return 0;
> +}
> +
> +/**
> + * kvm_handle_watch - handle HW assisted watch point
> + *
> + * @vcpu:	the vcpu pointer
> + *
> + * These are basically the same as breakpoints (and indeed may use the
> + * breakpoint in a linked fashion). However they generate a specific
> + * exception so we trap it here for reporting to the guest.
> + *
> + */
> +static int kvm_handle_watch(struct kvm_vcpu *vcpu, struct kvm_run *run)
> +{
> +	WARN_ON(!(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP));
> +
> +	run->exit_reason = KVM_EXIT_DEBUG;
> +	run->debug.arch.exit_type = KVM_DEBUG_EXIT_HW_WTPT;
> +	run->debug.arch.address = *vcpu_pc(vcpu);
> +	return 0;
> +}

the usual comments about the @run documentation and let's get rid of all
these WARN_ON() statements.

> +
>  static exit_handle_fn arm_exit_handlers[] = {
>  	[ESR_EL2_EC_WFI]	= kvm_handle_wfx,
>  	[ESR_EL2_EC_CP15_32]	= kvm_handle_cp15_32,
> @@ -125,6 +161,8 @@ static exit_handle_fn arm_exit_handlers[] = {
>  	[ESR_EL2_EC_IABT]	= kvm_handle_guest_abort,
>  	[ESR_EL2_EC_DABT]	= kvm_handle_guest_abort,
>  	[ESR_EL2_EC_SOFTSTP]    = kvm_handle_ss,
> +	[ESR_EL2_EC_WATCHPT]	= kvm_handle_watch,
> +	[ESR_EL2_EC_BREAKPT]	= kvm_handle_hw_bp,
>  	[ESR_EL2_EC_BKPT32]	= kvm_handle_bkpt,
>  	[ESR_EL2_EC_BRK64]	= kvm_handle_bkpt,
>  };
> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
> index b38ce3d..96f71ab 100644
> --- a/arch/arm64/kvm/hyp.S
> +++ b/arch/arm64/kvm/hyp.S
> @@ -18,6 +18,7 @@
>  #include <linux/linkage.h>
>  #include <linux/kvm.h>
>  
> +#include <uapi/asm/kvm.h>
>  #include <asm/assembler.h>
>  #include <asm/memory.h>
>  #include <asm/asm-offsets.h>
> @@ -174,6 +175,7 @@
>  	ldr	x3, [x0, #GUEST_DEBUG]
>  	tbz	x3, #KVM_GUESTDBG_ENABLE_SHIFT, 2f	// No guest debug
>  
> +	// Both Step and HW BP/WP ops need to modify spsr_el2 and mdscr_el1
>  	// x0 - preserved as VCPU ptr
>  	// x1 - spsr
>  	// x2 - mdscr
> @@ -191,6 +193,11 @@
>  	eor	x1, x1, #DBG_SPSR_SS
>  	eor	x2, x2, #DBG_MDSCR_SS
>  1:
> +	// If we are doing HW BP/WP - set MDSCR_EL1.KDE/MDE
> +	tbz	x3, #KVM_GUESTDBG_USE_HW_BP_SHIFT, 3f
> +	orr	x2, x2, #DBG_MDSCR_KDE
> +	orr	x2, x2, #DBG_MDSCR_MDE
> +3:
>  	msr	spsr_el2, x1
>  	msr	mdscr_el1, x2
>  2:
> @@ -815,6 +822,33 @@ __restore_debug:
>  
>  	ret
>  
> +/* Setup debug state for debug of guest */
> +__setup_debug:
> +	// x0: vcpu base address
> +	// x3: ptr to guest registers passed to setup_debug_registers
> +	// x5..x20/x26: trashed
> +
> +	mrs	x26, id_aa64dfr0_el1
> +	ubfx	x24, x26, #12, #4	// Extract BRPs
> +	ubfx	x25, x26, #20, #4	// Extract WRPs
> +	mov	w26, #15
> +	sub	w24, w26, w24		// How many BPs to skip
> +	sub	w25, w26, w25		// How many WPs to skip
> +
> +	mov     x4, x24
> +	add	x3, x0, #GUEST_DEBUG_BCR
> +	setup_debug_registers dbgbcr
> +	add	x3, x0, #GUEST_DEBUG_BVR
> +	setup_debug_registers dbgbvr
> +
> +	mov     x4, x25
> +	add	x3, x0, #GUEST_DEBUG_WCR
> +	setup_debug_registers dbgwcr
> +	add	x3, x0, #GUEST_DEBUG_WVR
> +	setup_debug_registers dbgwvr
> +
> +	ret
> +
>  __save_fpsimd:
>  	save_fpsimd
>  	ret
> @@ -861,6 +895,13 @@ ENTRY(__kvm_vcpu_run)
>  	bl __restore_sysregs
>  	bl __restore_fpsimd
>  
> +        // Now is the time to set-up the debug registers if we
> +        // are debugging the guest
> +	ldr	x3, [x0, #GUEST_DEBUG]
> +	tbz	x3, #KVM_GUESTDBG_USE_HW_BP_SHIFT, 2f
> +	bl	__setup_debug
> +	b	1f
> +2:
>  	skip_debug_state x3, 1f
>  	bl	__restore_debug
>  1:
> @@ -881,6 +922,11 @@ __kvm_vcpu_return:
>  	bl __save_fpsimd
>  	bl __save_sysregs
>  
> +	// If we are debugging the guest don't save debug registers
> +	// otherwise we'll be trashing are only good copy we have.
> +	ldr	x3, [x0, #GUEST_DEBUG]
> +	tbnz	x3, #KVM_GUESTDBG_USE_HW_BP_SHIFT, 1f
> +

we're introducing an awful lot of conditionals in the assembly code with
these patches, can you re-consider if there's a cleaner abstraction that
allows us to deal with some of this stuff in C-code?

-Christoffer

Re: [PATCH 7/7] KVM: arm64: guest debug, HW assisted debug support

[Alex Bennée]

Christoffer Dall <christoffer.dall@linaro.org> writes:

> On Tue, Nov 25, 2014 at 04:10:05PM +0000, Alex Bennée wrote:
<snip>
>> --- a/arch/arm64/kvm/hyp.S
>> +++ b/arch/arm64/kvm/hyp.S
>> @@ -18,6 +18,7 @@
>>  #include <linux/linkage.h>
>>  #include <linux/kvm.h>
>>  
>> +#include <uapi/asm/kvm.h>
>>  #include <asm/assembler.h>
>>  #include <asm/memory.h>
>>  #include <asm/asm-offsets.h>
>> @@ -174,6 +175,7 @@
>>  	ldr	x3, [x0, #GUEST_DEBUG]
>>  	tbz	x3, #KVM_GUESTDBG_ENABLE_SHIFT, 2f	// No guest debug
>>  
>> +	// Both Step and HW BP/WP ops need to modify spsr_el2 and mdscr_el1
>>  	// x0 - preserved as VCPU ptr
>>  	// x1 - spsr
>>  	// x2 - mdscr
>> @@ -191,6 +193,11 @@
>>  	eor	x1, x1, #DBG_SPSR_SS
>>  	eor	x2, x2, #DBG_MDSCR_SS
>>  1:
>> +	// If we are doing HW BP/WP - set MDSCR_EL1.KDE/MDE
>> +	tbz	x3, #KVM_GUESTDBG_USE_HW_BP_SHIFT, 3f
>> +	orr	x2, x2, #DBG_MDSCR_KDE
>> +	orr	x2, x2, #DBG_MDSCR_MDE
>> +3:
>>  	msr	spsr_el2, x1
>>  	msr	mdscr_el1, x2
>>  2:
>> @@ -815,6 +822,33 @@ __restore_debug:
>>  
>>  	ret
>>  
>> +/* Setup debug state for debug of guest */
>> +__setup_debug:
>> +	// x0: vcpu base address
>> +	// x3: ptr to guest registers passed to setup_debug_registers
>> +	// x5..x20/x26: trashed
>> +
>> +	mrs	x26, id_aa64dfr0_el1
>> +	ubfx	x24, x26, #12, #4	// Extract BRPs
>> +	ubfx	x25, x26, #20, #4	// Extract WRPs
>> +	mov	w26, #15
>> +	sub	w24, w26, w24		// How many BPs to skip
>> +	sub	w25, w26, w25		// How many WPs to skip
>> +
>> +	mov     x4, x24
>> +	add	x3, x0, #GUEST_DEBUG_BCR
>> +	setup_debug_registers dbgbcr
>> +	add	x3, x0, #GUEST_DEBUG_BVR
>> +	setup_debug_registers dbgbvr
>> +
>> +	mov     x4, x25
>> +	add	x3, x0, #GUEST_DEBUG_WCR
>> +	setup_debug_registers dbgwcr
>> +	add	x3, x0, #GUEST_DEBUG_WVR
>> +	setup_debug_registers dbgwvr
>> +
>> +	ret
>> +
>>  __save_fpsimd:
>>  	save_fpsimd
>>  	ret
>> @@ -861,6 +895,13 @@ ENTRY(__kvm_vcpu_run)
>>  	bl __restore_sysregs
>>  	bl __restore_fpsimd
>>  
>> +        // Now is the time to set-up the debug registers if we
>> +        // are debugging the guest
>> +	ldr	x3, [x0, #GUEST_DEBUG]
>> +	tbz	x3, #KVM_GUESTDBG_USE_HW_BP_SHIFT, 2f
>> +	bl	__setup_debug
>> +	b	1f
>> +2:
>>  	skip_debug_state x3, 1f
>>  	bl	__restore_debug
>>  1:
>> @@ -881,6 +922,11 @@ __kvm_vcpu_return:
>>  	bl __save_fpsimd
>>  	bl __save_sysregs
>>  
>> +	// If we are debugging the guest don't save debug registers
>> +	// otherwise we'll be trashing are only good copy we have.
>> +	ldr	x3, [x0, #GUEST_DEBUG]
>> +	tbnz	x3, #KVM_GUESTDBG_USE_HW_BP_SHIFT, 1f
>> +
>
> we're introducing an awful lot of conditionals in the assembly code with
> these patches, can you re-consider if there's a cleaner abstraction that
> allows us to deal with some of this stuff in C-code?

See previous mail. It would be good but we need a place to do it before
we enter hyp.S on a KVM_RUN ioctl. I'm open to suggestions.

>
> -Christoffer

-- 
Alex Bennée