Re: [PATCH] arm64: KVM: Configure TCR_EL2.PS at runtime

[Christoffer Dall <christoffer.dall@linaro.org>]

On Thu, Feb 11, 2016 at 02:27:08PM +0100, Christoffer Dall wrote:
> On Wed, Feb 10, 2016 at 10:46:53AM -0800, tchalamarla@caviumnetworks.com wrote:
> > From: Tirumalesh Chalamarla <tchalamarla@caviumnetworks.com>
> > 
> > Setting TCR_EL2.PS to 40 bits is wrong on systems with PS size is
> > less than 40 bits. and with systems where RAM is at higher address,
> > this will break KVM.
> > 
> > This patch sets TCR_EL2.PS at runtime similar to VTCR_EL2.PS
> > 
> > changes form V2:
> > 	- Modified subject line and commit message.
> > 	- Removed TCR_EL2_FLAGS.
> > 
> > changes from V1:
> >         - Moved to dynamic configuration.
> > 
> > Signed-off-by: Tirumalesh Chalamarla <tchalamarla@caviumnetworks.com>
> > ---
> >  arch/arm64/include/asm/kvm_arm.h |  2 --
> >  arch/arm64/kvm/hyp-init.S        | 12 +++++++-----
> >  2 files changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
> > index 738a95f..3776db0 100644
> > --- a/arch/arm64/include/asm/kvm_arm.h
> > +++ b/arch/arm64/include/asm/kvm_arm.h
> > @@ -107,8 +107,6 @@
> >  #define TCR_EL2_MASK	(TCR_EL2_TG0 | TCR_EL2_SH0 | \
> >  			 TCR_EL2_ORGN0 | TCR_EL2_IRGN0 | TCR_EL2_T0SZ)
> >  
> > -#define TCR_EL2_FLAGS	(TCR_EL2_RES1 | TCR_EL2_PS_40B)
> > -
> >  /* VTCR_EL2 Registers bits */
> >  #define VTCR_EL2_RES1		(1 << 31)
> >  #define VTCR_EL2_PS_MASK	(7 << 16)
> > diff --git a/arch/arm64/kvm/hyp-init.S b/arch/arm64/kvm/hyp-init.S
> > index 3e568dc..b5ab1b0 100644
> > --- a/arch/arm64/kvm/hyp-init.S
> > +++ b/arch/arm64/kvm/hyp-init.S
> > @@ -64,7 +64,7 @@ __do_hyp_init:
> >  	mrs	x4, tcr_el1
> >  	ldr	x5, =TCR_EL2_MASK
> >  	and	x4, x4, x5
> > -	ldr	x5, =TCR_EL2_FLAGS
> > +	ldr	x5, =TCR_EL2_RES1
> >  	orr	x4, x4, x5
> >  
> >  #ifndef CONFIG_ARM64_VA_BITS_48
> > @@ -85,15 +85,17 @@ __do_hyp_init:
> >  	ldr_l	x5, idmap_t0sz
> >  	bfi	x4, x5, TCR_T0SZ_OFFSET, TCR_TxSZ_WIDTH
> >  #endif
> > -	msr	tcr_el2, x4
> > -
> > -	ldr	x4, =VTCR_EL2_FLAGS
> >  	/*
> >  	 * Read the PARange bits from ID_AA64MMFR0_EL1 and set the PS bits in
> > -	 * VTCR_EL2.
> > +	 * TCR_EL2 and VTCR_EL2.
> >  	 */
> >  	mrs	x5, ID_AA64MMFR0_EL1
> >  	bfi	x4, x5, #16, #3
> > +
> > +	msr	tcr_el2, x4
> > +
> > +	ldr	x4, =VTCR_EL2_FLAGS
> > +	bfi	x4, x5, #16, #3
> >  	/*
> >  	 * Read the VMIDBits bits from ID_AA64MMFR1_EL1 and set the VS bit in
> >  	 * VTCR_EL2.
> > -- 
> > 2.1.0
> > 
> 
> This looks reasonable enough for me.  Did we limit Linux to PS=40 bits
> in the past and have lifted this restriction, or why did we have this
> limitation for EL2 translations in the past?
> 
Hmm, just went back and looked at my comments [1] on Suzuki's 16K page
series, and regarding our stage-2 page table configuration I wrote this:

<quote>
... the constraints of
this whole thing is, and make sure we agree on this:

1. We fix the IPA max width to 40 bits
2. We don't support systems with a PARange smaller than 40 bits (do we
   check this anywhere or document this anywhere?)
   3. We always assume we are running on a system with PARange of 40
   bits
      and we are therefore constrained to use concatination.
</quote>

So this change is actually making a bit nervous...  Marc, did you check
if all the stuff in arch/arm64/include/asm/kvm_mmu.h still holds after
we relax this?

Thanks,
-Christoffer

[1]: https://lkml.org/lkml/2015/10/10/96