Re: [Qemu-devel] [PATCH 1/2] target-ppc: Add pvr_match() callback

[Alexander Graf <agraf@suse.de>]

On 30.06.14 16:30, Alexey Kardashevskiy wrote:
> So far it was enough to have a base PVR value and mask per CPU
> family such as POWER7 or POWER8. However there CPUs which are
> completely architecturally compatible but have different PVRs such
> as POWER7/POWER7+ and POWER8/POWER8E. For these CPUs, top 16 bits
> are CPU family and low 16 bits are the version. The families have
> PVR base values different enough so defining a mask which
> would cover both (or potentially more) CPUs within the family is
> not possible.
>
> This adds a pvr_match() callback to PowerPCCPUClass. The default
> handler simply compares PVR defined in the class.
>
> This implements ppc_pvr_match_power7/ppc_pvr_match_power8 callbacks
> for POWER7/8 families. These check for POWER7/POWER7+ and POWER8/POWER8E.
>
> This changes ppc_cpu_compare_class_pvr_mask() not to check masks but
> use the pvr_match() callback.
>
> Since all server CPUs use the same mask, this defines one mask
> value - CPU_POWERPC_POWER_SERVER_MASK - which is used everywhere now.
> This removes other mask definitions.
>
> This removes pvr_mask from PowerPCCPUClass as it is not used anymore.
> This removes pvr initialization for POWER7/8 families as it is not used
> to find the class, the pvr_match() callback is used instead.
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>   target-ppc/cpu-models.c     |  1 -
>   target-ppc/cpu-models.h     |  5 +----
>   target-ppc/cpu-qom.h        |  2 +-
>   target-ppc/translate_init.c | 49 ++++++++++++++++++++++++++++++++-------------
>   4 files changed, 37 insertions(+), 20 deletions(-)
>
> diff --git a/target-ppc/cpu-models.c b/target-ppc/cpu-models.c
> index 9a91af9..c9112e9 100644
> --- a/target-ppc/cpu-models.c
> +++ b/target-ppc/cpu-models.c
> @@ -44,7 +44,6 @@
>           PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);                       \
>                                                                               \
>           pcc->pvr          = _pvr;                                           \
> -        pcc->pvr_mask     = CPU_POWERPC_DEFAULT_MASK;                       \

We don't need the DEFAULT_MASK anymore now, no?

