Re: [Qemu-devel] [PATCH v2] target-ppc: move POWER7+ to a separate family

[Alexander Graf]

On 18.11.2013, at 03:55, Alexey Kardashevskiy <address@hidden> wrote:

> On 11/12/2013 06:18 PM, Alexey Kardashevskiy wrote:
>> On 11/09/2013 11:20 AM, Alexey Kardashevskiy wrote:
>>> On 11/09/2013 03:59 AM, Andreas Färber wrote:
>>>> Am 08.11.2013 15:54, schrieb Alexey Kardashevskiy:
>>>>> On 11/09/2013 12:44 AM, Andreas Färber wrote:
>>>>>> Am 08.11.2013 03:37, schrieb Alexey Kardashevskiy:
>>>>>>> So far POWER7+ was a part of POWER7 family. However it has a different
>>>>>>> PVR base value so in order to support PVR masks, it needs a separate
>>>>>>> family class.
>>>>>>> 
>>>>>> 
>>>>>> Alexey,
>>>>>> 
>>>>>>> Another reason to make a POWER7+ family is that its name in the device
>>>>>>> tree (/proc/device-tree/cpus/cpu*) should be "Power7+" but not "Power7"
>>>>>>> and this cannot be easily fixed without a new family class.
>>>>>>> 
>>>>>>> This adds a new family class, PVR base and mask values and moves
>>>>>>> Power7+ v2.1 CPU to a new family. The class init function is copied
>>>>>>> from the POWER7 family.
>>>>>>> 
>>>>>>> Signed-off-by: Alexey Kardashevskiy <address@hidden>
>>>>>>> ---
>>>>>>> Changes:
>>>>>>> v2:
>>>>>>> * added VSX enable bit
>>>>>>> ---
>>>>>>> target-ppc/cpu-models.c     |  2 +-
>>>>>>> target-ppc/cpu-models.h     |  2 ++
>>>>>>> target-ppc/translate_init.c | 38 ++++++++++++++++++++++++++++++++++++++
>>>>>>> 3 files changed, 41 insertions(+), 1 deletion(-)
>>>>>>> 
>>>>>>> diff --git a/target-ppc/cpu-models.c b/target-ppc/cpu-models.c
>>>>>>> index 04d88c5..7c9466f 100644
>>>>>>> --- a/target-ppc/cpu-models.c
>>>>>>> +++ b/target-ppc/cpu-models.c
>>>>>>> @@ -1140,7 +1140,7 @@
>>>>>>>                 "POWER7 v2.1")
>>>>>>>     POWERPC_DEF("POWER7_v2.3",   CPU_POWERPC_POWER7_v23,             
>>>>>>> POWER7,
>>>>>>>                 "POWER7 v2.3")
>>>>>>> -    POWERPC_DEF("POWER7+_v2.1",  CPU_POWERPC_POWER7P_v21,            
>>>>>>> POWER7,
>>>>>>> +    POWERPC_DEF("POWER7+_v2.1",  CPU_POWERPC_POWER7P_v21,            
>>>>>>> POWER7P,
>>>>>>>                 "POWER7+ v2.1")
>>>>>>>     POWERPC_DEF("POWER8_v1.0",   CPU_POWERPC_POWER8_v10,             
>>>>>>> POWER8,
>>>>>>>                 "POWER8 v1.0")
>>>>>>> diff --git a/target-ppc/cpu-models.h b/target-ppc/cpu-models.h
>>>>>>> index 731ec4a..49ba4a4 100644
>>>>>>> --- a/target-ppc/cpu-models.h
>>>>>>> +++ b/target-ppc/cpu-models.h
>>>>>>> @@ -558,6 +558,8 @@ enum {
>>>>>>>     CPU_POWERPC_POWER7_v20         = 0x003F0200,
>>>>>>>     CPU_POWERPC_POWER7_v21         = 0x003F0201,
>>>>>>>     CPU_POWERPC_POWER7_v23         = 0x003F0203,
>>>>>>> +    CPU_POWERPC_POWER7P_BASE       = 0x004A0000,
>>>>>>> +    CPU_POWERPC_POWER7P_MASK       = 0xFFFF0000,
>>>>>>>     CPU_POWERPC_POWER7P_v21        = 0x004A0201,
>>>>>>>     CPU_POWERPC_POWER8_BASE        = 0x004B0000,
>>>>>>>     CPU_POWERPC_POWER8_MASK        = 0xFFFF0000,
>>>>>>> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
>>>>>>> index 35d1389..c030a20 100644
>>>>>>> --- a/target-ppc/translate_init.c
>>>>>>> +++ b/target-ppc/translate_init.c
>>>>>>> @@ -7253,6 +7253,44 @@ POWERPC_FAMILY(POWER7)(ObjectClass *oc, void 
>>>>>>> *data)
>>>>>>>     pcc->l1_icache_size = 0x8000;
>>>>>>> }
>>>>>>> 
>>>>>>> +POWERPC_FAMILY(POWER7P)(ObjectClass *oc, void *data)
>>>>>>> +{
>>>>>>> +    DeviceClass *dc = DEVICE_CLASS(oc);
>>>>>>> +    PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
>>>>>>> +
>>>>>>> +    dc->fw_name = "PowerPC,POWER7+";
>>>>>> 
>>>>>> Apart from the commit message differing from the code...
>>>>> 
>>>>> 
>>>>> In what part?
>>>> 
>>>> The spelling of POWER7. You write it should be "Power7+" but implement
>>>> it as upper-case "POWER7+" (ignoring the "PowerPC," prefix, that is).
>>> 
>>> 
>>> Ah. Sorry.
>>> 
>>> 
>>>>>> We've had this discussion before: Jacques reported that on his POWER7+
>>>>>> box only "POWER7" is shown, not "POWER7+", equivalent to my POWER5+ box
>>>>>> showing only "PowerPC,POWER5". Compare my commit, which documents this:
>>>>>> 
>>>>>> http://git.qemu.org/?p=qemu.git;a=commit;h=793826cd460828975591f289de78672af4a47ef9
>>>>>> 
>>>>>> So, adding a POWER7P family seems correct to me, just the fw_name seems
>>>>>> wrong - or you'll need to investigate further why there are conflicting
>>>>>> reports of how it is shown. Possibly based on revision or pHyp vs. SLOF?
>>>>> 
>>>>> 
>>>>> Yes we have had this discussion. Paul said it should "POWER7+". The only
>>>>> P7+ machine I have handy shows "+":
>>>>> 
>>>>> address@hidden ~]$ ls -d /proc/device-tree/cpus/PowerPC*
>>>>> /proc/device-tree/cpus/PowerPC,address@hidden
>>>>> /proc/device-tree/cpus/PowerPC,address@hidden
>>>>> /proc/device-tree/cpus/PowerPC,address@hidden
>>>>> /proc/device-tree/cpus/PowerPC,address@hidden
>>>>> /proc/device-tree/cpus/PowerPC,address@hidden
>>>>> /proc/device-tree/cpus/PowerPC,address@hidden
>>>>> /proc/device-tree/cpus/PowerPC,address@hidden
>>>>> /proc/device-tree/cpus/PowerPC,address@hidden
>>>>> /proc/device-tree/cpus/PowerPC,address@hidden
>>>>> /proc/device-tree/cpus/PowerPC,address@hidden
>>>>> /proc/device-tree/cpus/PowerPC,address@hidden
>>>>> /proc/device-tree/cpus/PowerPC,address@hidden
>>>>> /proc/device-tree/cpus/PowerPC,address@hidden
>>>>> /proc/device-tree/cpus/PowerPC,address@hidden
>>>>> /proc/device-tree/cpus/PowerPC,address@hidden
>>>>> /proc/device-tree/cpus/PowerPC,address@hidden
>>>>> 
>>>>> And this is a host, not a guest. I do not see any good reason to make dt
>>>>> names different.
>>>>> 
>>>>> And this does not really matter if there is "+" or not for anybody as far
>>>>> as we concerned, ppc64_cpu works either way.
>>>> 
>>>> Right, it may not matter, but I expect you to reference the above commit
>>>> id and explain why it should be POWER7+ after all. You failed to come up
>>>> with that answer before that patch got applied, so we need to correct
>>>> me/it now.
>>>> 
>>>> I have checked with Dinar that under Linux using the Sapphire firmware
>>>> "PowerPC,address@hidden" does indeed show up in /proc/device-tree/cpus. So
>>>> that matches what this patch changes and what you report above.
>>>> What could be different in Jacques' setup that he reported it different
>>>> from us? He was checking from AIX, is that possibly using a different
>>>> firmware, pHyp as for my POWER5+?
>>> 
>>> It must be pHyp, I do not see any other options.
>>> 
>>>> In any case let's please document this properly in the commit message
>>>> and not just make contradictory statements about what things should be.
>>> 
>>> I have no idea how to document this. No specification tells what the naming
>>> should be so anything I write there is just my assumption.
>>> 
>>> "This defines the cpu node name as PowerPC,POWER7+ to stay in sync with the
>>> Sapphire host-side firmware"?
>>> 
>>> 
>>>> Also, in qemu.git POWER7 does not have the VSX flag, only the
>>>> instruction set VSX flag. The addition of this VSX flag for POWER7+ is
>>>> not mentioned in the commit message. Does it depend on any of the
>>>> lengthy VSX Stage X series on the list or something in ppc-next changing
>>>> it for POWER7?
>>> 
>>> The PPC-related patches I post are always made against Alex Graf "ppc-next"
>>> tree and his tree contains VSX fixes. Since my patch simply copies POWER7
>>> family, I do not see much sense in mentioning all the CPU features it
>>> enables for the new family.
>>> 
>>> 
>>>> Either way, if you or Alex improve on the commit message then you can
>>>> add my Reviewed-by, I verified that the VSX flag, desc and fw_name are
>>>> the only differences.
>> 
>> 
>> Would this commit message be ok?
>> 
>> ===
>>    target-ppc: move POWER7+ to a separate family
>> 
>>    So far POWER7+ was a part of POWER7 family. However it has a different
>>    PVR base value so in order to support PVR masks, it needs a separate
>>    family class.
>> 
>>    This adds a new family class, PVR base and mask values and moves
>>    Power7+ v2.1 CPU to a new family. The class init function is copied
>>    from the POWER7 family.
>> 
>>    This defines a firmware name for the new family as "PowerPC,POWER7+"
>>    instead of previously used "PowerPC,POWER7" from the POWER7 family.
>>    The reason for that is that the Sapphire firmware (a host firmware)
>>    uses "PowerPC,POWER7+" already and since no specification defines
>>    exactly the CPU nodes naming in the device tree, we better stay
>>    in sync with the host firmware.
>> ===
> 
> 
> Anyone, ping?

Works for me. Please repost.


Alex