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

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

[Fabiano Rosas]

Single stepping via GDB/gdbstub in POWER 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.

This was so far tested with single and multiple vcpus and with GDB
scheduler locking on and off [2].

I have not fully explored yet the potential issues when using
debuggers simultaneously inside and outside the guest, however I was
able to single step the ptrace code while single stepping a userspace
program inside the guest with GDB.

I'm looking for feedback on the general approach before I develop this
further.

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


Fabiano Rosas (3):
  target/ppc: Add macro definitions for relocated interrupt vectors
    offsets
  kvm-all: Introduce kvm_set_singlestep
  target/ppc: support single stepping with KVM HV

 accel/kvm/kvm-all.c      | 10 +++++++
 exec.c                   |  1 +
 include/sysemu/kvm.h     |  4 +++
 target/arm/kvm.c         |  4 +++
 target/i386/kvm.c        |  4 +++
 target/mips/kvm.c        |  4 +++
 target/ppc/cpu.h         |  3 ++
 target/ppc/excp_helper.c |  4 +--
 target/ppc/kvm.c         | 65 +++++++++++++++++++++++++++++++++++++++-
 target/s390x/kvm.c       |  4 +++
 10 files changed, 100 insertions(+), 3 deletions(-)

--
2.17.1

[Qemu-devel] [RFC PATCH v2 1/3] target/ppc: Add macro definitions for relocated interrupt vectors offsets

[Fabiano Rosas]

The PowerISA prescribes that depending on the values of MSR_IR,
MSR_DR, MSR_HV and LPCR_AIL, the interrupt vectors might be relocated
by specific offsets.

This patch defines macros for these offsets so that they can be used
by another part of the code in a future patch.

Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
---
 target/ppc/cpu.h         | 3 +++
 target/ppc/excp_helper.c | 4 ++--
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index ab68abe8a2..5147db4460 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -2390,6 +2390,9 @@ enum {
     AIL_C000_0000_0000_4000 = 3,
 };
 
+#define AIL_0001_8000_OFFSET 0x18000
+#define AIL_C000_0000_0000_4000_OFFSET 0xc000000000004000ull
+
 /*****************************************************************************/
 
 #define is_isa300(ctx) (!!(ctx->insns_flags2 & PPC2_ISA300))
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 0ec7ae1ad4..49bdf7dd54 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -687,10 +687,10 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
         new_msr |= (1 << MSR_IR) | (1 << MSR_DR);
         switch(ail) {
         case AIL_0001_8000:
-            vector |= 0x18000;
+            vector |= AIL_0001_8000_OFFSET;
             break;
         case AIL_C000_0000_0000_4000:
-            vector |= 0xc000000000004000ull;
+            vector |= AIL_C000_0000_0000_4000_OFFSET;
             break;
         default:
             cpu_abort(cs, "Invalid AIL combination %d\n", ail);
-- 
2.17.1

[Qemu-devel] [RFC PATCH v2 2/3] kvm-all: Introduce kvm_set_singlestep

[Fabiano Rosas]

This will be used in a future patch to implement an
architecture-specific single step mechanism for POWER.

Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
---
 accel/kvm/kvm-all.c  | 10 ++++++++++
 exec.c               |  1 +
 include/sysemu/kvm.h |  4 ++++
 target/arm/kvm.c     |  4 ++++
 target/i386/kvm.c    |  4 ++++
 target/mips/kvm.c    |  4 ++++
 target/ppc/kvm.c     |  4 ++++
 target/s390x/kvm.c   |  4 ++++
 8 files changed, 35 insertions(+)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 4880a05399..4fb7199a15 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2313,6 +2313,11 @@ int kvm_update_guest_debug(CPUState *cpu, unsigned long reinject_trap)
     return data.err;
 }
 
