Re: [PATCH v3 3/3] hw/loongarch/virt: Register reset interface with CPU object

[Igor Mammedov <imammedo@redhat.com>]

On Thu, 4 Sep 2025 19:55:49 +0800
Bibo Mao <maobibo@loongson.cn> wrote:

> On 2025/9/4 下午4:13, Igor Mammedov wrote:
> > On Wed,  3 Sep 2025 10:35:56 +0800
> > Bibo Mao <maobibo@loongson.cn> wrote:
> >   
> >> With cpu hotplug is implemented on LoongArch virt machine, reset
> >> interface with hot-added CPU should be registered. Otherwise there
> >> will be problem if system reboots after cpu is hot-added.
> >>
> >> Now register reset interface with CPU object is realized and remove
> >> the reset interface with CPU object unrealizd.
> >>
> >> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> >> ---
> >>   hw/loongarch/boot.c    | 13 -------------
> >>   target/loongarch/cpu.c |  4 ++++
> >>   2 files changed, 4 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/hw/loongarch/boot.c b/hw/loongarch/boot.c
> >> index 5799b4c75c..a516415822 100644
> >> --- a/hw/loongarch/boot.c
> >> +++ b/hw/loongarch/boot.c
> >> @@ -350,13 +350,6 @@ static int64_t load_kernel_info(struct loongarch_boot_info *info)
> >>       return kernel_entry;
> >>   }
> >>   
> >> -static void reset_load_elf(void *opaque)
> >> -{
> >> -    LoongArchCPU *cpu = opaque;
> >> -
> >> -    cpu_reset(CPU(cpu));
> >> -}
> >> -
> >>   static void fw_cfg_add_kernel_info(struct loongarch_boot_info *info,
> >>                                      FWCfgState *fw_cfg)
> >>   {
> >> @@ -439,12 +432,6 @@ static void loongarch_direct_kernel_boot(MachineState *ms,
> >>   void loongarch_load_kernel(MachineState *ms, struct loongarch_boot_info *info)
> >>   {
> >>       LoongArchVirtMachineState *lvms = LOONGARCH_VIRT_MACHINE(ms);
> >> -    int i;
> >> -
> >> -    /* register reset function */
> >> -    for (i = 0; i < ms->smp.cpus; i++) {
> >> -        qemu_register_reset(reset_load_elf, LOONGARCH_CPU(qemu_get_cpu(i)));
> >> -    }
> >>   
> >>       info->kernel_filename = ms->kernel_filename;
> >>       info->kernel_cmdline = ms->kernel_cmdline;
> >> diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
> >> index 3a7621c0ea..9edb8ebc4d 100644
> >> --- a/target/loongarch/cpu.c
> >> +++ b/target/loongarch/cpu.c
> >> @@ -668,6 +668,9 @@ static void loongarch_cpu_realizefn(DeviceState *dev, Error **errp)
> >>   
> >>       qemu_init_vcpu(cs);
> >>       cpu_reset(cs);
> >> + #ifndef CONFIG_USER_ONLY
> >> +    qemu_register_resettable(OBJECT(dev));
> >> + #endif  
> > 
> > I'd put this in virt_cpu_plug() as last step, which should work both for
> > cold and hotpluged cpus. And drop CONFIG_USER_ONLY while at it.  
> With symmetry is the same with qemu_unregister_resettable() 
> Symmetrically, to put it in virt_cpu_unplug()?

yep

> 
> If there are many boards in future, the boards do not need to care about 
> registering cpu reset interface, which is done in CPU object already.

lets worry about more boards when that arrives.
that said, yanking reset pin on CPU is usually the board/firmware responsibility.

PS:
we have reset registered in realizefn() across QEMU, largely due to historical reasons.

> 
> Regards
> Bibo Mao
> > 
> > with that
> > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> > 
> > PS:
> > the rest of the patches are very arch specific so I won't review them.
> >   
> >>   
> >>       lacc->parent_realize(dev, errp);
> >>   }
> >> @@ -678,6 +681,7 @@ static void loongarch_cpu_unrealizefn(DeviceState *dev)
> >>   
> >>   #ifndef CONFIG_USER_ONLY
> >>       cpu_remove_sync(CPU(dev));
> >> +    qemu_unregister_resettable(OBJECT(dev));
> >>   #endif
> >>   
> >>       lacc->parent_unrealize(dev);  
> >   
>