Re: [PATCH v4] target/ppc: Fix host PVR matching for KVM

[Daniel Henrique Barboza <danielhb413@gmail.com>]

On 7/30/22 22:33, Nicholas Piggin wrote:
> ppc_cpu_compare_class_pvr_mask() should match the best CPU class in the
> family, because it is used by the KVM subsystem to find the host CPU
> class. Since commit 03ae4133ab8 ("target-ppc: Add pvr_match()
> callback"), it matches any class in the family (the first one in the
> comparison list).
> 
> Since commit f30c843ced5 ("ppc/pnv: Introduce PowerNV machines with
> fixed CPU models"), pnv has relied on pnv_match having these new
> semantics to check machine compatibility with a CPU family.
> 
> Resolve this by adding a parameter to the pvr_match function to select
> the best or any match, and restore the old behaviour for the KVM case.
> 
> Prior to this fix, e.g., a POWER9 DD2.3 KVM host matches to the
> power9_v1.0 class (because that happens to be the first POWER9 family
> CPU compared). After the patch, it matches the power9_v2.0 class.
> 
> This approach requires pnv_match contain knowledge of the CPU classes
> implemented in the same family, which feels ugly. But pushing the 'best'
> match down to the class would still require they know about one another
> which is not obviously much better. For now this gets things working.
> 
> Fixes: 03ae4133ab8 ("target-ppc: Add pvr_match() callback")
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

And queued in gitlab.com/danielhb/qemu/tree/ppc-7.2.


Daniel

> 
> This is extracted from patch 1 in the series here:
> 
> https://lists.gnu.org/archive/html/qemu-ppc/2022-03/msg00226.html
> 
> I finally went back and worked out what the issue was, which is the
> new usage of pvr_match that pnv has. That should now be fixed.
> 
> Thanks,
> Nick
> 
>   hw/ppc/pnv.c          |  2 +-
>   target/ppc/cpu-qom.h  |  6 ++-
>   target/ppc/cpu_init.c | 91 +++++++++++++++++++++++++++++++++----------
>   target/ppc/machine.c  |  2 +-
>   4 files changed, 77 insertions(+), 24 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index d3f77c8367..a4cb4cf10b 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -714,7 +714,7 @@ static bool pnv_match_cpu(const char *default_type, const char *cpu_type)
>       PowerPCCPUClass *ppc =
>           POWERPC_CPU_CLASS(object_class_by_name(cpu_type));
>   
> -    return ppc_default->pvr_match(ppc_default, ppc->pvr);
> +    return ppc_default->pvr_match(ppc_default, ppc->pvr, false);
>   }
>   
>   static void pnv_ipmi_bt_init(ISABus *bus, IPMIBmc *bmc, uint32_t irq)
> diff --git a/target/ppc/cpu-qom.h b/target/ppc/cpu-qom.h
> index ad7e3c3db9..89ff88f28c 100644
> --- a/target/ppc/cpu-qom.h
> +++ b/target/ppc/cpu-qom.h
> @@ -158,7 +158,11 @@ struct PowerPCCPUClass {
>       void (*parent_parse_features)(const char *type, char *str, Error **errp);
>   
>       uint32_t pvr;
> -    bool (*pvr_match)(struct PowerPCCPUClass *pcc, uint32_t pvr);
> +    /*
> +     * If @best is false, match if pcc is in the family of pvr
> +     * Else match only if pcc is the best match for pvr in this family.
> +     */
> +    bool (*pvr_match)(struct PowerPCCPUClass *pcc, uint32_t pvr, bool best);
>       uint64_t pcr_mask;          /* Available bits in PCR register */
>       uint64_t pcr_supported;     /* Bits for supported PowerISA versions */
>       uint32_t svr;
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index d1493a660c..899c4a586e 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -5912,15 +5912,25 @@ static void init_proc_POWER7(CPUPPCState *env)
>       ppcPOWER7_irq_init(env_archcpu(env));
>   }
>   
> -static bool ppc_pvr_match_power7(PowerPCCPUClass *pcc, uint32_t pvr)
> +static bool ppc_pvr_match_power7(PowerPCCPUClass *pcc, uint32_t pvr, bool best)
>   {
> -    if ((pvr & CPU_POWERPC_POWER_SERVER_MASK) == CPU_POWERPC_POWER7P_BASE) {
> -        return true;
> +    uint32_t base = pvr & CPU_POWERPC_POWER_SERVER_MASK;
> +    uint32_t pcc_base = pcc->pvr & CPU_POWERPC_POWER_SERVER_MASK;
> +
> +    if (!best) {
> +        if (base == CPU_POWERPC_POWER7_BASE) {
> +            return true;
> +        }
> +        if (base == CPU_POWERPC_POWER7P_BASE) {
> +            return true;
> +        }
>       }
> -    if ((pvr & CPU_POWERPC_POWER_SERVER_MASK) == CPU_POWERPC_POWER7_BASE) {
> -        return true;
> +
> +    if (base != pcc_base) {
> +        return false;
>       }
> -    return false;
> +
> +    return true;
>   }
>   
>   static bool cpu_has_work_POWER7(CPUState *cs)
> @@ -6073,18 +6083,27 @@ static void init_proc_POWER8(CPUPPCState *env)
>       ppcPOWER7_irq_init(env_archcpu(env));
>   }
>   
> -static bool ppc_pvr_match_power8(PowerPCCPUClass *pcc, uint32_t pvr)
> +static bool ppc_pvr_match_power8(PowerPCCPUClass *pcc, uint32_t pvr, bool best)
>   {
> -    if ((pvr & CPU_POWERPC_POWER_SERVER_MASK) == CPU_POWERPC_POWER8NVL_BASE) {
> -        return true;
> -    }
> -    if ((pvr & CPU_POWERPC_POWER_SERVER_MASK) == CPU_POWERPC_POWER8E_BASE) {
> -        return true;
> +    uint32_t base = pvr & CPU_POWERPC_POWER_SERVER_MASK;
> +    uint32_t pcc_base = pcc->pvr & CPU_POWERPC_POWER_SERVER_MASK;
> +
> +    if (!best) {
> +        if (base == CPU_POWERPC_POWER8_BASE) {
> +            return true;
> +        }
> +        if (base == CPU_POWERPC_POWER8E_BASE) {
> +            return true;
> +        }
> +        if (base == CPU_POWERPC_POWER8NVL_BASE) {
> +            return true;
> +        }
>       }
> -    if ((pvr & CPU_POWERPC_POWER_SERVER_MASK) == CPU_POWERPC_POWER8_BASE) {
> -        return true;
> +    if (base != pcc_base) {
> +        return false;
>       }
> -    return false;
> +
> +    return true;
>   }
>   
>   static bool cpu_has_work_POWER8(CPUState *cs)
> @@ -6282,11 +6301,26 @@ static void init_proc_POWER9(CPUPPCState *env)
>       ppcPOWER9_irq_init(env_archcpu(env));
>   }
>   
> -static bool ppc_pvr_match_power9(PowerPCCPUClass *pcc, uint32_t pvr)
> +static bool ppc_pvr_match_power9(PowerPCCPUClass *pcc, uint32_t pvr, bool best)
>   {
> -    if ((pvr & CPU_POWERPC_POWER_SERVER_MASK) == CPU_POWERPC_POWER9_BASE) {
> +    uint32_t base = pvr & CPU_POWERPC_POWER_SERVER_MASK;
> +    uint32_t pcc_base = pcc->pvr & CPU_POWERPC_POWER_SERVER_MASK;
> +
> +    if (!best) {
> +        if (base == CPU_POWERPC_POWER9_BASE) {
> +            return true;
> +        }
> +    }
> +
> +    if (base != pcc_base) {
> +        return false;
> +    }
> +
> +    if ((pvr & 0x0f00) == (pcc->pvr & 0x0f00)) {
> +        /* Major DD version matches to power9_v1.0 and power9_v2.0 */
>           return true;
>       }
> +
>       return false;
>   }
>   
> @@ -6499,11 +6533,26 @@ static void init_proc_POWER10(CPUPPCState *env)
>       ppcPOWER9_irq_init(env_archcpu(env));
>   }
>   
> -static bool ppc_pvr_match_power10(PowerPCCPUClass *pcc, uint32_t pvr)
> +static bool ppc_pvr_match_power10(PowerPCCPUClass *pcc, uint32_t pvr, bool best)
>   {
> -    if ((pvr & CPU_POWERPC_POWER_SERVER_MASK) == CPU_POWERPC_POWER10_BASE) {
> +    uint32_t base = pvr & CPU_POWERPC_POWER_SERVER_MASK;
> +    uint32_t pcc_base = pcc->pvr & CPU_POWERPC_POWER_SERVER_MASK;
> +
> +    if (!best) {
> +        if (base == CPU_POWERPC_POWER10_BASE) {
> +            return true;
> +        }
> +    }
> +
> +    if (base != pcc_base) {
> +        return false;
> +    }
> +
> +    if ((pvr & 0x0f00) == (pcc->pvr & 0x0f00)) {
> +        /* Major DD version matches to power10_v1.0 and power10_v2.0 */
>           return true;
>       }
> +
>       return false;
>   }
>   
> @@ -6910,7 +6959,7 @@ static gint ppc_cpu_compare_class_pvr_mask(gconstpointer a, gconstpointer b)
>           return -1;
>       }
>   
> -    if (pcc->pvr_match(pcc, pvr)) {
> +    if (pcc->pvr_match(pcc, pvr, true)) {
>           return 0;
>       }
>   
> @@ -7308,7 +7357,7 @@ static void ppc_cpu_instance_finalize(Object *obj)
>       ppc_hash64_finalize(cpu);
>   }
>   
> -static bool ppc_pvr_match_default(PowerPCCPUClass *pcc, uint32_t pvr)
> +static bool ppc_pvr_match_default(PowerPCCPUClass *pcc, uint32_t pvr, bool best)
>   {
>       return pcc->pvr == pvr;
>   }
> diff --git a/target/ppc/machine.c b/target/ppc/machine.c
> index a7d9036c09..be6eb3d968 100644
> --- a/target/ppc/machine.c
> +++ b/target/ppc/machine.c
> @@ -234,7 +234,7 @@ static bool pvr_match(PowerPCCPU *cpu, uint32_t pvr)
>       if (pvr == pcc->pvr) {
>           return true;
>       }
> -    return pcc->pvr_match(pcc, pvr);
> +    return pcc->pvr_match(pcc, pvr, true);
>   }
>   
>   static int cpu_post_load(void *opaque, int version_id)