Re: [Qemu-devel] [PATCH v5] powerpc: add PVR mask support

[Alexey Kardashevskiy]

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


Andreas, could you please comment on that? Thanks.



-- 
Alexey