Re: [PATCH] cpu: Add starts_halted() method

[Eduardo Habkost]

On Wed, Jul 08, 2020 at 06:09:49PM +0100, Peter Maydell wrote:
> On Wed, 8 Jul 2020 at 17:03, Eduardo Habkost <ehabkost@redhat.com> wrote:
> >
> > On Wed, Jul 08, 2020 at 04:32:51PM +0100, Peter Maydell wrote:
> > > On Wed, 8 Jul 2020 at 16:25, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > > On Wed, Jul 08, 2020 at 02:14:03PM +0100, Peter Maydell wrote:
> > > > > The original bug as described in the commit message sounds
> > > > > to me like something we should look to fix in the implementation
> > > > > of async_run_on_cpu() -- it shouldn't cause a CPU that's halfway
> > > > > through reset to do a KVM_RUN or otherwise run guest code,
> > > > > whether that CPU is going to start powered-up or powered-down.
> > > >
> > > > What "halfway through reset" means, exactly?  Isn't halted==1
> > > > enough to indicate the CPU is in that state?
> > >
> > > I mean "while we're in the middle of the CPU method that's
> > > called by cpu_reset()". "halted==1" says "the CPU is halted";
> > > that's not the same thing. KVM_RUN happening
> > > as a side effect in the middle of that code is a bug
> > > whether the CPU happens to be intended to be put into the
> > > halted state or not. If the CPU is intended to be created
> > > not-halted then KVM_RUN can happen after cpu reset
> > > completes, but not before.
> >
> > Wait, I thought we already had mechanisms to prevent that from
> > happening.  Otherwise, it would never be safe for cpu_reset() to
> > touch the CPU registers.
> 
> Exactly. It appears that there's a bug in our mechanisms,
> which is why I'm suggesting that the right thing is
> to fix that bug rather than marking the CPU as halted
> earlier in the reset process so that the KVM_RUN happens
> to do nothing...

I agree this is necessary, but it doesn't seem sufficient.

Having cpu_reset() set halted=0 on spapr (and probably other
machines) is also a bug, as it could still trigger unwanted
KVM_RUN when cpu_reset() returns (and before machine code sets
halted=1).

-- 
Eduardo