Re: [PATCH v5 03/12] KVM: arm64: guest debug, define API headers
On Thu, Jun 04, 2015 at 02:49:17PM +0100, Alex Bennée wrote:
Christoffer Dall christoffer.d...@linaro.org writes:
On Fri, May 29, 2015 at 10:30:19AM +0100, 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 exception and possible faulting address
The type of debugging being used is controlled by the architecture
specific control bits of the kvm_guest_debug-control flags in the ioctl
structure.
Signed-off-by: Alex Bennée alex.ben...@linaro.org
Reviewed-by: David Hildenbrand d...@linux.vnet.ibm.com
Reviewed-by: Andrew Jones drjo...@redhat.com
Acked-by: Christoffer Dall christoffer.d...@linaro.org
I can re-confirm my ack despite the changes in v4, but this really is
borderline to keep exiting r-b and a-b tags around from previous patches
I would think, but ok.
I was wondering how much a patch has to change for the r-b tags to
become invalid. The meat of the API hasn't changed much though.
Drew/David,
Are you still happy with this patch?
I'm fine with it. Sorry I haven't had time to look over the rest of the
series yet this round.
drew
-Christoffer
---
v2
- expose hsr and pc directly to user-space
v3
- s/control/controlled/ in commit message
- add v8 to ARM ARM comment (ARM Architecture Reference Manual)
- add rb tag
- rm pc, add far
- re-word comments on alignment
- rename KVM_ARM_NDBG_REGS - KVM_ARM_MAX_DBG_REGS
v4
- now uses common HW/SW BP define
- add a-b-tag
- use u32 for control regs
v5
- revert to have arch specific KVM_GUESTDBG_USE_SW/HW_BP
- rm stale comments dbgctrl was stored as u64
---
arch/arm64/include/uapi/asm/kvm.h | 20
1 file changed, 20 insertions(+)
diff --git a/arch/arm64/include/uapi/asm/kvm.h
b/arch/arm64/include/uapi/asm/kvm.h
index d268320..43758e7 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -100,12 +100,32 @@ struct kvm_sregs {
struct kvm_fpu {
};
+/*
+ * See v8 ARM ARM D7.3: Debug Registers
+ *
+ * The architectural limit is 16 debug registers of each type although
+ * in practice there are usually less (see ID_AA64DFR0_EL1).
+ */
+#define KVM_ARM_MAX_DBG_REGS 16
struct kvm_guest_debug_arch {
+ __u32 dbg_bcr[KVM_ARM_MAX_DBG_REGS];
+ __u64 dbg_bvr[KVM_ARM_MAX_DBG_REGS];
+ __u32 dbg_wcr[KVM_ARM_MAX_DBG_REGS];
+ __u64 dbg_wvr[KVM_ARM_MAX_DBG_REGS];
};
struct kvm_debug_exit_arch {
+ __u32 hsr;
+ __u64 far;
};
+/*
+ * Architecture specific defines for kvm_guest_debug-control
+ */
+
+#define KVM_GUESTDBG_USE_SW_BP(1 16)
+#define KVM_GUESTDBG_USE_HW_BP(1 17)
+
struct kvm_sync_regs {
};
--
2.4.1
--
Alex Bennée
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH] KVM: arm64: fix misleading comments in save/restore
On 04/06/15 11:20, Alex Bennée wrote: Marc Zyngier marc.zyng...@arm.com writes: On 04/06/15 10:34, Christoffer Dall wrote: On Thu, May 28, 2015 at 10:43:06AM +0100, Alex Bennée wrote: The elr_el2 and spsr_el2 registers in fact contain the processor state before entry into the hypervisor code. be careful with your use of the hypervisor, in the KVM design the hypervisor is split across EL1 and EL2. before entry into EL2. In the case of guest state it could be in either el0 or el1. true Signed-off-by: Alex Bennée alex.ben...@linaro.org --- arch/arm64/kvm/hyp.S | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S index d755922..1940a4c 100644 --- a/arch/arm64/kvm/hyp.S +++ b/arch/arm64/kvm/hyp.S @@ -50,8 +50,8 @@ stp x29, lr, [x3, #80] mrs x19, sp_el0 - mrs x20, elr_el2// EL1 PC - mrs x21, spsr_el2 // EL1 pstate + mrs x20, elr_el2// PC before hyp entry + mrs x21, spsr_el2 // pstate before hyp entry stp x19, x20, [x3, #96] str x21, [x3, #112] @@ -82,8 +82,8 @@ ldr x21, [x3, #16] msr sp_el0, x19 - msr elr_el2, x20// EL1 PC - msr spsr_el2, x21 // EL1 pstate + msr elr_el2, x20// PC to restore + msr spsr_el2, x21 // pstate to restore I don't feel like 'to restore' is much more meaningful here. I would actually vote for removin the comments all together, since one should really understand the code as opposed to the comments when reading this kind of stuff. Meh, I'm not sure. Your patch is definitely better than doing nothing. Marc? While I definitely agree that people should pay more attention to the code rather than blindly trusting comments, I still think there is some value in disambiguating the exception entry/return, because this bit of code assumes some intimate knowledge of the ARMv8 exception model. As for the comments themselves, I'd rather have some wording that clearly indicate that we're dealing with guest information, i.e: mrs x20, elr_el2// Guest PC mrs x21, spsr_el2 // Guest pstate (and the same for the exception return). The before hyp entry and to restore are not really useful (all the registers we are saving/restoring fall into these categories). What I wanted to convey here was that despite using an EL2 register, we are dealing with guest registers. Which would be great it we were. However the code is used to save/restore the host context as well as the guest context hence my weasely words. Gahhh. You're right. I'm spending too much time on the VHE code these days. Guess I'll stick to the weasel words then. Can you respin it with Christoffer's comment addressed? Thanks, M. -- Jazz is not dead. It just smells funny... ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v5 10/12] KVM: arm64: guest debug, HW assisted debug support
On Fri, May 29, 2015 at 10:30:26AM +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.
Signed-off-by: Alex Bennée alex.ben...@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
v5
- don't set MDSCR_EL1.KDE (not needed)
---
Documentation/virtual/kvm/api.txt | 7 ++-
arch/arm/kvm/arm.c | 7 +++
arch/arm64/include/asm/hw_breakpoint.h | 12 +++
arch/arm64/include/asm/kvm_host.h | 3 ++-
arch/arm64/include/uapi/asm/kvm.h | 2 +-
arch/arm64/kernel/hw_breakpoint.c | 12 ---
arch/arm64/kvm/debug.c | 37
+-
arch/arm64/kvm/handle_exit.c | 6 ++
arch/arm64/kvm/reset.c | 12 +++
include/uapi/linux/kvm.h | 2 ++
10 files changed, 80 insertions(+), 20 deletions(-)
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) {
does KVM_GUESTDBG_USE_HW_BP cover watch points as well or why is this
mentionend in the comment?
I asked this once before already...
+ 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)
}
Re: [PATCH] KVM: arm64: fix misleading comments in save/restore
Marc Zyngier marc.zyng...@arm.com writes: On 04/06/15 11:20, Alex Bennée wrote: Marc Zyngier marc.zyng...@arm.com writes: On 04/06/15 10:34, Christoffer Dall wrote: On Thu, May 28, 2015 at 10:43:06AM +0100, Alex Bennée wrote: The elr_el2 and spsr_el2 registers in fact contain the processor state before entry into the hypervisor code. be careful with your use of the hypervisor, in the KVM design the hypervisor is split across EL1 and EL2. before entry into EL2. In the case of guest state it could be in either el0 or el1. true Signed-off-by: Alex Bennée alex.ben...@linaro.org --- arch/arm64/kvm/hyp.S | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S index d755922..1940a4c 100644 --- a/arch/arm64/kvm/hyp.S +++ b/arch/arm64/kvm/hyp.S @@ -50,8 +50,8 @@ stp x29, lr, [x3, #80] mrs x19, sp_el0 - mrs x20, elr_el2// EL1 PC - mrs x21, spsr_el2 // EL1 pstate + mrs x20, elr_el2// PC before hyp entry + mrs x21, spsr_el2 // pstate before hyp entry stp x19, x20, [x3, #96] str x21, [x3, #112] @@ -82,8 +82,8 @@ ldr x21, [x3, #16] msr sp_el0, x19 - msr elr_el2, x20// EL1 PC - msr spsr_el2, x21 // EL1 pstate + msr elr_el2, x20// PC to restore + msr spsr_el2, x21 // pstate to restore I don't feel like 'to restore' is much more meaningful here. I would actually vote for removin the comments all together, since one should really understand the code as opposed to the comments when reading this kind of stuff. Meh, I'm not sure. Your patch is definitely better than doing nothing. Marc? While I definitely agree that people should pay more attention to the code rather than blindly trusting comments, I still think there is some value in disambiguating the exception entry/return, because this bit of code assumes some intimate knowledge of the ARMv8 exception model. As for the comments themselves, I'd rather have some wording that clearly indicate that we're dealing with guest information, i.e: mrs x20, elr_el2// Guest PC mrs x21, spsr_el2 // Guest pstate (and the same for the exception return). The before hyp entry and to restore are not really useful (all the registers we are saving/restoring fall into these categories). What I wanted to convey here was that despite using an EL2 register, we are dealing with guest registers. Which would be great it we were. However the code is used to save/restore the host context as well as the guest context hence my weasely words. Gahhh. You're right. I'm spending too much time on the VHE code these days. Guess I'll stick to the weasel words then. Can you respin it with Christoffer's comment addressed? Sure. Do you want it separated from the guest debug series or will you be happy to take it with it when ready? Thanks, M. -- Alex Bennée ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v5 2/6] target-arm: kvm64: introduce kvm_arm_init_debug()
On 29 May 2015 at 16:19, Alex Bennée alex.ben...@linaro.org wrote: As we haven't always had guest debug support we need to probe for it. Additionally we don't do this in the start-up capability code so we don't fall over on old kernels. Signed-off-by: Alex Bennée alex.ben...@linaro.org --- Reviewed-by: Peter Maydell peter.mayd...@linaro.org thanks -- PMM ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v5 07/12] KVM: arm64: guest debug, add support for single-step
On Fri, May 29, 2015 at 10:30:23AM +0100, Alex Bennée wrote:
This adds support for single-stepping the guest. To do this we need to
manipulate the guests PSTATE.SS and MDSCR_EL1.SS bits which we do in the
kvm_arm_setup/clear_debug() so we don't affect the apparent state of the
guest. Additionally while the host is debugging the guest we suppress
the ability of the guest to single-step itself.
I feel like there should be a slightly more elaborate explanation of
exactly what works and what doesn't work when the guest is single
stepping something and which choices we've made for supporting or not
supporting this.
Signed-off-by: Alex Bennée alex.ben...@linaro.org
---
v2
- Move pstate/mdscr manipulation into C
- don't export guest_debug to assembly
- add accessor for saved_debug regs
- tweak save/restore of mdscr_el1
v3
- don't save PC in debug information struct
- rename debug_saved_regs-guest_debug_state
- save whole value, only use bits in restore
- add save/restore_guest-debug_regs helper functions
- simplify commit message for clarity
- rm vcpu_debug_saved_reg access fn
v4
- added more comments based on suggestions
- guest_debug_state-guest_debug_preserved
- no point masking restore, we will trap out
v5
- more comments
- don't bother preserving pstate.ss
it would have been good if there was some comment explaining the reason
for this change.
---
arch/arm/kvm/arm.c| 4 ++-
arch/arm64/include/asm/kvm_host.h | 11
arch/arm64/kvm/debug.c| 58
---
arch/arm64/kvm/handle_exit.c | 2 ++
4 files changed, 70 insertions(+), 5 deletions(-)
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 064c105..9b3ed6d 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -302,7 +302,9 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
kvm_arm_set_running_vcpu(NULL);
}
-#define KVM_GUESTDBG_VALID_MASK (KVM_GUESTDBG_ENABLE |
KVM_GUESTDBG_USE_SW_BP)
+#define KVM_GUESTDBG_VALID_MASK (KVM_GUESTDBG_ENABLE |\
+ KVM_GUESTDBG_USE_SW_BP | \
+ KVM_GUESTDBG_SINGLESTEP)
/**
* kvm_arch_vcpu_ioctl_set_guest_debug - set up guest debugging
diff --git a/arch/arm64/include/asm/kvm_host.h
b/arch/arm64/include/asm/kvm_host.h
index 7cb99b5..e2db6a6 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -123,6 +123,17 @@ struct kvm_vcpu_arch {
* here.
*/
+ /*
+ * Guest registers we preserve during guest debugging.
+ *
+ * These shadow registers are updated by the kvm_handle_sys_reg
+ * trap handler if the guest accesses or updates them while we
+ * are using guest debug.
+ */
+ struct {
+ u32 mdscr_el1;
+ } guest_debug_preserved;
+
/* Don't run the guest */
bool pause;
diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
index 8d1bfa4..10a6baa 100644
--- a/arch/arm64/kvm/debug.c
+++ b/arch/arm64/kvm/debug.c
@@ -19,11 +19,41 @@
#include linux/kvm_host.h
+#include asm/debug-monitors.h
+#include asm/kvm_asm.h
#include asm/kvm_arm.h
+#include asm/kvm_emulate.h
+
+/* These are the bits of MDSCR_EL1 we may manipulate */
+#define MDSCR_EL1_DEBUG_MASK (DBG_MDSCR_SS | \
+ DBG_MDSCR_KDE | \
+ DBG_MDSCR_MDE)
static DEFINE_PER_CPU(u32, mdcr_el2);
/**
+ * save/restore_guest_debug_regs
+ *
+ * For some debug operations we need to tweak some guest registers. As
+ * a result we need to save the state of those registers before we
+ * make those modifications. This does get confused if the guest
+ * attempts to control single step while being debugged. It will start
+ * working again once it is no longer being debugged by the host.
What gets confused and what starts working?
+ *
+ * Guest access to MDSCR_EL1 is trapped by the hypervisor and handled
+ * after we have restored the preserved value to the main context.
+ */
+static void save_guest_debug_regs(struct kvm_vcpu *vcpu)
+{
+ vcpu-arch.guest_debug_preserved.mdscr_el1 = vcpu_sys_reg(vcpu,
MDSCR_EL1);
+}
+
+static void restore_guest_debug_regs(struct kvm_vcpu *vcpu)
+{
+ vcpu_sys_reg(vcpu, MDSCR_EL1) =
vcpu-arch.guest_debug_preserved.mdscr_el1;
+}
+
+/**
* kvm_arm_init_debug - grab what we need for debug
*
* Currently the sole task of this function is to retrieve the initial
@@ -38,7 +68,6 @@ void kvm_arm_init_debug(void)
__this_cpu_write(mdcr_el2, kvm_call_hyp(__kvm_get_mdcr_el2));
}
-
/**
* kvm_arm_setup_debug - set up debug related stuff
*
@@ -73,12 +102,33 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
if (trap_debug)
vcpu-arch.mdcr_el2 |= MDCR_EL2_TDA;
- /* Trap breakpoints? */
- if (vcpu-guest_debug
Re: [PATCH] KVM: arm64: fix misleading comments in save/restore
Marc Zyngier marc.zyng...@arm.com writes: On 04/06/15 10:34, Christoffer Dall wrote: On Thu, May 28, 2015 at 10:43:06AM +0100, Alex Bennée wrote: The elr_el2 and spsr_el2 registers in fact contain the processor state before entry into the hypervisor code. be careful with your use of the hypervisor, in the KVM design the hypervisor is split across EL1 and EL2. before entry into EL2. In the case of guest state it could be in either el0 or el1. true Signed-off-by: Alex Bennée alex.ben...@linaro.org --- arch/arm64/kvm/hyp.S | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S index d755922..1940a4c 100644 --- a/arch/arm64/kvm/hyp.S +++ b/arch/arm64/kvm/hyp.S @@ -50,8 +50,8 @@ stp x29, lr, [x3, #80] mrs x19, sp_el0 - mrs x20, elr_el2// EL1 PC - mrs x21, spsr_el2 // EL1 pstate + mrs x20, elr_el2// PC before hyp entry + mrs x21, spsr_el2 // pstate before hyp entry stp x19, x20, [x3, #96] str x21, [x3, #112] @@ -82,8 +82,8 @@ ldr x21, [x3, #16] msr sp_el0, x19 - msr elr_el2, x20// EL1 PC - msr spsr_el2, x21 // EL1 pstate + msr elr_el2, x20// PC to restore + msr spsr_el2, x21 // pstate to restore I don't feel like 'to restore' is much more meaningful here. I would actually vote for removin the comments all together, since one should really understand the code as opposed to the comments when reading this kind of stuff. Meh, I'm not sure. Your patch is definitely better than doing nothing. Marc? While I definitely agree that people should pay more attention to the code rather than blindly trusting comments, I still think there is some value in disambiguating the exception entry/return, because this bit of code assumes some intimate knowledge of the ARMv8 exception model. As for the comments themselves, I'd rather have some wording that clearly indicate that we're dealing with guest information, i.e: mrs x20, elr_el2// Guest PC mrs x21, spsr_el2 // Guest pstate (and the same for the exception return). The before hyp entry and to restore are not really useful (all the registers we are saving/restoring fall into these categories). What I wanted to convey here was that despite using an EL2 register, we are dealing with guest registers. Which would be great it we were. However the code is used to save/restore the host context as well as the guest context hence my weasely words. Would this address your concerns? Thanks, M. -- Alex Bennée ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v5 08/12] KVM: arm64: re-factor hyp.S debug register code
On Thu, Jun 04, 2015 at 11:34:46AM +0100, Alex Bennée wrote: Christoffer Dall christoffer.d...@linaro.org writes: On Fri, May 29, 2015 at 10:30:24AM +0100, 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). Oops I'd better fix that commit comment. Signed-off-by: Alex Bennée alex.ben...@linaro.org --- v3: - return to the patch series - add save and restore targets - change register use and document v4: - keep original setup/restore names - don't use split u32/u64 structure yet --- arch/arm64/kvm/hyp.S | 519 ++- 1 file changed, 140 insertions(+), 379 deletions(-) diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S index 74e63d8..9c4897d 100644 --- a/arch/arm64/kvm/hyp.S +++ b/arch/arm64/kvm/hyp.S [...] @@ -465,195 +318,52 @@ msr mdscr_el1, x25 .endm -.macro restore_debug - // x2: base address for cpu context - // x3: tmp register - - mrs x26, id_aa64dfr0_el1 - ubfxx24, x26, #12, #4 // Extract BRPs - ubfxx25, 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) +.macro restore_debug type +// x4: pointer to register set +// x5: number of registers to skip nit: use tabs instead of spaces here... + // x6..x22 trashed [...] @@ -887,12 +597,63 @@ __restore_sysregs: restore_sysregs ret +/* Save debug state */ __save_debug: - save_debug + // x0: base address for vcpu context + // x2: ptr to current CPU context + // x4/x5: trashed I had a bunch of questions here which I think you missed last time around: 1. why do we need the vcpu context? We don't, I'll drop that 2. what does 'current' mean here? Either the host or vcpu context depending which way we are currently going. drop 'current' please. 3. you're also trashing everything that save_debug trashes, so x4/5, x6-x22 trashed would be more correct OK + + mrs x26, id_aa64dfr0_el1 + ubfxx24, x26, #12, #4 // Extract BRPs + ubfxx25, 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 x5, x24 + add x4, x2, #CPU_SYSREG_OFFSET(DBGBCR0_EL1) + save_debug dbgbcr + add x4, x2, #CPU_SYSREG_OFFSET(DBGBVR0_EL1) + save_debug dbgbvr + + mov x5, x25 + add x4, x2, #CPU_SYSREG_OFFSET(DBGWCR0_EL1) + save_debug dbgwcr + add x4, x2, #CPU_SYSREG_OFFSET(DBGWVR0_EL1) + save_debug dbgwvr + + mrs x21, mdccint_el1 + str x21, [x2, #CPU_SYSREG_OFFSET(MDCCINT_EL1)] ret +/* Restore debug state */ __restore_debug: - restore_debug + // x0: base address for cpu context + // x2: ptr to current CPU context + // x4/x5: trashed and you missed these comments too, basically same stuff, but why was it 'cpu' here and not 'vcpu' like above? Again we use the functions both for restoring host and vcpu debug context. Well, the two functions operate on the same structures, so I would like them to be consistent... -Christoffer ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v5 6/6] target-arm: kvm - re-inject guest debug exceptions
On 29 May 2015 at 16:19, Alex Bennée alex.ben...@linaro.org wrote:
From: Alex Bennée a...@bennee.com
If we can't find details for the debug exception in our debug state
then we can assume the exception is due to debugging inside the guest.
To inject the exception into the guest state we re-use the TCG exception
code (do_interupt).
However while guest debugging is in effect we currently can't handle the
guest using single step which is heavily used by GDB.
Is this just the kernel not supporting this, or would QEMU need
to change as well? Worth mentioning either way.
Signed-off-by: Alex Bennée alex.ben...@linaro.org
---
v5:
- new for v5
---
target-arm/cpu.h| 1 +
target-arm/helper-a64.c | 17 ++---
target-arm/internals.h | 1 +
target-arm/kvm.c| 30 ++
4 files changed, 38 insertions(+), 11 deletions(-)
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 083211c..95ae3a8 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -56,6 +56,7 @@
#define EXCP_SMC13 /* Secure Monitor Call */
#define EXCP_VIRQ 14
#define EXCP_VFIQ 15
+#define EXCP_WAPT 16
Why do we need a new EXCP value here? In hardware watchpoints
are reported via a particular syndrome value and (for AArch32)
as Data Aborts, so I would expect that we would just do the same.
The code in this patch doesn't seem to treat EXCP_WAPT any
differently from EXCP_DABT...
#define ARMV7M_EXCP_RESET 1
#define ARMV7M_EXCP_NMI 2
diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
index 861f6fa..32bd27d 100644
--- a/target-arm/helper-a64.c
+++ b/target-arm/helper-a64.c
@@ -25,6 +25,7 @@
#include qemu/bitops.h
#include internals.h
#include qemu/crc32c.h
+#include sysemu/kvm.h
#include zlib.h /* For crc32 */
/* C2.4.7 Multiply and divide */
@@ -478,10 +479,13 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
}
arm_log_exception(cs-exception_index);
-qemu_log_mask(CPU_LOG_INT, ...from EL%d\n, arm_current_el(env));
+qemu_log_mask(CPU_LOG_INT, ...from EL%d PC 0x% PRIx64 \n,
+ arm_current_el(env), env-pc);
+
if (qemu_loglevel_mask(CPU_LOG_INT)
!excp_is_internal(cs-exception_index)) {
-qemu_log_mask(CPU_LOG_INT, ...with ESR 0x% PRIx32 \n,
+qemu_log_mask(CPU_LOG_INT, ...with ESR %x/0x% PRIx32 \n,
+ env-exception.syndrome ARM_EL_EC_SHIFT,
env-exception.syndrome);
}
If you just want to improve our debug logging that should be
a separate patch.
@@ -494,6 +498,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
switch (cs-exception_index) {
case EXCP_PREFETCH_ABORT:
case EXCP_DATA_ABORT:
+case EXCP_WAPT:
env-cp15.far_el[new_el] = env-exception.vaddress;
qemu_log_mask(CPU_LOG_INT, ...with FAR 0x% PRIx64 \n,
env-cp15.far_el[new_el]);
@@ -539,6 +544,12 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
aarch64_restore_sp(env, new_el);
env-pc = addr;
-cs-interrupt_request |= CPU_INTERRUPT_EXITTB;
+
+qemu_log_mask(CPU_LOG_INT, ...to EL%d PC 0x% PRIx64 PSTATE 0x%x\n,
+ new_el, env-pc, pstate_read(env));
+
+if (!kvm_enabled()) {
+cs-interrupt_request |= CPU_INTERRUPT_EXITTB;
+}
Do we need a similar change in the AArch32 do_interrupt
code, to handle the case of debugging a 32-bit guest?
}
#endif
diff --git a/target-arm/internals.h b/target-arm/internals.h
index 2cc3017..10e8999 100644
--- a/target-arm/internals.h
+++ b/target-arm/internals.h
@@ -58,6 +58,7 @@ static const char * const excnames[] = {
[EXCP_SMC] = Secure Monitor Call,
[EXCP_VIRQ] = Virtual IRQ,
[EXCP_VFIQ] = Virtual FIQ,
+[EXCP_WAPT] = Watchpoint,
};
static inline void arm_log_exception(int idx)
diff --git a/target-arm/kvm.c b/target-arm/kvm.c
index e1fccdd..6f608d8 100644
--- a/target-arm/kvm.c
+++ b/target-arm/kvm.c
@@ -523,9 +523,11 @@ static int kvm_handle_debug(CPUState *cs, struct kvm_run
*run)
struct kvm_debug_exit_arch *arch_info = run-debug.arch;
int hsr_ec = arch_info-hsr ARM_EL_EC_SHIFT;
ARMCPU *cpu = ARM_CPU(cs);
+CPUClass *cc = CPU_GET_CLASS(cs);
CPUARMState *env = cpu-env;
+int forward_excp = EXCP_BKPT;
-/* Ensure PC is synchronised */
+/* Ensure all state is synchronised */
kvm_cpu_synchronize_state(cs);
Not sure the comment is providing any useful information now;
it probably should either be more explanatory or just deleted.
switch (hsr_ec) {
@@ -533,7 +535,14 @@ static int kvm_handle_debug(CPUState *cs, struct kvm_run
*run)
if (cs-singlestep_enabled) {
return true;
} else {
-error_report(Came out of SINGLE STEP when not enabled);
+/*
+ * The kernel should have supressed the guests ability
Re: [PATCH v5 0/6] QEMU support for KVM Guest Debug on arm64
On 29 May 2015 at 16:19, Alex Bennée alex.ben...@linaro.org wrote: You may be wondering what happened to v3 and v4. They do exist but they didn't change much from the the original patches as I've been mostly looking the kernel side of the equation. So in summary the changes are: - updates to the kernel ABI - don't fall over on kernels without debug support - better logging, syncing and use of internals.h - debug exception re-injection for guest events* Some generic remarks (which we've talked about in irc): * does this correctly handle single step over emulated MMIO insns? how about single step over insns emulated in the kernel without trapping out to userspace? (eg some of the sysregs) kvm_skip_instr() doesn't seem to update PSTATE.SS... * the kernel currently does kvm_skip_instr() before the emulated MMIO exit, not afterwards. That feels conceptually the wrong way round -- are there any interesting corner cases we would get wrong currently but that naturally fall out in the wash if it's done afterwards? * what about debugging a 32-bit guest which uses the 32-bit ARM/Thumb bkpt insns? thanks -- PMM ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[PATCH v2] KVM: arm64: fix misleading comments in save/restore
The elr_el2 and spsr_el2 registers in fact contain the processor state before entry into EL2. In the case of guest state it could be in either el0 or el1. Signed-off-by: Alex Bennée alex.ben...@linaro.org --- v2 - s/hypervisor code/EL2/ - comment: pc/pstate before entering/on return from el2 --- arch/arm64/kvm/hyp.S | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S index 5befd01..519805f 100644 --- a/arch/arm64/kvm/hyp.S +++ b/arch/arm64/kvm/hyp.S @@ -50,8 +50,8 @@ stp x29, lr, [x3, #80] mrs x19, sp_el0 - mrs x20, elr_el2// EL1 PC - mrs x21, spsr_el2 // EL1 pstate + mrs x20, elr_el2// pc before entering el2 + mrs x21, spsr_el2 // pstate before entering el2 stp x19, x20, [x3, #96] str x21, [x3, #112] @@ -82,8 +82,8 @@ ldr x21, [x3, #16] msr sp_el0, x19 - msr elr_el2, x20// EL1 PC - msr spsr_el2, x21 // EL1 pstate + msr elr_el2, x20// pc on return from el2 + msr spsr_el2, x21 // pstate on return from el2 add x3, x2, #CPU_XREG_OFFSET(19) ldp x19, x20, [x3] -- 2.4.2 ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