>           pcc->svr          = _svr;                                           \
>           dc->desc          = _desc;                                          \
>       }                                                                       \
> diff --git a/target-ppc/cpu-models.h b/target-ppc/cpu-models.h
> index c39d03a..9d2ee91 100644
> --- a/target-ppc/cpu-models.h
> +++ b/target-ppc/cpu-models.h
> @@ -553,17 +553,14 @@ enum {
>       CPU_POWERPC_POWER6             = 0x003E0000,
>       CPU_POWERPC_POWER6_5           = 0x0F000001, /* POWER6 in POWER5 mode */
>       CPU_POWERPC_POWER6A            = 0x0F000002,
> +    CPU_POWERPC_POWER_SERVER_MASK  = 0xFFFF0000,
>       CPU_POWERPC_POWER7_BASE        = 0x003F0000,
> -    CPU_POWERPC_POWER7_MASK        = 0xFFFF0000,
>       CPU_POWERPC_POWER7_v23         = 0x003F0203,
>       CPU_POWERPC_POWER7P_BASE       = 0x004A0000,
> -    CPU_POWERPC_POWER7P_MASK       = 0xFFFF0000,
>       CPU_POWERPC_POWER7P_v21        = 0x004A0201,
>       CPU_POWERPC_POWER8E_BASE       = 0x004B0000,
> -    CPU_POWERPC_POWER8E_MASK       = 0xFFFF0000,
>       CPU_POWERPC_POWER8E_v10        = 0x004B0100,
>       CPU_POWERPC_POWER8_BASE        = 0x004D0000,
> -    CPU_POWERPC_POWER8_MASK        = 0xFFFF0000,
>       CPU_POWERPC_POWER8_v10         = 0x004D0100,
>       CPU_POWERPC_970                = 0x00390202,
>       CPU_POWERPC_970FX_v10          = 0x00391100,
> diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h
> index f1f0a52..0fee36f 100644
> --- a/target-ppc/cpu-qom.h
> +++ b/target-ppc/cpu-qom.h
> @@ -56,7 +56,7 @@ typedef struct PowerPCCPUClass {
>       void (*parent_reset)(CPUState *cpu);
>   
>       uint32_t pvr;
> -    uint32_t pvr_mask;
> +    bool (*pvr_match)(struct PowerPCCPUClass *pcc, uint32_t pvr);
>       uint64_t pcr_mask;
>       uint32_t svr;
>       uint64_t insns_flags;
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index 2ab2810..5d4fee7 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -8062,6 +8062,17 @@ static void init_proc_POWER7 (CPUPPCState *env)
>       init_proc_book3s_64(env, BOOK3S_CPU_POWER7);
>   }
>   
> +static bool ppc_pvr_match_power7(PowerPCCPUClass *pcc, uint32_t pvr)
> +{
> +    if ((pvr & CPU_POWERPC_POWER_SERVER_MASK) == CPU_POWERPC_POWER7P_BASE) {
> +        return true;
> +    }
> +    if ((pvr & CPU_POWERPC_POWER_SERVER_MASK) == CPU_POWERPC_POWER7_BASE) {
> +        return true;
> +    }
> +    return false;
> +}
> +
>   POWERPC_FAMILY(POWER7)(ObjectClass *oc, void *data)
>   {
>       DeviceClass *dc = DEVICE_CLASS(oc);
> @@ -8070,8 +8081,7 @@ POWERPC_FAMILY(POWER7)(ObjectClass *oc, void *data)
>       dc->fw_name = "PowerPC,POWER7";
>       dc->desc = "POWER7";
>       dc->props = powerpc_servercpu_properties;
> -    pcc->pvr = CPU_POWERPC_POWER7_BASE;
> -    pcc->pvr_mask = CPU_POWERPC_POWER7_MASK;
> +    pcc->pvr_match = ppc_pvr_match_power7;
>       pcc->pcr_mask = PCR_COMPAT_2_05 | PCR_COMPAT_2_06;
>       pcc->init_proc = init_proc_POWER7;
>       pcc->check_pow = check_pow_nocheck;
> @@ -8131,8 +8141,7 @@ POWERPC_FAMILY(POWER7P)(ObjectClass *oc, void *data)
>       dc->fw_name = "PowerPC,POWER7+";
>       dc->desc = "POWER7+";
>       dc->props = powerpc_servercpu_properties;
> -    pcc->pvr = CPU_POWERPC_POWER7P_BASE;
> -    pcc->pvr_mask = CPU_POWERPC_POWER7P_MASK;
> +    pcc->pvr_match = ppc_pvr_match_power7;
>       pcc->pcr_mask = PCR_COMPAT_2_05 | PCR_COMPAT_2_06;
>       pcc->init_proc = init_proc_POWER7;
>       pcc->check_pow = check_pow_nocheck;
> @@ -8189,6 +8198,17 @@ static void init_proc_POWER8(CPUPPCState *env)
>       init_proc_book3s_64(env, BOOK3S_CPU_POWER8);
>   }
>   
> +static bool ppc_pvr_match_power8(PowerPCCPUClass *pcc, uint32_t pvr)
> +{
> +    if ((pvr & CPU_POWERPC_POWER_SERVER_MASK) == CPU_POWERPC_POWER8E_BASE) {
> +        return true;
> +    }
> +    if ((pvr & CPU_POWERPC_POWER_SERVER_MASK) == CPU_POWERPC_POWER8_BASE) {
> +        return true;
> +    }
> +    return false;
> +}
> +
>   POWERPC_FAMILY(POWER8E)(ObjectClass *oc, void *data)
>   {
>       DeviceClass *dc = DEVICE_CLASS(oc);
> @@ -8197,8 +8217,7 @@ POWERPC_FAMILY(POWER8E)(ObjectClass *oc, void *data)
>       dc->fw_name = "PowerPC,POWER8";
>       dc->desc = "POWER8E";
>       dc->props = powerpc_servercpu_properties;
> -    pcc->pvr = CPU_POWERPC_POWER8E_BASE;
> -    pcc->pvr_mask = CPU_POWERPC_POWER8E_MASK;
> +    pcc->pvr_match = ppc_pvr_match_power8;
>       pcc->pcr_mask = PCR_COMPAT_2_05 | PCR_COMPAT_2_06;
>       pcc->init_proc = init_proc_POWER8;
>       pcc->check_pow = check_pow_nocheck;
> @@ -8256,13 +8275,10 @@ POWERPC_FAMILY(POWER8E)(ObjectClass *oc, void *data)
>   POWERPC_FAMILY(POWER8)(ObjectClass *oc, void *data)
>   {
>       DeviceClass *dc = DEVICE_CLASS(oc);
> -    PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
>   
>       ppc_POWER8E_cpu_family_class_init(oc, data);
>   
>       dc->desc = "POWER8";
> -    pcc->pvr = CPU_POWERPC_POWER8_BASE;
> -    pcc->pvr_mask = CPU_POWERPC_POWER8_MASK;
>   }
>   #endif /* defined (TARGET_PPC64) */
>   
> @@ -9245,7 +9261,6 @@ 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),
> @@ -9257,9 +9272,11 @@ static gint ppc_cpu_compare_class_pvr_mask(gconstpointer a, gconstpointer b)
>           return -1;
>       }
>   
> -    ret = (((pcc->pvr & pcc->pvr_mask) == (pvr & pcc->pvr_mask)) ? 0 : -1);
> +    if (pcc->pvr_match && pcc->pvr_match(pcc, pvr)) {

We can safely assume that pvr_match() always exists.


Alex