[PATCH v4 00/28] arm/arm64: KVM: Rework the hyp-stub API

On 23/03/17 14:39, Christoffer Dall wrote:

On Thu, Mar 23, 2017 at 10:53:04AM +0000, Marc Zyngier wrote:

quoted

On 22/03/17 17:27, Christoffer Dall wrote:

quoted

I don't think there is a great use case beyond what we already do, it
would just be to have one set of hyp vectors fewer, so that either the
hyp stub is in place, or a hypervisor is in place, not kvm-init vectors.

So instead of doing:
  __hyp_set_vectors(kvm_get_idmap_vector());
  hvc(); /* via the misused kvm_call_hyp thing */

you would do:

  __hyp_call_function(__kvm_hyp_init, arg1, arg2);

which would change the vector and initialize anything.

Not sure if that can work though, or if there are any downsides that I
haven't thought about?

I've given it a go, and that seems to work, at least on arm64. We may 
have to set the vectors in a slightly different way on 32bit because 
we're going to run out of registers (we only have two left once we 
reach the function being called).

Right, that's a challenge I clearly didn't foresee.

quoted

Another thing is that the function called is not really a function. It 
is an exception handler, as it has to end with an eret (or we may need 
to save LR in funky ways).

Shouldn't it have the same semantics as the KVM calling function thing
where it pushes the LR on the stack?  But of course, that requires
having a stack for each runtime that owns hyp mode at any given time.
Potentially not great.

Indeed, and that's the point where I'm starting to think that we may be
trying to beautify something that may not be worth it.

If possible, the idea would be that you'd call functions, just like you
normally do, but the functions you call can choose to never return -
again just like a C function can do if it messes with your machine
configuration.

But maybe the idea is fundamentally flawed, if the only function
you'd ever call from the hyp stub is one that takes over the hyp world,
and then the original design was just based on using set_vectors.  Hmmm.

That's my point above. The only use case we have so far for this method
is to take over EL2. On the other have, I've noticed that the more I
rework this area, the more old stuff surfaces, ready to be nuked. Maybe
I should keep digging.

quoted

 The potential blocker for this is that the
32bit decompressor does use set_vectors in funky ways to deal with
relocation. If we get rid of set_vectors like I just did on 64bit,
we'll need to mess with that as well.

Interesting.  I didn't think that we'd necessarily get rid of
set_vectors, but I agree with you that if we wanted to do this, it
should probably go.

quoted

Anyway, here's the hack, 64bit only, quickly tested on Mustang. I'm not
completely sure this is any prettier, but it is certainly manageable.

There are two things I like about it.  First, it gets rid of the
'additional runtime' in hyp and second the changes to
cpu_init_hyp_mode() are quite nice, imho.

Thanks being said, this series is pretty nice as it is, so I don't want
to impose more work on you at the moment, so maybe we save your patch
snipped below for later if we want to consider looking at it some other
time?

What I can do is prepare an extra series that would include the
corresponding 32bit changes, and we then evaluate that separately. I
don't mind spending a bit of time on it.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...