On Wed, 20 Feb 2019 19:14:53 +0000 Dave Martin <Dave.Martin@xxxxxxx> wrote: > On Fri, Jan 25, 2019 at 10:46:52AM +0100, Christoffer Dall wrote: > > Resetting the VCPU state modifies the system register state in memory, > > but this may interact with vcpu_load/vcpu_put if running with preemption > > disabled, which in turn may lead to corrupted system register state. > > Should this be "enabled"? Yup. Never mind. The patches are firmly in mainline now. > > Too late now, but I want to make sure I understand this right for > patches that will go on top. > > > Address this by disabling preemption and doing put/load if required > > around the reset logic. > > > > Signed-off-by: Christoffer Dall <christoffer.dall@xxxxxxx> > > Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx> > > --- > > arch/arm64/kvm/reset.c | 26 ++++++++++++++++++++++++-- > > 1 file changed, 24 insertions(+), 2 deletions(-) > > > > diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c > > index b72a3dd56204..f21a2a575939 100644 > > --- a/arch/arm64/kvm/reset.c > > +++ b/arch/arm64/kvm/reset.c > > @@ -105,16 +105,33 @@ int kvm_arch_vm_ioctl_check_extension(struct kvm *kvm, long ext) > > * This function finds the right table above and sets the registers on > > * the virtual CPU struct to their architecturally defined reset > > * values. > > + * > > + * Note: This function can be called from two paths: The KVM_ARM_VCPU_INIT > > + * ioctl or as part of handling a request issued by another VCPU in the PSCI > > + * handling code. In the first case, the VCPU will not be loaded, and in the > > + * second case the VCPU will be loaded. Because this function operates purely > > + * on the memory-backed valus of system registers, we want to do a full put if > > + * we were loaded (handling a request) and load the values back at the end of > > + * the function. Otherwise we leave the state alone. In both cases, we > > + * disable preemption around the vcpu reset as we would otherwise race with > > + * preempt notifiers which also call put/load. > > */ > > int kvm_reset_vcpu(struct kvm_vcpu *vcpu) > > { > > const struct kvm_regs *cpu_reset; > > + int ret = -EINVAL; > > + bool loaded; > > + > > + preempt_disable(); > > + loaded = (vcpu->cpu != -1); > > + if (loaded) > > + kvm_arch_vcpu_put(vcpu); > > > > switch (vcpu->arch.target) { > > default: > > if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features)) { > > if (!cpu_has_32bit_el1()) > > - return -EINVAL; > > + goto out; > > cpu_reset = &default_regs_reset32; > > } else { > > cpu_reset = &default_regs_reset; > > @@ -137,7 +154,12 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu) > > vcpu->arch.workaround_flags |= VCPU_WORKAROUND_2_FLAG; > > > > /* Reset timer */ > > - return kvm_timer_vcpu_reset(vcpu); > > + ret = kvm_timer_vcpu_reset(vcpu); > > +out: > > + if (loaded) > > + kvm_arch_vcpu_load(vcpu, smp_processor_id()); > > + preempt_enable(); > > + return ret; > > } > > > > void kvm_set_ipa_limit(void) > > I was really confused by this: as far as I can see, we don't really need > to disable preemption here once kvm_arch_vcpu_put() is complete -- at > least not for the purpose of avoiding corruption of the reg state. But > we _do_ need to disable the preempt notifier so that it doesn't fire > before we are ready. Just reading this patch alone won't help. You need to read it in conjunction with the following patch, which resets the vcpu from a preemptible section. > It actually seems a bit surprising for a powered-off CPU to sit with the > VM regs live and preempt notifier armed, when the vcpu thread is > heading to interruptible sleep anyway until someone turns it on. All it takes is a signal for that vcpu to wake up, power-off or not. > Perhaps an alternative approach would be to nobble the preempt notifier > and stick an explicit vcpu_put()...vcpu_load() around the > swait_event_interruptible_exclusive() call in vcpu_req_sleep(). This > is not fast path. What does it buy us? The problem we're solving here is a powered-off, spuriously woken-up vcpu racing against the reset performed from another vcpu. I don't see what adding more put/load would solve. > Any, with the code as-is, it looks like the SVE regs resetting should > go in the preempt_disable() region, after the kvm_arch_vcpu_put() call. All resets must go there. This is the only safe location. > Does it sound like I've understood that right? I'm not sure. Your analysis of what we're trying to achieve with this series seems a bit off. Or I'm completely misunderstanding what you're suggesting, which is the most likely explanation. M. -- Without deviation from the norm, progress is not possible.