Re: [PATCH v1 13/24] kvm: arm64: Add CPU entry point in nVHE hyp

[David Brazdil <dbrazdil@google.com>]

> >  #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...