[Qemu-devel] [PATCH 0/5]

[Qemu-devel] [PATCH 0/5]

[Liu Yu]

The whole patchset includes:
patch 1: fix kvmppc build error
patch 2: fix kvmppc init error
patch 3~5: add kvmppc guest debug support

The guest debug still have some problems I haven't solved.

1. gdb 'next' command uses software breakpoint
software breakpoint is implemented via modify guest's code.
In most case it works well,
but when used by 'next' it's easy to make trouble on powerpc booke.

For example booke has a code template for
jumping to and returning from interrupt handlers:

	bl transfer
	.long handler_addr
	.long ret_addr

when call transfer, it never return but
in transfer assembly code it will read the handler_addr
and ultimately call the handler.
Gdb doesn't know that and treat it as a normal function call.
so gdb put a software breakpoint instruction at handler_addr,
in order to get trap there when return from transfer.

Then guest will read software breakpoint as handler_addr and jump to there..

I'm not sure if x86 suffer this kind of issue.
Is there any way to avoid this?


2. gdb 'watch' command
Jan told me gdb>6.8 can issue hardware watchpoint request via command 'watch',
my gdb is 6.8.50.20080821-cvs and our toolchain provider confirm that it supports hardware watch
However when I use 'watch', I can only see single step from gdbstub side.
Did I miss anything?

[Qemu-devel] [PATCH 1/5] Fix kvmppc build error

[Liu Yu]

like this:
/home/liuyu/git/qemu.git/target-ppc/kvm_ppc.c: In function 'kvmppc_read_host_property':
/home/liuyu/git/qemu.git/target-ppc/kvm_ppc.c:55: error: label 'out' defined but not used

Signed-off-by: Liu Yu <yu.liu@freescale.com>
---
 target-ppc/kvm_ppc.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/target-ppc/kvm_ppc.c b/target-ppc/kvm_ppc.c
index 10cfdb3..be47469 100644
--- a/target-ppc/kvm_ppc.c
+++ b/target-ppc/kvm_ppc.c
@@ -52,7 +52,6 @@ close:
     fclose(f);
 free:
     free(path);
-out:
     return ret;
 }
 
-- 
1.5.4

[Qemu-devel] [PATCH 2/5] Fix booke registers init

[Liu Yu]

Commit 8d2ba1fb9c8e7006e10d71fa51a020977f14c8b0
introduces a new new reset order.

So that we have to synchronize registers explicitly.

Signed-off-by: Liu Yu <yu.liu@freescale.com>
---
 hw/ppc440_bamboo.c     |    4 +++-
 hw/ppce500_mpc8544ds.c |    4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/ppc440_bamboo.c b/hw/ppc440_bamboo.c
index d9ef3ec..f1ba130 100644
--- a/hw/ppc440_bamboo.c
+++ b/hw/ppc440_bamboo.c
@@ -182,8 +182,10 @@ static void bamboo_init(ram_addr_t ram_size,
         /* XXX we currently depend on KVM to create some initial TLB entries. */
     }
 
-    if (kvm_enabled())
+    if (kvm_enabled()) {
+        kvm_arch_put_registers(env);
         kvmppc_init();
+    }
 }
 
 static QEMUMachine bamboo_machine = {
diff --git a/hw/ppce500_mpc8544ds.c b/hw/ppce500_mpc8544ds.c
index c0e367d..f1b3c1a 100644
--- a/hw/ppce500_mpc8544ds.c
+++ b/hw/ppce500_mpc8544ds.c
@@ -276,8 +276,10 @@ static void mpc8544ds_init(ram_addr_t ram_size,
         /* XXX we currently depend on KVM to create some initial TLB entries. */
     }
 
-    if (kvm_enabled())
+    if (kvm_enabled()) {
+        kvm_arch_put_registers(env);
         kvmppc_init();
+    }
 
     return;
 }
-- 
1.5.4

[Qemu-devel] [PATCH 3/5] Add guest debug support for kvmppc

[Liu Yu]

Signed-off-by: Liu Yu <yu.liu@freescale.com>
---
 target-ppc/kvm.c |  197 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 197 insertions(+), 0 deletions(-)

diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index b53d6e9..d8dbdb4 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -8,6 +8,9 @@
  *  Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
  *  Hollis Blanchard <hollisb@us.ibm.com>
  *
+ * Copyright (C) 2009 Freescale Semiconductor, Inc. All rights reserved.
+ *  Yu Liu <yu.liu@freescale.com>
+ *
  * This work is licensed under the terms of the GNU GPL, version 2 or later.
  * See the COPYING file in the top-level directory.
  *
@@ -18,6 +21,7 @@
 #include <sys/mman.h>
 
 #include <linux/kvm.h>
+#include <asm/kvm_asm.h>
 
 #include "qemu-common.h"
 #include "qemu-timer.h"
@@ -26,6 +30,7 @@
 #include "kvm_ppc.h"
 #include "cpu.h"
 #include "device_tree.h"
+#include "gdbstub.h"
 
 //#define DEBUG_KVM
 
@@ -216,3 +221,195 @@ int kvm_arch_handle_exit(CPUState *env, struct kvm_run *run)
     return ret;
 }
 
+#ifdef KVM_CAP_SET_GUEST_DEBUG
+int kvm_arch_insert_sw_breakpoint(CPUState *env, struct kvm_sw_breakpoint *bp)
+{
+    uint32_t sc = tswap32(KVM_INST_GUESTGDB);
+    uint32_t tmp;
+
+    if (cpu_memory_rw_debug(env, bp->pc, (uint8_t *)&bp->saved_insn, 4, 0) ||
+        cpu_memory_rw_debug(env, bp->pc, (uint8_t *)&sc, 4, 1))
+        return -EINVAL;
+    cpu_memory_rw_debug(env, bp->pc, (uint8_t *)&tmp, 4, 0);
+    return 0;
+}
+
+int kvm_arch_remove_sw_breakpoint(CPUState *env, struct kvm_sw_breakpoint *bp)
+{
+    uint32_t sc;
+
+    if (cpu_memory_rw_debug(env, bp->pc, (uint8_t *)&sc, 4, 0) ||
+        sc != tswap32(KVM_INST_GUESTGDB) ||
+        cpu_memory_rw_debug(env, bp->pc, (uint8_t *)&bp->saved_insn, 4, 1))
+        return -EINVAL;
+    return 0;
+}
+
+static struct {
+    target_ulong addr;
+    int type;
+} hw_breakpoint[6];
+
+static int nb_hw_breakpoint;
+static int nb_hw_watchpoint;
+static int max_hw_breakpoint;
+static int max_hw_watchpoint;
+
+void kvmppc_debug_init(int max_hw_bp, int max_hw_wp)
+{
+    max_hw_breakpoint = max_hw_bp > 4? 4 : max_hw_bp;
+    max_hw_watchpoint = max_hw_wp > 2? 2 : max_hw_wp;
+}
+
+static int find_hw_breakpoint(target_ulong addr, int type)
+{
+    int n;
+
+    for (n = 0; n < nb_hw_breakpoint + nb_hw_watchpoint; n++)
+        if (hw_breakpoint[n].addr == addr && hw_breakpoint[n].type == type)
+            return n;
+    return -1;
+}
+
+int kvm_arch_insert_hw_breakpoint(target_ulong addr,
+                                  target_ulong len, int type)
+{
+    hw_breakpoint[nb_hw_breakpoint + nb_hw_watchpoint].addr = addr;
+    hw_breakpoint[nb_hw_breakpoint + nb_hw_watchpoint].type = type;
+
+    switch (type) {
+    case GDB_BREAKPOINT_HW:
+        if (nb_hw_breakpoint >= max_hw_breakpoint)
+            return -ENOBUFS;
+
+        if (find_hw_breakpoint(addr, type) >= 0)
+            return -EEXIST;
+
+        nb_hw_breakpoint++;
+        break;
+
+    case GDB_WATCHPOINT_WRITE:
+    case GDB_WATCHPOINT_ACCESS:
+        if (nb_hw_watchpoint >= max_hw_watchpoint)
+            return -ENOBUFS;
+
+        if (find_hw_breakpoint(addr, type) >= 0)
+            return -EEXIST;
+
+        nb_hw_watchpoint++;
+        break;
+
+    default:
+        return -ENOSYS;
+    }
+
+    return 0;
+}
+
+int kvm_arch_remove_hw_breakpoint(target_ulong addr,
+                                  target_ulong len, int type)
+{
+    int n;
+
+    n = find_hw_breakpoint(addr, type);
+    if (n < 0)
+        return -ENOENT;
+
+    switch (type) {
+    case GDB_BREAKPOINT_HW:
+        nb_hw_breakpoint--;
+        break;
+
+    case GDB_WATCHPOINT_WRITE:
+    case GDB_WATCHPOINT_ACCESS:
+        nb_hw_watchpoint--;
+        break;
+
+    default:
+        return -ENOSYS;
+    }
+    hw_breakpoint[n] = hw_breakpoint[nb_hw_breakpoint + nb_hw_watchpoint];
+
+    return 0;
+}
+
+void kvm_arch_remove_all_hw_breakpoints(void)
+{
+    nb_hw_breakpoint = nb_hw_watchpoint = 0;
+}
+
+static CPUWatchpoint hw_watchpoint;
+
+int kvm_arch_debug(struct kvm_debug_exit_arch *arch_info)
+{
+    int handle = 0;
+    int n;
+
+    if (cpu_single_env->singlestep_enabled) {
+        handle = 1;
+
+    } else if (arch_info->status) {
+        if (arch_info->status == KVMPPC_DEBUG_BREAKPOINT) {
+            n = find_hw_breakpoint(arch_info->pc, GDB_BREAKPOINT_HW);
+            if (n >= 0)
+                handle = 1;
+
+        } else if (arch_info->status == KVMPPC_DEBUG_WATCH_ACCESS) {
+            n = find_hw_breakpoint(arch_info->pc, GDB_WATCHPOINT_ACCESS);
+            if (n >= 0) {
+                handle = 1;
+                cpu_single_env->watchpoint_hit = &hw_watchpoint;
+                hw_watchpoint.vaddr = hw_breakpoint[n].addr;
+                hw_watchpoint.flags = BP_MEM_ACCESS;
+            }
+
+        } else if (arch_info->status == KVMPPC_DEBUG_WATCH_WRITE) {
+            n = find_hw_breakpoint(arch_info->pc, GDB_WATCHPOINT_WRITE);
+            if (n >= 0) {
+                handle = 1;
+                cpu_single_env->watchpoint_hit = &hw_watchpoint;
+                hw_watchpoint.vaddr = hw_breakpoint[n].addr;
+                hw_watchpoint.flags = BP_MEM_WRITE;
+            }
+        }
+
+    } else if (kvm_find_sw_breakpoint(cpu_single_env, arch_info->pc))
+        handle = 1;
+
+    /* XXX inject guest debug exception */
+    if (!handle)
+        printf("Unhandled debug exception!\n");
+
+    return handle;
+}
+
+void kvm_arch_update_guest_debug(CPUState *env, struct kvm_guest_debug *dbg)
+{
+    if (kvm_sw_breakpoints_active(env))
+        dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;
+
+    if (nb_hw_breakpoint + nb_hw_watchpoint > 0) {
+        int n;
+
+        dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_HW_BP;
+        memset(dbg->arch.bp, 0, sizeof(dbg->arch.bp));
+        for (n = 0; n < nb_hw_breakpoint + nb_hw_watchpoint; n++) {
+            switch (hw_breakpoint[n].type) {
+            case GDB_BREAKPOINT_HW:
+                dbg->arch.bp[n].type = KVMPPC_DEBUG_BREAKPOINT;
+                break;
+            case GDB_WATCHPOINT_ACCESS:
+                dbg->arch.bp[n].type = KVMPPC_DEBUG_WATCH_ACCESS;
+                break;
+            case GDB_WATCHPOINT_WRITE:
+                dbg->arch.bp[n].type = KVMPPC_DEBUG_WATCH_WRITE;
+                break;
+            default:
+                printf("Unsupported breakpoint type\n");
+                exit(-1);
+            }
+            dbg->arch.bp[n].addr = hw_breakpoint[n].addr;
+        }
+    }
+}
+#endif /* KVM_CAP_SET_GUEST_DEBUG */
-- 
1.5.4

