[Qemu-devel] [PATCH v2 0/5] kvmppc: Add guest debug support

[Qemu-devel] [PATCH v2 0/5] kvmppc: Add guest debug support

[Liu Yu]

The whole patchset add guest debug support for kvmppc.
patch 1: fix kvmppc build error
patch 2: fix kvmppc init error
patch 3: add guest debug support
patch 4: translate eaddr for fsl_booke mmu
patch 5: guest debug init

[v2]:
1. use cpu_synchronize_state() instead of kvm_arch_put_registers()
2. move guest debug init work into kvm_arch_init_vcpu()
3. add GDB_WATCHPOINT_READ support
4. some cleanup

[Qemu-devel] [PATCH 1/5] kvmppc: Fix 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 6b7f411..4ed6fc7 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] kvmppc: Synchronize booke registers

[Liu Yu]

Commit 8d2ba1fb9c8e7006e10d71fa51a020977f14c8b0
introduces a new new reset order.

So that we have to synchronize the register after updating them.

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

diff --git a/hw/ppc440_bamboo.c b/hw/ppc440_bamboo.c
index 5011679..db78639 100644
--- a/hw/ppc440_bamboo.c
+++ b/hw/ppc440_bamboo.c
@@ -181,6 +181,8 @@ static void bamboo_init(ram_addr_t ram_size,
         env->gpr[3] = dt_base;
         env->nip = entry;
         /* XXX we currently depend on KVM to create some initial TLB entries. */
+
+	cpu_synchronize_state(env, 1);
     }
 
     if (kvm_enabled())
diff --git a/hw/ppce500_mpc8544ds.c b/hw/ppce500_mpc8544ds.c
index d154c7f..4a397f7 100644
--- a/hw/ppce500_mpc8544ds.c
+++ b/hw/ppce500_mpc8544ds.c
@@ -275,6 +275,8 @@ static void mpc8544ds_init(ram_addr_t ram_size,
         env->gpr[3] = dt_base;
         env->nip = entry;
         /* XXX we currently depend on KVM to create some initial TLB entries. */
+
+	cpu_synchronize_state(env, 1);
     }
 
     if (kvm_enabled())
-- 
1.5.4

[Qemu-devel] [PATCH 3/5] kvmppc: Guest debug support

[Liu Yu]

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

diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index b53d6e9..97a0737 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,201 @@ 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;
+
+static 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_READ:
+    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_READ:
+    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_READ |
+                                        KVMPPC_DEBUG_WATCH_WRITE)) {
+            if ((n = find_hw_breakpoint(arch_info->pc, GDB_WATCHPOINT_ACCESS)) >= 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 ((n = find_hw_breakpoint(arch_info->pc, GDB_WATCHPOINT_WRITE) >= 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 ((n = find_hw_breakpoint(arch_info->pc, GDB_WATCHPOINT_READ) >= 0)) {
+                handle = 1;
+                cpu_single_env->watchpoint_hit = &hw_watchpoint;
+                hw_watchpoint.vaddr = hw_breakpoint[n].addr;
+                hw_watchpoint.flags = BP_MEM_READ;
+            }
+        }
+
+    } 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_WRITE:
+                dbg->arch.bp[n].type = KVMPPC_DEBUG_WATCH_WRITE;
+                break;
+            case GDB_WATCHPOINT_READ:
+                dbg->arch.bp[n].type = KVMPPC_DEBUG_WATCH_READ;
+                break;
+            case GDB_WATCHPOINT_ACCESS:
+                dbg->arch.bp[n].type = KVMPPC_DEBUG_WATCH_WRITE | KVMPPC_DEBUG_WATCH_READ;
+                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] kvmppc: Translate eaddr 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 6eca2e5..07e56a4 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] kvmppc: guest debug init

[Liu Yu]

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

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

diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 97a0737..82e7897 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -42,6 +42,10 @@
     do { } while (0)
 #endif
 
+#ifdef KVM_CAP_SET_GUEST_DEBUG
+static void kvmppc_debug_init(int, int);
+#endif
+
 int kvm_arch_init(KVMState *s, int smp_cpus)
 {
     return 0;
@@ -55,6 +59,12 @@ int kvm_arch_init_vcpu(CPUState *cenv)
     sregs.pvr = cenv->spr[SPR_PVR];
     ret = kvm_vcpu_ioctl(cenv, KVM_SET_SREGS, &sregs);
 
+#ifdef KVM_CAP_SET_GUEST_DEBUG
+    if (strcmp(cenv->cpu_model_str, "405"))
+        kvmppc_debug_init(4, 2);
+    if (strcmp(cenv->cpu_model_str, "e500v2_v30"))
+        kvmppc_debug_init(2, 2);   /* E500v2 doesn't support IAC3,IAC4 */
+#endif
     return ret;
 }
 
-- 
1.5.4

[Qemu-devel] Re: [PATCH 4/5] kvmppc: Translate eaddr for fsl_booke mmu

[Hollis Blanchard]

On Tue, 2009-08-04 at 17:36 +0800, Liu Yu wrote:
> 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 6eca2e5..07e56a4 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");

