Re: [RFC PATCH 00/12] cpu_reset clean-ups for arm, sh4, mips, m68k and tricore

["Alex Bennée" <alex.bennee@linaro.org>]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> On 8/1/26 15:34, Alex Bennée wrote:
>> We tend to apply cpu_reset inconsistently throughout our various
>> models which leads to unintended ordering dependencies. This got in
>> the way in my last plugins series:
>>    https://patchew.org/QEMU/20251219190849.238323-1-alex.bennee@linaro.org/
>> where I needed to shuffle things around to ensure that gdb register
>> creation was done after dependant peripherals had created their cpu
>> interfaces.
>> Regardless of that we do have a proper reset interface now and most
>> architectures have moved to it. This series attempts to clean-up the
>> remaining cases with proper qemu_register_reset() calls so reset is
>> called when we intend to.
>
> Last time I was blocked at this comment:
> https://lore.kernel.org/qemu-devel/20231128170008.57ddb03e@imammedo.users.ipa.redhat.com/

From that:

 --cpu_reset()  <- brings cpu to known (after reset|poweron) state
   cpu_common_realizefn()
       if (dev->hotplugged) {                                                       
           cpu_synchronize_post_init(cpu);                                          
           cpu_resume(cpu);                                                         
       }
 ++cpu_reset()  <-- looks to be too late, we already told hypervisor to run undefined CPU, didn't we?

I would posit that the hotplug path is different as we online the CPU as
soon as its ready. Maybe that is just special cased as:

       if (dev->hotplugged) {
           cpu_reset(cpu);
           cpu_synchronize_post_init(cpu);                                          
           cpu_resume(cpu);                                                         
       }

Unless hotplug should also honour the reset tree in which case that
logic could be moved:

  void cpu_reset(CPUState *cpu)
  {
      DeviceState *dev = DEVICE(cpu);

      if (!dev->hotplugged) {
          device_cold_reset(DEVICE(cpu));
      } else {
          /* hotplugging implies we should know how to setup */
          cpu_synchronize_post_init(cpu);    
      }
      trace_cpu_reset(cpu->cpu_index);

  #ifdef CONFIG_TCG
      /*
       * Because vCPUs get "started" before the machine is ready we often
       * can't provide all the information plugins need during
       * cpu_common_initfn. However the vCPU will be reset a few times
       * before we eventually set things going giving plugins an
       * opportunity to update things.
       */
      qemu_plugin_vcpu_reset_hook(cpu);
  #endif
  }

Do we have test cases for hotplugging CPUs?

>
>> Alex Bennée (12):
>>    target/sh4: drop cpu_reset from realizefn
>>    target/m68k: introduce env->reset_pc
>>    hw/m68k: register a nextcube_cpu_reset handler
>>    hw/m68k: register a mcf5208evb_cpu_reset handler
>>    hw/m68k: register a an5206_cpu_reset handler
>>    hw/m68k: just use reset_pc for virt platform
>>    target/m68k: drop cpu_reset on realizefn
>>    hw/mips: defer finalising gcr_base until reset time
>>    hw/mips: drop cpu_reset in mips_cpu_realizefn
>>    target/tricore: move cpu_reset from tricore_cpu_realizefn
>>    target/arm: remove extraneous cpu_reset from realizefn
>>    include/hw: expand cpu_reset function docs

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro