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

[cdall@linaro.org (Christoffer Dall)]

On Thu, Mar 23, 2017 at 03:16:49PM +0000, Marc Zyngier wrote:
> On 23/03/17 14:39, Christoffer Dall wrote:
> > On Thu, Mar 23, 2017 at 10:53:04AM +0000, Marc Zyngier wrote:
> >> On 22/03/17 17:27, Christoffer Dall wrote:
> >>>
> >>> 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.
> > 
> >>
> >> 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.
> 
> > 
> >>  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.
> > 
> >>
> >> 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.
> 

At this point I definitely think we should separate those efforts, and
get this series merged.  If you feel like looking at the other aspect,
that's great, and we can also have a look together at some point (I
could at least write the documentation), but let's do it in two stages.

/me stops making noise and goes back to reviewing the series at hand.

Thanks,
-Christoffer