On 08/28/2013 09:01 PM, Alexey Kardashevskiy wrote:
> On 08/28/2013 08:49 PM, Andreas Färber wrote:
>> Am 28.08.2013 12:37, schrieb Alexey Kardashevskiy:
>>> IBM POWERPC processors encode PVR as a CPU family in higher 16 bits and
>>> a CPU version in lower 16 bits. Since there is no significant change
>>> in behavior between versions, there is no point to add every single CPU
>>> version in QEMU's CPU list. Also, new CPU versions of already supported
>>> CPU won't break the existing code.
>>>
>>> This does the following:
>>> 1. add a PVR mask support for a CPU family;
>>> 2. make CPU family not abstract;
>>> 3. hide family CPU classes from "-cpu ?" list.
>>>
>>> As CPU family class name for POWER7 is "POWER7-family", there is no need
>>> to touch aliases.
>>>
>>> Cc: Andreas Färber <afaer...@suse.de>
>>> Signed-off-by: Alexey Kardashevskiy <a...@ozlabs.ru>
>>>
>>> ---
>>>
>>> I would really love to avoid adding masks to other classes as long as
>>> possible -
>>> I do not know them well, they do not know me, I am scared of breaking them
>>> :)
>>>
>>>
>>> ---
>>> Changes:
>>> v5:
>>> * removed pvr_default
>>> * added hiding of family CPU classes (should not appear in -cpu ?)
>>> * separated POWER7+ into a class (it used to be POWER7) and added a mask
>>> for it
>>> * added mask for POWER8
>>> * added _BASE suffix to PVR mask constants and moved them before actual CPUs
>>>
>>> v4:
>>> * removed bogus layer from hierarchy
>>>
>>> v3:
>>> * renamed macros to describe the functionality better
>>> * added default PVR value for the powerpc cpu family (what alias used to do)
>>>
>>> v2:
>>> * aliases are replaced with another level in class hierarchy
>>> ---
>>> target-ppc/cpu-models.c | 3 ++-
>>> target-ppc/cpu-models.h | 7 ++++++
>>> target-ppc/cpu-qom.h | 1 +
>>> target-ppc/kvm.c | 1 +
>>> target-ppc/translate_init.c | 53
>>> +++++++++++++++++++++++++++++++++++++++++++--
>>> 5 files changed, 62 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/target-ppc/cpu-models.c b/target-ppc/cpu-models.c
>>> index 8dea560..7c9466f 100644
>>> --- a/target-ppc/cpu-models.c
>>> +++ b/target-ppc/cpu-models.c
>>> @@ -44,6 +44,7 @@
>>> PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
>>> \
>>>
>>> \
>>> pcc->pvr = _pvr;
>>> \
>>> + pcc->pvr_mask = CPU_POWERPC_DEFAULT_MASK;
>>> \
>>> pcc->svr = _svr;
>>> \
>>> dc->desc = _desc;
>>> \
>>> }
>>> \
>>> @@ -1139,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")
>>
>> As always, please put independent changes like this POWER7P introduction
>> in its own patch.
>>
>>> diff --git a/target-ppc/cpu-models.h b/target-ppc/cpu-models.h
>>> index d9145d1..49ba4a4 100644
>>> --- a/target-ppc/cpu-models.h
>>> +++ b/target-ppc/cpu-models.h
>>> @@ -39,6 +39,7 @@ extern PowerPCCPUAlias ppc_cpu_aliases[];
>>>
>>> /*****************************************************************************/
>>> /* PVR definitions for most known PowerPC
>>> */
>>> enum {
>>> + CPU_POWERPC_DEFAULT_MASK = 0xFFFFFFFF,
>>> /* PowerPC 401 family */
>>> /* Generic PowerPC 401 */
>>> #define CPU_POWERPC_401 CPU_POWERPC_401G2
>>> @@ -552,10 +553,16 @@ enum {
>>> CPU_POWERPC_POWER6 = 0x003E0000,
>>> CPU_POWERPC_POWER6_5 = 0x0F000001, /* POWER6 in POWER5 mode
>>> */
>>> CPU_POWERPC_POWER6A = 0x0F000002,
>>> + CPU_POWERPC_POWER7_BASE = 0x003F0000,
>>> + CPU_POWERPC_POWER7_MASK = 0xFFFF0000,
>>> 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,
>>> CPU_POWERPC_POWER8_v10 = 0x004B0100,
>>> CPU_POWERPC_970 = 0x00390202,
>>> CPU_POWERPC_970FX_v10 = 0x00391100,
>>> diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h
>>> index f3c710a..0ae8b09 100644
>>> --- a/target-ppc/cpu-qom.h
>>> +++ b/target-ppc/cpu-qom.h
>>> @@ -54,6 +54,7 @@ typedef struct PowerPCCPUClass {
>>> void (*parent_reset)(CPUState *cpu);
>>>
>>> uint32_t pvr;
>>> + uint32_t pvr_mask;
>>> uint32_t svr;
>>> uint64_t insns_flags;
>>> uint64_t insns_flags2;
>>> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
>>> index 30a870e..d7d9865 100644
>>> --- a/target-ppc/kvm.c
>>> +++ b/target-ppc/kvm.c
>>> @@ -1732,6 +1732,7 @@ static void kvmppc_host_cpu_class_init(ObjectClass
>>> *oc, void *data)
>>> uint32_t icache_size = kvmppc_read_int_cpu_dt("i-cache-size");
>>>
>>> /* Now fix up the class with information we can query from the host */
>>> + pcc->pvr = mfpvr();
>>>
>>> if (vmx != -1) {
>>> /* Only override when we know what the host supports */
>>> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
>>> index 761b2e5..39cb69b 100644
>>> --- a/target-ppc/translate_init.c
>>> +++ b/target-ppc/translate_init.c
>>> @@ -3105,7 +3105,6 @@ static int check_pow_hid0_74xx (CPUPPCState *env)
>>> glue(glue(ppc_, _name), _cpu_family_type_info) = {
>>> \
>>> .name = stringify(_name) "-family-" TYPE_POWERPC_CPU,
>>> \
>>> .parent = TYPE_POWERPC_CPU,
>>> \
>>> - .abstract = true,
>>> \
>>> .class_init = glue(glue(ppc_, _name), _cpu_family_class_init),
>>> \
>>> };
>>> \
>>>
>>> \
>>
>> Comment not yet incorporated.
>>
>>> @@ -7216,6 +7215,45 @@ POWERPC_FAMILY(POWER7)(ObjectClass *oc, void *data)
>>>
>>> dc->fw_name = "PowerPC,POWER7";
>>> dc->desc = "POWER7";
>>> + pcc->pvr = CPU_POWERPC_POWER7_BASE;
>>> + pcc->pvr_mask = CPU_POWERPC_POWER7_MASK;
>>> + pcc->init_proc = init_proc_POWER7;
>>> + pcc->check_pow = check_pow_nocheck;
>>> + pcc->insns_flags = PPC_INSNS_BASE | PPC_ISEL | PPC_STRING | PPC_MFTB |
>>> + PPC_FLOAT | PPC_FLOAT_FSEL | PPC_FLOAT_FRES |
>>> + PPC_FLOAT_FSQRT | PPC_FLOAT_FRSQRTE |
>>> + PPC_FLOAT_STFIWX |
>>> + PPC_CACHE | PPC_CACHE_ICBI | PPC_CACHE_DCBZ |
>>> + PPC_MEM_SYNC | PPC_MEM_EIEIO |
>>> + PPC_MEM_TLBIE | PPC_MEM_TLBSYNC |
>>> + PPC_64B | PPC_ALTIVEC |
>>> + PPC_SEGMENT_64B | PPC_SLBI |
>>> + PPC_POPCNTB | PPC_POPCNTWD;
>>> + pcc->insns_flags2 = PPC2_VSX | PPC2_DFP | PPC2_DBRX | PPC2_ISA205;
>>> + pcc->msr_mask = 0x800000000204FF37ULL;
>>> + pcc->mmu_model = POWERPC_MMU_2_06;
>>> +#if defined(CONFIG_SOFTMMU)
>>> + pcc->handle_mmu_fault = ppc_hash64_handle_mmu_fault;
>>> +#endif
>>> + pcc->excp_model = POWERPC_EXCP_POWER7;
>>> + pcc->bus_model = PPC_FLAGS_INPUT_POWER7;
>>> + pcc->bfd_mach = bfd_mach_ppc64;
>>> + pcc->flags = POWERPC_FLAG_VRE | POWERPC_FLAG_SE |
>>> + POWERPC_FLAG_BE | POWERPC_FLAG_PMM |
>>> + POWERPC_FLAG_BUS_CLK | POWERPC_FLAG_CFAR;
>>> + pcc->l1_dcache_size = 0x8000;
>>> + 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+";
>>
>> According to the only reply I received, it's "POWER7", not "POWER7+" -
>> see my patch description. If that information was wrong, we'll need to
>> move POWER7P introduction before my fw_name patch and update it accordingly.
>
>
> This I will double check tomorrow with Paul.
>
>>
>>> + dc->desc = "POWER7+";
>>> + pcc->pvr = CPU_POWERPC_POWER7P_BASE;
>>> + pcc->pvr_mask = CPU_POWERPC_POWER7P_MASK;
>>> pcc->init_proc = init_proc_POWER7;
>>> pcc->check_pow = check_pow_nocheck;
>>> pcc->insns_flags = PPC_INSNS_BASE | PPC_ISEL | PPC_STRING | PPC_MFTB |
>>> @@ -7251,6 +7289,8 @@ POWERPC_FAMILY(POWER8)(ObjectClass *oc, void *data)
>>>
>>> dc->fw_name = "PowerPC,POWER8";
>>> dc->desc = "POWER8";
>>> + pcc->pvr = CPU_POWERPC_POWER8_BASE;
>>> + pcc->pvr_mask = CPU_POWERPC_POWER8_MASK;
>>> pcc->init_proc = init_proc_POWER7;
>>> pcc->check_pow = check_pow_nocheck;
>>> pcc->insns_flags = PPC_INSNS_BASE | PPC_STRING | PPC_MFTB |
>>> @@ -8165,7 +8205,7 @@ static gint ppc_cpu_compare_class_pvr(gconstpointer
>>> a, gconstpointer b)
>>> }
>>> #endif
>>>
>>> - return pcc->pvr == pvr ? 0 : -1;
>>> + return ((pcc->pvr == (pvr & pcc->pvr_mask)) ? 0 : -1);
>>
>> As discussed in lengths this is the wrong place IMO.
>
> Sorry, I missed that. What is the correct place? Or do you mean here a
> am-I-compatible-with-this-host callback?
Andreas, could you please comment on that? Thanks.
--
Alexey
