From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:46292)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <thuth@redhat.com>) id 1bEzlu-0004gm-8B
	for qemu-devel@nongnu.org; Mon, 20 Jun 2016 10:01:52 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <thuth@redhat.com>) id 1bEzlp-00062B-6U
	for qemu-devel@nongnu.org; Mon, 20 Jun 2016 10:01:49 -0400
References: <OF0C52A6C2.AC06C8D6-ON86257FD1.007B3DE1-86257FD1.007F1F64@ddci.com>
	<20160615041757.GI4882@voom.fritz.box>
	<OFB9FC4520.28F3555F-ON86257FD3.006DFC02-86257FD3.006F050A@ddci.com>
	<57624B69.1070809@redhat.com>
	<OF244CFF25.7308AE37-ON86257FD6.0002FE3B-86257FD6.0004A91F@ddci.com>
From: Thomas Huth <thuth@redhat.com>
Message-ID: <5767F744.3000708@redhat.com>
Date: Mon, 20 Jun 2016 16:01:40 +0200
MIME-Version: 1.0
In-Reply-To: <OF244CFF25.7308AE37-ON86257FD6.0002FE3B-86257FD6.0004A91F@ddci.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] PPC e500spin pir improperly initialized
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel/>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: alarson@ddci.com, agraf@suse.de
Cc: David Gibson <david@gibson.dropbear.id.au>, qemu-devel@nongnu.org, qemu-ppc@nongnu.org

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