| Message ID | 1249378568-18008-5-git-send-email-yu.liu@freescale.com |
|---|---|
| State | Superseded |
| Headers | show |
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.
> -----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?
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
> -----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.
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.
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
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");
Signed-off-by: Liu Yu <yu.liu@freescale.com> --- target-ppc/helper.c | 17 +++++++++++++++-- 1 files changed, 15 insertions(+), 2 deletions(-)