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

[Greg Kurz]

On Wed, 28 Jun 2017 18:18:06 +0200
Laurent Vivier <address@hidden> wrote:

> On 28/06/2017 13:59, Greg Kurz wrote:
> > On Wed, 28 Jun 2017 12:23:06 +0200
> > Cédric Le Goater <address@hidden> 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, address@hidden 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 <address@hidden>
> >>>>>>>>>>> ---
> >>>>>>>>>>>  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_POWERPC_970_v22,            
> >>>>>>>>>>>     970,
> >>>>>>>>>>>                  "PowerPC 970 v2.2")
> >>>>>>>>>>>  
> >>>>>>>>>>> -    POWERPC_DEF("POWER9_v1.0",   CPU_POWERPC_POWER9_BASE,        
> >>>>>>>>>>>     POWER9,
> >>>>>>>>>>> +    POWERPC_DEF("POWER9_v1.0",   CPU_POWERPC_POWER9_DD1,         
> >>>>>>>>>>>     POWER9,
> >>>>>>>>>>>                  "POWER9 v1.0")
> >>>>>>>>>>>  
> >>>>>>>>>>>      POWERPC_DEF("970fx_v1.0",    CPU_POWERPC_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".

> Thanks,
> Laurent
> 
>

pgpQInEHZaPwj.pgp
Description: OpenPGP digital signature