[Qemu-devel] [PATCH 4/5] Add eaddr translator for fsl_booke mmu

[Liu Yu]

Signed-off-by: Liu Yu <yu.liu@freescale.com>
---
 target-ppc/helper.c |   17 +++++++++++++++--
 1 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/target-ppc/helper.c b/target-ppc/helper.c
index b7162df..f4af124 100644
--- a/target-ppc/helper.c
+++ b/target-ppc/helper.c
@@ -22,6 +22,7 @@
 #include <string.h>
 #include <inttypes.h>
 #include <signal.h>
+#include <linux/kvm.h>
 
 #include "cpu.h"
 #include "exec-all.h"
@@ -1325,8 +1326,20 @@ static always_inline int check_physical (CPUState *env, mmu_ctx_t *ctx,
         cpu_abort(env, "MPC8xx MMU model is not implemented\n");
         break;
     case POWERPC_MMU_BOOKE_FSL:
-        /* XXX: TODO */
-        cpu_abort(env, "BookE FSL MMU model not implemented\n");
+        if (kvm_enabled()) {
+            struct kvm_translation tr;
+
+            /* For now we only debug guest kernel */
+            tr.linear_address = eaddr;
+            ret = kvm_vcpu_ioctl(env, KVM_TRANSLATE, &tr);
+            if (ret < 0)
+                return ret;
+
+            ctx->raddr = tr.physical_address;
+        } else {
+            /* XXX: TODO */
+            cpu_abort(env, "BookE FSL MMU model not implemented\n");
+        }
         break;
     default:
         cpu_abort(env, "Unknown or invalid MMU model\n");
-- 
1.5.4

[Qemu-devel] [PATCH 5/5] guest debug init for 440 and e500 core

[Liu Yu]

e500 only support 2 hardware breakpoints,
440(BOOKE) supports 4.

Signed-off-by: Liu Yu <yu.liu@freescale.com>
---
 hw/ppc440_bamboo.c     |    1 +
 hw/ppce500_mpc8544ds.c |    1 +
 target-ppc/kvm_ppc.h   |    1 +
 3 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/hw/ppc440_bamboo.c b/hw/ppc440_bamboo.c
index f1ba130..8c9c3b6 100644
--- a/hw/ppc440_bamboo.c
+++ b/hw/ppc440_bamboo.c
@@ -185,6 +185,7 @@ static void bamboo_init(ram_addr_t ram_size,
     if (kvm_enabled()) {
         kvm_arch_put_registers(env);
         kvmppc_init();
+        kvmppc_debug_init(4, 2);
     }
 }
 
diff --git a/hw/ppce500_mpc8544ds.c b/hw/ppce500_mpc8544ds.c
index f1b3c1a..6c2aa61 100644
--- a/hw/ppce500_mpc8544ds.c
+++ b/hw/ppce500_mpc8544ds.c
@@ -279,6 +279,7 @@ static void mpc8544ds_init(ram_addr_t ram_size,
     if (kvm_enabled()) {
         kvm_arch_put_registers(env);
         kvmppc_init();
+        kvmppc_debug_init(2, 2); /* E500v2 doesn't support IAC3,IAC4 */
     }
 
     return;
diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
index 3792ef7..8b4edca 100644
--- a/target-ppc/kvm_ppc.h
+++ b/target-ppc/kvm_ppc.h
@@ -13,5 +13,6 @@ void kvmppc_init(void);
 void kvmppc_fdt_update(void *fdt);
 int kvmppc_read_host_property(const char *node_path, const char *prop,
                                      void *val, size_t len);
+void kvmppc_debug_init(int max_hw_bp, int max_hw_wp);
 
 #endif /* __KVM_PPC_H__ */
-- 
1.5.4

[Qemu-devel] Re: [PATCH 2/5] Fix booke registers init

[Jan Kiszka]

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

Liu Yu wrote:
> Commit 8d2ba1fb9c8e7006e10d71fa51a020977f14c8b0
> introduces a new new reset order.
> 
> So that we have to synchronize registers explicitly.
> 
> Signed-off-by: Liu Yu <yu.liu-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> ---
>  hw/ppc440_bamboo.c     |    4 +++-
>  hw/ppce500_mpc8544ds.c |    4 +++-
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc440_bamboo.c b/hw/ppc440_bamboo.c
> index d9ef3ec..f1ba130 100644
> --- a/hw/ppc440_bamboo.c
> +++ b/hw/ppc440_bamboo.c
> @@ -182,8 +182,10 @@ static void bamboo_init(ram_addr_t ram_size,
>          /* XXX we currently depend on KVM to create some initial TLB entries. */
>      }
>  
> -    if (kvm_enabled())
> +    if (kvm_enabled()) {
> +        kvm_arch_put_registers(env);
>          kvmppc_init();
> +    }
>  }
>  
>  static QEMUMachine bamboo_machine = {
> diff --git a/hw/ppce500_mpc8544ds.c b/hw/ppce500_mpc8544ds.c
> index c0e367d..f1b3c1a 100644
> --- a/hw/ppce500_mpc8544ds.c
> +++ b/hw/ppce500_mpc8544ds.c
> @@ -276,8 +276,10 @@ static void mpc8544ds_init(ram_addr_t ram_size,
>          /* XXX we currently depend on KVM to create some initial TLB entries. */
>      }
>  
> -    if (kvm_enabled())
> +    if (kvm_enabled()) {
> +        kvm_arch_put_registers(env);
>          kvmppc_init();
> +    }
>  
>      return;
>  }

These are required when loading a device tree and, thus, changing some
registers after cpu_init, right? Then please add
cpu_synchronize_state(env, 1) to the corresponding code blocks instead
of this explicit, kvm-specific loading.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: [PATCH 3/5] Add guest debug support for kvmppc

[Jan Kiszka]

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

Liu Yu wrote:
> Signed-off-by: Liu Yu <yu.liu-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> ---
>  target-ppc/kvm.c |  197 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 197 insertions(+), 0 deletions(-)
> 
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index b53d6e9..d8dbdb4 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -8,6 +8,9 @@
>   *  Christian Ehrhardt <ehrhardt-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
>   *  Hollis Blanchard <hollisb-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
>   *
> + * Copyright (C) 2009 Freescale Semiconductor, Inc. All rights reserved.
> + *  Yu Liu <yu.liu-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> + *
>   * This work is licensed under the terms of the GNU GPL, version 2 or later.
>   * See the COPYING file in the top-level directory.
>   *
> @@ -18,6 +21,7 @@
>  #include <sys/mman.h>
>  
>  #include <linux/kvm.h>
> +#include <asm/kvm_asm.h>
>  
>  #include "qemu-common.h"
>  #include "qemu-timer.h"
> @@ -26,6 +30,7 @@
>  #include "kvm_ppc.h"
>  #include "cpu.h"
>  #include "device_tree.h"
> +#include "gdbstub.h"
>  
>  //#define DEBUG_KVM
>  
> @@ -216,3 +221,195 @@ int kvm_arch_handle_exit(CPUState *env, struct kvm_run *run)
>      return ret;
>  }
>  
> +#ifdef KVM_CAP_SET_GUEST_DEBUG
> +int kvm_arch_insert_sw_breakpoint(CPUState *env, struct kvm_sw_breakpoint *bp)
> +{
> +    uint32_t sc = tswap32(KVM_INST_GUESTGDB);
> +    uint32_t tmp;
> +
> +    if (cpu_memory_rw_debug(env, bp->pc, (uint8_t *)&bp->saved_insn, 4, 0) ||
> +        cpu_memory_rw_debug(env, bp->pc, (uint8_t *)&sc, 4, 1))
> +        return -EINVAL;
> +    cpu_memory_rw_debug(env, bp->pc, (uint8_t *)&tmp, 4, 0);
> +    return 0;
> +}
> +
> +int kvm_arch_remove_sw_breakpoint(CPUState *env, struct kvm_sw_breakpoint *bp)
> +{
> +    uint32_t sc;
> +
> +    if (cpu_memory_rw_debug(env, bp->pc, (uint8_t *)&sc, 4, 0) ||
> +        sc != tswap32(KVM_INST_GUESTGDB) ||
> +        cpu_memory_rw_debug(env, bp->pc, (uint8_t *)&bp->saved_insn, 4, 1))
> +        return -EINVAL;
> +    return 0;
> +}
> +
> +static struct {
> +    target_ulong addr;
> +    int type;
> +} hw_breakpoint[6];
> +
> +static int nb_hw_breakpoint;
> +static int nb_hw_watchpoint;
> +static int max_hw_breakpoint;
> +static int max_hw_watchpoint;
> +
> +void kvmppc_debug_init(int max_hw_bp, int max_hw_wp)
> +{
> +    max_hw_breakpoint = max_hw_bp > 4? 4 : max_hw_bp;
> +    max_hw_watchpoint = max_hw_wp > 2? 2 : max_hw_wp;
> +}
> +
> +static int find_hw_breakpoint(target_ulong addr, int type)
> +{
> +    int n;
> +
> +    for (n = 0; n < nb_hw_breakpoint + nb_hw_watchpoint; n++)
> +        if (hw_breakpoint[n].addr == addr && hw_breakpoint[n].type == type)
> +            return n;
> +    return -1;
> +}
> +
> +int kvm_arch_insert_hw_breakpoint(target_ulong addr,
> +                                  target_ulong len, int type)
> +{
> +    hw_breakpoint[nb_hw_breakpoint + nb_hw_watchpoint].addr = addr;
> +    hw_breakpoint[nb_hw_breakpoint + nb_hw_watchpoint].type = type;
> +
> +    switch (type) {
> +    case GDB_BREAKPOINT_HW:
> +        if (nb_hw_breakpoint >= max_hw_breakpoint)
> +            return -ENOBUFS;
> +
> +        if (find_hw_breakpoint(addr, type) >= 0)
> +            return -EEXIST;
> +
> +        nb_hw_breakpoint++;
> +        break;
> +
> +    case GDB_WATCHPOINT_WRITE:
> +    case GDB_WATCHPOINT_ACCESS:
> +        if (nb_hw_watchpoint >= max_hw_watchpoint)
> +            return -ENOBUFS;
> +
> +        if (find_hw_breakpoint(addr, type) >= 0)
> +            return -EEXIST;
> +
> +        nb_hw_watchpoint++;
> +        break;
> +
> +    default:
> +        return -ENOSYS;
> +    }
> +
> +    return 0;
> +}
> +
> +int kvm_arch_remove_hw_breakpoint(target_ulong addr,
> +                                  target_ulong len, int type)
> +{
> +    int n;
> +
> +    n = find_hw_breakpoint(addr, type);
> +    if (n < 0)
> +        return -ENOENT;
> +
> +    switch (type) {
> +    case GDB_BREAKPOINT_HW:
> +        nb_hw_breakpoint--;
> +        break;
> +
> +    case GDB_WATCHPOINT_WRITE:
> +    case GDB_WATCHPOINT_ACCESS:
> +        nb_hw_watchpoint--;
> +        break;
> +
> +    default:
> +        return -ENOSYS;
> +    }
> +    hw_breakpoint[n] = hw_breakpoint[nb_hw_breakpoint + nb_hw_watchpoint];
> +
> +    return 0;
> +}
> +
> +void kvm_arch_remove_all_hw_breakpoints(void)
> +{
> +    nb_hw_breakpoint = nb_hw_watchpoint = 0;
> +}
> +
> +static CPUWatchpoint hw_watchpoint;
> +
> +int kvm_arch_debug(struct kvm_debug_exit_arch *arch_info)
> +{
> +    int handle = 0;
> +    int n;
> +
> +    if (cpu_single_env->singlestep_enabled) {
> +        handle = 1;
> +
> +    } else if (arch_info->status) {
> +        if (arch_info->status == KVMPPC_DEBUG_BREAKPOINT) {
> +            n = find_hw_breakpoint(arch_info->pc, GDB_BREAKPOINT_HW);
> +            if (n >= 0)
> +                handle = 1;
> +
> +        } else if (arch_info->status == KVMPPC_DEBUG_WATCH_ACCESS) {
> +            n = find_hw_breakpoint(arch_info->pc, GDB_WATCHPOINT_ACCESS);
> +            if (n >= 0) {
> +                handle = 1;
> +                cpu_single_env->watchpoint_hit = &hw_watchpoint;
> +                hw_watchpoint.vaddr = hw_breakpoint[n].addr;
> +                hw_watchpoint.flags = BP_MEM_ACCESS;
> +            }
> +
> +        } else if (arch_info->status == KVMPPC_DEBUG_WATCH_WRITE) {
> +            n = find_hw_breakpoint(arch_info->pc, GDB_WATCHPOINT_WRITE);
> +            if (n >= 0) {
> +                handle = 1;
> +                cpu_single_env->watchpoint_hit = &hw_watchpoint;
> +                hw_watchpoint.vaddr = hw_breakpoint[n].addr;
> +                hw_watchpoint.flags = BP_MEM_WRITE;
> +            }
> +        }
> +
> +    } else if (kvm_find_sw_breakpoint(cpu_single_env, arch_info->pc))
> +        handle = 1;
> +
> +    /* XXX inject guest debug exception */
> +    if (!handle)
> +        printf("Unhandled debug exception!\n");

Out of curiosity: Not yet implemented here, or is PPC also lacking some
kernel bits to support it?

> +
> +    return handle;
> +}
> +
> +void kvm_arch_update_guest_debug(CPUState *env, struct kvm_guest_debug *dbg)
> +{
> +    if (kvm_sw_breakpoints_active(env))
> +        dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;
> +
> +    if (nb_hw_breakpoint + nb_hw_watchpoint > 0) {
> +        int n;
> +
> +        dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_HW_BP;
> +        memset(dbg->arch.bp, 0, sizeof(dbg->arch.bp));
> +        for (n = 0; n < nb_hw_breakpoint + nb_hw_watchpoint; n++) {
> +            switch (hw_breakpoint[n].type) {
> +            case GDB_BREAKPOINT_HW:
> +                dbg->arch.bp[n].type = KVMPPC_DEBUG_BREAKPOINT;
> +                break;
> +            case GDB_WATCHPOINT_ACCESS:
> +                dbg->arch.bp[n].type = KVMPPC_DEBUG_WATCH_ACCESS;
> +                break;
> +            case GDB_WATCHPOINT_WRITE:
> +                dbg->arch.bp[n].type = KVMPPC_DEBUG_WATCH_WRITE;
> +                break;
> +            default:
> +                printf("Unsupported breakpoint type\n");
> +                exit(-1);
> +            }
> +            dbg->arch.bp[n].addr = hw_breakpoint[n].addr;
> +        }
> +    }
> +}
> +#endif /* KVM_CAP_SET_GUEST_DEBUG */

