[PATCH v4 01/12] KVM: add comments for kvm_debug_exit_arch struct

[PATCH v4 01/12] KVM: add comments for kvm_debug_exit_arch struct

[Alex Bennée]

Bring into line with the comments for the other structures and their
KVM_EXIT_* cases. Also update api.txt to reflect use in kvm_run
documentation.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
Reviewed-by: Andrew Jones <drjones@redhat.com>
Acked-by: Christoffer Dall <christoffer.dall@linaro.org>

---

v2
  - add comments for other exit types
v3
  - s/commentary/comments/
  - add rb tags
  - update api.txt kvm_run to include KVM_EXIT_DEBUG desc
v4
  - sp fixes
  - add a-b

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index 9fa2bf8..c34c32d 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -3070,11 +3070,13 @@ data_offset describes where the data is located (KVM_EXIT_IO_OUT) or
 where kvm expects application code to place the data for the next
 KVM_RUN invocation (KVM_EXIT_IO_IN).  Data format is a packed array.
 
+		/* KVM_EXIT_DEBUG */
 		struct {
 			struct kvm_debug_exit_arch arch;
 		} debug;
 
-Unused.
+If the exit_reason is KVM_EXIT_DEBUG, then a vcpu is processing a debug event
+for which architecture specific information is returned.
 
 		/* KVM_EXIT_MMIO */
 		struct {
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 4b60056..70ac641 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -237,6 +237,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;
@@ -285,6 +286,7 @@ struct kvm_run {
 			__u32 data;
 			__u8  is_write;
 		} dcr;
+		/* KVM_EXIT_INTERNAL_ERROR */
 		struct {
 			__u32 suberror;
 			/* Available with KVM_CAP_INTERNAL_ERROR_DATA: */
@@ -295,6 +297,7 @@ struct kvm_run {
 		struct {
 			__u64 gprs[32];
 		} osi;
+		/* KVM_EXIT_PAPR_HCALL */
 		struct {
 			__u64 nr;
 			__u64 ret;
-- 
2.3.5

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

[PATCH v4 02/12] KVM: define common KVM_GUESTDBG_USE_SW/HW_BP bits

[Alex Bennée]

Currently x86, powerpc and soon arm64 use the same two architecture
specific bits for guest debug support for software and hardware
breakpoints. This makes the shared values explicit.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Andrew Jones <drjones@redhat.com>

-
v4
  - claim more bits for the common functionality
v5
  - don't use __ mechanism to move values common

diff --git a/arch/powerpc/include/uapi/asm/kvm.h b/arch/powerpc/include/uapi/asm/kvm.h
index ab4d473..6ea24a5 100644
--- a/arch/powerpc/include/uapi/asm/kvm.h
+++ b/arch/powerpc/include/uapi/asm/kvm.h
@@ -307,11 +307,9 @@ struct kvm_guest_debug_arch {
 /* Debug related defines */
 /*
  * kvm_guest_debug->control is a 32 bit field. The lower 16 bits are generic
- * and upper 16 bits are architecture specific. Architecture specific defines
+ * and upper 14 bits are architecture specific. Architecture specific defines
  * that ioctl is for setting hardware breakpoint or software breakpoint.
  */
-#define KVM_GUESTDBG_USE_SW_BP		0x00010000
-#define KVM_GUESTDBG_USE_HW_BP		0x00020000
 
 /* definition of registers in kvm_run */
 struct kvm_sync_regs {
diff --git a/arch/s390/include/uapi/asm/kvm.h b/arch/s390/include/uapi/asm/kvm.h
index ef1a5fc..aca4f86 100644
--- a/arch/s390/include/uapi/asm/kvm.h
+++ b/arch/s390/include/uapi/asm/kvm.h
@@ -114,8 +114,6 @@ struct kvm_fpu {
 	__u64 fprs[16];
 };
 
-#define KVM_GUESTDBG_USE_HW_BP		0x00010000
-
 #define KVM_HW_BP			1
 #define KVM_HW_WP_WRITE			2
 #define KVM_SINGLESTEP			4
diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
index d7dcef5..ca51d4c 100644
--- a/arch/x86/include/uapi/asm/kvm.h
+++ b/arch/x86/include/uapi/asm/kvm.h
@@ -250,8 +250,6 @@ struct kvm_debug_exit_arch {
 	__u64 dr7;
 };
 
-#define KVM_GUESTDBG_USE_SW_BP		0x00010000
-#define KVM_GUESTDBG_USE_HW_BP		0x00020000
 #define KVM_GUESTDBG_INJECT_DB		0x00040000
 #define KVM_GUESTDBG_INJECT_BP		0x00080000
 
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 70ac641..7c5dd11 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -570,8 +570,10 @@ struct kvm_s390_irq_state {
 
 /* for KVM_SET_GUEST_DEBUG */
 
-#define KVM_GUESTDBG_ENABLE		0x00000001
-#define KVM_GUESTDBG_SINGLESTEP		0x00000002
+#define KVM_GUESTDBG_ENABLE		(1 << 0)
+#define KVM_GUESTDBG_SINGLESTEP	(1 << 1)
+#define KVM_GUESTDBG_USE_SW_BP		(1 << 16)
+#define KVM_GUESTDBG_USE_HW_BP		(1 << 17)
 
 struct kvm_guest_debug {
 	__u32 control;
-- 
2.3.5

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

[PATCH v4 10/12] 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. In the debug ioctl we copy the IMPDEF defined number of
registers into a new register set called host_debug_state. There is now
a new vcpu parameter called debug_ptr which selects which register set
is to copied into the real registers when world switch occurs.

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 so we need to save and restore those bits.

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>

---
v2
   - switched to C setup
   - replace host debug registers directly into context
   - minor tweak to api docs
   - setup right register for debug
   - add FAR_EL2 to debug exit structure
   - add support for trapping debug register access
v3
   - remove stray trace statement
   - fix spacing around operators (various)
   - clean-up usage of trap_debug
   - introduce debug_ptr, replace excessive memcpy stuff
   - don't use memcpy in ioctl, just assign
   - update cap ioctl documentation
   - reword a number comments
   - rename host_debug_state->external_debug_state
v4
   - use the new u32/u64 split debug_ptr approach
   - fix some wording/comments

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index 33c8143..ada57df 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -2668,7 +2668,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]
@@ -2683,6 +2683,11 @@ updated to the correct (supplied) values.
 The second part of the structure is architecture specific and
 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 which return a positive number
+indicating the number of supported registers.
+
 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
 structure containing architecture specific debug information.
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 0d17c7b..6df47c1 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -307,6 +307,7 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
 
 #define KVM_GUESTDBG_VALID_MASK (KVM_GUESTDBG_ENABLE |    \
 			    KVM_GUESTDBG_USE_SW_BP | \
+			    KVM_GUESTDBG_USE_HW_BP | \
 			    KVM_GUESTDBG_SINGLESTEP)
 
 /**
@@ -327,6 +328,12 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
 
 	if (dbg->control & KVM_GUESTDBG_ENABLE) {
 		vcpu->guest_debug = dbg->control;
+
+		/* Hardware assisted Break and Watch points */
+		if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP) {
+			vcpu->arch.external_debug_state = dbg->arch;
+		}
+
 	} else {
 		/* If not enabled clear all flags */
 		vcpu->guest_debug = 0;
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 ad792cb..ba3a1c5 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -113,12 +113,13 @@ struct kvm_vcpu_arch {
 
 	/*
 	 * For debugging the guest we need to keep a set of debug
-	 * registers which can override the guests own debug state
+	 * registers which can override the guest's own debug state
 	 * while being used. These are set via the KVM_SET_GUEST_DEBUG
 	 * ioctl.
 	 */
 	struct kvm_guest_debug_arch *debug_ptr;
 	struct kvm_guest_debug_arch vcpu_debug_state;
+	struct kvm_guest_debug_arch external_debug_state;
 
 	/* Pointer to host CPU context */
 	kvm_cpu_context_t *host_cpu_context;
diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index 8796610..144edee 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -121,7 +121,7 @@ struct kvm_guest_debug_arch {
 
 struct kvm_debug_exit_arch {
 	__u32 hsr;
-	__u64 far;
+	__u64 far;	/* used for watchpoints */
 };
 
 struct kvm_sync_regs {
diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
index e7d934d..3a41bbf 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/debug.c b/arch/arm64/kvm/debug.c
index f630f4d..7cfcb53 100644
--- a/arch/arm64/kvm/debug.c
+++ b/arch/arm64/kvm/debug.c
@@ -103,10 +103,6 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
 				MDCR_EL2_TDRA |
 				MDCR_EL2_TDOSA);
 
-	/* Trap on access to debug registers? */
-	if (trap_debug)
-		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA;
-
 	/* Is Guest debugging in effect? */
 	if (vcpu->guest_debug) {
 		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDE;
@@ -129,11 +125,43 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
 			*vcpu_cpsr(vcpu) &= ~DBG_SPSR_SS;
 			vcpu_sys_reg(vcpu, MDSCR_EL1) &= ~DBG_MDSCR_SS;
 		}
+
+		/*
+		 * HW Break/Watch points
+		 *
+		 * We simply switch the debug_ptr to point to our new
+		 * external_debug_state which has been populated by the
+		 * debug ioctl. The existing KVM_ARM64_DEBUG_DIRTY
+		 * mechanism ensures the registers are updated on the
+		 * world switch.
+		 */
+		if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP) {
+			/* Enable debug exceptions for all EL0/EL1 */
+			vcpu_sys_reg(vcpu, MDSCR_EL1) |=
+				(DBG_MDSCR_KDE | DBG_MDSCR_MDE);
+
+			vcpu->arch.debug_ptr = &vcpu->arch.external_debug_state;
+			vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_DIRTY;
+			trap_debug = true;
+		}
 	}
+
+	/* Trap debug register access */
+	if (trap_debug)
+		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA;
 }
 
 void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
 {
-	if (vcpu->guest_debug)
+	if (vcpu->guest_debug) {
 		restore_guest_debug_regs(vcpu);
+
+		/*
+		 * If we were using HW debug we need to restore the
+		 * debug_ptr to the guest debug state.
+		 */
+		if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP)
+			vcpu->arch.debug_ptr = &vcpu->arch.vcpu_debug_state;
+
+	}
 }
diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
index e9de13e..68a0759 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -103,7 +103,11 @@ static int kvm_handle_guest_debug(struct kvm_vcpu *vcpu, struct kvm_run *run)
 	run->debug.arch.hsr = hsr;
 
 	switch (hsr >> ESR_ELx_EC_SHIFT) {
+	case ESR_ELx_EC_WATCHPT_LOW:
+		run->debug.arch.far = vcpu->arch.fault.far_el2;
+		/* fall through */
 	case ESR_ELx_EC_SOFTSTP_LOW:
+	case ESR_ELx_EC_BREAKPT_LOW:
 	case ESR_ELx_EC_BKPT32:
 	case ESR_ELx_EC_BRK64:
 		break;
@@ -132,6 +136,8 @@ static exit_handle_fn arm_exit_handlers[] = {
 	[ESR_ELx_EC_IABT_LOW]	= kvm_handle_guest_abort,
 	[ESR_ELx_EC_DABT_LOW]	= kvm_handle_guest_abort,
 	[ESR_ELx_EC_SOFTSTP_LOW]= kvm_handle_guest_debug,
+	[ESR_ELx_EC_WATCHPT_LOW]= kvm_handle_guest_debug,
+	[ESR_ELx_EC_BREAKPT_LOW]= kvm_handle_guest_debug,
 	[ESR_ELx_EC_BKPT32]	= kvm_handle_guest_debug,
 	[ESR_ELx_EC_BRK64]	= kvm_handle_guest_debug,
 };
diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
index 0b43265..21d5a62 100644
--- a/arch/arm64/kvm/reset.c
+++ b/arch/arm64/kvm/reset.c
@@ -56,6 +56,12 @@ static bool cpu_has_32bit_el1(void)
 	return !!(pfr0 & 0x20);
 }
 
+/**
+ * kvm_arch_dev_ioctl_check_extension
+ *
+ * We currently assume that the number of HW registers is uniform
+ * across all CPUs (see cpuinfo_sanity_check).
+ */
 int kvm_arch_dev_ioctl_check_extension(long ext)
 {
 	int r;
@@ -64,6 +70,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 7c5dd11..d3e661d 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -819,6 +819,8 @@ struct kvm_ppc_smmu_info {
 #define KVM_CAP_S390_INJECT_IRQ 113
 #define KVM_CAP_S390_IRQ_STATE 114
 #define KVM_CAP_PPC_HWRNG 115
+#define KVM_CAP_GUEST_DEBUG_HW_BPS 116
+#define KVM_CAP_GUEST_DEBUG_HW_WPS 117
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
-- 
2.3.5

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

Re: [PATCH v4 10/12] KVM: arm64: guest debug, HW assisted debug support

[Mark Rutland]

Hi Alex,

On Fri, May 15, 2015 at 03:27:13PM +0100, Alex Bennée wrote:
> This adds support for userspace to control the HW debug registers for
> guest debug. In the debug ioctl we copy the IMPDEF defined number of
> registers into a new register set called host_debug_state. There is now
> a new vcpu parameter called debug_ptr which selects which register set
> is to copied into the real registers when world switch occurs.
> 
> 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 so we need to save and restore those bits.
> 
> 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.

There's the unfortunate possibility that these could vary across cores
in a big.LITTLE system (though we haven't seen that thus far). The
kernel sanity checks should currently explode if such a case is
encountered, but I don't know what we'd do were that to happen.

This gets more fun when you consider the context-aware breakpoints are
the highest numbered. So the set of (context-aware) breakpoints might
not intersect across all CPUs.

I'm not sure what the best thing to do is w.r.t. exposing that to
userspace.

Thanks,
Mark.

Re: [PATCH v4 02/12] KVM: define common KVM_GUESTDBG_USE_SW/HW_BP bits

[Christian Borntraeger]

Am 15.05.2015 um 16:27 schrieb Alex Bennée:
> +++ b/arch/s390/include/uapi/asm/kvm.h
> @@ -114,8 +114,6 @@ struct kvm_fpu {
>  	__u64 fprs[16];
>  };
> 
> -#define KVM_GUESTDBG_USE_HW_BP		0x00010000
[...]
> +++ b/include/uapi/linux/kvm.h
[...]
> +#define KVM_GUESTDBG_USE_SW_BP		(1 << 16)
> +#define KVM_GUESTDBG_USE_HW_BP		(1 << 17)

This is an ABI break for s390, no?

David, do you remember why we do not use KVM_GUESTDBG_USE_SW_BP?

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

Re: [PATCH v4 02/12] KVM: define common KVM_GUESTDBG_USE_SW/HW_BP bits

[Christian Borntraeger]

Am 15.05.2015 um 16:27 schrieb Alex Bennée:
> index ef1a5fc..aca4f86 100644
> --- a/arch/s390/include/uapi/asm/kvm.h
> +++ b/arch/s390/include/uapi/asm/kvm.h
> @@ -114,8 +114,6 @@ struct kvm_fpu {
>  	__u64 fprs[16];
>  };
> 
> -#define KVM_GUESTDBG_USE_HW_BP		0x00010000
> -


> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 70ac641..7c5dd11 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
[...]
> +#define KVM_GUESTDBG_USE_SW_BP		(1 << 16)
> +#define KVM_GUESTDBG_USE_HW_BP		(1 << 17)

This is a abi break for s390, no?

David do you remember why we did not use SW_BP?

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

Re: [PATCH v4 10/12] KVM: arm64: guest debug, HW assisted debug support

[Alex Bennée]

Mark Rutland <mark.rutland@arm.com> writes:

> Hi Alex,
>
> On Fri, May 15, 2015 at 03:27:13PM +0100, Alex Bennée wrote:
>> This adds support for userspace to control the HW debug registers for
>> guest debug. In the debug ioctl we copy the IMPDEF defined number of
>> registers into a new register set called host_debug_state. There is now
>> a new vcpu parameter called debug_ptr which selects which register set
>> is to copied into the real registers when world switch occurs.
>> 
>> 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 so we need to save and restore those bits.
>> 
>> 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.
>
> There's the unfortunate possibility that these could vary across cores
> in a big.LITTLE system (though we haven't seen that thus far). The
> kernel sanity checks should currently explode if such a case is
> encountered, but I don't know what we'd do were that to happen.

I suspect we would have to disable HW assisted breakpoints or return the
lowest common denominator if we can tell which cores we shall every be
scheduled on.

>
> This gets more fun when you consider the context-aware breakpoints are
> the highest numbered. So the set of (context-aware) breakpoints might
> not intersect across all CPUs.

I didn't see a reference to that in the ARM ARM. It seemed to imply any
breakpoint could be context aware is .BT was appropriately set and
linked to the VR.  

As it happens the gdb stub interface in QEMU is fairly limited so while
I expose the full debug registers I don't think there is currently a way
to expose any of the fancy linking/context stuff to the userspace
debugger. However I did make the ABI pass full raw values in so this
could become a possibility later without having to expand the ABI.


> I'm not sure what the best thing to do is w.r.t. exposing that to
> userspace.
>
> Thanks,
> Mark.

-- 
Alex Bennée
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v4 10/12] KVM: arm64: guest debug, HW assisted debug support

[Mark Rutland]

> > This gets more fun when you consider the context-aware breakpoints are
> > the highest numbered. So the set of (context-aware) breakpoints might
> > not intersect across all CPUs.
> 
> I didn't see a reference to that in the ARM ARM. It seemed to imply any
> breakpoint could be context aware is .BT was appropriately set and
> linked to the VR.  

The existence of ID_AA64_DFR0_EL1.CTX_CMPs implies otherwise, though I
haven't dug much deeper.

Thanks,
Mark.

Re: [PATCH v4 10/12] KVM: arm64: guest debug, HW assisted debug support

[Peter Maydell]

On 15 May 2015 at 17:16, Alex Bennée <alex.bennee@linaro.org> wrote:
> Mark Rutland <mark.rutland@arm.com> writes:
>> This gets more fun when you consider the context-aware breakpoints are
>> the highest numbered. So the set of (context-aware) breakpoints might
>> not intersect across all CPUs.
>
> I didn't see a reference to that in the ARM ARM. It seemed to imply any
> breakpoint could be context aware is .BT was appropriately set and
> linked to the VR.

No; see D2.9.2; there must be at least one context-aware
breakpoint, but no requirement for more than that.

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

Re: [PATCH v4 02/12] KVM: define common KVM_GUESTDBG_USE_SW/HW_BP bits

[David Hildenbrand]

> Am 15.05.2015 um 16:27 schrieb Alex Bennée:
> > +++ b/arch/s390/include/uapi/asm/kvm.h
> > @@ -114,8 +114,6 @@ struct kvm_fpu {
> >  	__u64 fprs[16];
> >  };
> > 
> > -#define KVM_GUESTDBG_USE_HW_BP		0x00010000
> [...]
> > +++ b/include/uapi/linux/kvm.h
> [...]
> > +#define KVM_GUESTDBG_USE_SW_BP		(1 << 16)
> > +#define KVM_GUESTDBG_USE_HW_BP		(1 << 17)
> 
> This is an ABI break for s390, no?
> 
> David, do you remember why we do not use KVM_GUESTDBG_USE_SW_BP?
> 

We never had to tell the kernel about software breakpoints as this is all
handled via 4 byte DIAG instructions until now. We don't have to turn this
mechanism on. QEMU can directly insert the desired DIAG instructions and gets
notified when they are about to get executed.

(But we still have 2 byte breakpoint support todo - still tbd how exactly this
will be realized - could be turned on via such a mechanism)

The problem is, that these bits are arch specific, now Alex wants to unify
them for all archs.

So yes, this is an ABI break for us and breaks hardware breakpoints.(I think
the first version of this patch didn't contain this ABI break when I had a look)

I wonder if it wouldn't make more sense to

- introduce new bits in the arch-unspecific section
- rework the existing implementers to accept both bits

Or to simply leave stuff as it is and handle it via arch specific bits.

David

Re: [PATCH v4 02/12] KVM: define common KVM_GUESTDBG_USE_SW/HW_BP bits

[Jan Kiszka]

On 2015-05-15 19:33, David Hildenbrand wrote:
>> Am 15.05.2015 um 16:27 schrieb Alex Bennée:
>>> +++ b/arch/s390/include/uapi/asm/kvm.h
>>> @@ -114,8 +114,6 @@ struct kvm_fpu {
>>>  	__u64 fprs[16];
>>>  };
>>>
>>> -#define KVM_GUESTDBG_USE_HW_BP		0x00010000
>> [...]
>>> +++ b/include/uapi/linux/kvm.h
>> [...]
>>> +#define KVM_GUESTDBG_USE_SW_BP		(1 << 16)
>>> +#define KVM_GUESTDBG_USE_HW_BP		(1 << 17)
>>
>> This is an ABI break for s390, no?
>>
>> David, do you remember why we do not use KVM_GUESTDBG_USE_SW_BP?
>>
> 
> We never had to tell the kernel about software breakpoints as this is all
> handled via 4 byte DIAG instructions until now. We don't have to turn this
> mechanism on. QEMU can directly insert the desired DIAG instructions and gets
> notified when they are about to get executed.
> 
> (But we still have 2 byte breakpoint support todo - still tbd how exactly this
> will be realized - could be turned on via such a mechanism)
> 
> The problem is, that these bits are arch specific, now Alex wants to unify
> them for all archs.
> 
> So yes, this is an ABI break for us and breaks hardware breakpoints.(I think
> the first version of this patch didn't contain this ABI break when I had a look)
> 
> I wonder if it wouldn't make more sense to
> 
> - introduce new bits in the arch-unspecific section
> - rework the existing implementers to accept both bits
> 
> Or to simply leave stuff as it is and handle it via arch specific bits.

With one arch proving the "all need this" theory wrong, just drop this
patch. Even quicker when it breaks an ABI.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm