Re: [PATCH 1/2] ARM: hyp-stub: improve ABI

[Christoffer Dall <christoffer.dall@linaro.org>]

On Mon, Jan 09, 2017 at 02:42:35PM +0000, Russell King - ARM Linux wrote:
> On Mon, Jan 09, 2017 at 02:05:00PM +0000, Russell King - ARM Linux wrote:
> > So, although Marc produced a patch which updates the KVM hypervisor for
> > the GET_VECTORS change, through reading the code today, it's become clear
> > that much more is needed, so I'm yet again banging on about documentation.
> > It's only become clear to me today that the KVM stub calling convention
> > for the host kernel is:
> > 
> > entry:
> > 	r0 = function pointer
> > 	r1 = 32-bit function argument 0
> > 	r2 = 32-bit function argument 1
> > 	r3 = 32-bit function argument 2
> > 	no further arguments are supported
> > 	--- or ---
> > 	r0 = -1 (or 0 post Marc's patch) for get_vectors
> > exit:
> > 	r0 = vectors (if get_vectors call was made)
> > 	otherwise, who knows...
> 
> Hang on, even this is nowhere near the full picture.
> 
> static inline void __cpu_init_hyp_mode(phys_addr_t pgd_ptr,
>                                        unsigned long hyp_stack_ptr,
>                                        unsigned long vector_ptr)
> {
>         /*
>          * Call initialization code, and switch to the full blown HYP
>          * code. The init code doesn't need to preserve these
>          * registers as r0-r3 are already callee saved according to
>          * the AAPCS.
>          * Note that we slightly misuse the prototype by casting the
>          * stack pointer to a void *.
> 
>          * The PGDs are always passed as the third argument, in order
>          * to be passed into r2-r3 to the init code (yes, this is
>          * compliant with the PCS!).
>          */
> 
>         kvm_call_hyp((void*)hyp_stack_ptr, vector_ptr, pgd_ptr);
> }
> 
> This results in a completely different calling convention -
> 
> 	r0 = hyp_stack_ptr
> 	r1 = vector_ptr
> 	r2,r3 = pgd_ptr
> 
> Which clearly doesn't fit the KVM hypervisor's calling requirements...
> and, looking deeper at this:
> 
>         /* Switch from the HYP stub to our own HYP init vector */
>         __hyp_set_vectors(kvm_get_idmap_vector());
> 
>         pgd_ptr = kvm_mmu_get_httbr();
>         stack_page = __this_cpu_read(kvm_arm_hyp_stack_page);
>         hyp_stack_ptr = stack_page + PAGE_SIZE;
>         vector_ptr = (unsigned long)kvm_ksym_ref(__kvm_hyp_vector);
> 
>         __cpu_init_hyp_mode(pgd_ptr, hyp_stack_ptr, vector_ptr);
> 
> So we actually have _another_ hypervisor stub to care about - should
> anything go wrong between __hyp_set_vectors() and __cpu_init_hyp_mode(),
> we will be hitting the __do_hyp_init assembly code with maybe a get
> vectors or soft reboot call, which, reading the code, would be bad
> news.
> 
> Since this code is run at several different times - CPU hotplug (when
> the system will be quiescent) and also cpuidle PM (when the system is
> not quiescent).  With kdump/kexec, I think this could be racy.
> Certainly if anything were to go wrong between the two with a kdump
> kernel in place, we'd be making HVC calls to the KVM init stub and
> expecting them to work.
> 
Indeed it looks like interrupts are enabled during cpu_init_hyp_mode,
and if it's possible to be preempted there or if kdump can be initiated
from interrupt context, this could go wrong, so you're probably right
that we need to support a common hyp-ABI for the kernel hyp stub, the
KVM stub (a.k.a. the trampoline code), and KVM's hyp layer itself.

Thanks,
-Christoffer