Looks fine. Just a style remark: My x86 code does not follow QEMU's
coding style /wrt code block braces, but this should not prevent you
from applying it to yours. :)

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: [PATCH 5/5] guest debug init for 440 and e500 core

[Jan Kiszka]

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

Liu Yu wrote:
> e500 only support 2 hardware breakpoints,
> 440(BOOKE) supports 4.
> 
> Signed-off-by: Liu Yu <yu.liu-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> ---
>  hw/ppc440_bamboo.c     |    1 +
>  hw/ppce500_mpc8544ds.c |    1 +
>  target-ppc/kvm_ppc.h   |    1 +
>  3 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/ppc440_bamboo.c b/hw/ppc440_bamboo.c
> index f1ba130..8c9c3b6 100644
> --- a/hw/ppc440_bamboo.c
> +++ b/hw/ppc440_bamboo.c
> @@ -185,6 +185,7 @@ static void bamboo_init(ram_addr_t ram_size,
>      if (kvm_enabled()) {
>          kvm_arch_put_registers(env);
>          kvmppc_init();
> +        kvmppc_debug_init(4, 2);
>      }
>  }
>  
> diff --git a/hw/ppce500_mpc8544ds.c b/hw/ppce500_mpc8544ds.c
> index f1b3c1a..6c2aa61 100644
> --- a/hw/ppce500_mpc8544ds.c
> +++ b/hw/ppce500_mpc8544ds.c
> @@ -279,6 +279,7 @@ static void mpc8544ds_init(ram_addr_t ram_size,
>      if (kvm_enabled()) {
>          kvm_arch_put_registers(env);
>          kvmppc_init();
> +        kvmppc_debug_init(2, 2); /* E500v2 doesn't support IAC3,IAC4 */

I think those two are better moved to kvm_arch_init_vcpu.

>      }
>  
>      return;
> diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
> index 3792ef7..8b4edca 100644
> --- a/target-ppc/kvm_ppc.h
> +++ b/target-ppc/kvm_ppc.h
> @@ -13,5 +13,6 @@ void kvmppc_init(void);
>  void kvmppc_fdt_update(void *fdt);
>  int kvmppc_read_host_property(const char *node_path, const char *prop,
>                                       void *val, size_t len);
> +void kvmppc_debug_init(int max_hw_bp, int max_hw_wp);
>  
>  #endif /* __KVM_PPC_H__ */

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: [PATCH 0/5]

[Jan Kiszka]

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

Liu Yu wrote:
> The whole patchset includes:
> patch 1: fix kvmppc build error
> patch 2: fix kvmppc init error
> patch 3~5: add kvmppc guest debug support
> 
> The guest debug still have some problems I haven't solved.
> 
> 1. gdb 'next' command uses software breakpoint
> software breakpoint is implemented via modify guest's code.
> In most case it works well,
> but when used by 'next' it's easy to make trouble on powerpc booke.
> 
> For example booke has a code template for
> jumping to and returning from interrupt handlers:
> 
> 	bl transfer
> 	.long handler_addr
> 	.long ret_addr
> 
> when call transfer, it never return but
> in transfer assembly code it will read the handler_addr
> and ultimately call the handler.
> Gdb doesn't know that and treat it as a normal function call.
> so gdb put a software breakpoint instruction at handler_addr,
> in order to get trap there when return from transfer.
> 
> Then guest will read software breakpoint as handler_addr and jump to there..
> 
> I'm not sure if x86 suffer this kind of issue.

It would if it had such a pattern.

> Is there any way to avoid this?

Unless there is a mechanism via the debug infos of a binary to tell gdb
about this, I think one can only avoid it by not using next here.

> 
> 
> 2. gdb 'watch' command
> Jan told me gdb>6.8 can issue hardware watchpoint request via command 'watch',
> my gdb is 6.8.50.20080821-cvs and our toolchain provider confirm that it supports hardware watch
> However when I use 'watch', I can only see single step from gdbstub side.
> Did I miss anything?

Did you install a watchpoint on a symbol? If yes, try if placing one on
an absolute address changes the picture.

Frankly, I didn't understand gdb's logic for selecting soft or hard
watchpoints so far. Soft watchpoints are those you saw: single step to
the program, checking after each step if the watched variable has
changed. In theory it should be clear when to use which. But practice
appears to be non-deterministic, at least with the versions we recently
tried on x86.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] RE: [PATCH 2/5] Fix booke registers init

[Liu Yu-B13201]

 

> -----Original Message-----
> From: jan.kiszka@web.de [mailto:jan.kiszka@web.de] 
> Sent: Saturday, July 25, 2009 6:05 PM
> To: Liu Yu-B13201
> Cc: qemu-devel; Hollis Blanchard; kvm-ppc; Nathan Froyd
> Subject: Re: [PATCH 2/5] Fix booke registers init
> 
> Liu Yu wrote:
> > Commit 8d2ba1fb9c8e7006e10d71fa51a020977f14c8b0
> > introduces a new new reset order.
> > 
> > So that we have to synchronize registers explicitly.
> > 
> > Signed-off-by: Liu Yu 
> <yu.liu-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> > ---
> >  hw/ppc440_bamboo.c     |    4 +++-
> >  hw/ppce500_mpc8544ds.c |    4 +++-
> >  2 files changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/ppc440_bamboo.c b/hw/ppc440_bamboo.c
> > index d9ef3ec..f1ba130 100644
> > --- a/hw/ppc440_bamboo.c
> > +++ b/hw/ppc440_bamboo.c
> > @@ -182,8 +182,10 @@ static void bamboo_init(ram_addr_t ram_size,
> >          /* XXX we currently depend on KVM to create some 
> initial TLB entries. */
> >      }
> >  
> > -    if (kvm_enabled())
> > +    if (kvm_enabled()) {
> > +        kvm_arch_put_registers(env);
> >          kvmppc_init();
> > +    }
> >  }
> >  
> >  static QEMUMachine bamboo_machine = {
> > diff --git a/hw/ppce500_mpc8544ds.c b/hw/ppce500_mpc8544ds.c
> > index c0e367d..f1b3c1a 100644
> > --- a/hw/ppce500_mpc8544ds.c
> > +++ b/hw/ppce500_mpc8544ds.c
> > @@ -276,8 +276,10 @@ static void mpc8544ds_init(ram_addr_t ram_size,
> >          /* XXX we currently depend on KVM to create some 
> initial TLB entries. */
> >      }
> >  
> > -    if (kvm_enabled())
> > +    if (kvm_enabled()) {
> > +        kvm_arch_put_registers(env);
> >          kvmppc_init();
> > +    }
> >  
> >      return;
> >  }
> 
> These are required when loading a device tree and, thus, changing some
> registers after cpu_init, right? 

