Patchwork [1/5] KVM: PPC: e500: Move VCPU's MMUCFG register initialization earlier

login
register
mail settings
Submitter Mihai Caraman
Date Jan. 30, 2013, 1:29 p.m.
Message ID <1359552584-17861-2-git-send-email-mihai.caraman@freescale.com>
Download mbox | patch
Permalink /patch/216913/
State Not Applicable
Headers show

Comments

Mihai Caraman - Jan. 30, 2013, 1:29 p.m.
VCPU's MMUCFG register initialization should not depend on KVM_CAP_SW_TLB
ioctl call. Move it earlier into tlb initalization phase.

Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
---
 arch/powerpc/kvm/e500_mmu.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Alexander Graf - Jan. 31, 2013, 1:21 p.m.
On 30.01.2013, at 14:29, Mihai Caraman wrote:

> VCPU's MMUCFG register initialization should not depend on KVM_CAP_SW_TLB
> ioctl call. Move it earlier into tlb initalization phase.

Quite the contrary. The fact that there is an mfspr() in e500_mmu.c already tells us that the code is broken. The TLB guest code should only depend on input from the SW_TLB configuration. It's completely orthogonal to the host capabilities.


Alex

> 
> Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
> ---
> arch/powerpc/kvm/e500_mmu.c |    4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/e500_mmu.c b/arch/powerpc/kvm/e500_mmu.c
> index 5c44759..bb1b2b0 100644
> --- a/arch/powerpc/kvm/e500_mmu.c
> +++ b/arch/powerpc/kvm/e500_mmu.c
> @@ -692,8 +692,6 @@ int kvm_vcpu_ioctl_config_tlb(struct kvm_vcpu *vcpu,
> 	vcpu_e500->gtlb_offset[0] = 0;
> 	vcpu_e500->gtlb_offset[1] = params.tlb_sizes[0];
> 
> -	vcpu->arch.mmucfg = mfspr(SPRN_MMUCFG) & ~MMUCFG_LPIDSIZE;
> -
> 	vcpu->arch.tlbcfg[0] &= ~(TLBnCFG_N_ENTRY | TLBnCFG_ASSOC);
> 	if (params.tlb_sizes[0] <= 2048)
> 		vcpu->arch.tlbcfg[0] |= params.tlb_sizes[0];
> @@ -781,6 +779,8 @@ int kvmppc_e500_tlb_init(struct kvmppc_vcpu_e500 *vcpu_e500)
> 	if (!vcpu_e500->g2h_tlb1_map)
> 		goto err;
> 
> +	vcpu->arch.mmucfg = mfspr(SPRN_MMUCFG) & ~MMUCFG_LPIDSIZE;
> +
> 	/* Init TLB configuration register */
> 	vcpu->arch.tlbcfg[0] = mfspr(SPRN_TLB0CFG) &
> 			     ~(TLBnCFG_N_ENTRY | TLBnCFG_ASSOC);
> -- 
> 1.7.4.1
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Caraman Mihai Claudiu-B02008 - Jan. 31, 2013, 2:56 p.m.
> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Thursday, January 31, 2013 3:21 PM
> To: Caraman Mihai Claudiu-B02008
> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; linuxppc-
> dev@lists.ozlabs.org
> Subject: Re: [PATCH 1/5] KVM: PPC: e500: Move VCPU's MMUCFG register
> initialization earlier
> 
> 
> On 30.01.2013, at 14:29, Mihai Caraman wrote:
> 
> > VCPU's MMUCFG register initialization should not depend on
> KVM_CAP_SW_TLB
> > ioctl call. Move it earlier into tlb initalization phase.
> 
> Quite the contrary. The fact that there is an mfspr() in e500_mmu.c
> already tells us that the code is broken. The TLB guest code should only
> depend on input from the SW_TLB configuration. It's completely orthogonal
> to the host capabilities.

Then we have the same issue for TLBnCFG registers which need to be configured
via SW_TLB ioctl. What is the purpose of guest tlb initalization in e500_mmu.c
if we rely on SW_TLB?

-Mike
Alexander Graf - Jan. 31, 2013, 2:58 p.m.
On 31.01.2013, at 15:56, Caraman Mihai Claudiu-B02008 wrote:

>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Thursday, January 31, 2013 3:21 PM
>> To: Caraman Mihai Claudiu-B02008
>> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; linuxppc-
>> dev@lists.ozlabs.org
>> Subject: Re: [PATCH 1/5] KVM: PPC: e500: Move VCPU's MMUCFG register
>> initialization earlier
>> 
>> 
>> On 30.01.2013, at 14:29, Mihai Caraman wrote:
>> 
>>> VCPU's MMUCFG register initialization should not depend on
>> KVM_CAP_SW_TLB
>>> ioctl call. Move it earlier into tlb initalization phase.
>> 
>> Quite the contrary. The fact that there is an mfspr() in e500_mmu.c
>> already tells us that the code is broken. The TLB guest code should only
>> depend on input from the SW_TLB configuration. It's completely orthogonal
>> to the host capabilities.
> 
> Then we have the same issue for TLBnCFG registers which need to be configured
> via SW_TLB ioctl. What is the purpose of guest tlb initalization in e500_mmu.c
> if we rely on SW_TLB?

It's to provide a fallback to user space that doesn't implement SW_TLB configuration yet.


Alex
Caraman Mihai Claudiu-B02008 - Jan. 31, 2013, 3:26 p.m.
> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Thursday, January 31, 2013 4:58 PM
> To: Caraman Mihai Claudiu-B02008
> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; linuxppc-
> dev@lists.ozlabs.org
> Subject: Re: [PATCH 1/5] KVM: PPC: e500: Move VCPU's MMUCFG register
> initialization earlier
> 
> 
> On 31.01.2013, at 15:56, Caraman Mihai Claudiu-B02008 wrote:
> 
> >> -----Original Message-----
> >> From: Alexander Graf [mailto:agraf@suse.de]
> >> Sent: Thursday, January 31, 2013 3:21 PM
> >> To: Caraman Mihai Claudiu-B02008
> >> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; linuxppc-
> >> dev@lists.ozlabs.org
> >> Subject: Re: [PATCH 1/5] KVM: PPC: e500: Move VCPU's MMUCFG register
> >> initialization earlier
> >>
> >>
> >> On 30.01.2013, at 14:29, Mihai Caraman wrote:
> >>
> >>> VCPU's MMUCFG register initialization should not depend on
> >> KVM_CAP_SW_TLB
> >>> ioctl call. Move it earlier into tlb initalization phase.
> >>
> >> Quite the contrary. The fact that there is an mfspr() in e500_mmu.c
> >> already tells us that the code is broken. The TLB guest code should
> only
> >> depend on input from the SW_TLB configuration. It's completely
> orthogonal
> >> to the host capabilities.
> >
> > Then we have the same issue for TLBnCFG registers which need to be
> configured
> > via SW_TLB ioctl. What is the purpose of guest tlb initalization in
> e500_mmu.c
> > if we rely on SW_TLB?
> 
> It's to provide a fallback to user space that doesn't implement SW_TLB
> configuration yet.

Do we have such a case now or is it just hypothetical? For the fallback we
need to initialize the MMUCFG register which I intended to say in the commit
message.

> 
> 
> Alex
>

Patch

diff --git a/arch/powerpc/kvm/e500_mmu.c b/arch/powerpc/kvm/e500_mmu.c
index 5c44759..bb1b2b0 100644
--- a/arch/powerpc/kvm/e500_mmu.c
+++ b/arch/powerpc/kvm/e500_mmu.c
@@ -692,8 +692,6 @@  int kvm_vcpu_ioctl_config_tlb(struct kvm_vcpu *vcpu,
 	vcpu_e500->gtlb_offset[0] = 0;
 	vcpu_e500->gtlb_offset[1] = params.tlb_sizes[0];
 
-	vcpu->arch.mmucfg = mfspr(SPRN_MMUCFG) & ~MMUCFG_LPIDSIZE;
-
 	vcpu->arch.tlbcfg[0] &= ~(TLBnCFG_N_ENTRY | TLBnCFG_ASSOC);
 	if (params.tlb_sizes[0] <= 2048)
 		vcpu->arch.tlbcfg[0] |= params.tlb_sizes[0];
@@ -781,6 +779,8 @@  int kvmppc_e500_tlb_init(struct kvmppc_vcpu_e500 *vcpu_e500)
 	if (!vcpu_e500->g2h_tlb1_map)
 		goto err;
 
+	vcpu->arch.mmucfg = mfspr(SPRN_MMUCFG) & ~MMUCFG_LPIDSIZE;
+
 	/* Init TLB configuration register */
 	vcpu->arch.tlbcfg[0] = mfspr(SPRN_TLB0CFG) &
 			     ~(TLBnCFG_N_ENTRY | TLBnCFG_ASSOC);