On 18.06.2016 02:50, alar...@ddci.com wrote: > Note change of subject from "Determining interest in PPC e500spin, > yield". > > Thomas Huth <address hidden> wrote on 06/16/2016 01:47:05 AM: > Aaron Larson <address hidden> wrote on 15.06.2016 22:12 > > in ppce500_spin.c > > AL> @@ -104,6 +108,16 @@ > AL> > AL> cpu_synchronize_state(cpu); > AL> stl_p(&curspin->pir, env->spr[SPR_PIR]); > AL> +/* The stl_p() above seems wrong to me. First of all, it seems more > appropriate > AL> + * in a guest ROM/BOOT code than in qemu emulation. However, SPR_PIR > is never > AL> + * initialized, so the effect of the stl_p() is to overwrite the > curspin->pir > AL> + * with 0. It makes more sense to load the SPR_PIR with the > curspin->pir, which > AL> + * is what the following does. > AL> + * env->spr[SPR_PIR]=ldl_p(&curspin->pir); > AL> + * Alternately SPR_PIR could be initialized from SPR_BOOKE_PIR which > is properly > AL> + * initialized, so this could also work: > AL> + * env->spr[SPR_PIR] = env->spr[SPR_BOOKE_PIR] > AL> +*/ > AL> env->nip = ldq_p(&curspin->addr) & (map_size - 1); > AL> env->gpr[3] = ldq_p(&curspin->r3); > AL> env->gpr[4] = 0; > > TH> I'm not very familiar with the e500 code, but as far as I understand > the > TH> ppce500_spin.c code, it provides the spin table facility from ePAPR > for the > TH> guests that is normally provided by the boot firmware instead. Some > more > TH> information why this has been done can be found in the original commit > TH> message here: > TH> http://git.qemu.org/?p=qemu.git;a=commitdiff;h=5c145dacacad04f751c > TH> > TH> So it's right to set up curspin->pir here (not the other way round), > but > TH> I think SPR_PIR was just a typo and should be SPR_BOOKE_PIR instead, > TH> since the PIR register for BookE CPUs has the number 286 and not 1023. > TH> So does it work for you if you simply replace the line with: > TH> > TH> stl_p(&curspin->pir, env->spr[SPR_BOOKE_PIR]); > > I verified that the spin table is properly initialized if > SPR_BOOKE_PIR is used. However this is superfluous since spin_reset() > has already initialized the PV spin table pir. I wasn't sure if there > was a desire to set the SPR_PIR as well for some reason. > > I think any of the following would be acceptable: > > 1. If the SPR_PIR needs to be set for some reason (why I'm not sure), > then set SPR_PIR to SPR_BOOKE_PIR.
SPR_PIR should not exist on a BookE processor, so I am pretty sure that this is the wrong option. > 2. Change SPR_PIR to SPR_BOOKE_PIR. > 3. Delete the line that sets curspin->pir to SPR_PIR. I don't know that code well enough, but I think both options 2 and 3 should be fine. Unless somebody has a better suggestion, I'd say go for option 2, that sounds like the safest approach to me. Thomas
