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