One objection: the comment is a little obscure. I think what you're
really saying is "in Linux guests, kernel addresses should always be
covered by TLB1, which means for those addresses we can expect this
ioctl to succeed." However, since you need to handle failures anyways, I
think you should remove the comment entirely.

Second, (and this isn't an objection but rather a question) do you have
any better ideas for struct kvm_translation? It only really makes sense
for x86. We don't need to stick with it.

-- 
Hollis Blanchard
IBM Linux Technology Center

[Qemu-devel] RE: [PATCH 4/5] kvmppc: Translate eaddr for fsl_booke mmu

[Liu Yu-B13201]

 

> -----Original Message-----
> From: Hollis Blanchard [mailto:hollisb@us.ibm.com] 
> Sent: Thursday, August 20, 2009 6:51 AM
> To: Liu Yu-B13201
> Cc: qemu-devel@nongnu.org; kvm-ppc@vger.kernel.org; 
> jan.kiszka@siemens.com; froydnj@codesourcery.com; Alexander Graf
> Subject: Re: [PATCH 4/5] kvmppc: Translate eaddr for fsl_booke mmu
> 
> On Tue, 2009-08-04 at 17:36 +0800, Liu Yu wrote:
> > 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 6eca2e5..07e56a4 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");
> 
> One objection: the comment is a little obscure. I think what you're
> really saying is "in Linux guests, kernel addresses should always be
> covered by TLB1, which means for those addresses we can expect this
> ioctl to succeed." However, since you need to handle failures 
> anyways, I
> think you should remove the comment entirely.

As BOOKE mmu translation needs AS + PID + address,
The infomations we pass to kvmppc here only count in address and set
AS=0, PID=0.
Which indicates that it's a kernel address.

If want to translate user space address, one way is read registers from
kvmppc at first 
and then pass the correct AS and PID to translator.
As we don't need to debug guest userspace, for simplicity, I didn't do
that.

> 
> Second, (and this isn't an objection but rather a question) 
> do you have
> any better ideas for struct kvm_translation? It only really 
> makes sense
> for x86. We don't need to stick with it.
> 

Hrr.. We need to combine AS, PID and 32-bit addr into 64-bit linear
address. it's not that convenient.
But except that, I am not sure if there is strong requirement to change
it...

BOOK3S KVM has more work in qemu (openbios, vga etc.),
Maybe Alex has some suggestion?

[Qemu-devel] Re: [PATCH 4/5] kvmppc: Translate eaddr for fsl_booke mmu

[Alexander Graf]

Am 20.08.2009 um 12:21 schrieb "Liu Yu-B13201" <Yu.Liu@freescale.com>:

>
>
>> -----Original Message-----
>> From: Hollis Blanchard [mailto:hollisb@us.ibm.com]
>> Sent: Thursday, August 20, 2009 6:51 AM
>> To: Liu Yu-B13201
>> Cc: qemu-devel@nongnu.org; kvm-ppc@vger.kernel.org;
>> jan.kiszka@siemens.com; froydnj@codesourcery.com; Alexander Graf
>> Subject: Re: [PATCH 4/5] kvmppc: Translate eaddr for fsl_booke mmu
>>
>> On Tue, 2009-08-04 at 17:36 +0800, Liu Yu wrote:
>>> 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 6eca2e5..07e56a4 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");
>>
>> One objection: the comment is a little obscure. I think what you're
>> really saying is "in Linux guests, kernel addresses should always be
>> covered by TLB1, which means for those addresses we can expect this
>> ioctl to succeed." However, since you need to handle failures
>> anyways, I
>> think you should remove the comment entirely.
>
> As BOOKE mmu translation needs AS + PID + address,
> The infomations we pass to kvmppc here only count in address and set
> AS=0, PID=0.
> Which indicates that it's a kernel address.
>
> If want to translate user space address, one way is read registers  
> from
> kvmppc at first
> and then pass the correct AS and PID to translator.
> As we don't need to debug guest userspace, for simplicity, I didn't do
> that.
>
>>
>> Second, (and this isn't an objection but rather a question)
>> do you have
>> any better ideas for struct kvm_translation? It only really
>> makes sense
>> for x86. We don't need to stick with it.
>>
>
> Hrr.. We need to combine AS, PID and 32-bit addr into 64-bit linear
> address. it's not that convenient.
> But except that, I am not sure if there is strong requirement to  
> change
> it...
>
> BOOK3S KVM has more work in qemu (openbios, vga etc.),
> Maybe Alex has some suggestion?
>

What does that do again? Enable userspace to do EA to PA translation?

IMHO userspace should do the translation and do an ioctl to fetch the  
required information (soft TLB cache / SLB / SDR1) so we can reuse the  
existing qemu infrastructure.

Alex

[Qemu-devel] RE: [PATCH 4/5] kvmppc: Translate eaddr for fsl_booke mmu

[Liu Yu-B13201]

 

> -----Original Message-----
> From: Alexander Graf [mailto:alex@csgraf.de] 
> Sent: Friday, August 21, 2009 8:39 PM
> To: Liu Yu-B13201
> Cc: Hollis Blanchard; <qemu-devel@nongnu.org>; 
> <kvm-ppc@vger.kernel.org>; <jan.kiszka@siemens.com>; 
> <froydnj@codesourcery.com>
> Subject: Re: [PATCH 4/5] kvmppc: Translate eaddr for fsl_booke mmu
> 
> 
> Am 20.08.2009 um 12:21 schrieb "Liu Yu-B13201" <Yu.Liu@freescale.com>:
> 
> >
> >
> >> -----Original Message-----
> >> From: Hollis Blanchard [mailto:hollisb@us.ibm.com]
> >> Sent: Thursday, August 20, 2009 6:51 AM
> >> To: Liu Yu-B13201
> >> Cc: qemu-devel@nongnu.org; kvm-ppc@vger.kernel.org;
> >> jan.kiszka@siemens.com; froydnj@codesourcery.com; Alexander Graf
> >> Subject: Re: [PATCH 4/5] kvmppc: Translate eaddr for fsl_booke mmu
> >>
> >> On Tue, 2009-08-04 at 17:36 +0800, Liu Yu wrote:
> >>> 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 6eca2e5..07e56a4 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");
> >>
> >> One objection: the comment is a little obscure. I think what you're
> >> really saying is "in Linux guests, kernel addresses should 
> always be
> >> covered by TLB1, which means for those addresses we can expect this
> >> ioctl to succeed." However, since you need to handle failures
> >> anyways, I
> >> think you should remove the comment entirely.
> >
> > As BOOKE mmu translation needs AS + PID + address,
> > The infomations we pass to kvmppc here only count in address and set
> > AS=0, PID=0.
> > Which indicates that it's a kernel address.
> >
> > If want to translate user space address, one way is read registers  
> > from
> > kvmppc at first
> > and then pass the correct AS and PID to translator.
> > As we don't need to debug guest userspace, for simplicity, 
> I didn't do
> > that.
> >
> >>
> >> Second, (and this isn't an objection but rather a question)
> >> do you have
> >> any better ideas for struct kvm_translation? It only really
> >> makes sense
> >> for x86. We don't need to stick with it.
> >>
> >
> > Hrr.. We need to combine AS, PID and 32-bit addr into 64-bit linear
> > address. it's not that convenient.
> > But except that, I am not sure if there is strong requirement to  
> > change
> > it...
> >
> > BOOK3S KVM has more work in qemu (openbios, vga etc.),
> > Maybe Alex has some suggestion?
> >
> 
> What does that do again? Enable userspace to do EA to PA translation?
> 
> IMHO userspace should do the translation and do an ioctl to 
> fetch the  
> required information (soft TLB cache / SLB / SDR1) so we can 
> reuse the  
> existing qemu infrastructure.
> 

BOOK3S has mmu implement in qemu, but BOOKE doesn't.

[Qemu-devel] RE: [PATCH 4/5] kvmppc: Translate eaddr for fsl_booke mmu

[Hollis Blanchard]

On Mon, 2009-08-24 at 10:44 +0800, Liu Yu-B13201 wrote:
> 
> > IMHO userspace should do the translation and do an ioctl to 
> > fetch the  
> > required information (soft TLB cache / SLB / SDR1) so we can 
> > reuse the  
> > existing qemu infrastructure.
> > 
> 
> BOOK3S has mmu implement in qemu, but BOOKE doesn't.

Even if it did, I'd be skeptical. But you're right that that's a
critical point: as things stand today, only KVM (not qemu) emulates the
Book E MMU.

-- 
Hollis Blanchard
IBM Linux Technology Center

[Qemu-devel] Re: [PATCH 4/5] kvmppc: Translate eaddr for fsl_booke mmu

[Alexander Graf]

On 24.08.2009, at 19:33, Hollis Blanchard wrote:

> On Mon, 2009-08-24 at 10:44 +0800, Liu Yu-B13201 wrote:
>>
>>> IMHO userspace should do the translation and do an ioctl to
>>> fetch the
>>> required information (soft TLB cache / SLB / SDR1) so we can
>>> reuse the
>>> existing qemu infrastructure.
>>>
>>
>> BOOK3S has mmu implement in qemu, but BOOKE doesn't.
>
> Even if it did, I'd be skeptical. But you're right that that's a
> critical point: as things stand today, only KVM (not qemu) emulates  
> the
> Book E MMU.

Well, that's the whole point I was trying to make.

If we had the booke mmu implemented in qemu debugging would be a lot  
easier I guess. Also it'd benefit people who for whatever reason want  
to emulate a booke cpu instead of virtualizing one, maybe because  
their development machines are x86 ;-).

If I remember the x86 KVM architecture correctly, CR3 (the register  
holding a phys addr to the current pagetable) gets pulled by userspace  
and then qemu does the complete translation based on that information.

So yes, we do have two separate MMU implementations for x86 here, but  
that's a good thing IMHO, because it makes it easier to spot bugs and  
find out where things went wrong.

So my suggestion is: Implement the BOOKE MMU in Qemu, make an IOCTL to  
pull the TLB to userspace and thus make life easier for everyone.

Alex