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

[Cédric Le Goater]

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

C.