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

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 09/05/2013 11:18 PM, Andreas Färber wrote:
> Am 05.09.2013 08:01, 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 adds PVR value/mask support for KVM, i.e. for -cpu host option.
>>
>> As CPU family class name for POWER7 is "POWER7-family", there is no need
>> to touch aliases.
>>
>> Cc: Andreas Färber <afaerber@suse.de>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>
>> ---
>> Changes:
>> v6:
>> * family classes are abstract again
>> * POWER7+ moved to a separate patch as it also need a separate family
>> * added ppc_cpu_class_by_pvr_mask() which is a copy of
>> ppc_cpu_class_by_pvr() but compares PVRs with masks; this function is
>> called from KVM code only to support the "-cpu host" option; unlike
>> the original search function, the new one also includes abstract family
>> classes.
>>
>> 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     |  1 +
>>  target-ppc/cpu-models.h     |  5 +++++
>>  target-ppc/cpu-qom.h        |  2 ++
>>  target-ppc/kvm.c            |  4 ++++
>>  target-ppc/translate_init.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
>>  5 files changed, 56 insertions(+), 1 deletion(-)
>>
>> diff --git a/target-ppc/cpu-models.c b/target-ppc/cpu-models.c
>> index 8dea560..04d88c5 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;                                          \
>>      }                                                                       \
>> diff --git a/target-ppc/cpu-models.h b/target-ppc/cpu-models.h
>> index d9145d1..731ec4a 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,14 @@ 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_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..3f82629 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;
>> @@ -99,6 +100,7 @@ static inline PowerPCCPU *ppc_env_get_cpu(CPUPPCState *env)
>>  #define ENV_OFFSET offsetof(PowerPCCPU, env)
>>  
>>  PowerPCCPUClass *ppc_cpu_class_by_pvr(uint32_t pvr);
>> +PowerPCCPUClass *ppc_cpu_class_by_pvr_mask(uint32_t pvr);
>>  
>>  void ppc_cpu_do_interrupt(CPUState *cpu);
>>  void ppc_cpu_dump_state(CPUState *cpu, FILE *f, fprintf_function cpu_fprintf,
>> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
>> index 8a196c6..d10dba2 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 */
>> @@ -1782,6 +1783,9 @@ static int kvm_ppc_register_host_cpu_type(void)
>>  
>>      pvr_pcc = ppc_cpu_class_by_pvr(host_pvr);
>>      if (pvr_pcc == NULL) {
>> +        pvr_pcc = ppc_cpu_class_by_pvr_mask(host_pvr);
>> +    }
>> +    if (pvr_pcc == NULL) {
>>          return -1;
>>      }
>>      type_info.parent = object_class_get_name(OBJECT_CLASS(pvr_pcc));
> 
> Not quite what I had expected but I'm okay with that as well - allows
> later reuse of the helper and doing it in two explicit steps, once
> without masking, once with, avoids having to do list filtering/ordering.


>From the discussion I understood that it should not affect the existing
code and this is all about KVM only but what was exactly  expected is still
unclear for me, sorry.



>> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
>> index 761b2e5..7e37cf2 100644
>> --- a/target-ppc/translate_init.c
>> +++ b/target-ppc/translate_init.c
>> @@ -7216,6 +7216,8 @@ 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 |
>> @@ -7251,6 +7253,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 |
>> @@ -8164,7 +8168,6 @@ static gint ppc_cpu_compare_class_pvr(gconstpointer a, gconstpointer b)
>>          return -1;
>>      }
>>  #endif
>> -
>>      return pcc->pvr == pvr ? 0 : -1;
>>  }
>>  
> 
> Stray whitespace change FWIW.


Oh.


>> @@ -8183,6 +8186,44 @@ PowerPCCPUClass *ppc_cpu_class_by_pvr(uint32_t pvr)
>>      return pcc;
>>  }
>>  
>> +static gint ppc_cpu_compare_class_pvr_mask(gconstpointer a, gconstpointer b)
>> +{
>> +    ObjectClass *oc = (ObjectClass *)a;
>> +    uint32_t pvr = *(uint32_t *)b;
>> +    PowerPCCPUClass *pcc = (PowerPCCPUClass *)a;
>> +    gint ret;
>> +
>> +    /* -cpu host does a PVR lookup during construction */
>> +    if (unlikely(strcmp(object_class_get_name(oc),
>> +                        TYPE_HOST_POWERPC_CPU) == 0)) {
>> +        return -1;
>> +    }
>> +
>> +#if defined(TARGET_PPCEMB)
>> +    if (pcc->mmu_model != POWERPC_MMU_BOOKE) {
>> +        return -1;
>> +    }
>> +#endif
>> +    ret = (((pcc->pvr & pcc->pvr_mask) == (pvr & pcc->pvr_mask)) ? 0 : -1);
>> +
>> +    return ret;
>> +}
>> +
>> +PowerPCCPUClass *ppc_cpu_class_by_pvr_mask(uint32_t pvr)
>> +{
>> +    GSList *list, *item;
>> +    PowerPCCPUClass *pcc = NULL;
>> +
>> +    list = object_class_get_list(TYPE_POWERPC_CPU, true);
>> +    item = g_slist_find_custom(list, &pvr, ppc_cpu_compare_class_pvr_mask);
>> +    if (item != NULL) {
>> +        pcc = POWERPC_CPU_CLASS(item->data);
>> +    }
>> +    g_slist_free(list);
>> +
>> +    return pcc;
>> +}
>> +
>>  static gint ppc_cpu_compare_class_name(gconstpointer a, gconstpointer b)
>>  {
>>      ObjectClass *oc = (ObjectClass *)a;
>> @@ -8557,6 +8598,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;
> 
> I guess you meant pcc->pvr_mask = CPU_POWERPC_DEFAULT_MASK instead?
> No need to zero-initialize fields in class_init.


Oh, right. I think I want it to be CPU_POWERPC_DEFAULT_MASK so I'll be able
to remove the check for TYPE_HOST_POWERPC_CPU in
ppc_cpu_compare_class_pvr_mask().

Thanks for the review.



>>      dc->realize = ppc_cpu_realizefn;
>>      dc->unrealize = ppc_cpu_unrealizefn;
>>  
> 
> Otherwise looking very good IMO. Alex?
> 
> Regards,
> Andreas
> 


-- 
Alexey