| Message ID | 20220307065527.156132-2-npiggin@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix PVR matching, add AIL cap compatibility | expand |
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.
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; > } >
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; > } >
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; }
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(-)