Re: [PATCH 02/11] hw/mips/loongson3_virt: Keep reference of vCPUs in machine_init()

[Richard Henderson]

On 1/15/25 12:32, Philippe Mathieu-Daudé wrote:
> On 15/1/25 06:18, Richard Henderson wrote:
> > On 1/12/25 13:58, Philippe Mathieu-Daudé wrote:
> > > Keep references of all vCPUs created. That allows
> > > to directly access the first vCPU without using the
> > > &first_cpu global.
> > >
> > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > ---
> > >   hw/mips/loongson3_virt.c | 9 +++++----
> > >   1 file changed, 5 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/hw/mips/loongson3_virt.c b/hw/mips/loongson3_virt.c
> > > index 47d112981a2..4b19941c1dc 100644
> > > --- a/hw/mips/loongson3_virt.c
> > > +++ b/hw/mips/loongson3_virt.c
> > > @@ -492,9 +492,8 @@ static void mips_loongson3_virt_init(MachineState *machine)
> > >   {
> > >       int i;
> > >       long bios_size;
> > > -    MIPSCPU *cpu;
> > > +    g_autofree MIPSCPU **cpus = NULL;
> > >       Clock *cpuclk;
> > > -    CPUMIPSState *env;
> > >       DeviceState *liointc;
> > >       DeviceState *ipi = NULL;
> > >       char *filename;
> > > @@ -569,13 +568,16 @@ static void mips_loongson3_virt_init(MachineState 
> > > *machine)
> > >       cpuclk = clock_new(OBJECT(machine), "cpu-refclk");
> > >       clock_set_hz(cpuclk, DEF_LOONGSON3_FREQ);
> > > +    cpus = g_new(MIPSCPU *, machine->smp.cpus);
> > >       for (i = 0; i < machine->smp.cpus; i++) {
> > > +        MIPSCPU *cpu;
> > >           int node = i / LOONGSON3_CORE_PER_NODE;
> > >           int core = i % LOONGSON3_CORE_PER_NODE;
> > >           int ip;
> > >           /* init CPUs */
> > >           cpu = mips_cpu_create_with_clock(machine->cpu_type, cpuclk, false);
> > > +        cpus[i] = cpu;
> > >           /* Init internal devices */
> > >           cpu_mips_irq_init_cpu(cpu);
> > > @@ -609,7 +611,6 @@ static void mips_loongson3_virt_init(MachineState *machine)
> > >                                  pin, cpu->env.irq[ip + 2]);
> > >           }
> > >       }
> > > -    env = &MIPS_CPU(first_cpu)->env;
> > >       /* Allocate RAM/BIOS, 0x00000000~0x10000000 is alias of 
> > > 0x80000000~0x90000000 */
> > >       memory_region_init_rom(bios, NULL, "loongson3.bios",
> > > @@ -640,7 +641,7 @@ static void mips_loongson3_virt_init(MachineState *machine)
> > >           loaderparams.kernel_filename = kernel_filename;
> > >           loaderparams.kernel_cmdline = kernel_cmdline;
> > >           loaderparams.initrd_filename = initrd_filename;
> > > -        loaderparams.kernel_entry = load_kernel(env);
> > > +        loaderparams.kernel_entry = load_kernel(&cpus[0]->env);
>
> > We only ever use cpu[0].  We don't really need the whole array.
>
> Yes. What about:
>
> -- >8 --
> commit ffc8c8873c0c102457f0e660437874555b022cc2
> Author: Philippe Mathieu-Daudé <philmd@linaro.org>
> Date:   Sun Jan 12 21:01:24 2025 +0100
>
>      hw/mips/loongson3_virt: Invert vCPU creation order to remove &first_cpu
>
>      Create vCPUs from the last one to the first one.
>      No need to use the &first_cpu global since we already
>      have it referenced.
>
>      Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
> diff --git a/hw/mips/loongson3_virt.c b/hw/mips/loongson3_virt.c
> index 47d112981a2..488eba495cd 100644
> --- a/hw/mips/loongson3_virt.c
> +++ b/hw/mips/loongson3_virt.c
> @@ -496,3 +496,2 @@ static void mips_loongson3_virt_init(MachineState *machine)
>       Clock *cpuclk;
> -    CPUMIPSState *env;
>       DeviceState *liointc;
> @@ -571,3 +570,3 @@ static void mips_loongson3_virt_init(MachineState *machine)
>
> -    for (i = 0; i < machine->smp.cpus; i++) {
> +    for (i = machine->smp.cpus - 1; i >= 0; --i) {
>           int node = i / LOONGSON3_CORE_PER_NODE;
> @@ -611,3 +610,2 @@ static void mips_loongson3_virt_init(MachineState *machine)
>       }
> -    env = &MIPS_CPU(first_cpu)->env;
>
> @@ -642,3 +640,3 @@ static void mips_loongson3_virt_init(MachineState *machine)
>           loaderparams.initrd_filename = initrd_filename;
> -        loaderparams.kernel_entry = load_kernel(env);
> +        loaderparams.kernel_entry = load_kernel(&cpu->env);
>
> ---

Looks good, thanks.


r~