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. Thanks, M. -- Jazz is not dead. It just smells funny...
