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

[David Gibson]

On Mon, Mar 07, 2022 at 04:55:24PM +1000, 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.

Ok... but I don't see how that question can possibly be answered
without reference to all the available options.

> 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, doesn't this indicate a problem in the check order, rather than
a problem with the matching function?

> 
> 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;
> +
> +    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;
>  }
>  

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature