[Qemu-devel] [RFC PATCH v4 0/5] target/ppc: single step for KVM HV

[Qemu-devel] [RFC PATCH v4 0/5] target/ppc: single step for KVM HV

[Fabiano Rosas]

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

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/msg03738.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/2018-11/msg04269.html

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,rt,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.gnu.org/archive/html/qemu-devel/2019-01/msg04627.html


Fabiano Rosas (5):
  target/ppc: Move exception vector offset computation into a function
  kvm-all: Introduce kvm_set_singlestep
  target/ppc: Move handling of hardware breakpoints to a separate
    function
  target/ppc: Refactor kvm_handle_debug
  target/ppc: support single stepping with KVM HV

 accel/kvm/kvm-all.c             |  16 ++
 accel/stubs/kvm-stub.c          |   4 +
 exec.c                          |   2 +-
 include/sysemu/kvm.h            |   3 +
 stubs/Makefile.objs             |   1 +
 stubs/kvm-arch-set-singlestep.c |   8 +
 target/ppc/cpu.h                |  16 ++
 target/ppc/excp_helper.c        |  43 +++--
 target/ppc/gdbstub.c            |  35 ++++
 target/ppc/kvm.c                | 312 ++++++++++++++++++++++++++------
 10 files changed, 374 insertions(+), 66 deletions(-)
 create mode 100644 stubs/kvm-arch-set-singlestep.c

--
2.20.1

[Qemu-devel] [RFC PATCH v4 1/5] target/ppc: Move exception vector offset computation into a function

[Fabiano Rosas]

Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 target/ppc/excp_helper.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 39bedbb11d..beafcf1ebd 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -107,6 +107,24 @@ static int powerpc_reset_wakeup(CPUState *cs, CPUPPCState *env, int excp,
     return POWERPC_EXCP_RESET;
 }
 
+static uint64_t ppc_excp_vector_offset(CPUState *cs, int ail)
+{
+    uint64_t offset = 0;
+
+    switch (ail) {
+    case AIL_0001_8000:
+        offset = 0x18000;
+        break;
+    case AIL_C000_0000_0000_4000:
+        offset = 0xc000000000004000ull;
+        break;
+    default:
+        cpu_abort(cs, "Invalid AIL combination %d\n", ail);
+        break;
+    }
+
+    return offset;
+}
 
 /* Note that this function should be greatly optimized
  * when called with a constant excp, from ppc_hw_interrupt
@@ -708,17 +726,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
     /* Handle AIL */
     if (ail) {
         new_msr |= (1 << MSR_IR) | (1 << MSR_DR);
-        switch(ail) {
-        case AIL_0001_8000:
-            vector |= 0x18000;
-            break;
-        case AIL_C000_0000_0000_4000:
-            vector |= 0xc000000000004000ull;
-            break;
-        default:
-            cpu_abort(cs, "Invalid AIL combination %d\n", ail);
-            break;
-        }
+        vector |= ppc_excp_vector_offset(cs, ail);
     }
 
 #if defined(TARGET_PPC64)
-- 
2.20.1

[Qemu-devel] [RFC PATCH v4 2/5] kvm-all: Introduce kvm_set_singlestep

[Fabiano Rosas]

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             | 16 ++++++++++++++++
 accel/stubs/kvm-stub.c          |  4 ++++
 exec.c                          |  2 +-
 include/sysemu/kvm.h            |  3 +++
 stubs/Makefile.objs             |  1 +
 stubs/kvm-arch-set-singlestep.c |  8 ++++++++
 6 files changed, 33 insertions(+), 1 deletion(-)
 create mode 100644 stubs/kvm-arch-set-singlestep.c

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index fd92b6f375..d3ac5a9e5c 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2267,6 +2267,13 @@ bool kvm_arm_supports_user_irq(void)
     return kvm_check_extension(kvm_state, KVM_CAP_ARM_USER_IRQ);
 }
 
+/* Whether the KVM_SET_GUEST_DEBUG ioctl supports single stepping */
+int kvm_has_guestdbg_singlestep(void)
+{
+    /* return kvm_check_extension(kvm_state, KVM_CAP_GUEST_DEBUG_SSTEP); */
+    return 0;
+}
+
 #ifdef KVM_CAP_SET_GUEST_DEBUG
 struct kvm_sw_breakpoint *kvm_find_sw_breakpoint(CPUState *cpu,
                                                  target_ulong pc)
@@ -2316,6 +2323,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_guestdbg_singlestep()) {
+        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 02d5170031..69bd07f50e 100644
--- a/accel/stubs/kvm-stub.c
+++ b/accel/stubs/kvm-stub.c
@@ -79,6 +79,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 518064530b..8817513e26 100644
--- a/exec.c
+++ b/exec.c
@@ -1236,7 +1236,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 a6d1cd190f..e1ef2f5b99 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -214,6 +214,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);
+int kvm_has_guestdbg_singlestep(void);
 
 int kvm_init_vcpu(CPUState *cpu);
 int kvm_cpu_exec(CPUState *cpu);
@@ -246,6 +247,7 @@ bool kvm_memcrypt_enabled(void);
  */
 int kvm_memcrypt_encrypt_data(uint8_t *ptr, uint64_t len);
 
+void kvm_arch_set_singlestep(CPUState *cpu, int enabled);
 
 #ifdef NEED_CPU_H
 #include "cpu.h"
@@ -258,6 +260,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 269dfa5832..884f9b2268 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-set-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-set-singlestep.c b/stubs/kvm-arch-set-singlestep.c
new file mode 100644
index 0000000000..ba6e0323d6
--- /dev/null
+++ b/stubs/kvm-arch-set-singlestep.c
@@ -0,0 +1,8 @@
+#include "qemu/osdep.h"
+#include "qemu/error-report.h"
+#include "sysemu/kvm.h"
+
+void kvm_arch_set_singlestep(CPUState *cpu, int enabled)
+{
+    warn_report("KVM does not support single stepping");
+}
-- 
2.20.1

[Qemu-devel] [RFC PATCH v4 3/5] target/ppc: Move handling of hardware breakpoints to a separate function

[Fabiano Rosas]

This is in preparation for a refactoring of the kvm_handle_debug
function in the next patch.

Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
---
 target/ppc/kvm.c | 47 ++++++++++++++++++++++++++++-------------------
 1 file changed, 28 insertions(+), 19 deletions(-)

diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index d01852fe31..941c4e7523 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -1593,35 +1593,44 @@ void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
     }
 }
 
+static int kvm_handle_hw_breakpoint(CPUState *cs,
+                                    struct kvm_debug_exit_arch *arch_info)
+{
+    int handle = 0;
+    int n;
+    int flag = 0;
+
+    if (nb_hw_breakpoint + nb_hw_watchpoint > 0) {
+        if (arch_info->status & KVMPPC_DEBUG_BREAKPOINT) {
+            n = find_hw_breakpoint(arch_info->address, GDB_BREAKPOINT_HW);
+            if (n >= 0) {
+                handle = 1;
+            }
+        } else if (arch_info->status & (KVMPPC_DEBUG_WATCH_READ |
+                                        KVMPPC_DEBUG_WATCH_WRITE)) {
+            n = find_hw_watchpoint(arch_info->address,  &flag);
+            if (n >= 0) {
+                handle = 1;
+                cs->watchpoint_hit = &hw_watchpoint;
+                hw_watchpoint.vaddr = hw_debug_points[n].addr;
+                hw_watchpoint.flags = flag;
+            }
+        }
+    }
+    return handle;
+}
+
 static int kvm_handle_debug(PowerPCCPU *cpu, struct kvm_run *run)
 {
     CPUState *cs = CPU(cpu);
     CPUPPCState *env = &cpu->env;
     struct kvm_debug_exit_arch *arch_info = &run->debug.arch;
     int handle = 0;
-    int n;
-    int flag = 0;
 
     if (cs->singlestep_enabled) {
         handle = 1;
     } else if (arch_info->status) {
-        if (nb_hw_breakpoint + nb_hw_watchpoint > 0) {
-            if (arch_info->status & KVMPPC_DEBUG_BREAKPOINT) {
-                n = find_hw_breakpoint(arch_info->address, GDB_BREAKPOINT_HW);
-                if (n >= 0) {
-                    handle = 1;
-                }
-            } else if (arch_info->status & (KVMPPC_DEBUG_WATCH_READ |
-                                            KVMPPC_DEBUG_WATCH_WRITE)) {
-                n = find_hw_watchpoint(arch_info->address,  &flag);
-                if (n >= 0) {
-                    handle = 1;
-                    cs->watchpoint_hit = &hw_watchpoint;
-                    hw_watchpoint.vaddr = hw_debug_points[n].addr;
-                    hw_watchpoint.flags = flag;
-                }
-            }
-        }
+        handle = kvm_handle_hw_breakpoint(cs, arch_info);
     } else if (kvm_find_sw_breakpoint(cs, arch_info->address)) {
         handle = 1;
     } else {
-- 
2.20.1

[Qemu-devel] [RFC PATCH v4 4/5] target/ppc: Refactor kvm_handle_debug

[Fabiano Rosas]

There are four scenarios being handled in this function:

- single stepping
- hardware breakpoints
- software breakpoints
- fallback (no debug supported)

A future patch will add code to handle specific single step and
software breakpoints cases so let's split each scenario into its own
function now to avoid hurting readability.

Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 target/ppc/kvm.c | 86 ++++++++++++++++++++++++++++--------------------
 1 file changed, 50 insertions(+), 36 deletions(-)

diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 941c4e7523..9392fba192 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -1620,52 +1620,66 @@ static int kvm_handle_hw_breakpoint(CPUState *cs,
     return handle;
 }
 
+static int kvm_handle_singlestep(void)
+{
+    return 1;
+}
+
+static int kvm_handle_sw_breakpoint(void)
+{
+    return 1;
+}
+
 static int kvm_handle_debug(PowerPCCPU *cpu, struct kvm_run *run)
 {
     CPUState *cs = CPU(cpu);
     CPUPPCState *env = &cpu->env;
     struct kvm_debug_exit_arch *arch_info = &run->debug.arch;
-    int handle = 0;
 
     if (cs->singlestep_enabled) {
-        handle = 1;
-    } else if (arch_info->status) {
-        handle = kvm_handle_hw_breakpoint(cs, arch_info);
-    } else if (kvm_find_sw_breakpoint(cs, arch_info->address)) {
-        handle = 1;
-    } else {
-        /* QEMU is not able to handle debug exception, so inject
-         * program exception to guest;
-         * Yes program exception NOT debug exception !!
-         * When QEMU is using debug resources then debug exception must
-         * be always set. To achieve this we set MSR_DE and also set
-         * MSRP_DEP so guest cannot change MSR_DE.
-         * When emulating debug resource for guest we want guest
-         * to control MSR_DE (enable/disable debug interrupt on need).
-         * Supporting both configurations are NOT possible.
-         * So the result is that we cannot share debug resources
-         * between QEMU and Guest on BOOKE architecture.
-         * In the current design QEMU gets the priority over guest,
-         * this means that if QEMU is using debug resources then guest
-         * cannot use them;
-         * For software breakpoint QEMU uses a privileged instruction;
-         * So there cannot be any reason that we are here for guest
-         * set debug exception, only possibility is guest executed a
-         * privileged / illegal instruction and that's why we are
-         * injecting a program interrupt.
-         */
+        return kvm_handle_singlestep();
+    }
+
+    if (arch_info->status) {
+        return kvm_handle_hw_breakpoint(cs, arch_info);
+    }
 
-        cpu_synchronize_state(cs);
-        /* env->nip is PC, so increment this by 4 to use
-         * ppc_cpu_do_interrupt(), which set srr0 = env->nip - 4.
-         */
-        env->nip += 4;
-        cs->exception_index = POWERPC_EXCP_PROGRAM;
-        env->error_code = POWERPC_EXCP_INVAL;
-        ppc_cpu_do_interrupt(cs);
+    if (kvm_find_sw_breakpoint(cs, arch_info->address)) {
+        return kvm_handle_sw_breakpoint();
     }
 
-    return handle;
+    /*
+     * QEMU is not able to handle debug exception, so inject
+     * program exception to guest;
+     * Yes program exception NOT debug exception !!
+     * When QEMU is using debug resources then debug exception must
+     * be always set. To achieve this we set MSR_DE and also set
+     * MSRP_DEP so guest cannot change MSR_DE.
+     * When emulating debug resource for guest we want guest
+     * to control MSR_DE (enable/disable debug interrupt on need).
+     * Supporting both configurations are NOT possible.
+     * So the result is that we cannot share debug resources
+     * between QEMU and Guest on BOOKE architecture.
+     * In the current design QEMU gets the priority over guest,
+     * this means that if QEMU is using debug resources then guest
+     * cannot use them;
+     * For software breakpoint QEMU uses a privileged instruction;
+     * So there cannot be any reason that we are here for guest
+     * set debug exception, only possibility is guest executed a
+     * privileged / illegal instruction and that's why we are
+     * injecting a program interrupt.
+     */
+    cpu_synchronize_state(cs);
+    /*
+     * env->nip is PC, so increment this by 4 to use
+     * ppc_cpu_do_interrupt(), which set srr0 = env->nip - 4.
+     */
+    env->nip += 4;
+    cs->exception_index = POWERPC_EXCP_PROGRAM;
+    env->error_code = POWERPC_EXCP_INVAL;
+    ppc_cpu_do_interrupt(cs);
+
+    return 0;
 }
 
 int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
-- 
2.20.1

[Qemu-devel] [RFC PATCH v4 5/5] target/ppc: support single stepping with KVM HV

[Fabiano Rosas]

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 26604ddf98..fb1bf1e9d7 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1171,6 +1171,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_)          \
@@ -1266,6 +1271,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, fprintf_function cpu_fprintf,
@@ -1281,6 +1287,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,
@@ -2227,6 +2239,10 @@ enum {
                         PPC2_ISA300)
 };
 
+#define XOP_RFID 18
+#define XOP_MFMSR 83
+#define XOP_MTSPR 467
+
 /*****************************************************************************/
 /* Memory access type :
  * may be needed for precise access rights control and precise exceptions.
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index beafcf1ebd..cf8c86de91 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;
@@ -768,6 +770,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 fbf3821f4b..a5b2705ade 100644
--- a/target/ppc/gdbstub.c
+++ b/target/ppc/gdbstub.c
@@ -380,3 +380,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 9392fba192..2bcb8814f4 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -1554,6 +1554,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;
@@ -1593,6 +1673,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 (kvm_has_guestdbg_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)
 {
@@ -1620,13 +1785,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 (kvm_has_guestdbg_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;
 }
 
@@ -1637,7 +1818,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) {
@@ -1645,7 +1826,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.20.1

Re: [Qemu-devel] [RFC PATCH v4 1/5] target/ppc: Move exception vector offset computation into a function

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2142 bytes --]

On Thu, Feb 28, 2019 at 07:57:55PM -0300, Fabiano Rosas wrote:
> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>

This is a nice cleanup, regardless of the rest of the series.  Applied
to ppc-for-4.0.

> ---
>  target/ppc/excp_helper.c | 30 +++++++++++++++++++-----------
>  1 file changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 39bedbb11d..beafcf1ebd 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -107,6 +107,24 @@ static int powerpc_reset_wakeup(CPUState *cs, CPUPPCState *env, int excp,
>      return POWERPC_EXCP_RESET;
>  }
>  
> +static uint64_t ppc_excp_vector_offset(CPUState *cs, int ail)
> +{
> +    uint64_t offset = 0;
> +
> +    switch (ail) {
> +    case AIL_0001_8000:
> +        offset = 0x18000;
> +        break;
> +    case AIL_C000_0000_0000_4000:
> +        offset = 0xc000000000004000ull;
> +        break;
> +    default:
> +        cpu_abort(cs, "Invalid AIL combination %d\n", ail);
> +        break;
> +    }
> +
> +    return offset;
> +}
>  
>  /* Note that this function should be greatly optimized
>   * when called with a constant excp, from ppc_hw_interrupt
> @@ -708,17 +726,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>      /* Handle AIL */
>      if (ail) {
>          new_msr |= (1 << MSR_IR) | (1 << MSR_DR);
> -        switch(ail) {
> -        case AIL_0001_8000:
> -            vector |= 0x18000;
> -            break;
> -        case AIL_C000_0000_0000_4000:
> -            vector |= 0xc000000000004000ull;
> -            break;
> -        default:
> -            cpu_abort(cs, "Invalid AIL combination %d\n", ail);
> -            break;
> -        }
> +        vector |= ppc_excp_vector_offset(cs, ail);
>      }
>  
>  #if defined(TARGET_PPC64)

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

Re: [Qemu-devel] [RFC PATCH v4 2/5] kvm-all: Introduce kvm_set_singlestep

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 5669 bytes --]

On Thu, Feb 28, 2019 at 07:57:56PM -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>
> ---
>  accel/kvm/kvm-all.c             | 16 ++++++++++++++++
>  accel/stubs/kvm-stub.c          |  4 ++++
>  exec.c                          |  2 +-
>  include/sysemu/kvm.h            |  3 +++
>  stubs/Makefile.objs             |  1 +
>  stubs/kvm-arch-set-singlestep.c |  8 ++++++++
>  6 files changed, 33 insertions(+), 1 deletion(-)
>  create mode 100644 stubs/kvm-arch-set-singlestep.c
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index fd92b6f375..d3ac5a9e5c 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -2267,6 +2267,13 @@ bool kvm_arm_supports_user_irq(void)
>      return kvm_check_extension(kvm_state, KVM_CAP_ARM_USER_IRQ);
>  }
>  
> +/* Whether the KVM_SET_GUEST_DEBUG ioctl supports single stepping */
> +int kvm_has_guestdbg_singlestep(void)
> +{
> +    /* return kvm_check_extension(kvm_state, KVM_CAP_GUEST_DEBUG_SSTEP); */

I don't see a KVM_CAP_GUEST_DEBUG_SSTEP in either the qemu or kernel
trees.  Where does that come from?

> +    return 0;
> +}
> +
>  #ifdef KVM_CAP_SET_GUEST_DEBUG
>  struct kvm_sw_breakpoint *kvm_find_sw_breakpoint(CPUState *cpu,
>                                                   target_ulong pc)
> @@ -2316,6 +2323,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_guestdbg_singlestep()) {
> +        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 02d5170031..69bd07f50e 100644
> --- a/accel/stubs/kvm-stub.c
> +++ b/accel/stubs/kvm-stub.c
> @@ -79,6 +79,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 518064530b..8817513e26 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1236,7 +1236,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 a6d1cd190f..e1ef2f5b99 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -214,6 +214,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);
> +int kvm_has_guestdbg_singlestep(void);
>  
>  int kvm_init_vcpu(CPUState *cpu);
>  int kvm_cpu_exec(CPUState *cpu);
> @@ -246,6 +247,7 @@ bool kvm_memcrypt_enabled(void);
>   */
>  int kvm_memcrypt_encrypt_data(uint8_t *ptr, uint64_t len);
>  
> +void kvm_arch_set_singlestep(CPUState *cpu, int enabled);
>  
>  #ifdef NEED_CPU_H
>  #include "cpu.h"
> @@ -258,6 +260,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 269dfa5832..884f9b2268 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-set-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-set-singlestep.c b/stubs/kvm-arch-set-singlestep.c
> new file mode 100644
> index 0000000000..ba6e0323d6
> --- /dev/null
> +++ b/stubs/kvm-arch-set-singlestep.c
> @@ -0,0 +1,8 @@
> +#include "qemu/osdep.h"
> +#include "qemu/error-report.h"
> +#include "sysemu/kvm.h"
> +
> +void kvm_arch_set_singlestep(CPUState *cpu, int enabled)
> +{
> +    warn_report("KVM does not support single stepping");
> +}

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

Re: [Qemu-devel] [RFC PATCH v4 3/5] target/ppc: Move handling of hardware breakpoints to a separate function

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3285 bytes --]

On Thu, Feb 28, 2019 at 07:57:57PM -0300, Fabiano Rosas wrote:
> This is in preparation for a refactoring of the kvm_handle_debug
> function in the next patch.
> 
> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>

Nice cleanup regardless of anything else.  Applied to ppc-for-4.0.

> ---
>  target/ppc/kvm.c | 47 ++++++++++++++++++++++++++++-------------------
>  1 file changed, 28 insertions(+), 19 deletions(-)
> 
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index d01852fe31..941c4e7523 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -1593,35 +1593,44 @@ void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
>      }
>  }
>  
> +static int kvm_handle_hw_breakpoint(CPUState *cs,
> +                                    struct kvm_debug_exit_arch *arch_info)
> +{
> +    int handle = 0;
> +    int n;
> +    int flag = 0;
> +
> +    if (nb_hw_breakpoint + nb_hw_watchpoint > 0) {
> +        if (arch_info->status & KVMPPC_DEBUG_BREAKPOINT) {
> +            n = find_hw_breakpoint(arch_info->address, GDB_BREAKPOINT_HW);
> +            if (n >= 0) {
> +                handle = 1;
> +            }
> +        } else if (arch_info->status & (KVMPPC_DEBUG_WATCH_READ |
> +                                        KVMPPC_DEBUG_WATCH_WRITE)) {
> +            n = find_hw_watchpoint(arch_info->address,  &flag);
> +            if (n >= 0) {
> +                handle = 1;
> +                cs->watchpoint_hit = &hw_watchpoint;
> +                hw_watchpoint.vaddr = hw_debug_points[n].addr;
> +                hw_watchpoint.flags = flag;
> +            }
> +        }
> +    }
> +    return handle;
> +}
> +
>  static int kvm_handle_debug(PowerPCCPU *cpu, struct kvm_run *run)
>  {
>      CPUState *cs = CPU(cpu);
>      CPUPPCState *env = &cpu->env;
>      struct kvm_debug_exit_arch *arch_info = &run->debug.arch;
>      int handle = 0;
> -    int n;
> -    int flag = 0;
>  
>      if (cs->singlestep_enabled) {
>          handle = 1;
>      } else if (arch_info->status) {
> -        if (nb_hw_breakpoint + nb_hw_watchpoint > 0) {
> -            if (arch_info->status & KVMPPC_DEBUG_BREAKPOINT) {
> -                n = find_hw_breakpoint(arch_info->address, GDB_BREAKPOINT_HW);
> -                if (n >= 0) {
> -                    handle = 1;
> -                }
> -            } else if (arch_info->status & (KVMPPC_DEBUG_WATCH_READ |
> -                                            KVMPPC_DEBUG_WATCH_WRITE)) {
> -                n = find_hw_watchpoint(arch_info->address,  &flag);
> -                if (n >= 0) {
> -                    handle = 1;
> -                    cs->watchpoint_hit = &hw_watchpoint;
> -                    hw_watchpoint.vaddr = hw_debug_points[n].addr;
> -                    hw_watchpoint.flags = flag;
> -                }
> -            }
> -        }
> +        handle = kvm_handle_hw_breakpoint(cs, arch_info);
>      } else if (kvm_find_sw_breakpoint(cs, arch_info->address)) {
>          handle = 1;
>      } else {

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

Re: [Qemu-devel] [RFC PATCH v4 4/5] target/ppc: Refactor kvm_handle_debug

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 5395 bytes --]

On Thu, Feb 28, 2019 at 07:57:58PM -0300, Fabiano Rosas wrote:
> There are four scenarios being handled in this function:
> 
> - single stepping
> - hardware breakpoints
> - software breakpoints
> - fallback (no debug supported)
> 
> A future patch will add code to handle specific single step and
> software breakpoints cases so let's split each scenario into its own
> function now to avoid hurting readability.
> 
> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>

Again, a nice cleanup regardless of anything else.  Applied.

> ---
>  target/ppc/kvm.c | 86 ++++++++++++++++++++++++++++--------------------
>  1 file changed, 50 insertions(+), 36 deletions(-)
> 
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 941c4e7523..9392fba192 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -1620,52 +1620,66 @@ static int kvm_handle_hw_breakpoint(CPUState *cs,
>      return handle;
>  }
>  
> +static int kvm_handle_singlestep(void)
> +{
> +    return 1;
> +}
> +
> +static int kvm_handle_sw_breakpoint(void)
> +{
> +    return 1;
> +}
> +
>  static int kvm_handle_debug(PowerPCCPU *cpu, struct kvm_run *run)
>  {
>      CPUState *cs = CPU(cpu);
>      CPUPPCState *env = &cpu->env;
>      struct kvm_debug_exit_arch *arch_info = &run->debug.arch;
> -    int handle = 0;
>  
>      if (cs->singlestep_enabled) {
> -        handle = 1;
> -    } else if (arch_info->status) {
> -        handle = kvm_handle_hw_breakpoint(cs, arch_info);
> -    } else if (kvm_find_sw_breakpoint(cs, arch_info->address)) {
> -        handle = 1;
> -    } else {
> -        /* QEMU is not able to handle debug exception, so inject
> -         * program exception to guest;
> -         * Yes program exception NOT debug exception !!
> -         * When QEMU is using debug resources then debug exception must
> -         * be always set. To achieve this we set MSR_DE and also set
> -         * MSRP_DEP so guest cannot change MSR_DE.
> -         * When emulating debug resource for guest we want guest
> -         * to control MSR_DE (enable/disable debug interrupt on need).
> -         * Supporting both configurations are NOT possible.
> -         * So the result is that we cannot share debug resources
> -         * between QEMU and Guest on BOOKE architecture.
> -         * In the current design QEMU gets the priority over guest,
> -         * this means that if QEMU is using debug resources then guest
> -         * cannot use them;
> -         * For software breakpoint QEMU uses a privileged instruction;
> -         * So there cannot be any reason that we are here for guest
> -         * set debug exception, only possibility is guest executed a
> -         * privileged / illegal instruction and that's why we are
> -         * injecting a program interrupt.
> -         */
> +        return kvm_handle_singlestep();
> +    }
> +
> +    if (arch_info->status) {
> +        return kvm_handle_hw_breakpoint(cs, arch_info);
> +    }
>  
> -        cpu_synchronize_state(cs);
> -        /* env->nip is PC, so increment this by 4 to use
> -         * ppc_cpu_do_interrupt(), which set srr0 = env->nip - 4.
> -         */
> -        env->nip += 4;
> -        cs->exception_index = POWERPC_EXCP_PROGRAM;
> -        env->error_code = POWERPC_EXCP_INVAL;
> -        ppc_cpu_do_interrupt(cs);
> +    if (kvm_find_sw_breakpoint(cs, arch_info->address)) {
> +        return kvm_handle_sw_breakpoint();
>      }
>  
> -    return handle;
> +    /*
> +     * QEMU is not able to handle debug exception, so inject
> +     * program exception to guest;
> +     * Yes program exception NOT debug exception !!
> +     * When QEMU is using debug resources then debug exception must
> +     * be always set. To achieve this we set MSR_DE and also set
> +     * MSRP_DEP so guest cannot change MSR_DE.
> +     * When emulating debug resource for guest we want guest
> +     * to control MSR_DE (enable/disable debug interrupt on need).
> +     * Supporting both configurations are NOT possible.
> +     * So the result is that we cannot share debug resources
> +     * between QEMU and Guest on BOOKE architecture.
> +     * In the current design QEMU gets the priority over guest,
> +     * this means that if QEMU is using debug resources then guest
> +     * cannot use them;
> +     * For software breakpoint QEMU uses a privileged instruction;
> +     * So there cannot be any reason that we are here for guest
> +     * set debug exception, only possibility is guest executed a
> +     * privileged / illegal instruction and that's why we are
> +     * injecting a program interrupt.
> +     */
> +    cpu_synchronize_state(cs);
> +    /*
> +     * env->nip is PC, so increment this by 4 to use
> +     * ppc_cpu_do_interrupt(), which set srr0 = env->nip - 4.
> +     */
> +    env->nip += 4;
> +    cs->exception_index = POWERPC_EXCP_PROGRAM;
> +    env->error_code = POWERPC_EXCP_INVAL;
> +    ppc_cpu_do_interrupt(cs);
> +
> +    return 0;
>  }
>  
>  int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)

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

Re: [Qemu-devel] [RFC PATCH v4 2/5] kvm-all: Introduce kvm_set_singlestep

[Fabiano Rosas]

David Gibson <david@gibson.dropbear.id.au> writes:

>> +/* Whether the KVM_SET_GUEST_DEBUG ioctl supports single stepping */
>> +int kvm_has_guestdbg_singlestep(void)
>> +{
>> +    /* return kvm_check_extension(kvm_state, KVM_CAP_GUEST_DEBUG_SSTEP); */
>
> I don't see a KVM_CAP_GUEST_DEBUG_SSTEP in either the qemu or kernel
> trees.  Where does that come from?
>

I'll submit that to the kernel this week. I was waiting to make sure this
wouldn't change too much on QEMU side first.

Re: [Qemu-devel] [RFC PATCH v4 2/5] kvm-all: Introduce kvm_set_singlestep

[Fabiano Rosas]

David Gibson <david@gibson.dropbear.id.au> writes:

>> +/* Whether the KVM_SET_GUEST_DEBUG ioctl supports single stepping */
>> +int kvm_has_guestdbg_singlestep(void)
>> +{
>> +    /* return kvm_check_extension(kvm_state, KVM_CAP_GUEST_DEBUG_SSTEP); */
>
> I don't see a KVM_CAP_GUEST_DEBUG_SSTEP in either the qemu or kernel
> trees.  Where does that come from?
>

I realized that this will cause a regression for the other architectures
(and even PPC-PR) that already have the feature when we run QEMU on
older kernels without this capability.

I am becoming inclined to rely on kvmppc_is_pr. Do you see another way
around this?

Re: [Qemu-devel] [RFC PATCH v4 5/5] target/ppc: support single stepping with KVM HV

[Alexey Kardashevskiy]

Are you reposting this any time soon?

In meanwhile I hit a problem when I cannot step over the "stdu" instruction.

I basically put this:
stdu    r1,-368(r1)

and "ni" in gdb does not stop on the next instruction which is quite
confusing. Ideas?


On 20/03/2019 12:42, Alexey Kardashevskiy wrote:
> 
> 
> On 20/03/2019 01:32, Fabiano Rosas wrote:
>> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
>>
>>> Looks good to me, does not break what already works. However I cannot
>>> debug SLOF real mode and I am not sure why.
>>>
>>> (gdb) set endian big
>>>
>>> The target is assumed to be big endian
>>> (gdb) b *0x3f00
>>>
>>> Breakpoint 2 at 0x3f00
>>
>> I think I'm missing the point here. Why 0x3f00?
> 
> Because I am stupid and did not realize that 0x3f00 is a relative offset
> and 0x4000 is the correct address which works.
> 
> 
> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> 
> 
>>
>> (qemu) info roms
>> addr=0000000000000000 size=0x0e22b8 mem=ram name="...qemu/slof.bin"                               
>> addr=0000000000400000 size=0x17976d0 mem=ram name="...vmlinux"
>>
>>
>> $ objdump -d board-qemu/llfw/stage1.elf | grep "_start>"
>> 0000000000000100 <__start>:
>>      100:       48 00 3f 00     b       4000 <_start>
>> 0000000000004000 <_start>:
>>
>>
>> Thread 1 hit Breakpoint 3, _start () at startup.S:82
>> (gdb) p/x $pc
>> $1 = 0x4000
>> (gdb) si
>> (gdb) p/x $pc
>> $3 = 0x4004
>> (gdb) c
>> Thread 1 hit Breakpoint 4, early_c_entry (start_addr=49056, fdt_addr=49024) at stage2.c:202
>> (gdb) p/x $pc
>> $4 = 0x4d18
>>
> 

-- 
Alexey

Re: [Qemu-devel] [RFC PATCH v4 5/5] target/ppc: support single stepping with KVM HV

[Fabiano Rosas]

Alexey Kardashevskiy <aik@ozlabs.ru> writes:

> Are you reposting this any time soon?

I have sent a v2 to the kernel side of it:

https://lore.kernel.org/kvm/20190529222219.27994-1-farosas@linux.ibm.com/

I'm depending on what we decide to do there. The core of this patchset
will not change, just the mechanism by which the feature is selected
(patch 2, kvm-all: Introduce kvm_set_singlestep).

> In meanwhile I hit a problem when I cannot step over the "stdu" instruction.
>
> I basically put this:
> stdu    r1,-368(r1)
>
> and "ni" in gdb does not stop on the next instruction which is quite
> confusing. Ideas?

Perhaps the next instruction is one that is not traced? See the programming
note at the end of section 6.5.15 in Book III.

Or maybe the step got preempted? You should see GDB messages indicating
changing threads right before or after the stdu. However that would only
happen with more than one VCPU and if 'show scheduler-locking' in GDB is
'off'. And even then, that should not cause any issues, but it is a more
complex scenario so there could be a bug in the code.

> On 20/03/2019 12:42, Alexey Kardashevskiy wrote:
>> 
>> 
>> On 20/03/2019 01:32, Fabiano Rosas wrote:
>>> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
>>>
>>>> Looks good to me, does not break what already works. However I cannot
>>>> debug SLOF real mode and I am not sure why.
>>>>
>>>> (gdb) set endian big
>>>>
>>>> The target is assumed to be big endian
>>>> (gdb) b *0x3f00
>>>>
>>>> Breakpoint 2 at 0x3f00
>>>
>>> I think I'm missing the point here. Why 0x3f00?
>> 
>> Because I am stupid and did not realize that 0x3f00 is a relative offset
>> and 0x4000 is the correct address which works.
>> 
>> 
>> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> 
>> 
>>>
>>> (qemu) info roms
>>> addr=0000000000000000 size=0x0e22b8 mem=ram name="...qemu/slof.bin"                               
>>> addr=0000000000400000 size=0x17976d0 mem=ram name="...vmlinux"
>>>
>>>
>>> $ objdump -d board-qemu/llfw/stage1.elf | grep "_start>"
>>> 0000000000000100 <__start>:
>>>      100:       48 00 3f 00     b       4000 <_start>
>>> 0000000000004000 <_start>:
>>>
>>>
>>> Thread 1 hit Breakpoint 3, _start () at startup.S:82
>>> (gdb) p/x $pc
>>> $1 = 0x4000
>>> (gdb) si
>>> (gdb) p/x $pc
>>> $3 = 0x4004
>>> (gdb) c
>>> Thread 1 hit Breakpoint 4, early_c_entry (start_addr=49056, fdt_addr=49024) at stage2.c:202
>>> (gdb) p/x $pc
>>> $4 = 0x4d18
>>>
>> 

Re: [Qemu-devel] [RFC PATCH v4 5/5] target/ppc: support single stepping with KVM HV

[Alexey Kardashevskiy]

On 12/06/2019 23:34, Fabiano Rosas wrote:
> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
> 
>> Are you reposting this any time soon?
> 
> I have sent a v2 to the kernel side of it:
> 
> https://lore.kernel.org/kvm/20190529222219.27994-1-farosas@linux.ibm.com/
> 
> I'm depending on what we decide to do there. The core of this patchset
> will not change, just the mechanism by which the feature is selected
> (patch 2, kvm-all: Introduce kvm_set_singlestep).
> 
>> In meanwhile I hit a problem when I cannot step over the "stdu" instruction.
>>
>> I basically put this:
>> stdu    r1,-368(r1)
>>
>> and "ni" in gdb does not stop on the next instruction which is quite
>> confusing. Ideas?
> 
> Perhaps the next instruction is one that is not traced? See the programming
> note at the end of section 6.5.15 in Book III.

No, it is not it, it is more subtle. The problem piece was originally this:

c0000000010f16b4:       f0 ff c1 fb     std     r30,-16(r1)
c0000000010f16b8:       f8 ff e1 fb     std     r31,-8(r1)
c0000000010f16bc:       91 fe 21 f8     stdu    r1,-368(r1)
c0000000010f16c0:       39 00 22 3d     addis   r9,r2,57
c0000000010f16c4:       18 97 49 39     addi    r10,r9,-26856
c0000000010f16c8:       08 00 22 3d     addis   r9,r2,8
c0000000010f16cc:       00 78 29 39     addi    r9,r9,30720

It is Linux'es prom_init().


> Or maybe the step got preempted? You should see GDB messages indicating
> changing threads right before or after the stdu. However that would only
> happen with more than one VCPU and if 'show scheduler-locking' in GDB is
> 'off'. And even then, that should not cause any issues, but it is a more
> complex scenario so there could be a bug in the code.

It is TCG, a single CPU with a single thread and no matter where I put
this "stdu    r1,-368(r1)" - GDB does not stop on the next one and just
runs.

In the example above:
1. "b *0x10f16bc" makes GDB stop there, "ni" continues without stopping
on at 0x10f16c0.
2. "b *0x10f16bc" and "b *0x10f16c0" make GDB stop at 0x10f16bc and "ni"
steps to 0x10f16c0 but it is rather because it is a breakpoint and not
the next instruction.
3. "b *0x10f16bc" and "b *0x10f16c4" make GDB stop at 0x10f16bc and "ni"
stops GDB at 0x10f16bc but again it is a breakpoint.

In 2 and 3 it is possible to continue step debugging till the next "stdu".


> 
>> On 20/03/2019 12:42, Alexey Kardashevskiy wrote:
>>>
>>>
>>> On 20/03/2019 01:32, Fabiano Rosas wrote:
>>>> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
>>>>
>>>>> Looks good to me, does not break what already works. However I cannot
>>>>> debug SLOF real mode and I am not sure why.
>>>>>
>>>>> (gdb) set endian big
>>>>>
>>>>> The target is assumed to be big endian
>>>>> (gdb) b *0x3f00
>>>>>
>>>>> Breakpoint 2 at 0x3f00
>>>>
>>>> I think I'm missing the point here. Why 0x3f00?
>>>
>>> Because I am stupid and did not realize that 0x3f00 is a relative offset
>>> and 0x4000 is the correct address which works.
>>>
>>>
>>> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>
>>>
>>>>
>>>> (qemu) info roms
>>>> addr=0000000000000000 size=0x0e22b8 mem=ram name="...qemu/slof.bin"                               
>>>> addr=0000000000400000 size=0x17976d0 mem=ram name="...vmlinux"
>>>>
>>>>
>>>> $ objdump -d board-qemu/llfw/stage1.elf | grep "_start>"
>>>> 0000000000000100 <__start>:
>>>>      100:       48 00 3f 00     b       4000 <_start>
>>>> 0000000000004000 <_start>:
>>>>
>>>>
>>>> Thread 1 hit Breakpoint 3, _start () at startup.S:82
>>>> (gdb) p/x $pc
>>>> $1 = 0x4000
>>>> (gdb) si
>>>> (gdb) p/x $pc
>>>> $3 = 0x4004
>>>> (gdb) c
>>>> Thread 1 hit Breakpoint 4, early_c_entry (start_addr=49056, fdt_addr=49024) at stage2.c:202
>>>> (gdb) p/x $pc
>>>> $4 = 0x4d18
>>>>
>>>

-- 
Alexey

Re: [Qemu-devel] [RFC PATCH v4 5/5] target/ppc: support single stepping with KVM HV

[Fabiano Rosas]

Alexey Kardashevskiy <aik@ozlabs.ru> writes:

> It is TCG, a single CPU with a single thread and no matter where I put

Ok, but then none of this code gets executed because it is kvm-only:

qemu/exec.c
void cpu_single_step(CPUState *cpu, int enabled)
{
    if (cpu->singlestep_enabled != enabled) {
        cpu->singlestep_enabled = enabled;
        if (kvm_enabled()) {
->           kvm_set_singlestep(cpu, enabled);
        } else {
            /* must flush all the translated code to avoid inconsistencies */
            /* XXX: only flush what is necessary */
            tb_flush(cpu);
        }
    }
}

> this "stdu    r1,-368(r1)" - GDB does not stop on the next one and just
> runs.
>
> In the example above:
> 1. "b *0x10f16bc" makes GDB stop there, "ni" continues without stopping
> on at 0x10f16c0.

But this seems wrong anyway. Let me try to reproduce it and see what I
can find.

> 2. "b *0x10f16bc" and "b *0x10f16c0" make GDB stop at 0x10f16bc and "ni"
> steps to 0x10f16c0 but it is rather because it is a breakpoint and not
> the next instruction.
> 3. "b *0x10f16bc" and "b *0x10f16c4" make GDB stop at 0x10f16bc and "ni"
> stops GDB at 0x10f16bc but again it is a breakpoint.
>
> In 2 and 3 it is possible to continue step debugging till the next "stdu".
>

Re: [Qemu-devel] [RFC PATCH v4 5/5] target/ppc: support single stepping with KVM HV

[Alexey Kardashevskiy]

On 13/06/2019 12:01, Fabiano Rosas wrote:
> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
> 
>> It is TCG, a single CPU with a single thread and no matter where I put
> 
> Ok, but then none of this code gets executed because it is kvm-only:


I was not clear, this is a generic issue, not related to your patchset,
I just thought since you are looking at this already, you might find the
answer faster, that's it :)


> 
> qemu/exec.c
> void cpu_single_step(CPUState *cpu, int enabled)
> {
>     if (cpu->singlestep_enabled != enabled) {
>         cpu->singlestep_enabled = enabled;
>         if (kvm_enabled()) {
> ->           kvm_set_singlestep(cpu, enabled);
>         } else {
>             /* must flush all the translated code to avoid inconsistencies */
>             /* XXX: only flush what is necessary */
>             tb_flush(cpu);
>         }
>     }
> }
> 
>> this "stdu    r1,-368(r1)" - GDB does not stop on the next one and just
>> runs.
>>
>> In the example above:
>> 1. "b *0x10f16bc" makes GDB stop there, "ni" continues without stopping
>> on at 0x10f16c0.
> 
> But this seems wrong anyway. Let me try to reproduce it and see what I
> can find.


Thanks!

> 
>> 2. "b *0x10f16bc" and "b *0x10f16c0" make GDB stop at 0x10f16bc and "ni"
>> steps to 0x10f16c0 but it is rather because it is a breakpoint and not
>> the next instruction.
>> 3. "b *0x10f16bc" and "b *0x10f16c4" make GDB stop at 0x10f16bc and "ni"
>> stops GDB at 0x10f16bc but again it is a breakpoint.
>>
>> In 2 and 3 it is possible to continue step debugging till the next "stdu".
>>
> 

-- 
Alexey