+Leon3 maintainers On Wed, Aug 25, 2021 at 12:39 PM Mark Cave-Ayland <mark.cave-ayl...@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-ayl...@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é <f4...@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-ayl...@ilande.co.uk> > > Signed-off-by: Philippe Mathieu-Daudé <f4...@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?