Re: [Qemu-devel] [PATCH v4 5/8] spapr: Consolidate cpu init code into a routine

[David Gibson]

On Tue, Jun 16, 2015 at 08:36:34AM +0200, Thomas Huth wrote:
> On Tue, 16 Jun 2015 15:40:05 +1000
> David Gibson <address@hidden> wrote:
> 
> > On Mon, Jun 15, 2015 at 10:15:09AM +0200, Thomas Huth wrote:
> > > On Mon, 15 Jun 2015 16:59:08 +1000
> > > David Gibson <address@hidden> wrote:
> > > 
> > > > On Fri, Jun 05, 2015 at 09:55:55AM +0530, Bharata B Rao wrote:
> > > > > Factor out bits of sPAPR specific CPU initialization code into
> > > > > a separate routine so that it can be called from CPU hotplug
> > > > > path too.
> > > > > 
> > > > > While at this, use MSR_EP define instead of using 6 directly.
> > > > 
> > > > Don't do this please.  MSR[EP] is an obsolete flag from 601.  The
> > > > MSR[IP] flag that we're controlling here just happened to re-use the
> > > > same bit position, so using the existing MSR_EP define is misleading.
> > > 
> > > Actually, I had the same discussion with Bharata already some weeks ago:
> > > 
> > > http://lists.gnu.org/archive/html/qemu-ppc/2015-05/msg00133.html
> > > 
> > > > A symbolic name is good, but you should create a new one for MSR[IP]
> > > > instead.
> > > 
> > > ... and I had to realize that IP = EP. IP likely stands for "interrupt
> > > prefix" (I guess), and EP simply means "exception prefix", so just two
> > > words for the same meaning. It's just the "on 601" comment in QEMU that
> > > is completely misleading. So IMHO it should be fine to keep the
> > > "MSR_EP" here (and maybe update the comment in cpu.h with a separate
> > > patch?).
> > 
> > I don't entirely agree.  Yes EP and IP have related functions - it's
> > pretty common in ppc history that when an MSR bit is re-used it's for
> > something similar (for example IS/IR).  But MSR[IP] is still a
> > different name from MSR[EP], and I don't know if the semantics are
> > identical, though I'm sure they're similar.
> 
> I had a look at the 601 User's Manual, and it says:
> 
> Bit Name Description
> 25   EP  Exception prefix. The setting of this bit specifies whether an
>          exception vector offset is prepended with Fs or 0s.
> 
> Then, looking at the 603 User's Manual, it says:
> 
> Bit Name Description
> 25   IP  Exception prefix. The setting of this bit specifies whether an
>          exception vector offset is prepended with Fs or 0s.
> 
> So it's the very same bit, just the name has already been changed
> between the 601 and 603 already.
> 
> So I think it's either fine to keep the MSR_EP in the patch (and change
> the comment for the define in a separate patch?), or to introduce
> another define, MSR_IP, with a comment that it is the same as MSR_EP,
> just with a different name.

I'd still prefer to see the modern name in the #define.

> However, I still wonder whether this bit applies to the spapr code at
> all since it is not defined in the modern PowerISA spec anymore, as far
> as I can see.

Hrm.. yeah.  I am wondering if it is not architected, but actually
present as an implementation specific MSR bit on the modern CPUs.

I see that Bharata has reposted leaving the literal 6 as is, which is
probably a fair call until we find a consensus here.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

pgpuxLYw7aGPW.pgp
Description: PGP signature