On Mon, 16 Nov 2020 20:43:13 +0000,
David Brazdil <dbraz...@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 <dbraz...@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.

Reply via email to