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. 2. Change SPR_PIR to SPR_BOOKE_PIR. 3. Delete the line that sets curspin->pir to SPR_PIR. I'm fine submitting a patch for whatever solution folks deem appropriate.