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

[Christoffer Dall]

On Mon, Jan 09, 2017 at 02:05:00PM +0000, Russell King - ARM Linux wrote:
> On Mon, Jan 09, 2017 at 02:26:36PM +0100, Christoffer Dall wrote:
> > On Mon, Jan 09, 2017 at 12:26:39PM +0000, Russell King - ARM Linux wrote:
> > > On Tue, Jan 03, 2017 at 10:51:49AM +0100, Christoffer Dall wrote:
> > > > Hi Russell,
> > > > 
> > > > On Thu, Dec 15, 2016 at 06:57:18PM +0000, Russell King - ARM Linux wrote:
> > > > > What's also coming clear is that there's very few people who understand
> > > > > all the interactions here, and the whole thing seems to be an undocumented
> > > > > mess.  
> > > > 
> > > > I think the hyp stub has just served a very limited purpose so far, and
> > > > therefore is a somewhat immature implementation.  Now we've discovered a
> > > > need to clean it up, and we're all for that.  Again, I don't think the
> > > > problem is any larger than that, we just need to fix it, and it seems to
> > > > me everyone is willing to work on that.
> > > 
> > > What I want to see is some documentation of the hyp-stub, so that there
> > > can be some element of confidence that changes there are properly
> > > coordinated.  As I said in a follow up email:
> > > 
> > > | Either we need more people to have an understanding (so if one of them
> > > | gets run over by a bus, we're not left floundering around) or we need
> > > | it to be documented - even if it's just a simple comment "the ABI in
> > > | this file is shared with XYZ, if you change the ABI here, also update
> > > | XYZ too."
> > > 
> > > > Marc even offered to work on your suggestion to support the general
> > > > hyp ABI commands in KVM.
> > > 
> > > ... which is pointless, because it's a duplication of the effort I've
> > > already put in.  My patches already do the:
> > > 
> > > #define HVC_GET_VECTORS 0
> > > #define HVC_SET_VECTORS 1
> > > #define HVC_SOFT_RESTART 2
> > > 
> > > thing which ARM64 does, passing the arguments in via the appropriate
> > > registers.  However, such a change is a major revision of hyp-stub's
> > > ABI, which completely changes the way it works.
> > 
> > Sorry, I'm afraid I might have been unclear.  What I meant with "general
> > hyp ABI commands in KVM" was, that if there's a need for KVM to support
> > the operations (using a unified and documented ABI) that the hyp stub
> > supports, then we could add that in KVM as well.  I thought your patches
> > added the functionality for the hyp stub, and Marc would add whichever
> > remaining pieces in the KVM side.
> 
> The view over Christmas was "we only need to ensure the GET_VECTORS call
> continues to work", which is what Marc's patch does.  I've come to
> realise (through no help of the documentation situation) that that is
> far from the full story, because of the way kdump works.
> 
> Let me refresh...
> 
> In normal kexec(), kernel_restart_prepare() is called, which calls the
> reboot_notifier_list.  KVM uses this via kvm_reboot() and
> kvm_arch_hardware_disable() to call cpu_hyp_reset(), and in turn
> __kvm_hyp_reset in HYP mode.  That sets the stub vectors back to the
> hyp-stub implementation.  So, normal kexec should work fine.
> 
> However, kernel_restart_prepare() is _not_ called in the kdump case.

Thanks for this, it was slightly unclear to me in which way kdump
different from the other cases, but makes sense now.

> So, with my two patches in place, we will issue a HVC call with the
> arguments that I've given to whatever hypervisor is in place at the
> time, and as I've pointed out, with the KVM hypervisor, we will try to
> branch to address 2 or 3 - who knows what effect that'll have.
> 
> 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:

Just for clarity: I understood that Marc said he was willing to write
the code to support the common ABI that we agree on in KVM.  The fact
that he sent another small patch was a separate contribution, but that's
how I understood. it.

> 
> 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...
> 
> I specify "32-bit" there because they're shifted by one register, which,
> if a 64-bit argument is passed with EABI, the arguments will no longer be
> appropriately aligned... so it's an important detail to be aware of with
> the current KVM hypervisor interface.
> 
> What I want to do here is to fix this kexec issue completely, not in a
> piecemeal fashion - I'm not interested in fixing one small problem, then
> coming back to it in a few months time to fix another problem.  That's a
> waste of time (well, unless you're into job creation.)  I've always been
> for "if you're going to do the job, damn well do the job properly".  So
> I'm not going to accept anything short of fixing _both_ kexec and kdump
> together.
> 
> So, given that the hyp-stub has this ABI after my patches:
> 
> entry:
> 	r0 = argument (0 = get vectors, 1 = set vectors, 2 = call function)
> 	r1 = vectors for r0 = 1
> 	r3 = function pointer (with bit 0 already set for thumb functions)
> 	     for r0 = 2
> exit:
> 	r0 = -1 for invalid calls
> 	r0 = current vectors address (for r0 = 0 on entry)
> 	is not expected to return for r0 = 2 on entry
> 	otherwise registers preserved preserved
> 
> which is clearly incompatible with the current KVM stub, can we come up
> with a common ABI that is satisfactory to both.
> 
> The above are probably the very first time anyone has written out the
> ABI of these things, and as can be seen, it's still something of a mess.
> 
> > > Longer term, I'd like to see the existing hypervisor documentation in
> > > Documentation/virtual/kvm/hypercalls.txt updated with the ARM details.
> > > According to that document, KVM effectively only exists on PPC and x86
> > > at the present time...
> > > 
> > 
> > I'm afraid I don't think this is right place to document this behavior.
> > 
> > There's a difference between an internal ABI between code running in two
> > CPU modes but both part of the same kernel, and a hypervisor running
> > a guest OS on top.
> > 
> > I believe that Documentation/virtual/kvm/hypercalls.txt documents the
> > latter case (i.e. guest hypercalls supported by the KVM host
> > hypervisor), not the former case, and these things should not be
> > combined.
> > 
> > I would suggest adding something like
> > Documentation/virtual/kvm/arm/hyp-abi.txt instead.
> 
> I'm fine with that - I just want there to be some documentation so we can
> stop poking around in the dark, with people stating random different and
> incorrect opinions about the code when problems like broken kexec/kdump
> come up.
> 
> The only way to make me shut up about the documentation thing is to do
> something about it...
> 

Nobody is arguing with the need for documentation, and I think I
understan the reason for needing to support a common ABI with a common
set of functions regardless of the logic in place in Hyp mode.

We just have to agree on a sane ABI and I'll be happy to help
write/review the API docs and clean things up.

Thanks,
-Christoffer