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 <a...@ozlabs.ru>
>>>>> ---
>>>>> 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 "+":
>>>
>>> [aik@vpl4 ~]$ ls -d /proc/device-tree/cpus/PowerPC*
>>> /proc/device-tree/cpus/PowerPC,POWER7+@0
>>> /proc/device-tree/cpus/PowerPC,POWER7+@2c
>>> /proc/device-tree/cpus/PowerPC,POWER7+@10
>>> /proc/device-tree/cpus/PowerPC,POWER7+@30
>>> /proc/device-tree/cpus/PowerPC,POWER7+@14
>>> /proc/device-tree/cpus/PowerPC,POWER7+@34
>>> /proc/device-tree/cpus/PowerPC,POWER7+@18
>>> /proc/device-tree/cpus/PowerPC,POWER7+@38
>>> /proc/device-tree/cpus/PowerPC,POWER7+@1c
>>> /proc/device-tree/cpus/PowerPC,POWER7+@3c
>>> /proc/device-tree/cpus/PowerPC,POWER7+@20
>>> /proc/device-tree/cpus/PowerPC,POWER7+@4
>>> /proc/device-tree/cpus/PowerPC,POWER7+@24
>>> /proc/device-tree/cpus/PowerPC,POWER7+@8
>>> /proc/device-tree/cpus/PowerPC,POWER7+@28
>>> /proc/device-tree/cpus/PowerPC,POWER7+@c
>>>
>>> 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,POWER7+@0" 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.
===
--
Alexey
