Re: [PATCH 4/5] KVM: arm64: pkvm: Use direct function pointers for cpu_{on,resume}

[Marc Zyngier <maz@kernel.org>]

On Sun, 22 Mar 2026 15:49:50 +0000,
Fuad Tabba <tabba@google.com> wrote:
> 
> Hi Marc,
> 
> On Sat, 21 Mar 2026 at 21:24, Marc Zyngier <maz@kernel.org> wrote:
> >
> > Instead of using a boolean to decide whether a CPU is booting or
> > resuming, just pass an actual function pointer around.
> >
> > This makes the code a bit more straightforward to understand.
> >
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  arch/arm64/include/asm/kvm_asm.h     |  3 ++-
> >  arch/arm64/kvm/hyp/nvhe/hyp-init.S   |  9 +++----
> >  arch/arm64/kvm/hyp/nvhe/psci-relay.c | 39 +++++++++++++++++-----------
> >  3 files changed, 29 insertions(+), 22 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
> > index a1ad12c72ebf1..f4c769857fdfc 100644
> > --- a/arch/arm64/include/asm/kvm_asm.h
> > +++ b/arch/arm64/include/asm/kvm_asm.h
> > @@ -291,7 +291,8 @@ asmlinkage void __noreturn hyp_panic_bad_stack(void);
> >  asmlinkage void kvm_unexpected_el2_exception(void);
> >  struct kvm_cpu_context;
> >  void handle_trap(struct kvm_cpu_context *host_ctxt);
> > -asmlinkage void __noreturn __kvm_host_psci_cpu_entry(bool is_cpu_on);
> > +asmlinkage void __noreturn __kvm_host_psci_cpu_on_entry(void);
> > +asmlinkage void __noreturn __kvm_host_psci_cpu_resume_entry(void);
> >  void __noreturn __pkvm_init_finalise(void);
> >  void kvm_nvhe_prepare_backtrace(unsigned long fp, unsigned long pc);
> >  void kvm_patch_vector_branch(struct alt_instr *alt,
> > diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-init.S b/arch/arm64/kvm/hyp/nvhe/hyp-init.S
> > index 2e80fcbff2dff..64296b31da73d 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/hyp-init.S
> > +++ b/arch/arm64/kvm/hyp/nvhe/hyp-init.S
> > @@ -173,7 +173,7 @@ SYM_CODE_END(___kvm_hyp_init)
> >   * x0: struct kvm_nvhe_init_params PA
> >   */
> >  SYM_CODE_START(kvm_hyp_cpu_entry)
> > -       mov     x1, #1                          // is_cpu_on = true
> > +       ldr     x29, =__kvm_host_psci_cpu_on_entry
> >         b       __kvm_hyp_init_cpu
> >
> >  /*
> > @@ -182,11 +182,10 @@ SYM_CODE_START(kvm_hyp_cpu_entry)
> >   * x0: struct kvm_nvhe_init_params PA
> >   */
> >  SYM_CODE_START(kvm_hyp_cpu_resume)
> > -       mov     x1, #0                          // is_cpu_on = false
> > +       ldr     x29, =__kvm_host_psci_cpu_resume_entry
> >
> >  SYM_INNER_LABEL(__kvm_hyp_init_cpu, SYM_L_LOCAL)
> >         mov     x28, x0                         // Stash arguments
> > -       mov     x29, x1
> >
> >         /* Check that the core was booted in EL2. */
> >         mrs     x0, CurrentEL
> > @@ -204,9 +203,7 @@ SYM_INNER_LABEL(__kvm_hyp_init_cpu, SYM_L_LOCAL)
> >         bl      ___kvm_hyp_init                 // Clobbers x0..x2
> >
> >         /* Leave idmap -- using BLR is OK, LR is restored from host context */
> > -       mov     x0, x29
> > -       ldr     x1, =__kvm_host_psci_cpu_entry
> > -       blr     x1
> > +       blr     x29
> >
> >         // The core booted in EL1, or the C code unexpectedly returned.
> >         // Either way, KVM cannot be initialized on it.
> > diff --git a/arch/arm64/kvm/hyp/nvhe/psci-relay.c b/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> > index c3e196fb8b18f..cc698ceee9c8c 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> > +++ b/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> > @@ -200,23 +200,12 @@ static int psci_system_suspend(u64 func_id, struct kvm_cpu_context *host_ctxt)
> >                          __hyp_pa(init_params), 0);
> >  }
> >
> > -asmlinkage void __noreturn __kvm_host_psci_cpu_entry(bool is_cpu_on)
> > +static void __noreturn __kvm_host_psci_cpu_entry(unsigned long pc, unsigned long r0)
> >  {
> > -       struct psci_boot_args *boot_args;
> > -       struct kvm_cpu_context *host_ctxt;
> > -
> > -       host_ctxt = host_data_ptr(host_ctxt);
> > -
> > -       if (is_cpu_on)
> > -               boot_args = this_cpu_ptr(&cpu_on_args);
> > -       else
> > -               boot_args = this_cpu_ptr(&suspend_args);
> > -
> > -       cpu_reg(host_ctxt, 0) = boot_args->r0;
> > -       write_sysreg_el2(boot_args->pc, SYS_ELR);
> > +       struct kvm_cpu_context *host_ctxt = host_data_ptr(host_ctxt);
> >
> > -       if (is_cpu_on)
> > -               release_boot_args(boot_args);
> > +       cpu_reg(host_ctxt, 0) = r0;
> > +       write_sysreg_el2(pc, SYS_ELR);
> >
> >         write_sysreg_el1(INIT_SCTLR_EL1_MMU_OFF, SYS_SCTLR);
> >         write_sysreg(INIT_PSTATE_EL1, SPSR_EL2);
> > @@ -224,6 +213,26 @@ asmlinkage void __noreturn __kvm_host_psci_cpu_entry(bool is_cpu_on)
> >         __host_enter(host_ctxt);
> >  }
> >
> > +asmlinkage void __noreturn __kvm_host_psci_cpu_on_entry(void)
> > +{
> > +       struct psci_boot_args *boot_args = this_cpu_ptr(&cpu_on_args);
> > +       unsigned long pc, r0;
> > +
> > +       pc = READ_ONCE(boot_args->pc);
> > +       r0 = READ_ONCE(boot_args->r0);
> > +
> > +       release_boot_args(boot_args);
> > +
> > +       __kvm_host_psci_cpu_entry(pc, r0);
> > +}
> > +
> > +asmlinkage void __noreturn __kvm_host_psci_cpu_resume_entry(void)
> > +{
> > +       struct psci_boot_args *boot_args = this_cpu_ptr(&cpu_on_args);
> 
> This should be suspend_args:
> +     struct psci_boot_args *boot_args = this_cpu_ptr(&suspend_args);
>

Arghhh! How did I miss that??? Thanks a bunch for spotting it!

> With this fixed:
> 
> Reviewed-by: Fuad Tabba <tabba@google.com>

Thanks again,

	M.

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