Re: [PATCH] sun4m: fix setting CPU id when more than one CPU is present

[Philippe Mathieu-Daudé]

+Leon3 maintainers

On Wed, Aug 25, 2021 at 12:39 PM Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
> On 25/08/2021 11:29, Philippe Mathieu-Daudé wrote:
> > On 8/25/21 11:51 AM, Mark Cave-Ayland wrote:
> >> Commit 24f675cd3b ("sparc/sun4m: Use start-powered-off CPUState property") 
> >> changed
> >> the sun4m CPU reset code to use the start-powered-off property and so 
> >> split the
> >> creation of the CPU into separate instantiation and realization phases to 
> >> enable
> >> the new start-powered-off property to be set.
> >>
> >> This accidentally broke sun4m machines with more than one CPU present since
> >> sparc_cpu_realizefn() sets a default CPU id, and now that realization 
> >> occurs after
> >> calling cpu_sparc_set_id() in cpu_devinit() the CPU id gets reset back to 
> >> the
> >> default instead of being uniquely encoded based upon the CPU number. As 
> >> soon as
> >> another CPU is brought online, the OS gets confused between them and 
> >> promptly
> >> panics.
> >>
> >> Resolve the issue by moving the cpu_sparc_set_id() call in cpu_devinit() 
> >> to after
> >> the point where the CPU device has been realized as before.
> >>
> >> Fixes: 24f675cd3b ("sparc/sun4m: Use start-powered-off CPUState property")
> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> >> ---
> >>   hw/sparc/sun4m.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
> >> index 42e139849e..7f3a7c0027 100644
> >> --- a/hw/sparc/sun4m.c
> >> +++ b/hw/sparc/sun4m.c
> >> @@ -803,11 +803,11 @@ static void cpu_devinit(const char *cpu_type, 
> >> unsigned int id,
> >>       cpu = SPARC_CPU(object_new(cpu_type));
> >>       env = &cpu->env;
> >>
> >> -    cpu_sparc_set_id(env, id);
> >>       qemu_register_reset(sun4m_cpu_reset, cpu);
> >>       object_property_set_bool(OBJECT(cpu), "start-powered-off", id != 0,
> >>                                &error_fatal);
> >>       qdev_realize_and_unref(DEVICE(cpu), NULL, &error_fatal);
> >> +    cpu_sparc_set_id(env, id);
> >>       *cpu_irqs = qemu_allocate_irqs(cpu_set_irq, cpu, MAX_PILS);
> >>       env->prom_addr = prom_addr;
> >>   }
> >
> > What about directly passing the CPU ID as property (untested):
> >
> > -- >8 --
> > Author: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > Date:   Wed Aug 25 12:26:02 2021 +0200
> >
> >      sun4m: fix setting CPU id when more than one CPU is present
> >
> >      Commit 24f675cd3b ("sparc/sun4m: Use start-powered-off CPUState
> > property") changed
> >      the sun4m CPU reset code to use the start-powered-off property and
> > so split the
> >      creation of the CPU into separate instantiation and realization
> > phases to enable
> >      the new start-powered-off property to be set.
> >
> >      This accidentally broke sun4m machines with more than one CPU
> > present since
> >      sparc_cpu_realizefn() sets a default CPU id, and now that
> > realization occurs after
> >      calling cpu_sparc_set_id() in cpu_devinit() the CPU id gets reset
> > back to the
> >      default instead of being uniquely encoded based upon the CPU number.
> > As soon as
> >      another CPU is brought online, the OS gets confused between them and
> > promptly
> >      panics.
> >
> >      Resolve the issue by adding a 'cpu-id' property to CPUSPARCState,
> > removing
> >      cpu_sparc_set_id().
> >
> >      Fixes: 24f675cd3b ("sparc/sun4m: Use start-powered-off CPUState
> > property")
> >      Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> >      Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >
> > diff --git a/target/sparc/cpu.h b/target/sparc/cpu.h
> > index ff8ae73002a..78ca0925d25 100644
> > --- a/target/sparc/cpu.h
> > +++ b/target/sparc/cpu.h
> > @@ -262,6 +262,7 @@ struct sparc_def_t {
> >       uint32_t mmu_cxr_mask;
> >       uint32_t mmu_sfsr_mask;
> >       uint32_t mmu_trcr_mask;

 #if !defined(TARGET_SPARC64)

> > +    uint8_t mxcc_cpuid;

#endif

> >       uint32_t mxcc_version;
> >       uint32_t features;
> >       uint32_t nwindows;
> > @@ -583,7 +584,6 @@ void cpu_raise_exception_ra(CPUSPARCState *, int,
> > uintptr_t) QEMU_NORETURN;
> >
> >   #ifndef NO_CPU_IO_DEFS
> >   /* cpu_init.c */
> > -void cpu_sparc_set_id(CPUSPARCState *env, unsigned int cpu);
> >   void sparc_cpu_list(void);
> >   /* mmu_helper.c */
> >   bool sparc_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> > diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c
> > index 7b4dec17211..8189045fdbf 100644
> > --- a/hw/sparc/leon3.c
> > +++ b/hw/sparc/leon3.c
> > @@ -238,8 +238,6 @@ static void leon3_generic_hw_init(MachineState *machine)
> >       cpu = SPARC_CPU(cpu_create(machine->cpu_type));
> >       env = &cpu->env;
> >
> > -    cpu_sparc_set_id(env, 0);
> > -
> >       /* Reset data */
> >       reset_info        = g_malloc0(sizeof(ResetData));
> >       reset_info->cpu   = cpu;
> > diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
> > index 42e139849ed..5be2e8e73f2 100644
> > --- a/hw/sparc/sun4m.c
> > +++ b/hw/sparc/sun4m.c
> > @@ -803,10 +803,10 @@ static void cpu_devinit(const char *cpu_type,
> > unsigned int id,
> >       cpu = SPARC_CPU(object_new(cpu_type));
> >       env = &cpu->env;
> >
> > -    cpu_sparc_set_id(env, id);
> >       qemu_register_reset(sun4m_cpu_reset, cpu);
> >       object_property_set_bool(OBJECT(cpu), "start-powered-off", id != 0,
> >                                &error_fatal);
> > +    object_property_set_uint(OBJECT(cpu), "cpu-id", id, &error_fatal);
> >       qdev_realize_and_unref(DEVICE(cpu), NULL, &error_fatal);
> >       *cpu_irqs = qemu_allocate_irqs(cpu_set_irq, cpu, MAX_PILS);
> >       env->prom_addr = prom_addr;
> > diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c
> > index da6b30ec747..d76929c68c7 100644
> > --- a/target/sparc/cpu.c
> > +++ b/target/sparc/cpu.c
> > @@ -194,13 +194,6 @@ static void sparc_cpu_parse_features(const char
> > *typename, char *features,
> >       g_list_free_full(minus_features, g_free);
> >   }
> >
> > -void cpu_sparc_set_id(CPUSPARCState *env, unsigned int cpu)
> > -{
> > -#if !defined(TARGET_SPARC64)
> > -    env->mxccregs[7] = ((cpu + 8) & 0xf) << 24;
> > -#endif
> > -}
> > -
> >   static const sparc_def_t sparc_defs[] = {
> >   #ifdef TARGET_SPARC64
> >       {
> > @@ -754,7 +747,7 @@ static void sparc_cpu_realizefn(DeviceState *dev,
> > Error **errp)
> >       env->nwindows = env->def.nwindows;
> >   #if !defined(TARGET_SPARC64)
> >       env->mmuregs[0] |= env->def.mmu_version;
> > -    cpu_sparc_set_id(env, 0);
> > +    env->mxccregs[7] = ((env->def.mxcc_cpuid + 8) & 0xf) << 24;
> >       env->mxccregs[7] |= env->def.mxcc_version;
> >   #else
> >       env->mmu_version = env->def.mmu_version;
> > @@ -843,6 +836,7 @@ static Property sparc_cpu_properties[] = {
> >                            qdev_prop_uint64, target_ulong),
> >       DEFINE_PROP_UINT32("fpu-version", SPARCCPU, env.def.fpu_version, 0),
> >       DEFINE_PROP_UINT32("mmu-version", SPARCCPU, env.def.mmu_version, 0),

 #if !defined(TARGET_SPARC64)

> > +    DEFINE_PROP_UINT8("cpu-id", SPARCCPU, env.def.mxcc_cpuid, 0),

#endif

> >       DEFINE_PROP("nwindows", SPARCCPU, env.def.nwindows,
> >                   qdev_prop_nwindows, uint32_t),
> >       DEFINE_PROP_END_OF_LIST()
>
> The existing code was obviously written with some flexibility here as to why 
> id !=
> cs->cpu_index, but since Blue Swirl is no longer around I don't really know 
> what the
> test cases are and why the default is required, so I'd rather preserve the 
> existing
> behaviour for now.
>
> Also MXCC is specific to 32-bit SPARC: maybe it was written this way to allow 
> for
> future multi-CPU support for 64-bit SPARC?

Will this happen?