Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 16 Nov 2020 11:49:52 +0000
From:   David Brazdil <dbrazdil@google.com>
To:     Marc Zyngier <maz@kernel.org>
Cc:     kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org, James Morse <james.morse@arm.com>,
        Julien Thierry <julien.thierry.kdev@gmail.com>,
        Suzuki K Poulose <suzuki.poulose@arm.com>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>, Dennis Zhou <dennis@kernel.org>,
        Tejun Heo <tj@kernel.org>, Christoph Lameter <cl@linux.com>,
        Mark Rutland <mark.rutland@arm.com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
        Quentin Perret <qperret@google.com>,
        Andrew Scull <ascull@google.com>,
        Andrew Walbran <qwandor@google.com>, kernel-team@android.com
Subject: Re: [PATCH v1 13/24] kvm: arm64: Add CPU entry point in nVHE hyp
Message-ID: <20201116114952.ybj24wx44lofqs25@google.com>
References: <20201109113233.9012-1-dbrazdil@google.com>
 <20201109113233.9012-14-dbrazdil@google.com>
 <7b8f9dec846f172c2919e1d3f3c65245@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <7b8f9dec846f172c2919e1d3f3c65245@kernel.org>
Precedence: bulk

> >  #ifdef CONFIG_CPU_PM
> >    DEFINE(CPU_CTX_SP,		offsetof(struct cpu_suspend_ctx, sp));
> > diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-init.S
> > b/arch/arm64/kvm/hyp/nvhe/hyp-init.S
> > index 1697d25756e9..f999a35b2c8c 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/hyp-init.S
> > +++ b/arch/arm64/kvm/hyp/nvhe/hyp-init.S
> > @@ -6,6 +6,7 @@
> > 
> >  #include <linux/arm-smccc.h>
> >  #include <linux/linkage.h>
> > +#include <linux/irqchip/arm-gic-v3.h>
> 
> This should probably be included from the file that provides init_el2_state.

Agreed. This is a workaround for the fact that the arm-gic* headers don't play
nice with each other (define the same constants). Including arm-gic-v3.h in
kvm_asm.h will trigger macro redefine warnings in vgic*-v2.c because they
include arm-gic.h.

Another option is to create a header just for el2 init. Would that be
preferable? Other ideas?

> 
> > 
> >  #include <asm/alternative.h>
> >  #include <asm/assembler.h>
> > @@ -159,6 +160,35 @@ alternative_else_nop_endif
> >  	ret
> >  SYM_CODE_END(___kvm_hyp_init)
> > 
> > +SYM_CODE_START(__kvm_hyp_cpu_entry)
> > +	msr	SPsel, #1			// We want to use SP_EL{1,2}
> > +
> > +	/*
> > +	 * Check that the core was booted in EL2. Loop indefinitely if not
> > +	 * because it cannot be safely given to the host without installing
> > KVM.
> > +	 */
> > +	mrs	x1, CurrentEL
> > +	cmp	x1, #CurrentEL_EL2
> > +	b.ne	.
> 
> This is a bit brutal. Consider using a WFE/WFI loop as we have in other
> places already (see __secondary_too_slow for example).

Ack

> > +
> > +	/* Initialize EL2 CPU state to sane values. */
> > +	mov	x29, x0
> > +	init_el2_state nvhe
> > +	mov	x0, x29
> > +
> > +	/*
> > +	 * Load hyp VA of C entry function. Must do so before switching on the
> > +	 * MMU because the struct pointer is PA and not identity-mapped in
> > hyp.
> > +	 */
> > +	ldr	x29, [x0, #NVHE_INIT_PSCI_CPU_ENTRY_FN]
> > +
> > +	/* Enable MMU, set vectors and stack. */
> > +	bl	___kvm_hyp_init
> > +
> > +	/* Leave idmap. */
> > +	br	x29
> 
> To a point I made against an earlier patch: psci_cpu_entry_fn seems to be a
> HYP
> VA, and really needs to be documented as such, because this is pretty hard
> to
> follow otherwise.
> 
> > +SYM_CODE_END(__kvm_hyp_cpu_entry)
> > +
> >  SYM_CODE_START(__kvm_handle_stub_hvc)
> >  	cmp	x0, #HVC_SOFT_RESTART
> >  	b.ne	1f
> 
> Thanks,
> 
>         M.
> -- 
> Jazz is not dead. It just smells funny...