On Wed, 31 Oct 2018 16:56:54 +0000 Alex Bennée <alex.ben...@linaro.org> wrote:
> Emilio G. Cota <c...@braap.org> writes: > > > On Wed, Oct 31, 2018 at 16:13:05 +0000, Alex Bennée wrote: > >> > @@ -353,10 +355,12 @@ void s390_cpu_unhalt(S390CPU *cpu) > >> > CPUState *cs = CPU(cpu); > >> > trace_cpu_unhalt(cs->cpu_index); > >> > > >> > - if (cs->halted) { > >> > - cs->halted = 0; > >> > + cpu_mutex_lock(cs); > >> > + if (cpu_halted(cs)) { > >> > + cpu_halted_set(cs, 0); > >> > cs->exception_index = -1; > >> > } > >> > + cpu_mutex_unlock(cs); > >> > >> I think this locking is superfluous as you already added locking when > >> you introduced the helper. > > > > It gives a small perf gain, since it avoids locking & unlocking twice > > in a row (cpu_halted and cpu_halted_set both take the lock if needed). > > Maybe a one line comment then above the first lock: > > /* lock outside cpu_halted(_set) to avoid bouncing */ > > Or some such..... Yes, please. Or introduce something like cpu_halted_flip(cs, new_state) where you could convert the above to if (cpu_halted_flip(cs, 0)) { /* cpu halted before */ cs_exception_index = -1; } Or is that actually an uncommon pattern? > > > > >> Otherwise: > >> > >> Reviewed-by: Alex Bennée <alex.ben...@linaro.org> > > > > Thanks! > > > > Emilio > > > -- > Alex Bennée