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

Christoffer Dall
2017-03-23T15:45:09Z

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