Re: [Qemu-devel] PPC e500spin pir improperly initialized

[Thomas Huth <thuth@redhat.com>]

On 18.06.2016 02:50, alarson@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