On Tue, Jan 29, 2019 at 04:05:25PM +0000, Marc Zyngier wrote:
> Hi Drew,
>
> On Tue, 29 Jan 2019 15:48:58 +0000,
> Andrew Jones <drjones@xxxxxxxxxx> wrote:
> >
> > Hi Christoffer,
> >
> > 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.
> > ^ enabled
> > >
> > > Address this by disabling preemption and doing put/load if required
> > > around the reset logic.
> >
> > I'm having trouble understanding how disabling preemption helps here.
> > There shouldn't be an issue with the KVM_ARM_VCPU_INIT case, since the
> > target vcpu is guaranteed not to be loaded and therefore it doesn't
> > have preempt notifiers registered either. Also, KVM_ARM_VCPU_INIT holds
> > the vcpu mutex, so there's no chance for a load to occur until it's
> > complete.
> >
> > For the PSCI case it makes sense to force a vcpu load after the reset,
> > otherwise the sleeping target vcpu won't have the correct state loaded.
> > The initial put also makes sense in case we're not resetting everything.
> > I don't understand how we're ensuring the target vcpu thread's preemption
> > is disabled though. This modified kvm_reset_vcpu would need to be run
> > from the target vcpu thread to work, but that's not how the PSCI code
> > currently does it.
>
> And that's exactly where we're going with the following patch in the
> series. Ultimately, we need a vcpu to reset itself, as we otherwise
> have a window where a vcpu can be spuriously loaded whilst being
> reset.
>
FWIW, I think the confusion here comes from having re-ordered the
patches compared to how the commit text was originally written. We
should probably explain in the commit message that this is in
preparation for doing the reset from the VCPU itself.
Thanks,
Christoffer
