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

[Thomas Huth]

On 29.06.2017 07:37, 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 <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
>>>>>>>>>>>>>> .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?
>>>
>>
>> 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?

Yes.

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

I assume that we will remove DD1 again once DD2 is widely available
(just like we did on POWER8), so I would not bother adding special
logic here.

> I think the absence of -cpu on the CLI should give -cpu host for KVM-
> HV.

Yes, we've got this in spapr.c:

    /* init CPUs */
    if (machine->cpu_model == NULL) {
        machine->cpu_model = kvm_enabled() ? "host" : smc->tcg_default_cpu;
    }

> FWIW currently TCG defaults to POWER8, so I guess we have to decide
> if/when we want to bump that to POWER9.

The main reason for bumping to POWER8 was the fact that some (little
endian) Linux distros started to compile with -mcpu=power8 and thus suddenly
did not work by default anymore with QEMU. As long as we do notsee something
similar with POWER9 (which I do not expect), I think there is no
urgent need right now to increase the default CPU to POWER9.

 Thomas