* [PATCH v5 0/3] target/ppc: single step for KVM HV
@ 2019-12-11 19:10 Fabiano Rosas
2019-12-11 19:10 ` [PATCH v5 1/3] linux-headers: Update kvm.h for ppc single step capability Fabiano Rosas
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Fabiano Rosas @ 2019-12-11 19:10 UTC (permalink / raw)
To: qemu-devel; +Cc: Alexey Kardashevskiy, Paolo Bonzini, qemu-ppc, David Gibson
I'm resending this series now that the KVM capability got merged:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1a9167a214f
---
Original cover-letter:
Single stepping via GDB/gdbstub is currently not working with KVM
HV. When asking for a single step (stepi), KVM simply ignores the
request and execution continues.
This has the direct effect of breaking GDB's 'step', 'stepi', 'next',
'nexti' commands. The 'continue' command is also affected since
continuing right after a breakpoint requires that GDB first perform a
single step so that the breakpoint can be re-inserted before
continuing - in this case the breakpoint is not re-inserted and it
won't hit again.
The issue here is that single stepping in POWER makes use of an
interrupt (Trace Interrupt [1]) that does not reach the hypervisor, so
while the single step would happen if properly triggered, it would not
cause an exit to KVM so there would be no way of handing control back
to GDB. Aside from that, the guest kernel is not prepared to deal with
such an interrupt in kernel mode (when not using KGDB, or some other
debugging facility) and it causes an Oops.
This series implements a "software single step" approach that makes
use of: i) the Trace Interrupt to perform the step inside the guest
and ii) a breakpoint at the Trace Interrupt handler address to cause a
vm exit (Emulation Assist) so that we can return control to QEMU.
With (i), we basically get the single step for free, without having to
discover what are the possible targets of instructions that divert
execution.
With (ii), we hide the single step from the guest and keep all of the
step logic in QEMU.
Supported scenarios:
- Stepping of multiple vcpus;
- GDB scheduler locking on and off [2];
- single stepping of kernel code with QEMU while stepping with GDB
inside the guest (user space, KGDB).
1- PowerISA Section 6.5.15 - Trace Interrupt
2- https://sourceware.org/gdb/onlinedocs/gdb/All_002dStop-Mode.html
v4 -> v5:
- rebase to v4.2.0-rc5;
- use KVM_CAP_PPC_GUEST_DEBUG_SSTEP (#176) which is now in kernel
v5.5-rc1:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1a9167a214f
v3 -> v4:
- patch 1: fix uninitialized 'offset' variable;
- patch 2: squash with patch 7/7 (now 5/5);
fix exception vector offset calculation when AIL == 0;
- patch 3: squash with 4/7 (now 2/5);
- patch 7: introduce ppc_gdb_get_{op,xop,spr} functions;
introduce ppc_gdb_read_insn function;
define constants for mfmsr, rfid, mtspr extended opcodes;
fix bug where instructions would not be properly
recognized in SLOF due to guest endianness being different
from host's;
pass arch_info->address directly into functions that only
need the address;
improve indentation by returning early when possible.
https://lists.nongnu.org/archive/html/qemu-devel/2019-02/msg07994.html
v2 -> v3:
- take Alternate Interrupt Location (AIL) into consideration when
calculating the Trace Interrupt handler address (this allows single
stepping in SLOF code);
- check for a new KVM_GUEST_DEBUG_SSTEP capability (still to be
submitted to kernel ml);
- handle other vcpus (not currently stepping) hitting the single step
breakpoint - by ignoring the breakpoint;
- handle simultaneous single step by GDB inside guest - by first
performing our step into the trace interrupt handler itself and
returning to the guest afterwards;
- handle single stepping when at the first trace interrupt handler
instruction - by displacing the breakpoint to the next instruction;
- restore MSR, SRR0, SRR1 after the step, taking into consideration
possible mtspr, mtmsr instructions;
- use stubs for arch-specific code that will not be implemented by
other architectures at this point;
https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg04627.html
v1 -> v2:
- split in more patches to facilitate review
- use extract32 for decoding instruction instead of open-coding
- add more people to CC
https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg04269.html
v1:
https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg03738.html
Fabiano Rosas (3):
linux-headers: Update kvm.h for ppc single step capability
kvm-all: Introduce kvm_set_singlestep
target/ppc: support single stepping with KVM HV
accel/kvm/kvm-all.c | 14 +++
accel/stubs/kvm-stub.c | 4 +
exec.c | 2 +-
include/sysemu/kvm.h | 4 +
linux-headers/linux/kvm.h | 1 +
stubs/Makefile.objs | 1 +
stubs/kvm-arch-singlestep.c | 14 +++
target/ppc/cpu.h | 16 +++
target/ppc/excp_helper.c | 13 +++
target/ppc/gdbstub.c | 35 ++++++
target/ppc/kvm.c | 209 ++++++++++++++++++++++++++++++++++--
11 files changed, 305 insertions(+), 8 deletions(-)
create mode 100644 stubs/kvm-arch-singlestep.c
--
2.23.0
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v5 1/3] linux-headers: Update kvm.h for ppc single step capability
2019-12-11 19:10 [PATCH v5 0/3] target/ppc: single step for KVM HV Fabiano Rosas
@ 2019-12-11 19:10 ` Fabiano Rosas
2019-12-12 3:44 ` David Gibson
2019-12-11 19:10 ` [PATCH v5 2/3] kvm-all: Introduce kvm_set_singlestep Fabiano Rosas
2019-12-11 19:10 ` [PATCH v5 3/3] target/ppc: support single stepping with KVM HV Fabiano Rosas
2 siblings, 1 reply; 10+ messages in thread
From: Fabiano Rosas @ 2019-12-11 19:10 UTC (permalink / raw)
To: qemu-devel; +Cc: Alexey Kardashevskiy, Paolo Bonzini, qemu-ppc, David Gibson
Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
---
| 1 +
1 file changed, 1 insertion(+)
--git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index 3d9b18f7f8..488f3baf01 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -1000,6 +1000,7 @@ struct kvm_ppc_resize_hpt {
#define KVM_CAP_PMU_EVENT_FILTER 173
#define KVM_CAP_ARM_IRQ_LINE_LAYOUT_2 174
#define KVM_CAP_HYPERV_DIRECT_TLBFLUSH 175
+#define KVM_CAP_PPC_GUEST_DEBUG_SSTEP 176
#ifdef KVM_CAP_IRQ_ROUTING
--
2.23.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v5 2/3] kvm-all: Introduce kvm_set_singlestep
2019-12-11 19:10 [PATCH v5 0/3] target/ppc: single step for KVM HV Fabiano Rosas
2019-12-11 19:10 ` [PATCH v5 1/3] linux-headers: Update kvm.h for ppc single step capability Fabiano Rosas
@ 2019-12-11 19:10 ` Fabiano Rosas
2019-12-12 4:50 ` David Gibson
2019-12-11 19:10 ` [PATCH v5 3/3] target/ppc: support single stepping with KVM HV Fabiano Rosas
2 siblings, 1 reply; 10+ messages in thread
From: Fabiano Rosas @ 2019-12-11 19:10 UTC (permalink / raw)
To: qemu-devel; +Cc: Alexey Kardashevskiy, Paolo Bonzini, qemu-ppc, David Gibson
For single stepping (via KVM) of a guest vcpu to work, KVM needs not
only to support the SET_GUEST_DEBUG ioctl but to also recognize the
KVM_GUESTDBG_SINGLESTEP bit in the control field of the
kvm_guest_debug struct.
This patch adds support for querying the single step capability so
that QEMU can decide what to do for the platforms that do not have
such support.
This will allow architecture-specific implementations of a fallback
mechanism for single stepping in cases where KVM does not support it.
Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
---
accel/kvm/kvm-all.c | 14 ++++++++++++++
accel/stubs/kvm-stub.c | 4 ++++
exec.c | 2 +-
include/sysemu/kvm.h | 4 ++++
stubs/Makefile.objs | 1 +
stubs/kvm-arch-singlestep.c | 14 ++++++++++++++
target/ppc/kvm.c | 14 ++++++++++++++
7 files changed, 52 insertions(+), 1 deletion(-)
create mode 100644 stubs/kvm-arch-singlestep.c
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index ca00daa2f5..a61beb0b53 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2599,6 +2599,11 @@ bool kvm_arm_supports_user_irq(void)
}
#ifdef KVM_CAP_SET_GUEST_DEBUG
+bool kvm_has_guest_debug_singlestep(CPUState *cs)
+{
+ return kvm_arch_has_guest_debug_singlestep(cs);
+}
+
struct kvm_sw_breakpoint *kvm_find_sw_breakpoint(CPUState *cpu,
target_ulong pc)
{
@@ -2647,6 +2652,15 @@ int kvm_update_guest_debug(CPUState *cpu, unsigned long reinject_trap)
return data.err;
}
+void kvm_set_singlestep(CPUState *cs, int enabled)
+{
+ if (kvm_has_guest_debug_singlestep(cs)) {
+ kvm_update_guest_debug(cs, 0);
+ } else {
+ kvm_arch_set_singlestep(cs, enabled);
+ }
+}
+
int kvm_insert_breakpoint(CPUState *cpu, target_ulong addr,
target_ulong len, int type)
{
diff --git a/accel/stubs/kvm-stub.c b/accel/stubs/kvm-stub.c
index 82f118d2df..b4df48b6f1 100644
--- a/accel/stubs/kvm-stub.c
+++ b/accel/stubs/kvm-stub.c
@@ -78,6 +78,10 @@ int kvm_update_guest_debug(CPUState *cpu, unsigned long reinject_trap)
return -ENOSYS;
}
+void kvm_set_singlestep(CPUState *cs, int enabled)
+{
+}
+
int kvm_insert_breakpoint(CPUState *cpu, target_ulong addr,
target_ulong len, int type)
{
diff --git a/exec.c b/exec.c
index ffdb518535..ff46ea1846 100644
--- a/exec.c
+++ b/exec.c
@@ -1202,7 +1202,7 @@ void cpu_single_step(CPUState *cpu, int enabled)
if (cpu->singlestep_enabled != enabled) {
cpu->singlestep_enabled = enabled;
if (kvm_enabled()) {
- kvm_update_guest_debug(cpu, 0);
+ kvm_set_singlestep(cpu, enabled);
} else {
/* must flush all the translated code to avoid inconsistencies */
/* XXX: only flush what is necessary */
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index 9fe233b9bf..7a42978b11 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -215,6 +215,7 @@ int kvm_has_pit_state2(void);
int kvm_has_many_ioeventfds(void);
int kvm_has_gsi_routing(void);
int kvm_has_intx_set_mask(void);
+bool kvm_has_guest_debug_singlestep(CPUState *cs);
int kvm_init_vcpu(CPUState *cpu);
int kvm_cpu_exec(CPUState *cpu);
@@ -247,6 +248,8 @@ bool kvm_memcrypt_enabled(void);
*/
int kvm_memcrypt_encrypt_data(uint8_t *ptr, uint64_t len);
+bool kvm_arch_has_guest_debug_singlestep(CPUState *cs);
+void kvm_arch_set_singlestep(CPUState *cpu, int enabled);
#ifdef NEED_CPU_H
#include "cpu.h"
@@ -259,6 +262,7 @@ int kvm_remove_breakpoint(CPUState *cpu, target_ulong addr,
target_ulong len, int type);
void kvm_remove_all_breakpoints(CPUState *cpu);
int kvm_update_guest_debug(CPUState *cpu, unsigned long reinject_trap);
+void kvm_set_singlestep(CPUState *cs, int enabled);
int kvm_on_sigbus_vcpu(CPUState *cpu, int code, void *addr);
int kvm_on_sigbus(int code, void *addr);
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index 4a50e95ec3..361f4088fa 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -12,6 +12,7 @@ stub-obj-y += get-vm-name.o
stub-obj-y += iothread.o
stub-obj-y += iothread-lock.o
stub-obj-y += is-daemonized.o
+stub-obj-y += kvm-arch-singlestep.o
stub-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
stub-obj-y += machine-init-done.o
stub-obj-y += migr-blocker.o
diff --git a/stubs/kvm-arch-singlestep.c b/stubs/kvm-arch-singlestep.c
new file mode 100644
index 0000000000..18bfba61f6
--- /dev/null
+++ b/stubs/kvm-arch-singlestep.c
@@ -0,0 +1,14 @@
+#include "qemu/osdep.h"
+#include "qemu/error-report.h"
+#include "sysemu/kvm.h"
+
+bool kvm_arch_has_guest_debug_singlestep(CPUState *cs)
+{
+ /* for backwards compatibility assume the feature is present */
+ return true;
+}
+
+void kvm_arch_set_singlestep(CPUState *cpu, int enabled)
+{
+ warn_report("KVM does not support single stepping");
+}
diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index c77f9848ec..3a2cfe883c 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -85,6 +85,7 @@ static int cap_ppc_safe_indirect_branch;
static int cap_ppc_count_cache_flush_assist;
static int cap_ppc_nested_kvm_hv;
static int cap_large_decr;
+static int cap_ppc_singlestep;
static uint32_t debug_inst_opcode;
@@ -133,6 +134,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
kvmppc_get_cpu_characteristics(s);
cap_ppc_nested_kvm_hv = kvm_vm_check_extension(s, KVM_CAP_PPC_NESTED_HV);
cap_large_decr = kvmppc_get_dec_bits();
+ cap_ppc_singlestep = kvm_vm_check_extension(s, KVM_CAP_PPC_GUEST_DEBUG_SSTEP);
/*
* Note: setting it to false because there is not such capability
* in KVM at this moment.
@@ -1380,6 +1382,18 @@ static int kvmppc_handle_dcr_write(CPUPPCState *env,
return 0;
}
+bool kvm_arch_has_guest_debug_singlestep(CPUState *cs)
+{
+ PowerPCCPU *cpu = POWERPC_CPU(cs);
+ CPUPPCState *env = &cpu->env;
+
+ if (cap_ppc_singlestep) {
+ return true;
+ }
+
+ return env->excp_model == POWERPC_EXCP_BOOKE || kvmppc_is_pr(kvm_state);
+}
+
int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
{
/* Mixed endian case is not handled */
--
2.23.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v5 3/3] target/ppc: support single stepping with KVM HV
2019-12-11 19:10 [PATCH v5 0/3] target/ppc: single step for KVM HV Fabiano Rosas
2019-12-11 19:10 ` [PATCH v5 1/3] linux-headers: Update kvm.h for ppc single step capability Fabiano Rosas
2019-12-11 19:10 ` [PATCH v5 2/3] kvm-all: Introduce kvm_set_singlestep Fabiano Rosas
@ 2019-12-11 19:10 ` Fabiano Rosas
2019-12-12 5:08 ` David Gibson
2 siblings, 1 reply; 10+ messages in thread
From: Fabiano Rosas @ 2019-12-11 19:10 UTC (permalink / raw)
To: qemu-devel; +Cc: Alexey Kardashevskiy, Paolo Bonzini, qemu-ppc, David Gibson
The hardware singlestep mechanism in POWER works via a Trace Interrupt
(0xd00) that happens after any instruction executes, whenever MSR_SE =
1 (PowerISA Section 6.5.15 - Trace Interrupt).
However, with kvm_hv, the Trace Interrupt happens inside the guest and
KVM has no visibility of it. Therefore, when the gdbstub uses the
KVM_SET_GUEST_DEBUG ioctl to enable singlestep, KVM simply ignores it.
This patch takes advantage of the Trace Interrupt to perform the step
inside the guest, but uses a breakpoint at the Trace Interrupt handler
to return control to KVM. The exit is treated by KVM as a regular
breakpoint and it returns to the host (and QEMU eventually).
Before signalling GDB, QEMU sets the Next Instruction Pointer to the
instruction following the one being stepped and restores the MSR,
SRR0, SRR1 values from before the step, effectively skipping the
interrupt handler execution and hiding the trace interrupt breakpoint
from GDB.
This approach works with both of GDB's 'scheduler-locking' options
(off, step).
Note:
- kvm_arch_set_singlestep happens after GDB asks for a single step,
while the vcpus are stopped.
- kvm_handle_singlestep executes after the step, during the handling
of the Emulation Assist Interrupt (breakpoint).
Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
---
target/ppc/cpu.h | 16 ++++
target/ppc/excp_helper.c | 13 +++
target/ppc/gdbstub.c | 35 +++++++
target/ppc/kvm.c | 195 +++++++++++++++++++++++++++++++++++++--
4 files changed, 252 insertions(+), 7 deletions(-)
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index e3e82327b7..37119cd0b4 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1156,6 +1156,11 @@ struct CPUPPCState {
uint32_t tm_vscr;
uint64_t tm_dscr;
uint64_t tm_tar;
+
+ /* Used for software single step */
+ target_ulong sstep_msr;
+ target_ulong sstep_srr0;
+ target_ulong sstep_srr1;
};
#define SET_FIT_PERIOD(a_, b_, c_, d_) \
@@ -1253,6 +1258,7 @@ struct PPCVirtualHypervisorClass {
OBJECT_GET_CLASS(PPCVirtualHypervisorClass, (obj), \
TYPE_PPC_VIRTUAL_HYPERVISOR)
+target_ulong ppc_get_trace_int_handler_addr(CPUState *cs);
void ppc_cpu_do_interrupt(CPUState *cpu);
bool ppc_cpu_exec_interrupt(CPUState *cpu, int int_req);
void ppc_cpu_dump_state(CPUState *cpu, FILE *f, int flags);
@@ -1266,6 +1272,12 @@ int ppc_cpu_gdb_write_register_apple(CPUState *cpu, uint8_t *buf, int reg);
void ppc_gdb_gen_spr_xml(PowerPCCPU *cpu);
const char *ppc_gdb_get_dynamic_xml(CPUState *cs, const char *xml_name);
#endif
+uint32_t ppc_gdb_read_insn(CPUState *cs, target_ulong addr);
+uint32_t ppc_gdb_get_op(uint32_t insn);
+uint32_t ppc_gdb_get_xop(uint32_t insn);
+uint32_t ppc_gdb_get_spr(uint32_t insn);
+uint32_t ppc_gdb_get_rt(uint32_t insn);
+
int ppc64_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
int cpuid, void *opaque);
int ppc32_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
@@ -2217,6 +2229,10 @@ enum {
PPC2_ISA300)
};
+#define XOP_RFID 18
+#define XOP_MFMSR 83
+#define XOP_MTSPR 467
+
/*****************************************************************************/
/*
* Memory access type :
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 50b004d00d..8ce395004a 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -112,6 +112,8 @@ static uint64_t ppc_excp_vector_offset(CPUState *cs, int ail)
uint64_t offset = 0;
switch (ail) {
+ case AIL_NONE:
+ break;
case AIL_0001_8000:
offset = 0x18000;
break;
@@ -782,6 +784,17 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
check_tlb_flush(env, false);
}
+target_ulong ppc_get_trace_int_handler_addr(CPUState *cs)
+{
+ PowerPCCPU *cpu = POWERPC_CPU(cs);
+ CPUPPCState *env = &cpu->env;
+ int ail;
+
+ ail = (env->spr[SPR_LPCR] & LPCR_AIL) >> LPCR_AIL_SHIFT;
+ return env->excp_vectors[POWERPC_EXCP_TRACE] |
+ ppc_excp_vector_offset(cs, ail);
+}
+
void ppc_cpu_do_interrupt(CPUState *cs)
{
PowerPCCPU *cpu = POWERPC_CPU(cs);
diff --git a/target/ppc/gdbstub.c b/target/ppc/gdbstub.c
index 823759c92e..540b767445 100644
--- a/target/ppc/gdbstub.c
+++ b/target/ppc/gdbstub.c
@@ -383,3 +383,38 @@ const char *ppc_gdb_get_dynamic_xml(CPUState *cs, const char *xml_name)
return NULL;
}
#endif
+
+uint32_t ppc_gdb_read_insn(CPUState *cs, target_ulong addr)
+{
+ PowerPCCPU *cpu = POWERPC_CPU(cs);
+ CPUPPCState *env = &cpu->env;
+ uint32_t insn;
+
+ cpu_memory_rw_debug(cs, addr, (uint8_t *)&insn, sizeof(insn), 0);
+
+ if (msr_le) {
+ return ldl_le_p(&insn);
+ } else {
+ return ldl_be_p(&insn);
+ }
+}
+
+uint32_t ppc_gdb_get_op(uint32_t insn)
+{
+ return extract32(insn, 26, 6);
+}
+
+uint32_t ppc_gdb_get_xop(uint32_t insn)
+{
+ return extract32(insn, 1, 10);
+}
+
+uint32_t ppc_gdb_get_spr(uint32_t insn)
+{
+ return extract32(insn, 11, 5) << 5 | extract32(insn, 16, 5);
+}
+
+uint32_t ppc_gdb_get_rt(uint32_t insn)
+{
+ return extract32(insn, 21, 5);
+}
diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 3a2cfe883c..fedb8e787d 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -1542,6 +1542,86 @@ void kvm_arch_remove_all_hw_breakpoints(void)
nb_hw_breakpoint = nb_hw_watchpoint = 0;
}
+void kvm_arch_set_singlestep(CPUState *cs, int enabled)
+{
+ PowerPCCPU *cpu = POWERPC_CPU(cs);
+ CPUPPCState *env = &cpu->env;
+ target_ulong trace_handler_addr;
+ uint32_t insn;
+ bool rfid;
+
+ if (!enabled) {
+ return;
+ }
+
+ cpu_synchronize_state(cs);
+ insn = ppc_gdb_read_insn(cs, env->nip);
+
+ /*
+ * rfid needs special handling because it:
+ * - overwrites NIP with SRR0;
+ * - overwrites MSR with SRR1;
+ * - cannot be single stepped.
+ */
+ rfid = ppc_gdb_get_op(insn) == 19 && ppc_gdb_get_xop(insn) == XOP_RFID;
+
+ if (rfid && kvm_find_sw_breakpoint(cs, env->spr[SPR_SRR0])) {
+ /*
+ * There is a breakpoint at the next instruction address. It
+ * will already cause the vm exit we need for the single step,
+ * so there's nothing to be done.
+ */
+ return;
+ }
+
+ /*
+ * Save the registers that will be affected by the single step
+ * mechanism. These will be restored after the step at
+ * kvm_handle_singlestep.
+ */
+ env->sstep_msr = env->msr;
+ env->sstep_srr0 = env->spr[SPR_SRR0];
+ env->sstep_srr1 = env->spr[SPR_SRR1];
+
+ /*
+ * MSR_SE = 1 will cause a Trace Interrupt in the guest after the
+ * next instruction executes. If this is a rfid, use SRR1 instead
+ * of MSR.
+ */
+ if (rfid) {
+ if ((env->spr[SPR_SRR1] >> MSR_SE) & 1) {
+ /*
+ * The guest is doing a single step itself. Make sure we
+ * restore it later.
+ */
+ env->sstep_msr |= (1ULL << MSR_SE);
+ }
+
+ env->spr[SPR_SRR1] |= (1ULL << MSR_SE);
+ } else {
+ env->msr |= (1ULL << MSR_SE);
+ }
+
+ /*
+ * We set a breakpoint at the interrupt handler address so
+ * that the singlestep will be seen by KVM (this is treated by
+ * KVM like an ordinary breakpoint) and control is returned to
+ * QEMU.
+ */
+ trace_handler_addr = ppc_get_trace_int_handler_addr(cs);
+
+ if (env->nip == trace_handler_addr) {
+ /*
+ * We are trying to step over the interrupt handler
+ * address itself; move the breakpoint to the next
+ * instruction.
+ */
+ trace_handler_addr += 4;
+ }
+
+ kvm_insert_breakpoint(cs, trace_handler_addr, 4, GDB_BREAKPOINT_SW);
+}
+
void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
{
int n;
@@ -1581,6 +1661,91 @@ void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
}
}
+/* Revert any side-effects caused during single step */
+static void restore_singlestep_env(CPUState *cs)
+{
+ PowerPCCPU *cpu = POWERPC_CPU(cs);
+ CPUPPCState *env = &cpu->env;
+ uint32_t insn;
+ int reg;
+ int spr;
+
+ insn = ppc_gdb_read_insn(cs, env->spr[SPR_SRR0] - 4);
+
+ env->spr[SPR_SRR0] = env->sstep_srr0;
+ env->spr[SPR_SRR1] = env->sstep_srr1;
+
+ if (ppc_gdb_get_op(insn) != 31) {
+ return;
+ }
+
+ reg = ppc_gdb_get_rt(insn);
+
+ switch (ppc_gdb_get_xop(insn)) {
+ case XOP_MTSPR:
+ /*
+ * mtspr: the guest altered the SRR, so do not use the
+ * pre-step value.
+ */
+ spr = ppc_gdb_get_spr(insn);
+ if (spr == SPR_SRR0 || spr == SPR_SRR1) {
+ env->spr[spr] = env->gpr[reg];
+ }
+ break;
+ case XOP_MFMSR:
+ /*
+ * mfmsr: clear MSR_SE bit to avoid the guest knowing
+ * that it is being single-stepped.
+ */
+ env->gpr[reg] &= ~(1ULL << MSR_SE);
+ break;
+ }
+}
+
+static int kvm_handle_singlestep(CPUState *cs, target_ulong address)
+{
+ PowerPCCPU *cpu = POWERPC_CPU(cs);
+ CPUPPCState *env = &cpu->env;
+ target_ulong trace_handler_addr;
+
+ if (cap_ppc_singlestep) {
+ return 1;
+ }
+
+ cpu_synchronize_state(cs);
+ trace_handler_addr = ppc_get_trace_int_handler_addr(cs);
+
+ if (address == trace_handler_addr) {
+ kvm_remove_breakpoint(cs, trace_handler_addr, 4, GDB_BREAKPOINT_SW);
+
+ if (env->sstep_msr & (1ULL << MSR_SE)) {
+ /*
+ * The guest expects the last instruction to have caused a
+ * single step, go back into the interrupt handler.
+ */
+ return 1;
+ }
+
+ env->nip = env->spr[SPR_SRR0];
+ /* Bits 33-36, 43-47 are set by the interrupt */
+ env->msr = env->spr[SPR_SRR1] & ~(1ULL << MSR_SE |
+ PPC_BITMASK(33, 36) |
+ PPC_BITMASK(43, 47));
+ restore_singlestep_env(cs);
+
+ } else if (address == trace_handler_addr + 4) {
+ /*
+ * A step at trace_handler_addr would interfere with the
+ * singlestep mechanism itself, so we have previously
+ * displaced the breakpoint to the next instruction.
+ */
+ kvm_remove_breakpoint(cs, trace_handler_addr + 4, 4, GDB_BREAKPOINT_SW);
+ restore_singlestep_env(cs);
+ }
+
+ return 1;
+}
+
static int kvm_handle_hw_breakpoint(CPUState *cs,
struct kvm_debug_exit_arch *arch_info)
{
@@ -1608,13 +1773,29 @@ static int kvm_handle_hw_breakpoint(CPUState *cs,
return handle;
}
-static int kvm_handle_singlestep(void)
+static int kvm_handle_sw_breakpoint(CPUState *cs, target_ulong address)
{
- return 1;
-}
+ target_ulong trace_handler_addr;
-static int kvm_handle_sw_breakpoint(void)
-{
+ if (cap_ppc_singlestep) {
+ return 1;
+ }
+
+ cpu_synchronize_state(cs);
+ trace_handler_addr = ppc_get_trace_int_handler_addr(cs);
+
+ if (address == trace_handler_addr) {
+ CPU_FOREACH(cs) {
+ if (cs->singlestep_enabled) {
+ /*
+ * We hit this breakpoint while another cpu is doing a
+ * software single step. Go back into the guest to
+ * give chance for the single step to finish.
+ */
+ return 0;
+ }
+ }
+ }
return 1;
}
@@ -1625,7 +1806,7 @@ static int kvm_handle_debug(PowerPCCPU *cpu, struct kvm_run *run)
struct kvm_debug_exit_arch *arch_info = &run->debug.arch;
if (cs->singlestep_enabled) {
- return kvm_handle_singlestep();
+ return kvm_handle_singlestep(cs, arch_info->address);
}
if (arch_info->status) {
@@ -1633,7 +1814,7 @@ static int kvm_handle_debug(PowerPCCPU *cpu, struct kvm_run *run)
}
if (kvm_find_sw_breakpoint(cs, arch_info->address)) {
- return kvm_handle_sw_breakpoint();
+ return kvm_handle_sw_breakpoint(cs, arch_info->address);
}
/*
--
2.23.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v5 1/3] linux-headers: Update kvm.h for ppc single step capability
2019-12-11 19:10 ` [PATCH v5 1/3] linux-headers: Update kvm.h for ppc single step capability Fabiano Rosas
@ 2019-12-12 3:44 ` David Gibson
2019-12-13 21:03 ` Fabiano Rosas
0 siblings, 1 reply; 10+ messages in thread
From: David Gibson @ 2019-12-12 3:44 UTC (permalink / raw)
To: Fabiano Rosas; +Cc: Alexey Kardashevskiy, Paolo Bonzini, qemu-ppc, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1160 bytes --]
On Wed, Dec 11, 2019 at 04:10:11PM -0300, Fabiano Rosas wrote:
> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
Generally, imported linux header updates are done as a block, pulling
qemu up to a specified kernel commit id, rather than just grabbing
pieces for a particular feature.
There's actually already a header update to be2eca94d144 in my
ppc-for-5.0 tree. Is that recent enough for what you need here?
> ---
> linux-headers/linux/kvm.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
> index 3d9b18f7f8..488f3baf01 100644
> --- a/linux-headers/linux/kvm.h
> +++ b/linux-headers/linux/kvm.h
> @@ -1000,6 +1000,7 @@ struct kvm_ppc_resize_hpt {
> #define KVM_CAP_PMU_EVENT_FILTER 173
> #define KVM_CAP_ARM_IRQ_LINE_LAYOUT_2 174
> #define KVM_CAP_HYPERV_DIRECT_TLBFLUSH 175
> +#define KVM_CAP_PPC_GUEST_DEBUG_SSTEP 176
>
> #ifdef KVM_CAP_IRQ_ROUTING
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 2/3] kvm-all: Introduce kvm_set_singlestep
2019-12-11 19:10 ` [PATCH v5 2/3] kvm-all: Introduce kvm_set_singlestep Fabiano Rosas
@ 2019-12-12 4:50 ` David Gibson
2019-12-13 21:03 ` Fabiano Rosas
0 siblings, 1 reply; 10+ messages in thread
From: David Gibson @ 2019-12-12 4:50 UTC (permalink / raw)
To: Fabiano Rosas; +Cc: Alexey Kardashevskiy, Paolo Bonzini, qemu-ppc, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 7820 bytes --]
On Wed, Dec 11, 2019 at 04:10:12PM -0300, Fabiano Rosas wrote:
> For single stepping (via KVM) of a guest vcpu to work, KVM needs not
> only to support the SET_GUEST_DEBUG ioctl but to also recognize the
> KVM_GUESTDBG_SINGLESTEP bit in the control field of the
> kvm_guest_debug struct.
>
> This patch adds support for querying the single step capability so
> that QEMU can decide what to do for the platforms that do not have
> such support.
>
> This will allow architecture-specific implementations of a fallback
> mechanism for single stepping in cases where KVM does not support it.
>
> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
I think this is fine, but it took me a while to figure out what was
going on. I wonder if better comments and/or function names might
help.
> ---
> accel/kvm/kvm-all.c | 14 ++++++++++++++
> accel/stubs/kvm-stub.c | 4 ++++
> exec.c | 2 +-
> include/sysemu/kvm.h | 4 ++++
> stubs/Makefile.objs | 1 +
> stubs/kvm-arch-singlestep.c | 14 ++++++++++++++
> target/ppc/kvm.c | 14 ++++++++++++++
> 7 files changed, 52 insertions(+), 1 deletion(-)
> create mode 100644 stubs/kvm-arch-singlestep.c
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index ca00daa2f5..a61beb0b53 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -2599,6 +2599,11 @@ bool kvm_arm_supports_user_irq(void)
> }
>
> #ifdef KVM_CAP_SET_GUEST_DEBUG
> +bool kvm_has_guest_debug_singlestep(CPUState *cs)
AIUI, this function is saying if setting the singlestep flag in the
KVM debug regs is sufficient to single step the guest under host
control. The name doesn't make that terribly obvious to me, though a
better one isn't really obvious to me.
> +{
> + return kvm_arch_has_guest_debug_singlestep(cs);
I also don't see a lot of point to this wrapper, rather than just
calling the arch version directly.
> +}
> +
> struct kvm_sw_breakpoint *kvm_find_sw_breakpoint(CPUState *cpu,
> target_ulong pc)
> {
> @@ -2647,6 +2652,15 @@ int kvm_update_guest_debug(CPUState *cpu, unsigned long reinject_trap)
> return data.err;
> }
>
> +void kvm_set_singlestep(CPUState *cs, int enabled)
> +{
> + if (kvm_has_guest_debug_singlestep(cs)) {
> + kvm_update_guest_debug(cs, 0);
> + } else {
> + kvm_arch_set_singlestep(cs, enabled);
> + }
> +}
> +
> int kvm_insert_breakpoint(CPUState *cpu, target_ulong addr,
> target_ulong len, int type)
> {
> diff --git a/accel/stubs/kvm-stub.c b/accel/stubs/kvm-stub.c
> index 82f118d2df..b4df48b6f1 100644
> --- a/accel/stubs/kvm-stub.c
> +++ b/accel/stubs/kvm-stub.c
> @@ -78,6 +78,10 @@ int kvm_update_guest_debug(CPUState *cpu, unsigned long reinject_trap)
> return -ENOSYS;
> }
>
> +void kvm_set_singlestep(CPUState *cs, int enabled)
> +{
AFAICT this should only be called with KVM enabled, so this should
maybe be a g_assert_not_reached() rather than a no-op.
> +}
> +
> int kvm_insert_breakpoint(CPUState *cpu, target_ulong addr,
> target_ulong len, int type)
> {
> diff --git a/exec.c b/exec.c
> index ffdb518535..ff46ea1846 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1202,7 +1202,7 @@ void cpu_single_step(CPUState *cpu, int enabled)
> if (cpu->singlestep_enabled != enabled) {
> cpu->singlestep_enabled = enabled;
> if (kvm_enabled()) {
> - kvm_update_guest_debug(cpu, 0);
> + kvm_set_singlestep(cpu, enabled);
> } else {
> /* must flush all the translated code to avoid inconsistencies */
> /* XXX: only flush what is necessary */
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index 9fe233b9bf..7a42978b11 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -215,6 +215,7 @@ int kvm_has_pit_state2(void);
> int kvm_has_many_ioeventfds(void);
> int kvm_has_gsi_routing(void);
> int kvm_has_intx_set_mask(void);
> +bool kvm_has_guest_debug_singlestep(CPUState *cs);
>
> int kvm_init_vcpu(CPUState *cpu);
> int kvm_cpu_exec(CPUState *cpu);
> @@ -247,6 +248,8 @@ bool kvm_memcrypt_enabled(void);
> */
> int kvm_memcrypt_encrypt_data(uint8_t *ptr, uint64_t len);
>
> +bool kvm_arch_has_guest_debug_singlestep(CPUState *cs);
> +void kvm_arch_set_singlestep(CPUState *cpu, int enabled);
>
> #ifdef NEED_CPU_H
> #include "cpu.h"
> @@ -259,6 +262,7 @@ int kvm_remove_breakpoint(CPUState *cpu, target_ulong addr,
> target_ulong len, int type);
> void kvm_remove_all_breakpoints(CPUState *cpu);
> int kvm_update_guest_debug(CPUState *cpu, unsigned long reinject_trap);
> +void kvm_set_singlestep(CPUState *cs, int enabled);
>
> int kvm_on_sigbus_vcpu(CPUState *cpu, int code, void *addr);
> int kvm_on_sigbus(int code, void *addr);
> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> index 4a50e95ec3..361f4088fa 100644
> --- a/stubs/Makefile.objs
> +++ b/stubs/Makefile.objs
> @@ -12,6 +12,7 @@ stub-obj-y += get-vm-name.o
> stub-obj-y += iothread.o
> stub-obj-y += iothread-lock.o
> stub-obj-y += is-daemonized.o
> +stub-obj-y += kvm-arch-singlestep.o
> stub-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
> stub-obj-y += machine-init-done.o
> stub-obj-y += migr-blocker.o
> diff --git a/stubs/kvm-arch-singlestep.c b/stubs/kvm-arch-singlestep.c
> new file mode 100644
> index 0000000000..18bfba61f6
> --- /dev/null
> +++ b/stubs/kvm-arch-singlestep.c
> @@ -0,0 +1,14 @@
> +#include "qemu/osdep.h"
> +#include "qemu/error-report.h"
> +#include "sysemu/kvm.h"
> +
> +bool kvm_arch_has_guest_debug_singlestep(CPUState *cs)
> +{
> + /* for backwards compatibility assume the feature is present */
> + return true;
> +}
> +
> +void kvm_arch_set_singlestep(CPUState *cpu, int enabled)
> +{
> + warn_report("KVM does not support single stepping");
> +}
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index c77f9848ec..3a2cfe883c 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -85,6 +85,7 @@ static int cap_ppc_safe_indirect_branch;
> static int cap_ppc_count_cache_flush_assist;
> static int cap_ppc_nested_kvm_hv;
> static int cap_large_decr;
> +static int cap_ppc_singlestep;
>
> static uint32_t debug_inst_opcode;
>
> @@ -133,6 +134,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
> kvmppc_get_cpu_characteristics(s);
> cap_ppc_nested_kvm_hv = kvm_vm_check_extension(s, KVM_CAP_PPC_NESTED_HV);
> cap_large_decr = kvmppc_get_dec_bits();
> + cap_ppc_singlestep = kvm_vm_check_extension(s, KVM_CAP_PPC_GUEST_DEBUG_SSTEP);
> /*
> * Note: setting it to false because there is not such capability
> * in KVM at this moment.
> @@ -1380,6 +1382,18 @@ static int kvmppc_handle_dcr_write(CPUPPCState *env,
> return 0;
> }
>
> +bool kvm_arch_has_guest_debug_singlestep(CPUState *cs)
> +{
> + PowerPCCPU *cpu = POWERPC_CPU(cs);
> + CPUPPCState *env = &cpu->env;
> +
> + if (cap_ppc_singlestep) {
> + return true;
> + }
> +
Perhaps note explicitly that this is a fallback guess when the cap is
not available to give us a good answer.
> + return env->excp_model == POWERPC_EXCP_BOOKE || kvmppc_is_pr(kvm_state);
> +}
> +
> int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> {
> /* Mixed endian case is not handled */
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 3/3] target/ppc: support single stepping with KVM HV
2019-12-11 19:10 ` [PATCH v5 3/3] target/ppc: support single stepping with KVM HV Fabiano Rosas
@ 2019-12-12 5:08 ` David Gibson
2019-12-13 21:06 ` Fabiano Rosas
0 siblings, 1 reply; 10+ messages in thread
From: David Gibson @ 2019-12-12 5:08 UTC (permalink / raw)
To: Fabiano Rosas; +Cc: Alexey Kardashevskiy, Paolo Bonzini, qemu-ppc, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 14793 bytes --]
On Wed, Dec 11, 2019 at 04:10:13PM -0300, Fabiano Rosas wrote:
> The hardware singlestep mechanism in POWER works via a Trace Interrupt
> (0xd00) that happens after any instruction executes, whenever MSR_SE =
> 1 (PowerISA Section 6.5.15 - Trace Interrupt).
>
> However, with kvm_hv, the Trace Interrupt happens inside the guest and
> KVM has no visibility of it. Therefore, when the gdbstub uses the
> KVM_SET_GUEST_DEBUG ioctl to enable singlestep, KVM simply ignores it.
>
> This patch takes advantage of the Trace Interrupt to perform the step
> inside the guest, but uses a breakpoint at the Trace Interrupt handler
> to return control to KVM. The exit is treated by KVM as a regular
> breakpoint and it returns to the host (and QEMU eventually).
>
> Before signalling GDB, QEMU sets the Next Instruction Pointer to the
> instruction following the one being stepped and restores the MSR,
> SRR0, SRR1 values from before the step, effectively skipping the
> interrupt handler execution and hiding the trace interrupt breakpoint
> from GDB.
>
> This approach works with both of GDB's 'scheduler-locking' options
> (off, step).
>
> Note:
>
> - kvm_arch_set_singlestep happens after GDB asks for a single step,
> while the vcpus are stopped.
>
> - kvm_handle_singlestep executes after the step, during the handling
> of the Emulation Assist Interrupt (breakpoint).
>
> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
> ---
> target/ppc/cpu.h | 16 ++++
> target/ppc/excp_helper.c | 13 +++
> target/ppc/gdbstub.c | 35 +++++++
> target/ppc/kvm.c | 195 +++++++++++++++++++++++++++++++++++++--
> 4 files changed, 252 insertions(+), 7 deletions(-)
>
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index e3e82327b7..37119cd0b4 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1156,6 +1156,11 @@ struct CPUPPCState {
> uint32_t tm_vscr;
> uint64_t tm_dscr;
> uint64_t tm_tar;
> +
> + /* Used for software single step */
> + target_ulong sstep_msr;
> + target_ulong sstep_srr0;
> + target_ulong sstep_srr1;
Do we need to migrate these?
> };
>
> #define SET_FIT_PERIOD(a_, b_, c_, d_) \
> @@ -1253,6 +1258,7 @@ struct PPCVirtualHypervisorClass {
> OBJECT_GET_CLASS(PPCVirtualHypervisorClass, (obj), \
> TYPE_PPC_VIRTUAL_HYPERVISOR)
>
> +target_ulong ppc_get_trace_int_handler_addr(CPUState *cs);
> void ppc_cpu_do_interrupt(CPUState *cpu);
> bool ppc_cpu_exec_interrupt(CPUState *cpu, int int_req);
> void ppc_cpu_dump_state(CPUState *cpu, FILE *f, int flags);
> @@ -1266,6 +1272,12 @@ int ppc_cpu_gdb_write_register_apple(CPUState *cpu, uint8_t *buf, int reg);
> void ppc_gdb_gen_spr_xml(PowerPCCPU *cpu);
> const char *ppc_gdb_get_dynamic_xml(CPUState *cs, const char *xml_name);
> #endif
> +uint32_t ppc_gdb_read_insn(CPUState *cs, target_ulong addr);
AFAICT this is only used within the KVM code, so why does it need to
be public?
> +uint32_t ppc_gdb_get_op(uint32_t insn);
> +uint32_t ppc_gdb_get_xop(uint32_t insn);
> +uint32_t ppc_gdb_get_spr(uint32_t insn);
> +uint32_t ppc_gdb_get_rt(uint32_t insn);
> +
> int ppc64_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
> int cpuid, void *opaque);
> int ppc32_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
> @@ -2217,6 +2229,10 @@ enum {
> PPC2_ISA300)
> };
>
> +#define XOP_RFID 18
> +#define XOP_MFMSR 83
> +#define XOP_MTSPR 467
> +
> /*****************************************************************************/
> /*
> * Memory access type :
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 50b004d00d..8ce395004a 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -112,6 +112,8 @@ static uint64_t ppc_excp_vector_offset(CPUState *cs, int ail)
> uint64_t offset = 0;
>
> switch (ail) {
> + case AIL_NONE:
> + break;
How did we get away with missing this case before? I think this might
be clearer as a preliminary fixup patch.
> case AIL_0001_8000:
> offset = 0x18000;
> break;
> @@ -782,6 +784,17 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
> check_tlb_flush(env, false);
> }
>
> +target_ulong ppc_get_trace_int_handler_addr(CPUState *cs)
> +{
> + PowerPCCPU *cpu = POWERPC_CPU(cs);
> + CPUPPCState *env = &cpu->env;
> + int ail;
> +
> + ail = (env->spr[SPR_LPCR] & LPCR_AIL) >> LPCR_AIL_SHIFT;
> + return env->excp_vectors[POWERPC_EXCP_TRACE] |
> + ppc_excp_vector_offset(cs, ail);
> +}
> +
> void ppc_cpu_do_interrupt(CPUState *cs)
> {
> PowerPCCPU *cpu = POWERPC_CPU(cs);
> diff --git a/target/ppc/gdbstub.c b/target/ppc/gdbstub.c
> index 823759c92e..540b767445 100644
> --- a/target/ppc/gdbstub.c
> +++ b/target/ppc/gdbstub.c
> @@ -383,3 +383,38 @@ const char *ppc_gdb_get_dynamic_xml(CPUState *cs, const char *xml_name)
> return NULL;
> }
> #endif
> +
> +uint32_t ppc_gdb_read_insn(CPUState *cs, target_ulong addr)
> +{
> + PowerPCCPU *cpu = POWERPC_CPU(cs);
> + CPUPPCState *env = &cpu->env;
> + uint32_t insn;
> +
> + cpu_memory_rw_debug(cs, addr, (uint8_t *)&insn, sizeof(insn), 0);
> +
> + if (msr_le) {
> + return ldl_le_p(&insn);
> + } else {
> + return ldl_be_p(&insn);
> + }
I feel like there must be existing places that need to read
instructions, so I'm wondering why we need a new helper.
> +}
> +
> +uint32_t ppc_gdb_get_op(uint32_t insn)
> +{
> + return extract32(insn, 26, 6);
> +}
> +
> +uint32_t ppc_gdb_get_xop(uint32_t insn)
> +{
> + return extract32(insn, 1, 10);
> +}
> +
> +uint32_t ppc_gdb_get_spr(uint32_t insn)
> +{
> + return extract32(insn, 11, 5) << 5 | extract32(insn, 16, 5);
> +}
> +
> +uint32_t ppc_gdb_get_rt(uint32_t insn)
> +{
> + return extract32(insn, 21, 5);
> +}
These are all used in just the KVM code rather than the gdbstub code
directly, so this seems an odd place to put them.
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 3a2cfe883c..fedb8e787d 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -1542,6 +1542,86 @@ void kvm_arch_remove_all_hw_breakpoints(void)
> nb_hw_breakpoint = nb_hw_watchpoint = 0;
> }
>
> +void kvm_arch_set_singlestep(CPUState *cs, int enabled)
> +{
> + PowerPCCPU *cpu = POWERPC_CPU(cs);
> + CPUPPCState *env = &cpu->env;
> + target_ulong trace_handler_addr;
> + uint32_t insn;
> + bool rfid;
> +
> + if (!enabled) {
> + return;
> + }
> +
> + cpu_synchronize_state(cs);
> + insn = ppc_gdb_read_insn(cs, env->nip);
> +
> + /*
> + * rfid needs special handling because it:
> + * - overwrites NIP with SRR0;
> + * - overwrites MSR with SRR1;
> + * - cannot be single stepped.
> + */
> + rfid = ppc_gdb_get_op(insn) == 19 && ppc_gdb_get_xop(insn) ==
> XOP_RFID;
A symbolic constant rather than '19' would be nice.
> +
> + if (rfid && kvm_find_sw_breakpoint(cs, env->spr[SPR_SRR0])) {
> + /*
> + * There is a breakpoint at the next instruction address. It
> + * will already cause the vm exit we need for the single step,
> + * so there's nothing to be done.
> + */
> + return;
> + }
> +
> + /*
> + * Save the registers that will be affected by the single step
> + * mechanism. These will be restored after the step at
> + * kvm_handle_singlestep.
> + */
> + env->sstep_msr = env->msr;
> + env->sstep_srr0 = env->spr[SPR_SRR0];
> + env->sstep_srr1 = env->spr[SPR_SRR1];
Do we really need to save both MSR and SRR1? AFAICT we check for one
bit in sstep_msr, but we never restore it or otherwise look at it.
> +
> + /*
> + * MSR_SE = 1 will cause a Trace Interrupt in the guest after the
> + * next instruction executes. If this is a rfid, use SRR1 instead
> + * of MSR.
> + */
> + if (rfid) {
> + if ((env->spr[SPR_SRR1] >> MSR_SE) & 1) {
> + /*
> + * The guest is doing a single step itself. Make sure we
> + * restore it later.
> + */
> + env->sstep_msr |= (1ULL << MSR_SE);
> + }
> +
> + env->spr[SPR_SRR1] |= (1ULL << MSR_SE);
> + } else {
> + env->msr |= (1ULL << MSR_SE);
> + }
> +
> + /*
> + * We set a breakpoint at the interrupt handler address so
> + * that the singlestep will be seen by KVM (this is treated by
> + * KVM like an ordinary breakpoint) and control is returned to
> + * QEMU.
> + */
> + trace_handler_addr = ppc_get_trace_int_handler_addr(cs);
What happens if the instruction we're setting changes the AIL value?
> + if (env->nip == trace_handler_addr) {
> + /*
> + * We are trying to step over the interrupt handler
> + * address itself; move the breakpoint to the next
> + * instruction.
> + */
> + trace_handler_addr += 4;
> + }
> +
> + kvm_insert_breakpoint(cs, trace_handler_addr, 4, GDB_BREAKPOINT_SW);
> +}
> +
> void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
> {
> int n;
> @@ -1581,6 +1661,91 @@ void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
> }
> }
>
> +/* Revert any side-effects caused during single step */
> +static void restore_singlestep_env(CPUState *cs)
> +{
> + PowerPCCPU *cpu = POWERPC_CPU(cs);
> + CPUPPCState *env = &cpu->env;
> + uint32_t insn;
> + int reg;
> + int spr;
> +
> + insn = ppc_gdb_read_insn(cs, env->spr[SPR_SRR0] - 4);
Is the -4 always correct, even for branching instructions?
> +
> + env->spr[SPR_SRR0] = env->sstep_srr0;
> + env->spr[SPR_SRR1] = env->sstep_srr1;
> +
> + if (ppc_gdb_get_op(insn) != 31) {
> + return;
> + }
> +
> + reg = ppc_gdb_get_rt(insn);
> +
> + switch (ppc_gdb_get_xop(insn)) {
> + case XOP_MTSPR:
> + /*
> + * mtspr: the guest altered the SRR, so do not use the
> + * pre-step value.
> + */
> + spr = ppc_gdb_get_spr(insn);
> + if (spr == SPR_SRR0 || spr == SPR_SRR1) {
> + env->spr[spr] = env->gpr[reg];
> + }
I don't really understand why we need this.
> + break;
> + case XOP_MFMSR:
> + /*
> + * mfmsr: clear MSR_SE bit to avoid the guest knowing
> + * that it is being single-stepped.
> + */
> + env->gpr[reg] &= ~(1ULL << MSR_SE);
Shouldn't we be checking if the guest *also* set single stepping for
itself, and reporting it in that case?
> + break;
> + }
> +}
> +
> +static int kvm_handle_singlestep(CPUState *cs, target_ulong address)
> +{
> + PowerPCCPU *cpu = POWERPC_CPU(cs);
> + CPUPPCState *env = &cpu->env;
> + target_ulong trace_handler_addr;
> +
> + if (cap_ppc_singlestep) {
> + return 1;
> + }
> +
> + cpu_synchronize_state(cs);
> + trace_handler_addr = ppc_get_trace_int_handler_addr(cs);
Again, what happens if the stepped instruction changes AIL?
> + if (address == trace_handler_addr) {
> + kvm_remove_breakpoint(cs, trace_handler_addr, 4, GDB_BREAKPOINT_SW);
> +
> + if (env->sstep_msr & (1ULL << MSR_SE)) {
> + /*
> + * The guest expects the last instruction to have caused a
> + * single step, go back into the interrupt handler.
> + */
> + return 1;
> + }
> +
> + env->nip = env->spr[SPR_SRR0];
> + /* Bits 33-36, 43-47 are set by the interrupt */
> + env->msr = env->spr[SPR_SRR1] & ~(1ULL << MSR_SE |
> + PPC_BITMASK(33, 36) |
> + PPC_BITMASK(43, 47));
> + restore_singlestep_env(cs);
> +
> + } else if (address == trace_handler_addr + 4) {
> + /*
> + * A step at trace_handler_addr would interfere with the
> + * singlestep mechanism itself, so we have previously
> + * displaced the breakpoint to the next instruction.
> + */
> + kvm_remove_breakpoint(cs, trace_handler_addr + 4, 4, GDB_BREAKPOINT_SW);
> + restore_singlestep_env(cs);
> + }
And if the address is neither of those things..? Is that an error, or
is that a no-op?
> +
> + return 1;
> +}
> +
> static int kvm_handle_hw_breakpoint(CPUState *cs,
> struct kvm_debug_exit_arch *arch_info)
> {
> @@ -1608,13 +1773,29 @@ static int kvm_handle_hw_breakpoint(CPUState *cs,
> return handle;
> }
>
> -static int kvm_handle_singlestep(void)
> +static int kvm_handle_sw_breakpoint(CPUState *cs, target_ulong address)
> {
> - return 1;
> -}
> + target_ulong trace_handler_addr;
>
> -static int kvm_handle_sw_breakpoint(void)
> -{
> + if (cap_ppc_singlestep) {
> + return 1;
> + }
> +
> + cpu_synchronize_state(cs);
> + trace_handler_addr = ppc_get_trace_int_handler_addr(cs);
> +
> + if (address == trace_handler_addr) {
> + CPU_FOREACH(cs) {
> + if (cs->singlestep_enabled) {
> + /*
> + * We hit this breakpoint while another cpu is doing a
> + * software single step. Go back into the guest to
> + * give chance for the single step to finish.
> + */
> + return 0;
> + }
> + }
> + }
> return 1;
> }
>
> @@ -1625,7 +1806,7 @@ static int kvm_handle_debug(PowerPCCPU *cpu, struct kvm_run *run)
> struct kvm_debug_exit_arch *arch_info = &run->debug.arch;
>
> if (cs->singlestep_enabled) {
> - return kvm_handle_singlestep();
> + return kvm_handle_singlestep(cs, arch_info->address);
> }
>
> if (arch_info->status) {
> @@ -1633,7 +1814,7 @@ static int kvm_handle_debug(PowerPCCPU *cpu, struct kvm_run *run)
> }
>
> if (kvm_find_sw_breakpoint(cs, arch_info->address)) {
> - return kvm_handle_sw_breakpoint();
> + return kvm_handle_sw_breakpoint(cs, arch_info->address);
> }
>
> /*
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 2/3] kvm-all: Introduce kvm_set_singlestep
2019-12-12 4:50 ` David Gibson
@ 2019-12-13 21:03 ` Fabiano Rosas
0 siblings, 0 replies; 10+ messages in thread
From: Fabiano Rosas @ 2019-12-13 21:03 UTC (permalink / raw)
To: David Gibson; +Cc: Paolo Bonzini, qemu-ppc, qemu-devel
David Gibson <david@gibson.dropbear.id.au> writes:
> On Wed, Dec 11, 2019 at 04:10:12PM -0300, Fabiano Rosas wrote:
>> For single stepping (via KVM) of a guest vcpu to work, KVM needs not
>> only to support the SET_GUEST_DEBUG ioctl but to also recognize the
>> KVM_GUESTDBG_SINGLESTEP bit in the control field of the
>> kvm_guest_debug struct.
>>
>> This patch adds support for querying the single step capability so
>> that QEMU can decide what to do for the platforms that do not have
>> such support.
>>
>> This will allow architecture-specific implementations of a fallback
>> mechanism for single stepping in cases where KVM does not support it.
>>
>> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
>
> I think this is fine, but it took me a while to figure out what was
> going on. I wonder if better comments and/or function names might
> help.
>
Thanks for the feedback, I will review the patch with this in mind and
add more information.
>> ---
>> accel/kvm/kvm-all.c | 14 ++++++++++++++
>> accel/stubs/kvm-stub.c | 4 ++++
>> exec.c | 2 +-
>> include/sysemu/kvm.h | 4 ++++
>> stubs/Makefile.objs | 1 +
>> stubs/kvm-arch-singlestep.c | 14 ++++++++++++++
>> target/ppc/kvm.c | 14 ++++++++++++++
>> 7 files changed, 52 insertions(+), 1 deletion(-)
>> create mode 100644 stubs/kvm-arch-singlestep.c
>>
>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>> index ca00daa2f5..a61beb0b53 100644
>> --- a/accel/kvm/kvm-all.c
>> +++ b/accel/kvm/kvm-all.c
>> @@ -2599,6 +2599,11 @@ bool kvm_arm_supports_user_irq(void)
>> }
>>
>> #ifdef KVM_CAP_SET_GUEST_DEBUG
>> +bool kvm_has_guest_debug_singlestep(CPUState *cs)
>
> AIUI, this function is saying if setting the singlestep flag in the
> KVM debug regs is sufficient to single step the guest under host
> control. The name doesn't make that terribly obvious to me, though a
> better one isn't really obvious to me.
>
Right, I'll think of something more obvious.
>> +{
>> + return kvm_arch_has_guest_debug_singlestep(cs);
>
> I also don't see a lot of point to this wrapper, rather than just
> calling the arch version directly.
>
Ok. I'll remove the wrapper.
>> +}
>> +
>> struct kvm_sw_breakpoint *kvm_find_sw_breakpoint(CPUState *cpu,
>> target_ulong pc)
>> {
>> @@ -2647,6 +2652,15 @@ int kvm_update_guest_debug(CPUState *cpu, unsigned long reinject_trap)
>> return data.err;
>> }
>>
>> +void kvm_set_singlestep(CPUState *cs, int enabled)
>> +{
>> + if (kvm_has_guest_debug_singlestep(cs)) {
>> + kvm_update_guest_debug(cs, 0);
>> + } else {
>> + kvm_arch_set_singlestep(cs, enabled);
>> + }
>> +}
>> +
>> int kvm_insert_breakpoint(CPUState *cpu, target_ulong addr,
>> target_ulong len, int type)
>> {
>> diff --git a/accel/stubs/kvm-stub.c b/accel/stubs/kvm-stub.c
>> index 82f118d2df..b4df48b6f1 100644
>> --- a/accel/stubs/kvm-stub.c
>> +++ b/accel/stubs/kvm-stub.c
>> @@ -78,6 +78,10 @@ int kvm_update_guest_debug(CPUState *cpu, unsigned long reinject_trap)
>> return -ENOSYS;
>> }
>>
>> +void kvm_set_singlestep(CPUState *cs, int enabled)
>> +{
>
> AFAICT this should only be called with KVM enabled, so this should
> maybe be a g_assert_not_reached() rather than a no-op.
>
Ok.
>> +}
>> +
>> int kvm_insert_breakpoint(CPUState *cpu, target_ulong addr,
>> target_ulong len, int type)
>> {
>> diff --git a/exec.c b/exec.c
>> index ffdb518535..ff46ea1846 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -1202,7 +1202,7 @@ void cpu_single_step(CPUState *cpu, int enabled)
>> if (cpu->singlestep_enabled != enabled) {
>> cpu->singlestep_enabled = enabled;
>> if (kvm_enabled()) {
>> - kvm_update_guest_debug(cpu, 0);
>> + kvm_set_singlestep(cpu, enabled);
>> } else {
>> /* must flush all the translated code to avoid inconsistencies */
>> /* XXX: only flush what is necessary */
>> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
>> index 9fe233b9bf..7a42978b11 100644
>> --- a/include/sysemu/kvm.h
>> +++ b/include/sysemu/kvm.h
>> @@ -215,6 +215,7 @@ int kvm_has_pit_state2(void);
>> int kvm_has_many_ioeventfds(void);
>> int kvm_has_gsi_routing(void);
>> int kvm_has_intx_set_mask(void);
>> +bool kvm_has_guest_debug_singlestep(CPUState *cs);
>>
>> int kvm_init_vcpu(CPUState *cpu);
>> int kvm_cpu_exec(CPUState *cpu);
>> @@ -247,6 +248,8 @@ bool kvm_memcrypt_enabled(void);
>> */
>> int kvm_memcrypt_encrypt_data(uint8_t *ptr, uint64_t len);
>>
>> +bool kvm_arch_has_guest_debug_singlestep(CPUState *cs);
>> +void kvm_arch_set_singlestep(CPUState *cpu, int enabled);
>>
>> #ifdef NEED_CPU_H
>> #include "cpu.h"
>> @@ -259,6 +262,7 @@ int kvm_remove_breakpoint(CPUState *cpu, target_ulong addr,
>> target_ulong len, int type);
>> void kvm_remove_all_breakpoints(CPUState *cpu);
>> int kvm_update_guest_debug(CPUState *cpu, unsigned long reinject_trap);
>> +void kvm_set_singlestep(CPUState *cs, int enabled);
>>
>> int kvm_on_sigbus_vcpu(CPUState *cpu, int code, void *addr);
>> int kvm_on_sigbus(int code, void *addr);
>> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
>> index 4a50e95ec3..361f4088fa 100644
>> --- a/stubs/Makefile.objs
>> +++ b/stubs/Makefile.objs
>> @@ -12,6 +12,7 @@ stub-obj-y += get-vm-name.o
>> stub-obj-y += iothread.o
>> stub-obj-y += iothread-lock.o
>> stub-obj-y += is-daemonized.o
>> +stub-obj-y += kvm-arch-singlestep.o
>> stub-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
>> stub-obj-y += machine-init-done.o
>> stub-obj-y += migr-blocker.o
>> diff --git a/stubs/kvm-arch-singlestep.c b/stubs/kvm-arch-singlestep.c
>> new file mode 100644
>> index 0000000000..18bfba61f6
>> --- /dev/null
>> +++ b/stubs/kvm-arch-singlestep.c
>> @@ -0,0 +1,14 @@
>> +#include "qemu/osdep.h"
>> +#include "qemu/error-report.h"
>> +#include "sysemu/kvm.h"
>> +
>> +bool kvm_arch_has_guest_debug_singlestep(CPUState *cs)
>> +{
>> + /* for backwards compatibility assume the feature is present */
>> + return true;
>> +}
>> +
>> +void kvm_arch_set_singlestep(CPUState *cpu, int enabled)
>> +{
>> + warn_report("KVM does not support single stepping");
>> +}
>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>> index c77f9848ec..3a2cfe883c 100644
>> --- a/target/ppc/kvm.c
>> +++ b/target/ppc/kvm.c
>> @@ -85,6 +85,7 @@ static int cap_ppc_safe_indirect_branch;
>> static int cap_ppc_count_cache_flush_assist;
>> static int cap_ppc_nested_kvm_hv;
>> static int cap_large_decr;
>> +static int cap_ppc_singlestep;
>>
>> static uint32_t debug_inst_opcode;
>>
>> @@ -133,6 +134,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>> kvmppc_get_cpu_characteristics(s);
>> cap_ppc_nested_kvm_hv = kvm_vm_check_extension(s, KVM_CAP_PPC_NESTED_HV);
>> cap_large_decr = kvmppc_get_dec_bits();
>> + cap_ppc_singlestep = kvm_vm_check_extension(s, KVM_CAP_PPC_GUEST_DEBUG_SSTEP);
>> /*
>> * Note: setting it to false because there is not such capability
>> * in KVM at this moment.
>> @@ -1380,6 +1382,18 @@ static int kvmppc_handle_dcr_write(CPUPPCState *env,
>> return 0;
>> }
>>
>> +bool kvm_arch_has_guest_debug_singlestep(CPUState *cs)
>> +{
>> + PowerPCCPU *cpu = POWERPC_CPU(cs);
>> + CPUPPCState *env = &cpu->env;
>> +
>> + if (cap_ppc_singlestep) {
>> + return true;
>> + }
>> +
>
> Perhaps note explicitly that this is a fallback guess when the cap is
> not available to give us a good answer.
>
Ok.
>> + return env->excp_model == POWERPC_EXCP_BOOKE || kvmppc_is_pr(kvm_state);
>> +}
>> +
>> int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
>> {
>> /* Mixed endian case is not handled */
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 1/3] linux-headers: Update kvm.h for ppc single step capability
2019-12-12 3:44 ` David Gibson
@ 2019-12-13 21:03 ` Fabiano Rosas
0 siblings, 0 replies; 10+ messages in thread
From: Fabiano Rosas @ 2019-12-13 21:03 UTC (permalink / raw)
To: David Gibson; +Cc: Paolo Bonzini, qemu-ppc, qemu-devel
David Gibson <david@gibson.dropbear.id.au> writes:
> On Wed, Dec 11, 2019 at 04:10:11PM -0300, Fabiano Rosas wrote:
>> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
>
> Generally, imported linux header updates are done as a block, pulling
> qemu up to a specified kernel commit id, rather than just grabbing
> pieces for a particular feature.
>
I know, I thought it would make the review easier. =)
> There's actually already a header update to be2eca94d144 in my
> ppc-for-5.0 tree. Is that recent enough for what you need here?
>
It is. I need 1a9167a214f5.
>> ---
>> linux-headers/linux/kvm.h | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
>> index 3d9b18f7f8..488f3baf01 100644
>> --- a/linux-headers/linux/kvm.h
>> +++ b/linux-headers/linux/kvm.h
>> @@ -1000,6 +1000,7 @@ struct kvm_ppc_resize_hpt {
>> #define KVM_CAP_PMU_EVENT_FILTER 173
>> #define KVM_CAP_ARM_IRQ_LINE_LAYOUT_2 174
>> #define KVM_CAP_HYPERV_DIRECT_TLBFLUSH 175
>> +#define KVM_CAP_PPC_GUEST_DEBUG_SSTEP 176
>>
>> #ifdef KVM_CAP_IRQ_ROUTING
>>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 3/3] target/ppc: support single stepping with KVM HV
2019-12-12 5:08 ` David Gibson
@ 2019-12-13 21:06 ` Fabiano Rosas
0 siblings, 0 replies; 10+ messages in thread
From: Fabiano Rosas @ 2019-12-13 21:06 UTC (permalink / raw)
To: David Gibson; +Cc: Paolo Bonzini, qemu-ppc, qemu-devel
David Gibson <david@gibson.dropbear.id.au> writes:
Hi, thanks for the review! My comments below:
> On Wed, Dec 11, 2019 at 04:10:13PM -0300, Fabiano Rosas wrote:
>> The hardware singlestep mechanism in POWER works via a Trace Interrupt
>> (0xd00) that happens after any instruction executes, whenever MSR_SE =
>> 1 (PowerISA Section 6.5.15 - Trace Interrupt).
>>
>> However, with kvm_hv, the Trace Interrupt happens inside the guest and
>> KVM has no visibility of it. Therefore, when the gdbstub uses the
>> KVM_SET_GUEST_DEBUG ioctl to enable singlestep, KVM simply ignores it.
>>
>> This patch takes advantage of the Trace Interrupt to perform the step
>> inside the guest, but uses a breakpoint at the Trace Interrupt handler
>> to return control to KVM. The exit is treated by KVM as a regular
>> breakpoint and it returns to the host (and QEMU eventually).
>>
>> Before signalling GDB, QEMU sets the Next Instruction Pointer to the
>> instruction following the one being stepped and restores the MSR,
>> SRR0, SRR1 values from before the step, effectively skipping the
>> interrupt handler execution and hiding the trace interrupt breakpoint
>> from GDB.
>>
>> This approach works with both of GDB's 'scheduler-locking' options
>> (off, step).
>>
>> Note:
>>
>> - kvm_arch_set_singlestep happens after GDB asks for a single step,
>> while the vcpus are stopped.
>>
>> - kvm_handle_singlestep executes after the step, during the handling
>> of the Emulation Assist Interrupt (breakpoint).
>>
>> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
>> ---
>> target/ppc/cpu.h | 16 ++++
>> target/ppc/excp_helper.c | 13 +++
>> target/ppc/gdbstub.c | 35 +++++++
>> target/ppc/kvm.c | 195 +++++++++++++++++++++++++++++++++++++--
>> 4 files changed, 252 insertions(+), 7 deletions(-)
>>
>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
>> index e3e82327b7..37119cd0b4 100644
>> --- a/target/ppc/cpu.h
>> +++ b/target/ppc/cpu.h
>> @@ -1156,6 +1156,11 @@ struct CPUPPCState {
>> uint32_t tm_vscr;
>> uint64_t tm_dscr;
>> uint64_t tm_tar;
>> +
>> + /* Used for software single step */
>> + target_ulong sstep_msr;
>> + target_ulong sstep_srr0;
>> + target_ulong sstep_srr1;
>
> Do we need to migrate these?
>
Hm. I don't know. I think it depends on what happens in the general case
if we try to migrate, say, right after a breakpoint hits. I'll have to
investigate this.
>> };
>>
>> #define SET_FIT_PERIOD(a_, b_, c_, d_) \
>> @@ -1253,6 +1258,7 @@ struct PPCVirtualHypervisorClass {
>> OBJECT_GET_CLASS(PPCVirtualHypervisorClass, (obj), \
>> TYPE_PPC_VIRTUAL_HYPERVISOR)
>>
>> +target_ulong ppc_get_trace_int_handler_addr(CPUState *cs);
>> void ppc_cpu_do_interrupt(CPUState *cpu);
>> bool ppc_cpu_exec_interrupt(CPUState *cpu, int int_req);
>> void ppc_cpu_dump_state(CPUState *cpu, FILE *f, int flags);
>> @@ -1266,6 +1272,12 @@ int ppc_cpu_gdb_write_register_apple(CPUState *cpu, uint8_t *buf, int reg);
>> void ppc_gdb_gen_spr_xml(PowerPCCPU *cpu);
>> const char *ppc_gdb_get_dynamic_xml(CPUState *cs, const char *xml_name);
>> #endif
>> +uint32_t ppc_gdb_read_insn(CPUState *cs, target_ulong addr);
>
> AFAICT this is only used within the KVM code, so why does it need to
> be public?
>
I thought the implementations would be better suited alongside other
gdb-related code. But I don't mind moving it.
>> +uint32_t ppc_gdb_get_op(uint32_t insn);
>> +uint32_t ppc_gdb_get_xop(uint32_t insn);
>> +uint32_t ppc_gdb_get_spr(uint32_t insn);
>> +uint32_t ppc_gdb_get_rt(uint32_t insn);
>> +
>> int ppc64_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
>> int cpuid, void *opaque);
>> int ppc32_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
>> @@ -2217,6 +2229,10 @@ enum {
>> PPC2_ISA300)
>> };
>>
>> +#define XOP_RFID 18
>> +#define XOP_MFMSR 83
>> +#define XOP_MTSPR 467
>> +
>> /*****************************************************************************/
>> /*
>> * Memory access type :
>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
>> index 50b004d00d..8ce395004a 100644
>> --- a/target/ppc/excp_helper.c
>> +++ b/target/ppc/excp_helper.c
>> @@ -112,6 +112,8 @@ static uint64_t ppc_excp_vector_offset(CPUState *cs, int ail)
>> uint64_t offset = 0;
>>
>> switch (ail) {
>> + case AIL_NONE:
>> + break;
>
> How did we get away with missing this case before? I think this might
> be clearer as a preliminary fixup patch.
>
There was a single caller which did:
if (ail) {
...
vector |= ppc_excp_vector_offset(cs, ail);
}
I'll move this to a separate patch.
>> case AIL_0001_8000:
>> offset = 0x18000;
>> break;
>> @@ -782,6 +784,17 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>> check_tlb_flush(env, false);
>> }
>>
>> +target_ulong ppc_get_trace_int_handler_addr(CPUState *cs)
>> +{
>> + PowerPCCPU *cpu = POWERPC_CPU(cs);
>> + CPUPPCState *env = &cpu->env;
>> + int ail;
>> +
>> + ail = (env->spr[SPR_LPCR] & LPCR_AIL) >> LPCR_AIL_SHIFT;
>> + return env->excp_vectors[POWERPC_EXCP_TRACE] |
>> + ppc_excp_vector_offset(cs, ail);
>> +}
>> +
>> void ppc_cpu_do_interrupt(CPUState *cs)
>> {
>> PowerPCCPU *cpu = POWERPC_CPU(cs);
>> diff --git a/target/ppc/gdbstub.c b/target/ppc/gdbstub.c
>> index 823759c92e..540b767445 100644
>> --- a/target/ppc/gdbstub.c
>> +++ b/target/ppc/gdbstub.c
>> @@ -383,3 +383,38 @@ const char *ppc_gdb_get_dynamic_xml(CPUState *cs, const char *xml_name)
>> return NULL;
>> }
>> #endif
>> +
>> +uint32_t ppc_gdb_read_insn(CPUState *cs, target_ulong addr)
>> +{
>> + PowerPCCPU *cpu = POWERPC_CPU(cs);
>> + CPUPPCState *env = &cpu->env;
>> + uint32_t insn;
>> +
>> + cpu_memory_rw_debug(cs, addr, (uint8_t *)&insn, sizeof(insn), 0);
>> +
>> + if (msr_le) {
>> + return ldl_le_p(&insn);
>> + } else {
>> + return ldl_be_p(&insn);
>> + }
>
> I feel like there must be existing places that need to read
> instructions, so I'm wondering why we need a new helper.
>
I could not find any instance, specially not one that takes the current
MSR_LE into consideration.
>> +}
>> +
>> +uint32_t ppc_gdb_get_op(uint32_t insn)
>> +{
>> + return extract32(insn, 26, 6);
>> +}
>> +
>> +uint32_t ppc_gdb_get_xop(uint32_t insn)
>> +{
>> + return extract32(insn, 1, 10);
>> +}
>> +
>> +uint32_t ppc_gdb_get_spr(uint32_t insn)
>> +{
>> + return extract32(insn, 11, 5) << 5 | extract32(insn, 16, 5);
>> +}
>> +
>> +uint32_t ppc_gdb_get_rt(uint32_t insn)
>> +{
>> + return extract32(insn, 21, 5);
>> +}
>
> These are all used in just the KVM code rather than the gdbstub code
> directly, so this seems an odd place to put them.
>
Well, they are instruction decoding helpers so it seemed closer in
concept to what we do around gdbstub. As I said before, I don't mind
moving it elsewhere.
>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>> index 3a2cfe883c..fedb8e787d 100644
>> --- a/target/ppc/kvm.c
>> +++ b/target/ppc/kvm.c
>> @@ -1542,6 +1542,86 @@ void kvm_arch_remove_all_hw_breakpoints(void)
>> nb_hw_breakpoint = nb_hw_watchpoint = 0;
>> }
>>
>> +void kvm_arch_set_singlestep(CPUState *cs, int enabled)
>> +{
>> + PowerPCCPU *cpu = POWERPC_CPU(cs);
>> + CPUPPCState *env = &cpu->env;
>> + target_ulong trace_handler_addr;
>> + uint32_t insn;
>> + bool rfid;
>> +
>> + if (!enabled) {
>> + return;
>> + }
>> +
>> + cpu_synchronize_state(cs);
>> + insn = ppc_gdb_read_insn(cs, env->nip);
>> +
>> + /*
>> + * rfid needs special handling because it:
>> + * - overwrites NIP with SRR0;
>> + * - overwrites MSR with SRR1;
>> + * - cannot be single stepped.
>> + */
>> + rfid = ppc_gdb_get_op(insn) == 19 && ppc_gdb_get_xop(insn) ==
>> XOP_RFID;
>
> A symbolic constant rather than '19' would be nice.
>
Ok.
>> +
>> + if (rfid && kvm_find_sw_breakpoint(cs, env->spr[SPR_SRR0])) {
>> + /*
>> + * There is a breakpoint at the next instruction address. It
>> + * will already cause the vm exit we need for the single step,
>> + * so there's nothing to be done.
>> + */
>> + return;
>> + }
>> +
>> + /*
>> + * Save the registers that will be affected by the single step
>> + * mechanism. These will be restored after the step at
>> + * kvm_handle_singlestep.
>> + */
>> + env->sstep_msr = env->msr;
>> + env->sstep_srr0 = env->spr[SPR_SRR0];
>> + env->sstep_srr1 = env->spr[SPR_SRR1];
>
> Do we really need to save both MSR and SRR1? AFAICT we check for one
> bit in sstep_msr, but we never restore it or otherwise look at it.
>
The SRRs need to be saved because the interrupt alters them. We need to
restore them to hide from the guest the fact that we took the 0xd00.
Now for MSR, you are right, I could just use a flag.
>> +
>> + /*
>> + * MSR_SE = 1 will cause a Trace Interrupt in the guest after the
>> + * next instruction executes. If this is a rfid, use SRR1 instead
>> + * of MSR.
>> + */
>> + if (rfid) {
>> + if ((env->spr[SPR_SRR1] >> MSR_SE) & 1) {
>> + /*
>> + * The guest is doing a single step itself. Make sure we
>> + * restore it later.
>> + */
>> + env->sstep_msr |= (1ULL << MSR_SE);
>> + }
>> +
>> + env->spr[SPR_SRR1] |= (1ULL << MSR_SE);
>> + } else {
>> + env->msr |= (1ULL << MSR_SE);
>> + }
>> +
>> + /*
>> + * We set a breakpoint at the interrupt handler address so
>> + * that the singlestep will be seen by KVM (this is treated by
>> + * KVM like an ordinary breakpoint) and control is returned to
>> + * QEMU.
>> + */
>> + trace_handler_addr = ppc_get_trace_int_handler_addr(cs);
>
> What happens if the instruction we're setting changes the AIL value?
>
Huh, so this is why the single step fails and GDB hangs during kernel
early_setup... =)
>> + if (env->nip == trace_handler_addr) {
>> + /*
>> + * We are trying to step over the interrupt handler
>> + * address itself; move the breakpoint to the next
>> + * instruction.
>> + */
>> + trace_handler_addr += 4;
>> + }
>> +
>> + kvm_insert_breakpoint(cs, trace_handler_addr, 4, GDB_BREAKPOINT_SW);
>> +}
>> +
>> void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
>> {
>> int n;
>> @@ -1581,6 +1661,91 @@ void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
>> }
>> }
>>
>> +/* Revert any side-effects caused during single step */
>> +static void restore_singlestep_env(CPUState *cs)
>> +{
>> + PowerPCCPU *cpu = POWERPC_CPU(cs);
>> + CPUPPCState *env = &cpu->env;
>> + uint32_t insn;
>> + int reg;
>> + int spr;
>> +
>> + insn = ppc_gdb_read_insn(cs, env->spr[SPR_SRR0] - 4);
>
> Is the -4 always correct, even for branching instructions?
>
This function is about restoring what we touched with the singlestep
mechanism, so we don't care if the last instruction was a branch. That
is why I can use -4 and a few lines below there's this:
if (ppc_gdb_get_op(insn) != 31) {
return;
}
In other words, a branch instruction would not alter any state that
would conflict with the step.
Now that you asked, I realize I should document that assumption in the
code.
>> +
>> + env->spr[SPR_SRR0] = env->sstep_srr0;
>> + env->spr[SPR_SRR1] = env->sstep_srr1;
>> +
>> + if (ppc_gdb_get_op(insn) != 31) {
>> + return;
>> + }
>> +
>> + reg = ppc_gdb_get_rt(insn);
>> +
>> + switch (ppc_gdb_get_xop(insn)) {
>> + case XOP_MTSPR:
>> + /*
>> + * mtspr: the guest altered the SRR, so do not use the
>> + * pre-step value.
>> + */
>> + spr = ppc_gdb_get_spr(insn);
>> + if (spr == SPR_SRR0 || spr == SPR_SRR1) {
>> + env->spr[spr] = env->gpr[reg];
>> + }
>
> I don't really understand why we need this.
>
The trace interrupt will set SRR0 and SRR1 *after* the instruction
executes. If we step over a mtspr that touches SRR0 or SRR1, their value
after the step will not be what the guest put there, but what the trace
interrupt did.
So this code emulates the mtspr after the step.
>> + break;
>> + case XOP_MFMSR:
>> + /*
>> + * mfmsr: clear MSR_SE bit to avoid the guest knowing
>> + * that it is being single-stepped.
>> + */
>> + env->gpr[reg] &= ~(1ULL << MSR_SE);
>
> Shouldn't we be checking if the guest *also* set single stepping for
> itself, and reporting it in that case?
>
In that case the guest would set SRR1 and do a rfid. So in
kvm_arch_set_singlestep I check whether we are stepping an rfid and copy
the MSR_SE bit over to env->sstep_msr:
if (rfid) {
if ((env->spr[SPR_SRR1] >> MSR_SE) & 1) {
/*
* The guest is doing a single step itself. Make sure we
* restore it later.
*/
env->sstep_msr |= (1ULL << MSR_SE);
}
...
}
After the step, if env->sstep_msr has MSR_SE set, I let the interrupt
handler execute since it is a legitimate interrupt caused by the guest:
kvm_remove_breakpoint(cs, trace_handler_addr, 4, GDB_BREAKPOINT_SW);
if (env->sstep_msr & (1ULL << MSR_SE)) {
/*
* The guest expects the last instruction to have caused a
* single step, go back into the interrupt handler.
*/
return 1; <-- 1 means go back to GDB, 0 means back to the guest
}
At this point QEMU will be single stepping the trace interrupt handler
code (i.e. we're debugging the step that was caused by the guest) and
the code you asked about will not execute. When we return to the
instruction after the one stepped by the guest, the MSR will be whatever
the interrupt handler set it to.
>> + break;
>> + }
>> +}
>> +
>> +static int kvm_handle_singlestep(CPUState *cs, target_ulong address)
>> +{
>> + PowerPCCPU *cpu = POWERPC_CPU(cs);
>> + CPUPPCState *env = &cpu->env;
>> + target_ulong trace_handler_addr;
>> +
>> + if (cap_ppc_singlestep) {
>> + return 1;
>> + }
>> +
>> + cpu_synchronize_state(cs);
>> + trace_handler_addr = ppc_get_trace_int_handler_addr(cs);
>
> Again, what happens if the stepped instruction changes AIL?
>
Should I make the trace_handler_addr persistent across the step so I can
verify if it changed? Or maybe just use 2 (3?) breakpoints? I think
Alexey suggested that once...
>> + if (address == trace_handler_addr) {
>> + kvm_remove_breakpoint(cs, trace_handler_addr, 4, GDB_BREAKPOINT_SW);
>> +
>> + if (env->sstep_msr & (1ULL << MSR_SE)) {
>> + /*
>> + * The guest expects the last instruction to have caused a
>> + * single step, go back into the interrupt handler.
>> + */
>> + return 1;
>> + }
>> +
>> + env->nip = env->spr[SPR_SRR0];
>> + /* Bits 33-36, 43-47 are set by the interrupt */
>> + env->msr = env->spr[SPR_SRR1] & ~(1ULL << MSR_SE |
>> + PPC_BITMASK(33, 36) |
>> + PPC_BITMASK(43, 47));
>> + restore_singlestep_env(cs);
>> +
>> + } else if (address == trace_handler_addr + 4) {
>> + /*
>> + * A step at trace_handler_addr would interfere with the
>> + * singlestep mechanism itself, so we have previously
>> + * displaced the breakpoint to the next instruction.
>> + */
>> + kvm_remove_breakpoint(cs, trace_handler_addr + 4, 4, GDB_BREAKPOINT_SW);
>> + restore_singlestep_env(cs);
>> + }
>
> And if the address is neither of those things..? Is that an error, or
> is that a no-op?
>
This is a regular breakpoint that happened while single stepping (the
'return 1' means "return to gdb").
With GDB's scheduler-locking=off, threads other than the one being
debugged are allowed to execute when we run the program. If the vcpu
that is stepping gets preempted, the singlestep_enabled flag in QEMU
will still be set (it gets cleared only after the kvm_handle* code) so
if that vcpu hits a breakpoint we handle it through here. Since it is
not a "single-step breakpoint" we just ignore it and let GDB treat it
like a regular breakpoint (which it is).
>> +
>> + return 1;
>> +}
>> +
>> static int kvm_handle_hw_breakpoint(CPUState *cs,
>> struct kvm_debug_exit_arch *arch_info)
>> {
>> @@ -1608,13 +1773,29 @@ static int kvm_handle_hw_breakpoint(CPUState *cs,
>> return handle;
>> }
>>
>> -static int kvm_handle_singlestep(void)
>> +static int kvm_handle_sw_breakpoint(CPUState *cs, target_ulong address)
>> {
>> - return 1;
>> -}
>> + target_ulong trace_handler_addr;
>>
>> -static int kvm_handle_sw_breakpoint(void)
>> -{
>> + if (cap_ppc_singlestep) {
>> + return 1;
>> + }
>> +
>> + cpu_synchronize_state(cs);
>> + trace_handler_addr = ppc_get_trace_int_handler_addr(cs);
>> +
>> + if (address == trace_handler_addr) {
>> + CPU_FOREACH(cs) {
>> + if (cs->singlestep_enabled) {
>> + /*
>> + * We hit this breakpoint while another cpu is doing a
>> + * software single step. Go back into the guest to
>> + * give chance for the single step to finish.
>> + */
>> + return 0;
>> + }
>> + }
>> + }
>> return 1;
>> }
>>
>> @@ -1625,7 +1806,7 @@ static int kvm_handle_debug(PowerPCCPU *cpu, struct kvm_run *run)
>> struct kvm_debug_exit_arch *arch_info = &run->debug.arch;
>>
>> if (cs->singlestep_enabled) {
>> - return kvm_handle_singlestep();
>> + return kvm_handle_singlestep(cs, arch_info->address);
>> }
>>
>> if (arch_info->status) {
>> @@ -1633,7 +1814,7 @@ static int kvm_handle_debug(PowerPCCPU *cpu, struct kvm_run *run)
>> }
>>
>> if (kvm_find_sw_breakpoint(cs, arch_info->address)) {
>> - return kvm_handle_sw_breakpoint();
>> + return kvm_handle_sw_breakpoint(cs, arch_info->address);
>> }
>>
>> /*
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-12-13 21:27 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-11 19:10 [PATCH v5 0/3] target/ppc: single step for KVM HV Fabiano Rosas
2019-12-11 19:10 ` [PATCH v5 1/3] linux-headers: Update kvm.h for ppc single step capability Fabiano Rosas
2019-12-12 3:44 ` David Gibson
2019-12-13 21:03 ` Fabiano Rosas
2019-12-11 19:10 ` [PATCH v5 2/3] kvm-all: Introduce kvm_set_singlestep Fabiano Rosas
2019-12-12 4:50 ` David Gibson
2019-12-13 21:03 ` Fabiano Rosas
2019-12-11 19:10 ` [PATCH v5 3/3] target/ppc: support single stepping with KVM HV Fabiano Rosas
2019-12-12 5:08 ` David Gibson
2019-12-13 21:06 ` Fabiano Rosas
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).