[Qemu-ppc] PPC e500spin pir improperly initialized

[alarson]

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.