Fix PVR matching, add AIL cap compatibility

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

Posted by Nicholas Piggin 3 years, 11 months ago

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.

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

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

Posted by Alexey Kardashevskiy 3 years, 11 months ago


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

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

Posted by David Gibson 3 years, 11 months ago

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

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

Posted by Cédric Le Goater 3 years, 11 months ago


Hello Nick,

On 3/7/22 07: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.
> 
> 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(-)
make check-qtest-ppc64 fails with :

―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――― ✀  ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
stderr:
qemu-system-ppc64: invalid CPU model 'power8nvl_v1.0-powerpc64-cpu' for powernv8 machine
Broken pipe


TAP parsing error: Too few tests run (expected 6, got 1)
(test program exited with status code -6)
――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――

# QTEST_QEMU_BINARY=build/ppc64-softmmu/qemu-system-ppc64 QTEST_QEMU_IMG=qemu-img build/tests/qtest/pnv-xscom-test
1..6
# Start of ppc64 tests
# Start of pnv-xscom tests
# Start of cfam_id tests
# starting QEMU: exec build/ppc64-softmmu/qemu-system-ppc64 -qtest unix:/tmp/qtest-2994797.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-2994797.qmp,id=char0 -mon chardev=char0,mode=control -display none -M powernv8 -accel tcg -cpu POWER8 -accel qtest
ok 1 /ppc64/pnv-xscom/cfam_id/POWER8
# starting QEMU: exec build/ppc64-softmmu/qemu-system-ppc64 -qtest unix:/tmp/qtest-2994797.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-2994797.qmp,id=char0 -mon chardev=char0,mode=control -display none -M powernv8 -accel tcg -cpu POWER8NVL -accel qtest
qemu-system-ppc64: invalid CPU model 'power8nvl_v1.0-powerpc64-cpu' for powernv8 machine
Broken pipe
Aborted (core dumped)


Thanks,

C.

[PATCH v3 1/4] target/ppc: Fix masked PVR matching
[PATCH v3 2/4] target/ppc: Remove chip type field from POWER9 DD2.0 PVR
[PATCH v3 3/4] target/ppc: Add POWER9 DD2.2 model
[PATCH v3 4/4] spapr: Add SPAPR_CAP_AIL_MODE_3 for AIL mode 3 support for H_SET_MODE hcall