Re: [Qemu-devel] [PATCH v3] spapr: disable decrementer during reset

[Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>]

David Gibson <david@gibson.dropbear.id.au> writes:

> On Tue, Jul 18, 2017 at 10:53:01AM +0530, Nikunj A Dadhania wrote:
>> David Gibson <david@gibson.dropbear.id.au> writes:
>> 
>> > On Mon, Jul 17, 2017 at 09:46:39AM +0530, Nikunj A Dadhania wrote:
>> >> Rebooting a SMP TCG guest is broken for both single/multi threaded TCG.
>> >> 
>> >> When reset happens, all the CPUs are in halted state. First CPU is brought out
>> >> of reset and secondary CPUs would be initialized by the guest kernel using a
>> >> rtas call start-cpu.
>> >> 
>> >> However, in case of TCG, decrementer interrupts keep on coming and waking the
>> >> secondary CPUs up.
>> >> 
>> >> These secondary CPUs would see the decrementer interrupt pending, which makes
>> >> cpu::has_work() to bring them out of wait loop and start executing
>> >> tcg_exec_cpu().
>> >> 
>> >> The problem with this is all the CPUs wake up and start booting SLOF image,
>> >> causing the following exception(4 CPUs TCG VM):
>> >
>> > Ok, I'm still trying to understand why the behaviour on reboot is
>> > different from the first boot.
>> 
>> During first boot, the cpu is in the stopped state, so
>> cpus.c:cpu_thread_is_idle returns true and CPU remains in halted state
>> until rtas start-cpu. Therefore, we never check the cpu_has_work()
>> 
>> In case of reboot, all CPUs are resumed after reboot. So we check the
>> next condition cpu_has_work() in cpu_thread_is_idle(), where we see a
>> DECR interrupt and remove the CPU from halted state as the CPU has
>> work.
>
> Ok, so it sounds like we should set stopped on all the secondary CPUs
> on reset as well.  What's causing them to be resumed after the reset
> at the moment?

That is part of the main loop in vl.c, when reset is requested. All the
vcpus are paused (stopped == true) then system reset is issued, and all
cpus are resumed (stopped == false). Which is correct.

static bool main_loop_should_exit(void)
{
[...]
    request = qemu_reset_requested();
    if (request) {
        pause_all_vcpus();
        qemu_system_reset(request);
        resume_all_vcpus();
        if (!runstate_check(RUN_STATE_RUNNING) &&
                !runstate_check(RUN_STATE_INMIGRATE)) {
            runstate_set(RUN_STATE_PRELAUNCH);
        }
    }
[...]
}

The CPUs are in halted state, i.e. cpu::halted = 1. We have set
cpu::halted = 0 for the primary CPU, aka first_cpu, in
spapr.c::ppc_spapr_reset()

In case of TCG, we have a decr callback (per CPU) scheduled once the
machine is started which keeps on running and interrupting irrespective
of the state of the machine.

tb_env->decr_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, &cpu_ppc_decr_cb, cpu);

Which keeps on queueing the interrupt to the CPUs. All the other CPUs
are in a tight loop checking cpu_thread_is_idle(), which returns false
when it sees the decrementer interrupt, the cpu starts executing:

cpu_exec()
  -> cpu_handle_halt()
     -> sees cpu_has_work() and sets cpu->halted = 0

And the execution resumes, when it shouldnt have. Ideally, for secondary
CPUs, cpu::halted flag is cleared in rtas start-cpu call.

Initially, I thought we should not have interrupt during reset state.
That was the reason of ignoring decr interrupt when msr_ee was disabled
in my previous patch. BenH suggested that it is wrong from HW
perspective.

Regards,
Nikunj