Because we don't support bootloader for booke kvm, so we put necessary init work in qemu instead.

> Then please add cpu_synchronize_state(env, 1) to the corresponding code blocks instead
> of this explicit, kvm-specific loading.

Fixed.

[Qemu-devel] RE: [PATCH 0/5]

[Liu Yu-B13201]

 

> -----Original Message-----
> From: jan.kiszka@web.de [mailto:jan.kiszka@web.de] 
> Sent: Saturday, July 25, 2009 6:44 PM
> To: Liu Yu-B13201
> Cc: qemu-devel; Hollis Blanchard; kvm-ppc; Nathan Froyd
> Subject: Re: [PATCH 0/5]
> 
> Liu Yu wrote:
> > 2. gdb 'watch' command
> > Jan told me gdb>6.8 can issue hardware watchpoint request 
> via command 'watch',
> > my gdb is 6.8.50.20080821-cvs and our toolchain provider 
> confirm that it supports hardware watch
> > However when I use 'watch', I can only see single step from 
> gdbstub side.
> > Did I miss anything?
> 
> Did you install a watchpoint on a symbol? If yes, try if 
> placing one on
> an absolute address changes the picture.

Cool, it did use hardware watch when I used absolute address.
Seems I need to test more. :)

[Qemu-devel] Re: [PATCH 0/5]

