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

Russell King - ARM Linux
2017-01-09T14:41:06Z

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.

--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.