Re: [PATCH 12/15] arm: KVM: Simplify HYP init

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

On Thu, Jun 30, 2016 at 01:31:52PM +0100, Marc Zyngier wrote:
> On 28/06/16 22:50, Christoffer Dall wrote:
> > On Tue, Jun 07, 2016 at 11:58:32AM +0100, Marc Zyngier wrote:
> >> Just like for arm64, we can now make the HYP setup a lot simpler,
> >> and we can now initialise it in one go (instead of the two
> >> phases we currently have).
> >>
> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> >> ---
> >>  arch/arm/include/asm/kvm_host.h | 15 +++++--------
> >>  arch/arm/kvm/init.S             | 49 ++++++++---------------------------------
> >>  2 files changed, 14 insertions(+), 50 deletions(-)
> >>
> >> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> >> index 020f4eb..eafbfd5 100644
> >> --- a/arch/arm/include/asm/kvm_host.h
> >> +++ b/arch/arm/include/asm/kvm_host.h
> >> @@ -250,18 +250,13 @@ static inline void __cpu_init_hyp_mode(phys_addr_t pgd_ptr,
> >>  	 * code. The init code doesn't need to preserve these
> >>  	 * registers as r0-r3 are already callee saved according to
> >>  	 * the AAPCS.
> >> -	 * Note that we slightly misuse the prototype by casing the
> >> +	 * Note that we slightly misuse the prototype by casting the
> >>  	 * stack pointer to a void *.
> >> -	 *
> >> -	 * We don't have enough registers to perform the full init in
> >> -	 * one go.  Install the boot PGD first, and then install the
> >> -	 * runtime PGD, stack pointer and vectors. The PGDs are always
> >> -	 * passed as the third argument, in order to be passed into
> >> -	 * r2-r3 to the init code (yes, this is compliant with the
> >> -	 * PCS!).
> >> -	 */
> >>  
> >> -	kvm_call_hyp(NULL, 0, boot_pgd_ptr);
> >> +	 * The PGDs are always passed as the third argument, in order
> >> +	 * to be passed into r2-r3 to the init code (yes, this is
> >> +	 * compliant with the PCS!).
> >> +	 */
> >>  
> >>  	kvm_call_hyp((void*)hyp_stack_ptr, vector_ptr, pgd_ptr);
> >>  }
> >> diff --git a/arch/arm/kvm/init.S b/arch/arm/kvm/init.S
> >> index 1f9ae17..b82a99d 100644
> >> --- a/arch/arm/kvm/init.S
> >> +++ b/arch/arm/kvm/init.S
> >> @@ -32,23 +32,13 @@
> >>   *       r2,r3 = Hypervisor pgd pointer
> >>   *
> >>   * The init scenario is:
> >> - * - We jump in HYP with four parameters: boot HYP pgd, runtime HYP pgd,
> >> - *   runtime stack, runtime vectors
> >> - * - Enable the MMU with the boot pgd
> >> - * - Jump to a target into the trampoline page (remember, this is the same
> >> - *   physical page!)
> >> - * - Now switch to the runtime pgd (same VA, and still the same physical
> >> - *   page!)
> >> + * - We jump in HYP with 3 parameters: runtime HYP pgd, runtime stack,
> >> + *   runtime vectors
> > 
> > probably just call this HYP pgd, HYP stack, and HYP vectors now
> 
> Yup.
> 
> >>   * - Invalidate TLBs
> >>   * - Set stack and vectors
> >> + * - Setup the page tables
> >> + * - Enable the MMU
> >>   * - Profit! (or eret, if you only care about the code).
> >> - *
> >> - * As we only have four registers available to pass parameters (and we
> >> - * need six), we split the init in two phases:
> >> - * - Phase 1: r0 = 0, r1 = 0, r2,r3 contain the boot PGD.
> >> - *   Provides the basic HYP init, and enable the MMU.
> >> - * - Phase 2: r0 = ToS, r1 = vectors, r2,r3 contain the runtime PGD.
> >> - *   Switches to the runtime PGD, set stack and vectors.
> >>   */
> >>  
> >>  	.text
> >> @@ -68,8 +58,11 @@ __kvm_hyp_init:
> >>  	W(b)	.
> >>  
> >>  __do_hyp_init:
> >> -	cmp	r0, #0			@ We have a SP?
> >> -	bne	phase2			@ Yes, second stage init
> >> +	@ Set stack pointer
> >> +	mov	sp, r0
> >> +
> >> +	@ Set HVBAR to point to the HYP vectors
> >> +	mcr	p15, 4, r1, c12, c0, 0	@ HVBAR
> >>  
> >>  	@ Set the HTTBR to point to the hypervisor PGD pointer passed
> >>  	mcrr	p15, 4, rr_lo_hi(r2, r3), c2
> >> @@ -114,33 +107,9 @@ __do_hyp_init:
> >>   THUMB(	ldr	r2, =(HSCTLR_M | HSCTLR_A | HSCTLR_TE)		)
> >>  	orr	r1, r1, r2
> >>  	orr	r0, r0, r1
> >> -	isb
> >>  	mcr	p15, 4, r0, c1, c0, 0	@ HSCR
> >> -
> >> -	@ End of init phase-1
> >> -	eret
> >> -
> >> -phase2:
> >> -	@ Set stack pointer
> >> -	mov	sp, r0
> >> -
> >> -	@ Set HVBAR to point to the HYP vectors
> >> -	mcr	p15, 4, r1, c12, c0, 0	@ HVBAR
> >> -
> >> -	@ Jump to the trampoline page
> >> -	ldr	r0, =TRAMPOLINE_VA
> >> -	adr	r1, target
> >> -	bfi	r0, r1, #0, #PAGE_SHIFT
> >> -	ret	r0
> >> -
> >> -target:	@ We're now in the trampoline code, switch page tables
> >> -	mcrr	p15, 4, rr_lo_hi(r2, r3), c2
> >>  	isb
> >>  
> >> -	@ Invalidate the old TLBs
> >> -	mcr	p15, 4, r0, c8, c7, 0	@ TLBIALLH
> >> -	dsb	ish
> > 
> > how are we sure there are no stale entries in the TLB beyond the idmap
> > region?  Did we take care of this during kernel boot?  What about
> > hotplug/suspend stuff?
> 
> This is done just before installing the page tables (not visible in this
> patch). Hotplug/suspend goes through the same path as well, so it should
> be all taken care of.
> 

Right, ok.

Thanks,
-Christoffer