+void kvm_set_singlestep(CPUState *cs, int enabled)
+{
+    kvm_arch_set_singlestep(cs, enabled);
+}
+
 int kvm_insert_breakpoint(CPUState *cpu, target_ulong addr,
                           target_ulong len, int type)
 {
@@ -2439,6 +2444,11 @@ int kvm_remove_breakpoint(CPUState *cpu, target_ulong addr,
 void kvm_remove_all_breakpoints(CPUState *cpu)
 {
 }
+
+void kvm_set_singlestep(CPUState *cs, int enabled)
+{
+}
+
 #endif /* !KVM_CAP_SET_GUEST_DEBUG */
 
 static int kvm_set_signal_mask(CPUState *cpu, const sigset_t *sigset)
diff --git a/exec.c b/exec.c
index bb6170dbff..55614822c3 100644
--- a/exec.c
+++ b/exec.c
@@ -1233,6 +1233,7 @@ 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);
             kvm_update_guest_debug(cpu, 0);
         } else {
             /* must flush all the translated code to avoid inconsistencies */
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index 97d8d9d0d5..a01a8d58dd 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -259,6 +259,8 @@ int kvm_remove_breakpoint(CPUState *cpu, target_ulong addr,
 void kvm_remove_all_breakpoints(CPUState *cpu);
 int kvm_update_guest_debug(CPUState *cpu, unsigned long reinject_trap);
 
+void kvm_set_singlestep(CPUState *cpu, int enabled);
+
 int kvm_on_sigbus_vcpu(CPUState *cpu, int code, void *addr);
 int kvm_on_sigbus(int code, void *addr);
 
@@ -431,6 +433,8 @@ void kvm_arch_remove_all_hw_breakpoints(void);
 
 void kvm_arch_update_guest_debug(CPUState *cpu, struct kvm_guest_debug *dbg);
 
+void kvm_arch_set_singlestep(CPUState *cpu, int enabled);
+
 bool kvm_arch_stop_on_emulation_error(CPUState *cpu);
 
 int kvm_check_extension(KVMState *s, unsigned int extension);
diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index 44dd0ce6ce..dd8e43ab7e 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -670,6 +670,10 @@ int kvm_arch_process_async_events(CPUState *cs)
     return 0;
 }
 
+void kvm_arch_set_singlestep(CPUState *cs, int enabled)
+{
+}
+
 /* The #ifdef protections are until 32bit headers are imported and can
  * be removed once both 32 and 64 bit reach feature parity.
  */
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index f524e7d929..ba56f2ee1f 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -3521,6 +3521,10 @@ static int kvm_handle_debug(X86CPU *cpu,
     return ret;
 }
 
+void kvm_arch_set_singlestep(CPUState *cs, int enabled)
+{
+}
+
 void kvm_arch_update_guest_debug(CPUState *cpu, struct kvm_guest_debug *dbg)
 {
     const uint8_t type_code[] = {
diff --git a/target/mips/kvm.c b/target/mips/kvm.c
index 8e72850962..8035262131 100644
--- a/target/mips/kvm.c
+++ b/target/mips/kvm.c
@@ -119,6 +119,10 @@ int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
     return 0;
 }
 
+void kvm_arch_set_singlestep(CPUState *cs, int enabled)
+{
+}
+
 static inline int cpu_mips_io_interrupts_pending(MIPSCPU *cpu)
 {
     CPUMIPSState *env = &cpu->env;
diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index f81327d6cd..9d0b4f1f3f 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -1551,6 +1551,10 @@ void kvm_arch_remove_all_hw_breakpoints(void)
     nb_hw_breakpoint = nb_hw_watchpoint = 0;
 }
 
+void kvm_arch_set_singlestep(CPUState *cs, int enabled)
+{
+}
+
 void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
 {
     int n;
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 2ebf26adfe..4bde183458 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -975,6 +975,10 @@ void kvm_arch_remove_all_hw_breakpoints(void)
     hw_breakpoints = NULL;
 }
 
+void kvm_arch_set_singlestep(CPUState *cs, int enabled)
+{
+}
+
 void kvm_arch_update_guest_debug(CPUState *cpu, struct kvm_guest_debug *dbg)
 {
     int i;
-- 
2.17.1

[Qemu-devel] [RFC PATCH v2 3/3] 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, 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).

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

diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 9d0b4f1f3f..93c20099e6 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -94,6 +94,7 @@ static int cap_ppc_safe_indirect_branch;
 static int cap_ppc_nested_kvm_hv;
 
 static uint32_t debug_inst_opcode;
+static target_ulong trace_handler_addr;
 
 /* XXX We have a race condition where we actually have a level triggered
  *     interrupt, but the infrastructure can't expose that yet, so the guest
@@ -509,6 +510,9 @@ int kvm_arch_init_vcpu(CPUState *cs)
     kvm_get_one_reg(cs, KVM_REG_PPC_DEBUG_INST, &debug_inst_opcode);
     kvmppc_hw_debug_points_init(cenv);
 
+    trace_handler_addr = (cenv->excp_vectors[POWERPC_EXCP_TRACE] |
+                          AIL_C000_0000_0000_4000_OFFSET);
+
     return ret;
 }
 
@@ -1553,6 +1557,24 @@ void kvm_arch_remove_all_hw_breakpoints(void)
 
 void kvm_arch_set_singlestep(CPUState *cs, int enabled)
 {
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+    CPUPPCState *env = &cpu->env;
+
+    if (kvmppc_is_pr(kvm_state)) {
+            return;
+    }
+
+    if (enabled) {
+        /* MSR_SE = 1 will cause a Trace Interrupt in the guest after
+         * the next instruction executes. */
+        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. */
+        kvm_insert_breakpoint(cs, trace_handler_addr, 4, GDB_BREAKPOINT_SW);
+    }
 }
 
 void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
@@ -1594,6 +1616,43 @@ void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
     }
 }
 
+static int kvm_handle_singlestep(CPUState *cs,
+                                 struct kvm_debug_exit_arch *arch_info)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+    CPUPPCState *env = &cpu->env;
+    target_ulong msr = env->msr;
+    uint32_t insn;
+    int ret = 1;
+    int reg;
+
+    if (kvmppc_is_pr(kvm_state)) {
+        return ret;
+    }
+
+    if (arch_info->address == trace_handler_addr) {
+        cpu_synchronize_state(cs);
+        kvm_remove_breakpoint(cs, trace_handler_addr, 4, GDB_BREAKPOINT_SW);
+
+        cpu_memory_rw_debug(cs, env->spr[SPR_SRR0] - 4, (uint8_t *)&insn,
+                            sizeof(insn), 0);
+
+        /* If the last instruction was a mfmsr, make sure that the
+         * MSR_SE bit is not set to avoid the guest kernel knowing
+         * that it is being single-stepped */
+        if (extract32(insn, 26, 6) == 31 && extract32(insn, 1, 10) == 83) {
+            reg = extract32(insn, 21, 5);
+            env->gpr[reg] &= ~(1ULL << MSR_SE);
+        }
+
+        env->nip = env->spr[SPR_SRR0];
+        env->msr = msr &= ~(1ULL << MSR_SE);
+        cpu_synchronize_state(cs);
+    }
+
+    return ret;
+}
+
 static int kvm_handle_debug(PowerPCCPU *cpu, struct kvm_run *run)
 {
     CPUState *cs = CPU(cpu);
@@ -1604,7 +1663,7 @@ static int kvm_handle_debug(PowerPCCPU *cpu, struct kvm_run *run)
     int flag = 0;
 
     if (cs->singlestep_enabled) {
-        handle = 1;
+        handle = kvm_handle_singlestep(cs, arch_info);
     } else if (arch_info->status) {
         if (nb_hw_breakpoint + nb_hw_watchpoint > 0) {
             if (arch_info->status & KVMPPC_DEBUG_BREAKPOINT) {
-- 
2.17.1

Re: [Qemu-devel] [RFC PATCH v2 2/3] kvm-all: Introduce kvm_set_singlestep

[Philippe Mathieu-Daudé]

On 21/11/18 19:13, Fabiano Rosas wrote:
> This will be used in a future patch to implement an
> architecture-specific single step mechanism for POWER.
> 
> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>   accel/kvm/kvm-all.c  | 10 ++++++++++
>   exec.c               |  1 +
>   include/sysemu/kvm.h |  4 ++++
>   target/arm/kvm.c     |  4 ++++
>   target/i386/kvm.c    |  4 ++++
>   target/mips/kvm.c    |  4 ++++
>   target/ppc/kvm.c     |  4 ++++
>   target/s390x/kvm.c   |  4 ++++
>   8 files changed, 35 insertions(+)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 4880a05399..4fb7199a15 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -2313,6 +2313,11 @@ int kvm_update_guest_debug(CPUState *cpu, unsigned long reinject_trap)
>       return data.err;
>   }
>   
> +void kvm_set_singlestep(CPUState *cs, int enabled)
> +{
> +    kvm_arch_set_singlestep(cs, enabled);
> +}
> +
>   int kvm_insert_breakpoint(CPUState *cpu, target_ulong addr,
>                             target_ulong len, int type)
>   {
> @@ -2439,6 +2444,11 @@ int kvm_remove_breakpoint(CPUState *cpu, target_ulong addr,
>   void kvm_remove_all_breakpoints(CPUState *cpu)
>   {
>   }
> +
> +void kvm_set_singlestep(CPUState *cs, int enabled)
> +{
> +}
> +
>   #endif /* !KVM_CAP_SET_GUEST_DEBUG */
>   
>   static int kvm_set_signal_mask(CPUState *cpu, const sigset_t *sigset)
> diff --git a/exec.c b/exec.c
> index bb6170dbff..55614822c3 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1233,6 +1233,7 @@ 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);
>               kvm_update_guest_debug(cpu, 0);
>           } else {
>               /* must flush all the translated code to avoid inconsistencies */
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index 97d8d9d0d5..a01a8d58dd 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -259,6 +259,8 @@ int kvm_remove_breakpoint(CPUState *cpu, target_ulong addr,
>   void kvm_remove_all_breakpoints(CPUState *cpu);
>   int kvm_update_guest_debug(CPUState *cpu, unsigned long reinject_trap);
>   
> +void kvm_set_singlestep(CPUState *cpu, int enabled);
> +
>   int kvm_on_sigbus_vcpu(CPUState *cpu, int code, void *addr);
>   int kvm_on_sigbus(int code, void *addr);
>   
> @@ -431,6 +433,8 @@ void kvm_arch_remove_all_hw_breakpoints(void);
>   
>   void kvm_arch_update_guest_debug(CPUState *cpu, struct kvm_guest_debug *dbg);
>   
> +void kvm_arch_set_singlestep(CPUState *cpu, int enabled);
> +
>   bool kvm_arch_stop_on_emulation_error(CPUState *cpu);
>   
>   int kvm_check_extension(KVMState *s, unsigned int extension);
> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> index 44dd0ce6ce..dd8e43ab7e 100644
> --- a/target/arm/kvm.c
> +++ b/target/arm/kvm.c
> @@ -670,6 +670,10 @@ int kvm_arch_process_async_events(CPUState *cs)
>       return 0;
>   }
>   
> +void kvm_arch_set_singlestep(CPUState *cs, int enabled)
> +{
> +}
> +
>   /* The #ifdef protections are until 32bit headers are imported and can
>    * be removed once both 32 and 64 bit reach feature parity.
>    */
> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> index f524e7d929..ba56f2ee1f 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -3521,6 +3521,10 @@ static int kvm_handle_debug(X86CPU *cpu,
>       return ret;
>   }
>   
> +void kvm_arch_set_singlestep(CPUState *cs, int enabled)
> +{
> +}
> +
>   void kvm_arch_update_guest_debug(CPUState *cpu, struct kvm_guest_debug *dbg)
>   {
>       const uint8_t type_code[] = {
> diff --git a/target/mips/kvm.c b/target/mips/kvm.c
> index 8e72850962..8035262131 100644
> --- a/target/mips/kvm.c
> +++ b/target/mips/kvm.c
> @@ -119,6 +119,10 @@ int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
>       return 0;
>   }
>   
> +void kvm_arch_set_singlestep(CPUState *cs, int enabled)
> +{
> +}
> +
>   static inline int cpu_mips_io_interrupts_pending(MIPSCPU *cpu)
>   {
>       CPUMIPSState *env = &cpu->env;
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index f81327d6cd..9d0b4f1f3f 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -1551,6 +1551,10 @@ void kvm_arch_remove_all_hw_breakpoints(void)
>       nb_hw_breakpoint = nb_hw_watchpoint = 0;
>   }
>   
> +void kvm_arch_set_singlestep(CPUState *cs, int enabled)
> +{
> +}
> +
>   void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
>   {
>       int n;
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index 2ebf26adfe..4bde183458 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -975,6 +975,10 @@ void kvm_arch_remove_all_hw_breakpoints(void)
>       hw_breakpoints = NULL;
>   }
>   
> +void kvm_arch_set_singlestep(CPUState *cs, int enabled)
> +{
> +}
> +
>   void kvm_arch_update_guest_debug(CPUState *cpu, struct kvm_guest_debug *dbg)
>   {
>       int i;
> 

Re: [Qemu-devel] [RFC PATCH v2 1/3] target/ppc: Add macro definitions for relocated interrupt vectors offsets

[David Gibson]

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

On Wed, Nov 21, 2018 at 04:13:45PM -0200, Fabiano Rosas wrote:
> The PowerISA prescribes that depending on the values of MSR_IR,
> MSR_DR, MSR_HV and LPCR_AIL, the interrupt vectors might be relocated
> by specific offsets.
> 
> This patch defines macros for these offsets so that they can be used
> by another part of the code in a future patch.
> 
> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
> ---
>  target/ppc/cpu.h         | 3 +++
>  target/ppc/excp_helper.c | 4 ++--
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index ab68abe8a2..5147db4460 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -2390,6 +2390,9 @@ enum {
>      AIL_C000_0000_0000_4000 = 3,
>  };
>  
> +#define AIL_0001_8000_OFFSET 0x18000
> +#define AIL_C000_0000_0000_4000_OFFSET 0xc000000000004000ull

Hrm.  Is there really a point making a #define, if the name spells out
the value?  It's not like you can change the value without having to
change the places that use it that way?


>  /*****************************************************************************/
>  
>  #define is_isa300(ctx) (!!(ctx->insns_flags2 & PPC2_ISA300))
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 0ec7ae1ad4..49bdf7dd54 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -687,10 +687,10 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>          new_msr |= (1 << MSR_IR) | (1 << MSR_DR);
>          switch(ail) {
>          case AIL_0001_8000:
> -            vector |= 0x18000;
> +            vector |= AIL_0001_8000_OFFSET;
>              break;
>          case AIL_C000_0000_0000_4000:
> -            vector |= 0xc000000000004000ull;
> +            vector |= AIL_C000_0000_0000_4000_OFFSET;
>              break;
>          default:
>              cpu_abort(cs, "Invalid AIL combination %d\n", ail);

-- 
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 v2 1/3] target/ppc: Add macro definitions for relocated interrupt vectors offsets

[Fabiano Rosas]

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

> On Wed, Nov 21, 2018 at 04:13:45PM -0200, Fabiano Rosas wrote:
>> The PowerISA prescribes that depending on the values of MSR_IR,
>> MSR_DR, MSR_HV and LPCR_AIL, the interrupt vectors might be relocated
>> by specific offsets.
>> 
>> This patch defines macros for these offsets so that they can be used
>> by another part of the code in a future patch.
>> 
>> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
>> ---
>>  target/ppc/cpu.h         | 3 +++
>>  target/ppc/excp_helper.c | 4 ++--
>>  2 files changed, 5 insertions(+), 2 deletions(-)
>> 
>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
>> index ab68abe8a2..5147db4460 100644
>> --- a/target/ppc/cpu.h
>> +++ b/target/ppc/cpu.h
>> @@ -2390,6 +2390,9 @@ enum {
>>      AIL_C000_0000_0000_4000 = 3,
>>  };
>>  
>> +#define AIL_0001_8000_OFFSET 0x18000
>> +#define AIL_C000_0000_0000_4000_OFFSET 0xc000000000004000ull
>
> Hrm.  Is there really a point making a #define, if the name spells out
> the value?  It's not like you can change the value without having to
> change the places that use it that way?

You're right, this is a bit clumsy.

I just checked and the single step works within SLOF code as well, so
I'll probably need to borrow the AIL-checking logic from excp_helper.c
to get the correct offset so this patch is likely to go away.

Cheers.

Re: [Qemu-devel] [RFC PATCH v2 2/3] kvm-all: Introduce kvm_set_singlestep

[Cornelia Huck]

On Wed, 21 Nov 2018 16:13:46 -0200
Fabiano Rosas <farosas@linux.ibm.com> wrote:

> This will be used in a future patch to implement an
> architecture-specific single step mechanism for POWER.
> 
> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
> ---
>  accel/kvm/kvm-all.c  | 10 ++++++++++
>  exec.c               |  1 +
>  include/sysemu/kvm.h |  4 ++++
>  target/arm/kvm.c     |  4 ++++
>  target/i386/kvm.c    |  4 ++++
>  target/mips/kvm.c    |  4 ++++
>  target/ppc/kvm.c     |  4 ++++
>  target/s390x/kvm.c   |  4 ++++
>  8 files changed, 35 insertions(+)
> 

> @@ -431,6 +433,8 @@ void kvm_arch_remove_all_hw_breakpoints(void);
>  
>  void kvm_arch_update_guest_debug(CPUState *cpu, struct kvm_guest_debug *dbg);
>  
> +void kvm_arch_set_singlestep(CPUState *cpu, int enabled);

Might be useful to add a comment here that describes what common code
expects the arch-specific function to do here so they don't step on
each others toes.

> +
>  bool kvm_arch_stop_on_emulation_error(CPUState *cpu);
>  
>  int kvm_check_extension(KVMState *s, unsigned int extension);

Re: [Qemu-devel] [RFC PATCH v2 2/3] kvm-all: Introduce kvm_set_singlestep

[David Gibson]

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

On Wed, Nov 21, 2018 at 04:13:46PM -0200, Fabiano Rosas wrote:
> This will be used in a future patch to implement an
> architecture-specific single step mechanism for POWER.
> 
> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
> ---
>  accel/kvm/kvm-all.c  | 10 ++++++++++
>  exec.c               |  1 +
>  include/sysemu/kvm.h |  4 ++++
>  target/arm/kvm.c     |  4 ++++
>  target/i386/kvm.c    |  4 ++++
>  target/mips/kvm.c    |  4 ++++
>  target/ppc/kvm.c     |  4 ++++
>  target/s390x/kvm.c   |  4 ++++
>  8 files changed, 35 insertions(+)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 4880a05399..4fb7199a15 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -2313,6 +2313,11 @@ int kvm_update_guest_debug(CPUState *cpu, unsigned long reinject_trap)
>      return data.err;
>  }
>  
> +void kvm_set_singlestep(CPUState *cs, int enabled)
> +{
> +    kvm_arch_set_singlestep(cs, enabled);
> +}
> +
>  int kvm_insert_breakpoint(CPUState *cpu, target_ulong addr,
>                            target_ulong len, int type)
>  {
> @@ -2439,6 +2444,11 @@ int kvm_remove_breakpoint(CPUState *cpu, target_ulong addr,
>  void kvm_remove_all_breakpoints(CPUState *cpu)
>  {
>  }
> +
> +void kvm_set_singlestep(CPUState *cs, int enabled)
> +{
> +}

You could use stubs to avoid having to put this empty implementation
in every arch.

It also seems like it might be a good idea to report an error here,
rather than having set single step silently do nothing on arches which
don't support it yet.

>  #endif /* !KVM_CAP_SET_GUEST_DEBUG */
>  
>  static int kvm_set_signal_mask(CPUState *cpu, const sigset_t *sigset)
> diff --git a/exec.c b/exec.c
> index bb6170dbff..55614822c3 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1233,6 +1233,7 @@ 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);
>              kvm_update_guest_debug(cpu, 0);
>          } else {
>              /* must flush all the translated code to avoid inconsistencies */
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index 97d8d9d0d5..a01a8d58dd 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -259,6 +259,8 @@ int kvm_remove_breakpoint(CPUState *cpu, target_ulong addr,
>  void kvm_remove_all_breakpoints(CPUState *cpu);
>  int kvm_update_guest_debug(CPUState *cpu, unsigned long reinject_trap);
>  
> +void kvm_set_singlestep(CPUState *cpu, int enabled);
> +
>  int kvm_on_sigbus_vcpu(CPUState *cpu, int code, void *addr);
>  int kvm_on_sigbus(int code, void *addr);
>  
> @@ -431,6 +433,8 @@ void kvm_arch_remove_all_hw_breakpoints(void);
>  
>  void kvm_arch_update_guest_debug(CPUState *cpu, struct kvm_guest_debug *dbg);
>  
> +void kvm_arch_set_singlestep(CPUState *cpu, int enabled);
> +
>  bool kvm_arch_stop_on_emulation_error(CPUState *cpu);
>  
>  int kvm_check_extension(KVMState *s, unsigned int extension);
> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> index 44dd0ce6ce..dd8e43ab7e 100644
> --- a/target/arm/kvm.c
> +++ b/target/arm/kvm.c
> @@ -670,6 +670,10 @@ int kvm_arch_process_async_events(CPUState *cs)
>      return 0;
>  }
>  
> +void kvm_arch_set_singlestep(CPUState *cs, int enabled)
> +{
> +}
> +
>  /* The #ifdef protections are until 32bit headers are imported and can
>   * be removed once both 32 and 64 bit reach feature parity.
>   */
> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> index f524e7d929..ba56f2ee1f 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -3521,6 +3521,10 @@ static int kvm_handle_debug(X86CPU *cpu,
>      return ret;
>  }
>  
> +void kvm_arch_set_singlestep(CPUState *cs, int enabled)
> +{
> +}
> +
>  void kvm_arch_update_guest_debug(CPUState *cpu, struct kvm_guest_debug *dbg)
>  {
>      const uint8_t type_code[] = {
> diff --git a/target/mips/kvm.c b/target/mips/kvm.c
> index 8e72850962..8035262131 100644
> --- a/target/mips/kvm.c
> +++ b/target/mips/kvm.c
> @@ -119,6 +119,10 @@ int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
>      return 0;
>  }
>  
> +void kvm_arch_set_singlestep(CPUState *cs, int enabled)
> +{
> +}
> +
>  static inline int cpu_mips_io_interrupts_pending(MIPSCPU *cpu)
>  {
>      CPUMIPSState *env = &cpu->env;
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index f81327d6cd..9d0b4f1f3f 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -1551,6 +1551,10 @@ void kvm_arch_remove_all_hw_breakpoints(void)
>      nb_hw_breakpoint = nb_hw_watchpoint = 0;
>  }
>  
> +void kvm_arch_set_singlestep(CPUState *cs, int enabled)
> +{
> +}
> +
>  void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
>  {
>      int n;
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index 2ebf26adfe..4bde183458 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -975,6 +975,10 @@ void kvm_arch_remove_all_hw_breakpoints(void)
>      hw_breakpoints = NULL;
>  }
>  
> +void kvm_arch_set_singlestep(CPUState *cs, int enabled)
> +{
> +}
> +
>  void kvm_arch_update_guest_debug(CPUState *cpu, struct kvm_guest_debug *dbg)
>  {
>      int i;

-- 
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 v2 3/3] target/ppc: support single stepping with KVM HV

[David Gibson]

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

On Wed, Nov 21, 2018 at 04:13:47PM -0200, 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, 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).
> 
> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
> ---
>  target/ppc/kvm.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 60 insertions(+), 1 deletion(-)
> 
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 9d0b4f1f3f..93c20099e6 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -94,6 +94,7 @@ static int cap_ppc_safe_indirect_branch;
>  static int cap_ppc_nested_kvm_hv;
>  
>  static uint32_t debug_inst_opcode;
> +static target_ulong trace_handler_addr;
>  
>  /* XXX We have a race condition where we actually have a level triggered
>   *     interrupt, but the infrastructure can't expose that yet, so the guest
> @@ -509,6 +510,9 @@ int kvm_arch_init_vcpu(CPUState *cs)
>      kvm_get_one_reg(cs, KVM_REG_PPC_DEBUG_INST, &debug_inst_opcode);
>      kvmppc_hw_debug_points_init(cenv);
>  
> +    trace_handler_addr = (cenv->excp_vectors[POWERPC_EXCP_TRACE] |
> +                          AIL_C000_0000_0000_4000_OFFSET);

Effectively caching this as a global variable is pretty horrible.  A
function to make the above calculation when you need it would be
better.

Also, won't the calculation above be wrong if the guest is using the
low AIL value?

> +
>      return ret;
>  }
>  
> @@ -1553,6 +1557,24 @@ void kvm_arch_remove_all_hw_breakpoints(void)
>  
>  void kvm_arch_set_singlestep(CPUState *cs, int enabled)
>  {
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +    CPUPPCState *env = &cpu->env;
> +
> +    if (kvmppc_is_pr(kvm_state)) {

Explicitly testing for KVM PR is generally wrong (except as a
fallback).  Instead you should add a capability flag to KVM which you
use to test the feature's presence in qemu.  That capability can then
be set in HV, but not PR.

> +            return;
> +    }
> +
> +    if (enabled) {
> +        /* MSR_SE = 1 will cause a Trace Interrupt in the guest after
> +         * the next instruction executes. */
> +        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. */
> +        kvm_insert_breakpoint(cs, trace_handler_addr, 4, GDB_BREAKPOINT_SW);
> +    }
>  }
>  
>  void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
> @@ -1594,6 +1616,43 @@ void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
>      }
>  }
>  
> +static int kvm_handle_singlestep(CPUState *cs,
> +                                 struct kvm_debug_exit_arch *arch_info)
> +{
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +    CPUPPCState *env = &cpu->env;
> +    target_ulong msr = env->msr;
> +    uint32_t insn;
> +    int ret = 1;

You never set this to anything other than 1...

> +    int reg;
> +
> +    if (kvmppc_is_pr(kvm_state)) {
> +        return ret;
> +    }
> +
> +    if (arch_info->address == trace_handler_addr) {
> +        cpu_synchronize_state(cs);
> +        kvm_remove_breakpoint(cs, trace_handler_addr, 4, GDB_BREAKPOINT_SW);
> +
> +        cpu_memory_rw_debug(cs, env->spr[SPR_SRR0] - 4, (uint8_t *)&insn,
> +                            sizeof(insn), 0);
> +
> +        /* If the last instruction was a mfmsr, make sure that the
> +         * MSR_SE bit is not set to avoid the guest kernel knowing
> +         * that it is being single-stepped */
> +        if (extract32(insn, 26, 6) == 31 && extract32(insn, 1, 10) == 83) {
> +            reg = extract32(insn, 21, 5);
> +            env->gpr[reg] &= ~(1ULL << MSR_SE);
> +        }

Hm.  What happens if both qemu and the guest itself attempt to single
step at the same time?  How do you distinguish between the cases?

> +
> +        env->nip = env->spr[SPR_SRR0];
> +        env->msr = msr &= ~(1ULL << MSR_SE);

You clear SE after tracing one instruction; is that intentional?

> +        cpu_synchronize_state(cs);

You're effectively aborting the entry to the single step exception
vector; don't you need to set MSR to SRR1, not to the current MSR
value which will have been modified for the singlestep exception entry?

You only need the first call to sync_state.  It sets a flag causing
the state to be written back before returning to the guest.

> +    }
> +
> +    return ret;
> +}
> +
>  static int kvm_handle_debug(PowerPCCPU *cpu, struct kvm_run *run)
>  {
>      CPUState *cs = CPU(cpu);
> @@ -1604,7 +1663,7 @@ static int kvm_handle_debug(PowerPCCPU *cpu, struct kvm_run *run)
>      int flag = 0;
>  
>      if (cs->singlestep_enabled) {
> -        handle = 1;
> +        handle = kvm_handle_singlestep(cs, arch_info);
>      } else if (arch_info->status) {
>          if (nb_hw_breakpoint + nb_hw_watchpoint > 0) {
>              if (arch_info->status & KVMPPC_DEBUG_BREAKPOINT) {

-- 
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 v2 3/3] target/ppc: support single stepping with KVM HV

[Fabiano Rosas]

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

>> --- a/target/ppc/kvm.c
>> +++ b/target/ppc/kvm.c
>> @@ -94,6 +94,7 @@ static int cap_ppc_safe_indirect_branch;
>>  static int cap_ppc_nested_kvm_hv;
>>  
>>  static uint32_t debug_inst_opcode;
>> +static target_ulong trace_handler_addr;
>>  
>>  /* XXX We have a race condition where we actually have a level triggered
>>   *     interrupt, but the infrastructure can't expose that yet, so the guest
>> @@ -509,6 +510,9 @@ int kvm_arch_init_vcpu(CPUState *cs)
>>      kvm_get_one_reg(cs, KVM_REG_PPC_DEBUG_INST, &debug_inst_opcode);
>>      kvmppc_hw_debug_points_init(cenv);
>>  
>> +    trace_handler_addr = (cenv->excp_vectors[POWERPC_EXCP_TRACE] |
>> +                          AIL_C000_0000_0000_4000_OFFSET);
>
> Effectively caching this as a global variable is pretty horrible.  A
> function to make the above calculation when you need it would be
> better.
>
> Also, won't the calculation above be wrong if the guest is using the
> low AIL value?

Yes. As you suggested, I'll change this to calculate the offset based on AIL.

>>  void kvm_arch_set_singlestep(CPUState *cs, int enabled)
>>  {
>> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
>> +    CPUPPCState *env = &cpu->env;
>> +
>> +    if (kvmppc_is_pr(kvm_state)) {
>
> Explicitly testing for KVM PR is generally wrong (except as a
> fallback).  Instead you should add a capability flag to KVM which you
> use to test the feature's presence in qemu.  That capability can then
> be set in HV, but not PR.

My apologies, I did read the notice on the code about it. I was trying
to keep things simple for this RFC and forgot to mention that in the
cover letter.

>> +    if (arch_info->address == trace_handler_addr) {
>> +        cpu_synchronize_state(cs);
>> +        kvm_remove_breakpoint(cs, trace_handler_addr, 4, GDB_BREAKPOINT_SW);
>> +
>> +        cpu_memory_rw_debug(cs, env->spr[SPR_SRR0] - 4, (uint8_t *)&insn,
>> +                            sizeof(insn), 0);
>> +
>> +        /* If the last instruction was a mfmsr, make sure that the
>> +         * MSR_SE bit is not set to avoid the guest kernel knowing
>> +         * that it is being single-stepped */
>> +        if (extract32(insn, 26, 6) == 31 && extract32(insn, 1, 10) == 83) {
>> +            reg = extract32(insn, 21, 5);
>> +            env->gpr[reg] &= ~(1ULL << MSR_SE);
>> +        }
>
> Hm.  What happens if both qemu and the guest itself attempt to single
> step at the same time?  How do you distinguish between the cases?

There is currently no distinction being made. Since MSR_SE is only set
for the duration of the step (see my answer below) this mostly just
works and it is possible to single step inside/outside the guest while
the control changes from one debugger to the other as expected. However
there is a race as follows:

If the vcpu being stepped is preempted right after vm enter, with the
breakpoint at 0xd00 already in place *and* the other debugger starts
doing a step, it will cause a vm exit when it shouldn't:

    outside guest                  inside guest
set breakpoint at 0xd00     |
msr_se = 1                  |
vm enter                    |
                            |  msr_se = 1
                            |  exec next instruction
                            |  trace interrupt
                            |  jump to 0xd00
                            |  --- wrong from now on ---
                            |  emulation assist interrupt
                            |  vm exit

One way to avoid this race is by using GDB's `scheduler-locking step`
option which effectively negates the issue by running only the vcpu
being stepped.

Regardless of that workaround, I am exploring the idea of sending the
cpu back into the guest if we happen to break at the trace interrupt
handler while there is no cpu being stepped by QEMU
(cs->singlestep_enabled). That way, a step inside the guest would never
happen before the outside step has finished.

>
>> +
>> +        env->nip = env->spr[SPR_SRR0];
>> +        env->msr = msr &= ~(1ULL << MSR_SE);
>
> You clear SE after tracing one instruction; is that intentional?

Yes, both the kernel and KVM PR do the same:

from arch/powerpc/kernel/traps.c
void single_step_exception(struct pt_regs *regs)
{
        enum ctx_state prev_state = exception_enter();

->      clear_single_step(regs);
        ...
}

from arch/powerpc/kvm/book3s_pr.c
static int kvmppc_vcpu_run_pr(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
{
        ...
        ret = __kvmppc_vcpu_run(kvm_run, vcpu);

->      kvmppc_clear_debug(vcpu);
        ...
}

>
>> +        cpu_synchronize_state(cs);
>
> You're effectively aborting the entry to the single step exception
> vector; don't you need to set MSR to SRR1, not to the current MSR
> value which will have been modified for the singlestep exception entry?

I'm using the old MSR value (before synchronizing) so the effect is the same:

(gdb) p/x $msr
$2 = 0x8000000002803033
(gdb) si
1892            LOAD_REG_ADDR(r11, replay_interrupt_return)
(gdb) p/x $msr
$3 = 0x8000000002803033

Using SRR1 would also work but it would require clearing the bits set by
the interrupt (33:36, 44:47):

(gdb) p/x env->spr[0x01b]
$1 = 0x8000000042803433

I could do the latter, if you prefer.


Thank you for your attention.
Cheers.

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

[David Gibson]

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

On Fri, Nov 30, 2018 at 06:46:21PM -0200, Fabiano Rosas wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
> 
> >> --- a/target/ppc/kvm.c
> >> +++ b/target/ppc/kvm.c
> >> @@ -94,6 +94,7 @@ static int cap_ppc_safe_indirect_branch;
> >>  static int cap_ppc_nested_kvm_hv;
> >>  
> >>  static uint32_t debug_inst_opcode;
> >> +static target_ulong trace_handler_addr;
> >>  
> >>  /* XXX We have a race condition where we actually have a level triggered
> >>   *     interrupt, but the infrastructure can't expose that yet, so the guest
> >> @@ -509,6 +510,9 @@ int kvm_arch_init_vcpu(CPUState *cs)
> >>      kvm_get_one_reg(cs, KVM_REG_PPC_DEBUG_INST, &debug_inst_opcode);
> >>      kvmppc_hw_debug_points_init(cenv);
> >>  
> >> +    trace_handler_addr = (cenv->excp_vectors[POWERPC_EXCP_TRACE] |
> >> +                          AIL_C000_0000_0000_4000_OFFSET);
> >
> > Effectively caching this as a global variable is pretty horrible.  A
> > function to make the above calculation when you need it would be
> > better.
> >
> > Also, won't the calculation above be wrong if the guest is using the
> > low AIL value?
> 
> Yes. As you suggested, I'll change this to calculate the offset based on AIL.
> 
> >>  void kvm_arch_set_singlestep(CPUState *cs, int enabled)
> >>  {
> >> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> >> +    CPUPPCState *env = &cpu->env;
> >> +
> >> +    if (kvmppc_is_pr(kvm_state)) {
> >
> > Explicitly testing for KVM PR is generally wrong (except as a
> > fallback).  Instead you should add a capability flag to KVM which you
> > use to test the feature's presence in qemu.  That capability can then
> > be set in HV, but not PR.
> 
> My apologies, I did read the notice on the code about it. I was trying
> to keep things simple for this RFC and forgot to mention that in the
> cover letter.

Just put a comment on it explaining that you expect to replace this
before the final version.

> >> +    if (arch_info->address == trace_handler_addr) {
> >> +        cpu_synchronize_state(cs);
> >> +        kvm_remove_breakpoint(cs, trace_handler_addr, 4, GDB_BREAKPOINT_SW);
> >> +
> >> +        cpu_memory_rw_debug(cs, env->spr[SPR_SRR0] - 4, (uint8_t *)&insn,
> >> +                            sizeof(insn), 0);
> >> +
> >> +        /* If the last instruction was a mfmsr, make sure that the
> >> +         * MSR_SE bit is not set to avoid the guest kernel knowing
> >> +         * that it is being single-stepped */
> >> +        if (extract32(insn, 26, 6) == 31 && extract32(insn, 1, 10) == 83) {
> >> +            reg = extract32(insn, 21, 5);
> >> +            env->gpr[reg] &= ~(1ULL << MSR_SE);
> >> +        }
> >
> > Hm.  What happens if both qemu and the guest itself attempt to single
> > step at the same time?  How do you distinguish between the cases?
> 
> There is currently no distinction being made.

This seems incorrect to me.  Basically you're restoring !MSR_SE
unconditionaly when you finish the hypervisor side step, which might
not be correct if the guest is also attempting to single step itself.
AFAICT it should be possible to track what the guest thinks is the
value of MSR_SE and restore that.  If both hypervisor and guest
attempt to single step, I'd expect to have the hypervisor trap first,
then return to the guest's single step vector.

> Since MSR_SE is only set
> for the duration of the step (see my answer below) this mostly just
> works and it is possible to single step inside/outside the guest while
> the control changes from one debugger to the other as expected. However
> there is a race as follows:
> 
> If the vcpu being stepped is preempted right after vm enter, with the
> breakpoint at 0xd00 already in place *and* the other debugger starts
> doing a step, it will cause a vm exit when it shouldn't:
> 
>     outside guest                  inside guest
> set breakpoint at 0xd00     |
> msr_se = 1                  |
> vm enter                    |
>                             |  msr_se = 1
>                             |  exec next instruction
>                             |  trace interrupt
>                             |  jump to 0xd00
>                             |  --- wrong from now on ---
>                             |  emulation assist interrupt
>                             |  vm exit
> 
> One way to avoid this race is by using GDB's `scheduler-locking step`
> option which effectively negates the issue by running only the vcpu
> being stepped.
> 
> Regardless of that workaround, I am exploring the idea of sending the
> cpu back into the guest if we happen to break at the trace interrupt
> handler while there is no cpu being stepped by QEMU
> (cs->singlestep_enabled). That way, a step inside the guest would never
> happen before the outside step has finished.
> 
> >
> >> +
> >> +        env->nip = env->spr[SPR_SRR0];
> >> +        env->msr = msr &= ~(1ULL << MSR_SE);
> >
> > You clear SE after tracing one instruction; is that intentional?
> 
> Yes, both the kernel and KVM PR do the same:
> 
> from arch/powerpc/kernel/traps.c
> void single_step_exception(struct pt_regs *regs)
> {
>         enum ctx_state prev_state = exception_enter();
> 
> ->      clear_single_step(regs);
>         ...
> }
> 
> from arch/powerpc/kvm/book3s_pr.c
> static int kvmppc_vcpu_run_pr(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
> {
>         ...
>         ret = __kvmppc_vcpu_run(kvm_run, vcpu);
> 
> ->      kvmppc_clear_debug(vcpu);
>         ...
> }
> 
> >
> >> +        cpu_synchronize_state(cs);
> >
> > You're effectively aborting the entry to the single step exception
> > vector; don't you need to set MSR to SRR1, not to the current MSR
> > value which will have been modified for the singlestep exception entry?
> 
> I'm using the old MSR value (before synchronizing) so the effect is the same:
> 
> (gdb) p/x $msr
> $2 = 0x8000000002803033
> (gdb) si
> 1892            LOAD_REG_ADDR(r11, replay_interrupt_return)
> (gdb) p/x $msr
> $3 = 0x8000000002803033
> 
> Using SRR1 would also work but it would require clearing the bits set by
> the interrupt (33:36, 44:47):
> 
> (gdb) p/x env->spr[0x01b]
> $1 = 0x8000000042803433
> 
> I could do the latter, if you prefer.

I think that's better - I don't think it's safe to assume that you're
*not* synchronized with KVM.

-- 
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 v2 3/3] target/ppc: support single stepping with KVM HV

[Fabiano Rosas]

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

>> >> +    if (arch_info->address == trace_handler_addr) {
>> >> +        cpu_synchronize_state(cs);
>> >> +        kvm_remove_breakpoint(cs, trace_handler_addr, 4, GDB_BREAKPOINT_SW);
>> >> +
>> >> +        cpu_memory_rw_debug(cs, env->spr[SPR_SRR0] - 4, (uint8_t *)&insn,
>> >> +                            sizeof(insn), 0);
>> >> +
>> >> +        /* If the last instruction was a mfmsr, make sure that the
>> >> +         * MSR_SE bit is not set to avoid the guest kernel knowing
>> >> +         * that it is being single-stepped */
>> >> +        if (extract32(insn, 26, 6) == 31 && extract32(insn, 1, 10) == 83) {
>> >> +            reg = extract32(insn, 21, 5);
>> >> +            env->gpr[reg] &= ~(1ULL << MSR_SE);
>> >> +        }
>> >
>> > Hm.  What happens if both qemu and the guest itself attempt to single
>> > step at the same time?  How do you distinguish between the cases?
>> 
>> There is currently no distinction being made.
>
> This seems incorrect to me.  Basically you're restoring !MSR_SE
> unconditionaly when you finish the hypervisor side step, which might
> not be correct if the guest is also attempting to single step itself.
> AFAICT it should be possible to track what the guest thinks is the
> value of MSR_SE and restore that.

I was skeptical of being able to do both single steps at the same time
but I found a way to reproduce it by stepping over an rfid when
SRR1_SE is already 1.

>  If both hypervisor and guest
> attempt to single step, I'd expect to have the hypervisor trap first,
> then return to the guest's single step vector.

With the fix you suggest above, QEMU will be able to single step the
interrupt handler during the guest's single step. That means I'll have
to restore the SRRs as well so that the handler returns to the correct
place.

>> I could do the latter, if you prefer.
>
> I think that's better - I don't think it's safe to assume that you're
> *not* synchronized with KVM.

Ok, that's better indeed.

Thanks for the comments, I'll prepare another version with the
appropriate corrections.

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

[David Gibson]

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

On Mon, Dec 10, 2018 at 10:52:18AM -0200, Fabiano Rosas wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
> 
> >> >> +    if (arch_info->address == trace_handler_addr) {
> >> >> +        cpu_synchronize_state(cs);
> >> >> +        kvm_remove_breakpoint(cs, trace_handler_addr, 4, GDB_BREAKPOINT_SW);
> >> >> +
> >> >> +        cpu_memory_rw_debug(cs, env->spr[SPR_SRR0] - 4, (uint8_t *)&insn,
> >> >> +                            sizeof(insn), 0);
> >> >> +
> >> >> +        /* If the last instruction was a mfmsr, make sure that the
> >> >> +         * MSR_SE bit is not set to avoid the guest kernel knowing
> >> >> +         * that it is being single-stepped */
> >> >> +        if (extract32(insn, 26, 6) == 31 && extract32(insn, 1, 10) == 83) {
> >> >> +            reg = extract32(insn, 21, 5);
> >> >> +            env->gpr[reg] &= ~(1ULL << MSR_SE);
> >> >> +        }
> >> >
> >> > Hm.  What happens if both qemu and the guest itself attempt to single
> >> > step at the same time?  How do you distinguish between the cases?
> >> 
> >> There is currently no distinction being made.
> >
> > This seems incorrect to me.  Basically you're restoring !MSR_SE
> > unconditionaly when you finish the hypervisor side step, which might
> > not be correct if the guest is also attempting to single step itself.
> > AFAICT it should be possible to track what the guest thinks is the
> > value of MSR_SE and restore that.
> 
> I was skeptical of being able to do both single steps at the same time
> but I found a way to reproduce it by stepping over an rfid when
> SRR1_SE is already 1.
> 
> >  If both hypervisor and guest
> > attempt to single step, I'd expect to have the hypervisor trap first,
> > then return to the guest's single step vector.
> 
> With the fix you suggest above, QEMU will be able to single step the
> interrupt handler during the guest's single step. That means I'll have
> to restore the SRRs as well so that the handler returns to the correct
> place.
> 
> >> I could do the latter, if you prefer.
> >
> > I think that's better - I don't think it's safe to assume that you're
> > *not* synchronized with KVM.
> 
> Ok, that's better indeed.
> 
> Thanks for the comments, I'll prepare another version with the
> appropriate corrections.

Ok, great.

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