[Nathan Froyd]

On Sat, Jul 25, 2009 at 04:40:12PM +0800, Liu Yu wrote:
> For example booke has a code template for
> jumping to and returning from interrupt handlers:
>
> 	bl transfer
> 	.long handler_addr
> 	.long ret_addr
>
> when call transfer, it never return but
> in transfer assembly code it will read the handler_addr
> and ultimately call the handler.
> Gdb doesn't know that and treat it as a normal function call.
> so gdb put a software breakpoint instruction at handler_addr,
> in order to get trap there when return from transfer.
>
> Then guest will read software breakpoint as handler_addr and jump to there..
>
> I'm not sure if x86 suffer this kind of issue.
> Is there any way to avoid this?

You would need to modify GDB to recognize this sort of case with the
skip_trampoline_code gdbarch method.

-Nathan

[Qemu-devel] RE: [PATCH 3/5] Add guest debug support for kvmppc

[Liu Yu-B13201]

 

> -----Original Message-----
> From: jan.kiszka@web.de [mailto:jan.kiszka@web.de] 
> Sent: Saturday, July 25, 2009 6:19 PM
> To: Liu Yu-B13201
> Cc: qemu-devel; Hollis Blanchard; kvm-ppc; Nathan Froyd
> Subject: Re: [PATCH 3/5] Add guest debug support for kvmppc
> 
> Liu Yu wrote:
> > Signed-off-by: Liu Yu 
> <yu.liu-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> > ---
> >  target-ppc/kvm.c |  197 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 197 insertions(+), 0 deletions(-)
> > 
> > diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> > index b53d6e9..d8dbdb4 100644
> > --- a/target-ppc/kvm.c
> > +++ b/target-ppc/kvm.c
> > +
> > +int kvm_arch_debug(struct kvm_debug_exit_arch *arch_info)
> > +{
> > +    int handle = 0;
> > +    int n;
> > +
> > +    if (cpu_single_env->singlestep_enabled) {
> > +        handle = 1;
> > +
> > +    } else if (arch_info->status) {
> > +        if (arch_info->status == KVMPPC_DEBUG_BREAKPOINT) {
> > +            n = find_hw_breakpoint(arch_info->pc, 
> GDB_BREAKPOINT_HW);
> > +            if (n >= 0)
> > +                handle = 1;
> > +
> > +        } else if (arch_info->status == 
> KVMPPC_DEBUG_WATCH_ACCESS) {
> > +            n = find_hw_breakpoint(arch_info->pc, 
> GDB_WATCHPOINT_ACCESS);
> > +            if (n >= 0) {
> > +                handle = 1;
> > +                cpu_single_env->watchpoint_hit = &hw_watchpoint;
> > +                hw_watchpoint.vaddr = hw_breakpoint[n].addr;
> > +                hw_watchpoint.flags = BP_MEM_ACCESS;
> > +            }
> > +
> > +        } else if (arch_info->status == KVMPPC_DEBUG_WATCH_WRITE) {
> > +            n = find_hw_breakpoint(arch_info->pc, 
> GDB_WATCHPOINT_WRITE);
> > +            if (n >= 0) {
> > +                handle = 1;
> > +                cpu_single_env->watchpoint_hit = &hw_watchpoint;
> > +                hw_watchpoint.vaddr = hw_breakpoint[n].addr;
> > +                hw_watchpoint.flags = BP_MEM_WRITE;
> > +            }
> > +        }
> > +
> > +    } else if (kvm_find_sw_breakpoint(cpu_single_env, 
> arch_info->pc))
> > +        handle = 1;
> > +
> > +    /* XXX inject guest debug exception */
> > +    if (!handle)
> > +        printf("Unhandled debug exception!\n");
> 
> Out of curiosity: Not yet implemented here, or is PPC also 
> lacking some
> kernel bits to support it?

Yes, guest has no hardware debug support in booke kvm so far.
It's now useless for a guest to set debug register.

[Qemu-devel] RE: [PATCH 0/5]

[Liu Yu-B13201]

 

> -----Original Message-----
> From: Nathan Froyd [mailto:froydnj@codesourcery.com] 
> Sent: Monday, July 27, 2009 9:14 PM
> To: Liu Yu-B13201
> Cc: qemu-devel@nongnu.org; hollisb@us.ibm.com; 
> kvm-ppc@vger.kernel.org; jan.kiszka@siemens.com
> Subject: Re: [PATCH 0/5]
> 
> On Sat, Jul 25, 2009 at 04:40:12PM +0800, Liu Yu wrote:
> > For example booke has a code template for
> > jumping to and returning from interrupt handlers:
> >
> > 	bl transfer
> > 	.long handler_addr
> > 	.long ret_addr
> >
> > when call transfer, it never return but
> > in transfer assembly code it will read the handler_addr
> > and ultimately call the handler.
> > Gdb doesn't know that and treat it as a normal function call.
> > so gdb put a software breakpoint instruction at handler_addr,
> > in order to get trap there when return from transfer.
> >
> > Then guest will read software breakpoint as handler_addr 
> and jump to there..
> >
> > I'm not sure if x86 suffer this kind of issue.
> > Is there any way to avoid this?
> 
> You would need to modify GDB to recognize this sort of case with the
> skip_trampoline_code gdbarch method.
> 

Hmm.. I am not a gdb expert.
But even gdb can recognize this pattern, is it safe to skip it?

[Qemu-devel] Re: [PATCH 0/5]

[Stefan Hajnoczi]

On Wed, Nov 24, 2010 at 8:39 AM, Nicholas A. Bellinger
<nab@linux-iscsi.org> wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
>
> Greetings QEMU SCSI / BLOCK folks,
>
> This series adds rebased support for the hw/scsi-bsg.c backstore for scsi-bus
> compatible HBA emulation in QEMU-KVM on Linux hosts supporting the BSG driver
> against current mainline qemu-kvm.git/master code.

I don't know the Linux SCSI stack, so some basic questions for you :).

With scsi-generic I can send SCSI commands to a device.  How is bsg
different?  The bsg code looks cleaner than sg but they both boil down
to issuing SCSI requests using the Linux block layer AFAICT.

Can you explain what advantages this patch series brings over scsi-generic?

Thanks,
Stefan

Re: [Qemu-devel] Re: [PATCH 0/5]

[Christoph Hellwig]

On Wed, Nov 24, 2010 at 01:38:59PM +0000, Stefan Hajnoczi wrote:
> I don't know the Linux SCSI stack, so some basic questions for you :).
> 
> With scsi-generic I can send SCSI commands to a device.  How is bsg
> different?  The bsg code looks cleaner than sg but they both boil down
> to issuing SCSI requests using the Linux block layer AFAICT.

It's basically a new version of the sg driver with additional features,
and also dropping some old legacy features.

Re: [Qemu-devel] Re: [PATCH 0/5]

[FUJITA Tomonori]

On Wed, 24 Nov 2010 13:38:59 +0000
Stefan Hajnoczi <stefanha@gmail.com> wrote:

> > This series adds rebased support for the hw/scsi-bsg.c backstore for scsi-bus
> > compatible HBA emulation in QEMU-KVM on Linux hosts supporting the BSG driver
> > against current mainline qemu-kvm.git/master code.
> 
> I don't know the Linux SCSI stack, so some basic questions for you :).
> 
> With scsi-generic I can send SCSI commands to a device.  How is bsg
> different?  The bsg code looks cleaner than sg but they both boil down
> to issuing SCSI requests using the Linux block layer AFAICT.
> 
> Can you explain what advantages this patch series brings over scsi-generic?

The main reason why we invented bsg is that we need the common
interface to send non SCSI commands. For example, we already use bsg
for FC stuff:

http://lwn.net/Articles/326338/

We can use the common interface rather than each HW specific interface.

>From the perspective of SCSI, the major advantage of bsg is supporting
bidi commands. I'm not sure qemu will ever need bidi pass through
support though.

Re: [Qemu-devel] Re: [PATCH 0/5]

[Stefan Hajnoczi]

On Thu, Nov 25, 2010 at 2:30 AM, FUJITA Tomonori
<fujita.tomonori@lab.ntt.co.jp> wrote:
> On Wed, 24 Nov 2010 13:38:59 +0000
> Stefan Hajnoczi <stefanha@gmail.com> wrote:
>
>> > This series adds rebased support for the hw/scsi-bsg.c backstore for scsi-bus
>> > compatible HBA emulation in QEMU-KVM on Linux hosts supporting the BSG driver
>> > against current mainline qemu-kvm.git/master code.
>>
>> I don't know the Linux SCSI stack, so some basic questions for you :).
>>
>> With scsi-generic I can send SCSI commands to a device.  How is bsg
>> different?  The bsg code looks cleaner than sg but they both boil down
>> to issuing SCSI requests using the Linux block layer AFAICT.
>>
>> Can you explain what advantages this patch series brings over scsi-generic?
>
> The main reason why we invented bsg is that we need the common
> interface to send non SCSI commands. For example, we already use bsg
> for FC stuff:
>
> http://lwn.net/Articles/326338/
>
> We can use the common interface rather than each HW specific interface.
>
> From the perspective of SCSI, the major advantage of bsg is supporting
> bidi commands. I'm not sure qemu will ever need bidi pass through
> support though.

Thanks for the explanations Christoph and Tomonori.

Stefan