Re: [PATCH v3 1/4] target/ppc: Fix masked PVR matching

[Alexey Kardashevskiy]

On 3/7/22 17:55, Nicholas Piggin wrote:
> The pvr_match for a CPU class is not supposed to just match for any
> CPU in the family, but rather whether this particular CPU class is the
> best match in the family.
>
> Prior to this fix, e.g., a POWER9 DD2.3 KVM host matches to the
> power9_v1.0 class (because that's first in the list). After the patch,
> it matches the power9_v2.0 class.


So if we get now another revision of p10, this just won't work at all 
instead of matching DD1. Not that we have a reasonable chance of this 
happening though...




> Fixes: 03ae4133ab8 ("target-ppc: Add pvr_match() callback")
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   target/ppc/cpu_init.c | 51 ++++++++++++++++++++++++++++---------------
>   1 file changed, 34 insertions(+), 17 deletions(-)
>
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index 073fd10168..83ca741bea 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -5910,13 +5910,14 @@ static void init_proc_POWER7(CPUPPCState *env)
>   
>   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;
> +    uint32_t base = pvr & CPU_POWERPC_POWER_SERVER_MASK;
> +    uint32_t pcc_base = pcc->pvr & CPU_POWERPC_POWER_SERVER_MASK;
> +
> +    if (base != pcc_base) {
> +        return false;
>       }
> -    return false;
> +
> +    return true;
>   }
>   
>   static bool cpu_has_work_POWER7(CPUState *cs)
> @@ -6070,16 +6071,14 @@ static void init_proc_POWER8(CPUPPCState *env)
>   
>   static bool ppc_pvr_match_power8(PowerPCCPUClass *pcc, uint32_t pvr)
>   {
> -    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;
> -    }
> -    if ((pvr & CPU_POWERPC_POWER_SERVER_MASK) == CPU_POWERPC_POWER8_BASE) {
> -        return true;
> +    uint32_t base = pvr & CPU_POWERPC_POWER_SERVER_MASK;
> +    uint32_t pcc_base = pcc->pvr & CPU_POWERPC_POWER_SERVER_MASK;

(after looking at failing 
"QTEST_QEMU_BINARY=~/pbuild/qemu-localhost-ppc64/qemu-system-ppc64 
~/pbuild/qemu-localhost-ppc64/tests/qtest/pnv-xscom-test")

So this breaks using different family masks for the same machine type,
like, 0x004[BCD]xxxx for pnv_machine_power8_class_init()'s 
POWERPC_CPU_TYPE_NAME("power8_v2.0").

So I'd keep P7/P8 where they are today and create a new machine type for 
every family mask (higher 16 bits of PVR). Thanks,



> +
> +    if (base != pcc_base) {
> +        return false;
>       }
> -    return false;
> +
> +    return true;
>   }
>   
>   static bool cpu_has_work_POWER8(CPUState *cs)
> @@ -6277,9 +6276,18 @@ static void init_proc_POWER9(CPUPPCState *env)
>   
>   static bool ppc_pvr_match_power9(PowerPCCPUClass *pcc, uint32_t pvr)
>   {
> -    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 (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;
>   }
>   
> @@ -6489,9 +6497,18 @@ static void init_proc_POWER10(CPUPPCState *env)
>   
>   static bool ppc_pvr_match_power10(PowerPCCPUClass *pcc, uint32_t pvr)
>   {
> -    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 (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;
>   }
>