On Fri, Oct 13, 2017 at 8:38 PM, Radim Krčmář <rkrcmar@xxxxxxxxxx> wrote: > 2017-10-13 19:31+0200, Christoffer Dall: >> On Fri, Oct 13, 2017 at 07:13:07PM +0200, Radim Krčmář wrote: >> > I think that other (special) callsites of vcpu_load()/vcpu_put() have a >> > well defined IOCTL that can be used instead of vcpu->ioctl, so we could >> > just pass the ioctl value all the way to arch code and never save it >> > anywhere, >> >> I don't think that works; what would you do with preempt notifier calls? > > Right, BUG :), I didn't consider them before and they need to know. > >> One solution is to add a parameter to vcpu_put, lie for vcpu_load, which >> also sets the ioctl, and other callers than the final vcpu_put in >> kvm_vcpu_ioctl() just pass the existing value, where the kvm_vcpu_ioctl >> call can pass 0 which gets set before releasing the mutex. >> >> Can you think of a more elegant solution? > > Not really, only thought of touching preempt notifiers and it seems to > be more complicated. > > I think we shouldn't restore ioctl on vcpu_put() at all -- the value > isn't well defined outside of the mutex, so there is no point in looking > and we can just zero the ioctl. I agree, that fits with the semantics of vcpu_put() quit nicely actually. > > Actually, I wouldn't rely on the existing value at all because that. > The need for load/put depends on the current code path, not on the one > we race with. > > x86 seems to be the only user of vcpu_load() outside of kvm_vcpu_ioctl() > and the callers are either under a VM ioctl or under a VM destruction > paths (invalid IOCTL) and we can just hardcode that. Yes, we should. I honestly just wanted feedback on this before tracing through all call paths to the vcpu_load() calls on the x86 side. > > Passing 0 to all other vcpu_load()s and unconditionally zeroing ioctl > before mutex_unlock() should work. Agreed. I'll do that for the next version. Thanks for having a look! -Christoffer
