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
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
