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

[Andreas Färber]

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

>> 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+?
In any case let's please document this properly in the commit message
and not just make contradictory statements about what things should be.

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

Thanks,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg