[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5] powerpc: add PVR mask support
|
From: |
Alexey Kardashevskiy |
|
Subject: |
Re: [Qemu-devel] [PATCH v5] powerpc: add PVR mask support |
|
Date: |
Wed, 28 Aug 2013 21:01:53 +1000 |
|
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7 |
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 <address@hidden>
>> Signed-off-by: Alexey Kardashevskiy <address@hidden>
>>
>> ---
>>
>> 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?
> Also, the
> comparison should be:
> (pcc->pvr & pcc->pvr_mask) == (pvr & pcc->pvr_mask)
> to match specific models where available. Note that pvr_mask gets
> inherited from the family like any other class field.
Will it protect us from some real case (I cannot think of any good example
right now) or it is just against theoretical case when the mask is from one
class and the pvr from another?
>> }
>>
>> PowerPCCPUClass *ppc_cpu_class_by_pvr(uint32_t pvr)
>> @@ -8326,6 +8366,8 @@ static void ppc_cpu_list_entry(gpointer data, gpointer
>> user_data)
>> CPUListState *s = user_data;
>> PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
>> const char *typename = object_class_get_name(oc);
>> + ObjectClass *oc_parent = object_class_get_parent(oc);
>> + const char *typename_parent = object_class_get_name(oc_parent);
>> char *name;
>> int i;
>>
>> @@ -8338,6 +8380,11 @@ static void ppc_cpu_list_entry(gpointer data,
>> gpointer user_data)
>> return;
>> }
>>
>> + /* Do not show non-abstract family CPU classes */
>> + if (unlikely(strcmp(typename_parent, TYPE_POWERPC_CPU) == 0)) {
>> + return;
>> + }
>> +
>> name = g_strndup(typename,
>> strlen(typename) - strlen("-" TYPE_POWERPC_CPU));
>> (*s->cpu_fprintf)(s->file, "PowerPC %-16s PVR %08x\n",
>
> Becomes unnecessary when dropping the abstractness change.
Ah. Sorry. Just realized what you meant. Mosaic became a picture now :)
>
>> @@ -8557,6 +8604,8 @@ static void ppc_cpu_class_init(ObjectClass *oc, void
>> *data)
>> DeviceClass *dc = DEVICE_CLASS(oc);
>>
>> pcc->parent_realize = dc->realize;
>> + pcc->pvr = CPU_POWERPC_DEFAULT_MASK;
>> + pcc->pvr_mask = 0;
>> dc->realize = ppc_cpu_realizefn;
>> dc->unrealize = ppc_cpu_unrealizefn;
--
Alexey