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

[Laurent Vivier <lvivier@redhat.com>]

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@redhat.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_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?

Thanks,
Laurent