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, 23 Nov 2020 17:04:04 +0000
Message-ID: <87h7pg57jv.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     David Brazdil <dbrazdil@google.com>
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 v2 19/24] kvm: arm64: Intercept host's PSCI_CPU_ON SMCs
In-Reply-To: <20201116204318.63987-20-dbrazdil@google.com>
References: <20201116204318.63987-1-dbrazdil@google.com>
        <20201116204318.63987-20-dbrazdil@google.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 Emacs/27.1
 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Mon, 16 Nov 2020 20:43:13 +0000,
David Brazdil <dbrazdil@google.com> wrote:
> 
> Add a handler of the CPU_ON PSCI call from host. When invoked, it looks
> up the logical CPU ID corresponding to the provided MPIDR and populates
> the state struct of the target CPU with the provided x0, pc. It then
> calls CPU_ON itself, with an entry point in hyp that initializes EL2
> state before returning ERET to the provided PC in EL1.
> 
> There is a simple atomic lock around the reset state struct. If it is
> already locked, CPU_ON will return PENDING_ON error code.
> 
> Signed-off-by: David Brazdil <dbrazdil@google.com>
> ---
>  arch/arm64/include/asm/kvm_asm.h     |   8 ++-
>  arch/arm64/kvm/arm.c                 |   1 +
>  arch/arm64/kvm/hyp/nvhe/psci-relay.c | 104 +++++++++++++++++++++++++++
>  3 files changed, 110 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
> index 109867fb76f6..2e36ba4be748 100644
> --- a/arch/arm64/include/asm/kvm_asm.h
> +++ b/arch/arm64/include/asm/kvm_asm.h
> @@ -175,9 +175,11 @@ struct kvm_s2_mmu;
>  DECLARE_KVM_NVHE_SYM(__kvm_hyp_init);
>  DECLARE_KVM_NVHE_SYM(__kvm_hyp_host_vector);
>  DECLARE_KVM_HYP_SYM(__kvm_hyp_vector);
> -#define __kvm_hyp_init		CHOOSE_NVHE_SYM(__kvm_hyp_init)
> -#define __kvm_hyp_host_vector	CHOOSE_NVHE_SYM(__kvm_hyp_host_vector)
> -#define __kvm_hyp_vector	CHOOSE_HYP_SYM(__kvm_hyp_vector)
> +DECLARE_KVM_NVHE_SYM(__kvm_hyp_psci_cpu_entry);
> +#define __kvm_hyp_init			CHOOSE_NVHE_SYM(__kvm_hyp_init)
> +#define __kvm_hyp_host_vector		CHOOSE_NVHE_SYM(__kvm_hyp_host_vector)
> +#define __kvm_hyp_vector		CHOOSE_HYP_SYM(__kvm_hyp_vector)
> +#define __kvm_hyp_psci_cpu_entry	CHOOSE_NVHE_SYM(__kvm_hyp_psci_cpu_entry)
>  
>  extern unsigned long kvm_arm_hyp_percpu_base[NR_CPUS];
>  DECLARE_KVM_NVHE_SYM(__per_cpu_start);
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 7d2270eeecfb..c76a8e5bd19c 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -1365,6 +1365,7 @@ static void cpu_init_hyp_mode(void)
>  
>  	params->vector_hyp_va = (unsigned long)kern_hyp_va(kvm_ksym_ref(__kvm_hyp_host_vector));
>  	params->stack_hyp_va = kern_hyp_va(__this_cpu_read(kvm_arm_hyp_stack_page) + PAGE_SIZE);
> +	params->entry_hyp_va = (unsigned long)kern_hyp_va(kvm_ksym_ref(__kvm_hyp_psci_cpu_entry));

It feels really odd to use a per-CPU variable to keep track of
something that is essentially a constant. Why can't we just have an
assembly version of __kimg_hyp_va() and use that to compute the branch
target directly in __kvm_hyp_cpu_entry()? __kvm_hyp_host_vector is
another one.

>  	params->pgd_pa = kvm_mmu_get_httbr();
>  
>  	/*
> diff --git a/arch/arm64/kvm/hyp/nvhe/psci-relay.c b/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> index 7542de8bd679..2daf52b59846 100644
> --- a/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> +++ b/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> @@ -9,10 +9,15 @@
>  #include <asm/kvm_mmu.h>
>  #include <kvm/arm_hypercalls.h>
>  #include <linux/arm-smccc.h>
> +#include <linux/kvm_host.h>
>  #include <linux/psci.h>
>  #include <kvm/arm_psci.h>
>  #include <uapi/linux/psci.h>
>  
> +#define INVALID_CPU_ID UINT_MAX
> +
> +extern char __kvm_hyp_cpu_entry[];
> +
>  /* Config options set by the host. */
>  u32 __ro_after_init kvm_host_psci_version = PSCI_VERSION(0, 0);
>  u32 __ro_after_init kvm_host_psci_function_id[PSCI_FN_MAX];
> @@ -20,6 +25,14 @@ s64 __ro_after_init hyp_physvirt_offset;
>  
>  #define __hyp_pa(x) ((phys_addr_t)((x)) + hyp_physvirt_offset)
>  
> +struct kvm_host_psci_state {
> +	atomic_t pending_on;
> +	unsigned long pc;
> +	unsigned long r0;
> +};
> +
> +static DEFINE_PER_CPU(struct kvm_host_psci_state, kvm_host_psci_state);
> +
>  static u64 get_psci_func_id(struct kvm_cpu_context *host_ctxt)
>  {
>  	return host_ctxt->regs.regs[0];
> @@ -76,10 +89,99 @@ static __noreturn unsigned long psci_forward_noreturn(struct kvm_cpu_context *ho
>  	hyp_panic(); /* unreachable */
>  }
>  
> +static unsigned int find_cpu_id(u64 mpidr)
> +{
> +	int i;

nit: unsigned int?

> +
> +	if (mpidr != INVALID_HWID) {

This is a little ugly on the side [(c) FZ], and deserves a comment
("Reject MPIDRs matching the init value of the __cpu_logical_map[]
array"?).

Also, I personally prefer a construct that reduces the nesting:

	if (mpidr == INVALID_HWID)
		return INVALID_CPU_ID;

> +		for (i = 0; i < NR_CPUS; i++) {
> +			if (cpu_logical_map(i) == mpidr)
> +				return i;
> +		}
> +	}
> +
> +	return INVALID_CPU_ID;
> +}
> +
> +static bool try_acquire_reset_state(struct kvm_host_psci_state *cpu_state,
> +				    unsigned long pc, unsigned long r0)
> +{
> +	if (atomic_cmpxchg_acquire(&cpu_state->pending_on, 0, 1) != 0)

What guarantees that this cmpxchg is inlined here? Also, having some
names for 0 and 1 would be nice.

> +		return false;
> +
> +	cpu_state->pc = pc;
> +	cpu_state->r0 = r0;
> +	wmb();
> +
> +	return true;
> +}
> +
> +static void release_reset_state(struct kvm_host_psci_state *cpu_state)
> +{
> +	atomic_set_release(&cpu_state->pending_on, 0);
> +}
> +
> +static int psci_cpu_on(u64 func_id, struct kvm_cpu_context *host_ctxt)
> +{
> +	u64 mpidr = host_ctxt->regs.regs[1];
> +	unsigned long pc = host_ctxt->regs.regs[2];
> +	unsigned long r0 = host_ctxt->regs.regs[3];
> +	unsigned int cpu_id;
> +	struct kvm_host_psci_state *cpu_state;
> +	struct kvm_nvhe_init_params *cpu_params;
> +	int ret;
> +
> +	/*
> +	 * Find the logical CPU ID for the given MPIDR. The search set is
> +	 * the set of CPUs that were online at the point of KVM initialization.
> +	 * Booting other CPUs is rejected because their cpufeatures were not
> +	 * checked against the finalized capabilities. This could be relaxed
> +	 * by doing the feature checks in hyp.
> +	 */
> +	cpu_id = find_cpu_id(mpidr);
> +	if (cpu_id == INVALID_CPU_ID)
> +		return PSCI_RET_INVALID_PARAMS;
> +
> +	cpu_state = per_cpu_ptr(&kvm_host_psci_state, cpu_id);
> +	cpu_params = per_cpu_ptr(&kvm_init_params, cpu_id);
> +
> +	if (!try_acquire_reset_state(cpu_state, pc, r0))
> +		return PSCI_RET_ALREADY_ON;
> +
> +	ret = psci_call(func_id, mpidr,
> +			__hyp_pa(hyp_symbol_addr(__kvm_hyp_cpu_entry)),
> +			__hyp_pa(cpu_params));
> +
> +	/*
> +	 * If CPU_ON was successful, the reset state will be released in
> +	 * kvm_host_psci_cpu_entry().
> +	 */
> +	if (ret != PSCI_RET_SUCCESS)
> +		release_reset_state(cpu_state);
> +	return ret;
> +}
> +
> +void __noreturn __host_enter(struct kvm_cpu_context *host_ctxt);
> +
> +asmlinkage void __noreturn __kvm_hyp_psci_cpu_entry(void)
> +{
> +	struct kvm_host_psci_state *cpu_state = this_cpu_ptr(&kvm_host_psci_state);
> +	struct kvm_cpu_context *host_ctxt = &this_cpu_ptr(&kvm_host_data)->host_ctxt;
> +
> +	host_ctxt->regs.regs[0] = cpu_state->r0;
> +	write_sysreg_el2(cpu_state->pc, SYS_ELR);
> +
> +	release_reset_state(cpu_state);
> +
> +	__host_enter(host_ctxt);
> +}
> +
>  static unsigned long psci_0_1_handler(u64 func_id, struct kvm_cpu_context *host_ctxt)
>  {
>  	if (func_id == kvm_host_psci_function_id[PSCI_FN_CPU_OFF])
>  		return psci_forward(host_ctxt);
> +	else if (func_id == kvm_host_psci_function_id[PSCI_FN_CPU_ON])
> +		return psci_cpu_on(func_id, host_ctxt);
>  	else if (func_id == kvm_host_psci_function_id[PSCI_FN_MIGRATE])
>  		return psci_forward(host_ctxt);
>  	else
> @@ -100,6 +202,8 @@ static unsigned long psci_0_2_handler(u64 func_id, struct kvm_cpu_context *host_
>  	case PSCI_0_2_FN_SYSTEM_RESET:
>  		psci_forward_noreturn(host_ctxt);
>  		unreachable();
> +	case PSCI_0_2_FN64_CPU_ON:
> +		return psci_cpu_on(func_id, host_ctxt);
>  	default:
>  		return PSCI_RET_NOT_SUPPORTED;
>  	}
> -- 
> 2.29.2.299.gdc1121823c-goog
> 
> 

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.