Re: [Qemu-devel] [Qemu-ppc] [PATCH] target/ppc/cpu-models: set POWER9_v1.0 as POWER9 DD1

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 9531 bytes --]

On Thu, Jun 29, 2017 at 03:42:03PM +1000, Suraj Jitindar Singh wrote:
> On Thu, 2017-06-29 at 15:37 +1000, Suraj Jitindar Singh wrote:
> > On Wed, 2017-06-28 at 18:41 +0200, Greg Kurz wrote:
> > > On Wed, 28 Jun 2017 18:18:06 +0200
> > > Laurent Vivier <lvivier@redhat.com> wrote:
> > > 
> > > > On 28/06/2017 13:59, Greg Kurz wrote:
> > > > > On Wed, 28 Jun 2017 12:23:06 +0200
> > > > > Cédric Le Goater <clg@kaod.org> wrote:
> > > > >   
> > > > > > On 06/28/2017 11:18 AM, Laurent Vivier wrote:  
> > > > > > > On 28/06/2017 11:11, Cédric Le Goater wrote:    
> > > > > > > > On 06/28/2017 10:18 AM, David Gibson wrote:    
> > > > > > > > > On Wed, Jun 28, 2017 at 09:09:24AM +0200, Thomas Huth
> > > > > > > > > wrote:    
> > > > > > > > > > On 28.06.2017 03:42, joserz@linux.vnet.ibm.com
> > > > > > > > > > wrote:    
> > > > > > > > > > > On Fri, Jun 23, 2017 at 04:10:55PM +0200, Laurent
> > > > > > > > > > > Vivier wrote:    
> > > > > > > > > > > > On 23/06/2017 11:21, David Gibson wrote:    
> > > > > > > > > > > > > On Thu, Jun 22, 2017 at 01:31:24PM +0200,
> > > > > > > > > > > > > Thomas
> > > > > > > > > > > > > Huth wrote:    
> > > > > > > > > > > > > > On 22.06.2017 13:26, Laurent Vivier
> > > > > > > > > > > > > > wrote:    
> > > > > > > > > > > > > > > CPU_POWERPC_POWER9_DD1 is 0x004E0100, so
> > > > > > > > > > > > > > > this
> > > > > > > > > > > > > > > is the POWER9 v1.0.
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > When we run qemu on a POWER9 DD1 host, we
> > > > > > > > > > > > > > > must use either
> > > > > > > > > > > > > > > "-cpu host" or "-cpu POWER9", but in the
> > > > > > > > > > > > > > > latter case it fails with
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > >     Unable to find sPAPR CPU Core
> > > > > > > > > > > > > > > definition
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > because POWER9 DD1 doesn't appear in the
> > > > > > > > > > > > > > > list
> > > > > > > > > > > > > > > of known CPUs.
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > This patch fixes this by defining
> > > > > > > > > > > > > > > POWER9_v1.0
> > > > > > > > > > > > > > > with POWER9 DD1
> > > > > > > > > > > > > > > PVR instead of CPU_POWERPC_POWER9_BASE.
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > Signed-off-by: Laurent Vivier <lvivier@redh
> > > > > > > > > > > > > > > at
> > > > > > > > > > > > > > > .com>
> > > > > > > > > > > > > > > ---
> > > > > > > > > > > > > > >  target/ppc/cpu-models.c | 2 +-
> > > > > > > > > > > > > > >  1 file changed, 1 insertion(+), 1
> > > > > > > > > > > > > > > deletion(-
> > > > > > > > > > > > > > > )
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > diff --git a/target/ppc/cpu-models.c
> > > > > > > > > > > > > > > b/target/ppc/cpu-models.c
> > > > > > > > > > > > > > > index 4d3e635..a22363c 100644
> > > > > > > > > > > > > > > --- a/target/ppc/cpu-models.c
> > > > > > > > > > > > > > > +++ b/target/ppc/cpu-models.c
> > > > > > > > > > > > > > > @@ -1144,7 +1144,7 @@
> > > > > > > > > > > > > > >      POWERPC_DEF("970_v2.2",      CPU_POWER
> > > > > > > > > > > > > > > PC
> > > > > > > > > > > > > > > _970_v22,                970,
> > > > > > > > > > > > > > >                  "PowerPC 970 v2.2")
> > > > > > > > > > > > > > >  
> > > > > > > > > > > > > > > -    POWERPC_DEF("POWER9_v1.0",   CPU_POWER
> > > > > > > > > > > > > > > PC
> > > > > > > > > > > > > > > _POWER9_BASE,            POWER9,
> > > > > > > > > > > > > > > +    POWERPC_DEF("POWER9_v1.0",   CPU_POWER
> > > > > > > > > > > > > > > PC
> > > > > > > > > > > > > > > _POWER9_DD1,             POWER9,
> > > > > > > > > > > > > > >                  "POWER9 v1.0")
> > > > > > > > > > > > > > >  
> > > > > > > > > > > > > > >      POWERPC_DEF("970fx_v1.0",    CPU_POWER
> > > > > > > > > > > > > > > PC
> > > > > > > > > > > > > > > _970FX_v10,              970,
> > > > > > > > > > > > > > >    
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > I think this also makes sense for running in
> > > > > > > > > > > > > > TCG mode to get a valid
> > > > > > > > > > > > > > real PVR there.    
> > > > > > > > > > > > > 
> > > > > > > > > > > > > I'm not so convinced.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > IIUC, this will make TCG default (for now) to a
> > > > > > > > > > > > > DD1 POWER9.  That's a)
> > > > > > > > > > > > > probably not what anyone wants - who'd select a
> > > > > > > > > > > > > buggy prototype and b)
> > > > > > > > > > > > > not accurate - TCG does not implement DD1's
> > > > > > > > > > > > > bugs.    
> > > > > > > > > > > > 
> > > > > > > > > > > > According to the POWER8 user manual (I didn't
> > > > > > > > > > > > fine
> > > > > > > > > > > > the POWER9 one):
> > > > > > > > > > > > 
> > > > > > > > > > > > "3.6.3.1 Processor Version Register (PVR)
> > > > > > > > > > > > 
> > > > > > > > > > > > The processor revision level (PVR[16:31]) starts
> > > > > > > > > > > > at
> > > > > > > > > > > > x‘0100’, indicating
> > > > > > > > > > > > revision ‘1.0’. As revisions are made, bits
> > > > > > > > > > > > [29:31]
> > > > > > > > > > > > will indicate minor
> > > > > > > > > > > > revisions. Similarly, bits [20:23] indicate major
> > > > > > > > > > > > changes."
> > > > > > > > > > > > 
> > > > > > > > > > > > POWER9 DD1 PVR is 0x004E0100, so this is really
> > > > > > > > > > > > version 1.0 of the POWER9.
> > > > > > > > > > > > 
> > > > > > > > > > > > Perhaps we can define POWER9_v1.0 as
> > > > > > > > > > > > CPU_POWERPC_POWER9_DD1, and
> > > > > > > > > > > > introduce a POWER9_v0.0 set to
> > > > > > > > > > > > CPU_POWERPC_POWER9_BASE and define it as
> > > > > > > > > > > > the default one?    
> > > > > > > > > > > 
> > > > > > > > > > > I like the suggestion to set a v0.0 to
> > > > > > > > > > > CPU_POWERPC_POWER9_BASE. But, I
> > > > > > > > > > > think we could have only that option, removing the
> > > > > > > > > > > CPU_POWERPC_POWER9_DD1 entry.    
> > > > > > > > > > 
> > > > > > > > > > I really dislike the idea of having a CPU called
> > > > > > > > > > "v0.0"
> > > > > > > > > > ... we do not
> > > > > > > > > > have this for any other CPU generation, and it sounds
> > > > > > > > > > like it could be
> > > > > > > > > > very confusing for the users (you'd need to document
> > > > > > > > > > somewhere what the
> > > > > > > > > > v0.0 exactly means). If we really want to go this
> > > > > > > > > > way,
> > > > > > > > > > I think we should
> > > > > > > > > > name it "POWER9-generic" or "PowerISA-3.0" or
> > > > > > > > > > something
> > > > > > > > > > similar instead.
> > > > > > > > > > 
> > > > > > > > > > Or does somebody already know the exact PVR for DD2?
> > > > > > > > > > If
> > > > > > > > > > so, we could
> > > > > > > > > > simply add a POWER9_v2.0 CPU already and let the
> > > > > > > > > > POWER9
> > > > > > > > > > alias point to
> > > > > > > > > > that version instead.    
> > > > > > > > > 
> > > > > > > > > Yes, I think that's a better idea.  I don't know the
> > > > > > > > > DD2
> > > > > > > > > PVR, but I'm
> > > > > > > > > pretty sure we should be able to find out from someone
> > > > > > > > > at
> > > > > > > > > IBM.
> > > > > > > > > 
> > > > > > > > > I've CCed Sam & Suraj - can you ask Mikey or someone
> > > > > > > > > what
> > > > > > > > > the PVR
> > > > > > > > > value for DD2.0 will be?    
> > > > > > > > 
> > > > > > > > I would expect something like :
> > > > > > > > 
> > > > > > > >  0x200D104980000000UL; /* P9 Nimbus DD2.0 */    
> > > > > > > 
> > > > > > > 
> > > > > > > I would expect something like 0x004Exxxx.    
> > > > > > 
> > > > > > ah yes, I am mistaking the PVR and the CFAM ID. 
> > > > > > 
> > > > > > C. 
> > > > > >    
> > > > > 
> > > > > According to https://patchwork.ozlabs.org/patch/776052/
> > > > > 
> > > > > POWER9 DD2's PVR is expected to be 0x004e1200
> > > > >  
> > > > 
> > > > So, perhaps I can send a v2 of the patch with POWER9_v1.0 set to
> > > > DD1
> > > > PVR, and POWER9_v2.0 set to DD2 PVR?
> > > > 
> > > 
> > > FWIW Thomas suggested to do just that and David agreed it was "a
> > > better idea".
> > 
> > I assume we would have just -cpu POWER9 alias to DD2?
> > 
> > We probably need to have a nice abort if someone tries to run TCG
> > with
> > DD1, I'm not sure where it will break but I'm fairly sure it won't
> > boot.
> > 
> > That makes the assumption that DD2 doesn't require any work arounds
> > which TCG can't handle.
> 
> Actually TCG is really a non-issue since we'll just go into the POWER9
> architected mode.
> 
> Can't we just have -cpu POWER9 alias to DD1 for now and add DD2 when we
> know the pvr?

No, because calling what qemu does DD1 is simply not accurate, in
important and guest-visible ways.

What we should do is add in DD2.0 - we know the PVR, even if the
chip's not out yet.  Then alias POWER9 to that.

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]