From 58cb91b34d9b1e87353c4a21ff39062dd8b25dd5 Mon Sep 17 00:00:00 2001 From: Harsh Prateek Bora Date: Fri, 29 Mar 2024 10:04:36 +0530 Subject: [PATCH 1/8] spapr: nested: use bitwise NOT operator for flags check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Check for flag bit in H_GUEST_GETSET_STATE_FLAG_GUEST_WIDE need to use bitwise NOT operator to ensure no other flag bits are set. Resolves: Coverity CID 1540008 Resolves: Coverity CID 1540009 Reported-by: Peter Maydell Reviewed-by: Philippe Mathieu-Daudé Signed-off by: Harsh Prateek Bora Signed-off-by: Nicholas Piggin --- hw/ppc/spapr_nested.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/ppc/spapr_nested.c b/hw/ppc/spapr_nested.c index 936659b4c0..c02785756c 100644 --- a/hw/ppc/spapr_nested.c +++ b/hw/ppc/spapr_nested.c @@ -1511,7 +1511,7 @@ static target_ulong h_guest_getset_state(PowerPCCPU *cpu, if (flags & H_GUEST_GETSET_STATE_FLAG_GUEST_WIDE) { gsr.flags |= GUEST_STATE_REQUEST_GUEST_WIDE; } - if (flags & !H_GUEST_GETSET_STATE_FLAG_GUEST_WIDE) { + if (flags & ~H_GUEST_GETSET_STATE_FLAG_GUEST_WIDE) { return H_PARAMETER; /* flag not supported yet */ } From beb0b62c3e0f7e486eac680f41b140f4ed492f59 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Thu, 28 Mar 2024 18:19:16 +1000 Subject: [PATCH 2/8] hw/ppc/spapr: Include missing 'sysemu/tcg.h' header MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "sysemu/tcg.h" declares tcg_enabled(), and is implicitly included. Include it explicitly to avoid the following error when refactoring headers: hw/ppc/spapr.c:2612:9: error: call to undeclared function 'tcg_enabled'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration] if (tcg_enabled()) { ^ Reviewed-by: Harsh Prateek Bora Signed-off-by: Philippe Mathieu-Daudé Signed-off-by: Nicholas Piggin --- hw/ppc/spapr.c | 1 + 1 file changed, 1 insertion(+) diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index c417f9dd52..e9bc97fee0 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -35,6 +35,7 @@ #include "sysemu/sysemu.h" #include "sysemu/hostmem.h" #include "sysemu/numa.h" +#include "sysemu/tcg.h" #include "sysemu/qtest.h" #include "sysemu/reset.h" #include "sysemu/runstate.h" From d7d9c6071e6dc5d466b229457fc4ad34e101dccd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Tue, 19 Mar 2024 06:10:20 +0100 Subject: [PATCH 3/8] target/ppc/mmu-radix64: Use correct string format in walk_tree() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 'mask', 'nlb' and 'base_addr' are all uin64_t types. Use the corresponding PRIx64 format. Fixes: d2066bc50d ("target/ppc: Check page dir/table base alignment") Signed-off-by: Philippe Mathieu-Daudé Signed-off-by: Nicholas Piggin --- target/ppc/mmu-radix64.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c index 5823e039e6..690dff7a49 100644 --- a/target/ppc/mmu-radix64.c +++ b/target/ppc/mmu-radix64.c @@ -300,8 +300,8 @@ static int ppc_radix64_next_level(AddressSpace *as, vaddr eaddr, if (nlb & mask) { qemu_log_mask(LOG_GUEST_ERROR, - "%s: misaligned page dir/table base: 0x"TARGET_FMT_lx - " page dir size: 0x"TARGET_FMT_lx"\n", + "%s: misaligned page dir/table base: 0x%" PRIx64 + " page dir size: 0x%" PRIx64 "\n", __func__, nlb, mask + 1); nlb &= ~mask; } @@ -324,8 +324,8 @@ static int ppc_radix64_walk_tree(AddressSpace *as, vaddr eaddr, if (base_addr & mask) { qemu_log_mask(LOG_GUEST_ERROR, - "%s: misaligned page dir base: 0x"TARGET_FMT_lx - " page dir size: 0x"TARGET_FMT_lx"\n", + "%s: misaligned page dir base: 0x%" PRIx64 + " page dir size: 0x%" PRIx64 "\n", __func__, base_addr, mask + 1); base_addr &= ~mask; } From 978897a572e975faad912a473815a668a43d9f1f Mon Sep 17 00:00:00 2001 From: Benjamin Gray Date: Wed, 20 Mar 2024 12:50:24 +1100 Subject: [PATCH 4/8] target/ppc: Restore [H]DEXCR to 64-bits The DEXCR emulation was recently changed to a 32-bit register, possibly because it does have a 32-bit read-only view. It is a full 64-bit SPR though, so use the corresponding 64-bit write functions. Fixes: fbda88f7abdee ("target/ppc: Fix width of some 32-bit SPRs") Reviewed-by: Nicholas Piggin Signed-off-by: Benjamin Gray Signed-off-by: Nicholas Piggin --- target/ppc/cpu_init.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c index 7e65f08147..22fdea093b 100644 --- a/target/ppc/cpu_init.c +++ b/target/ppc/cpu_init.c @@ -5820,7 +5820,7 @@ static void register_power10_dexcr_sprs(CPUPPCState *env) { spr_register(env, SPR_DEXCR, "DEXCR", SPR_NOACCESS, SPR_NOACCESS, - &spr_read_generic, &spr_write_generic32, + &spr_read_generic, &spr_write_generic, 0); spr_register(env, SPR_UDEXCR, "UDEXCR", @@ -5831,7 +5831,7 @@ static void register_power10_dexcr_sprs(CPUPPCState *env) spr_register_hv(env, SPR_HDEXCR, "HDEXCR", SPR_NOACCESS, SPR_NOACCESS, SPR_NOACCESS, SPR_NOACCESS, - &spr_read_generic, &spr_write_generic32, + &spr_read_generic, &spr_write_generic, 0); spr_register(env, SPR_UHDEXCR, "UHDEXCR", From ed399ade3c85adf82fe507339560693e83a27020 Mon Sep 17 00:00:00 2001 From: Benjamin Gray Date: Wed, 20 Mar 2024 12:50:25 +1100 Subject: [PATCH 5/8] target/ppc: Fix GDB register indexing on secondary CPUs The GDB server protocol assigns an arbitrary numbering of the SPRs. We track this correspondence on each SPR with gdb_id, using it to resolve any SPR requests GDB makes. Early on we generate an XML representation of the SPRs to give GDB, including this numbering. However the XML is cached globally, and we skip setting the SPR gdb_id values on subsequent threads if we detect it is cached. This causes QEMU to fail to resolve SPR requests against secondary CPUs because it cannot find the matching gdb_id value on that thread's SPRs. This is a minimal fix to first assign the gdb_id values, then return early if the XML is cached. Otherwise we generate the XML using the now already initialised gdb_id values. Fixes: 1b53948ff8f7 ("target/ppc: Use GDBFeature for dynamic XML") Reviewed-by: Nicholas Piggin Signed-off-by: Benjamin Gray Signed-off-by: Nicholas Piggin --- target/ppc/gdbstub.c | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/target/ppc/gdbstub.c b/target/ppc/gdbstub.c index 3f1e61bdb7..3b28d4e21c 100644 --- a/target/ppc/gdbstub.c +++ b/target/ppc/gdbstub.c @@ -305,6 +305,25 @@ static void gdb_gen_spr_feature(CPUState *cs) unsigned int num_regs = 0; int i; + for (i = 0; i < ARRAY_SIZE(env->spr_cb); i++) { + ppc_spr_t *spr = &env->spr_cb[i]; + + if (!spr->name) { + continue; + } + + /* + * GDB identifies registers based on the order they are + * presented in the XML. These ids will not match QEMU's + * representation (which follows the PowerISA). + * + * Store the position of the current register description so + * we can make the correspondence later. + */ + spr->gdb_id = num_regs; + num_regs++; + } + if (pcc->gdb_spr.xml) { return; } @@ -321,18 +340,8 @@ static void gdb_gen_spr_feature(CPUState *cs) } gdb_feature_builder_append_reg(&builder, g_ascii_strdown(spr->name, -1), - TARGET_LONG_BITS, num_regs, + TARGET_LONG_BITS, spr->gdb_id, "int", "spr"); - /* - * GDB identifies registers based on the order they are - * presented in the XML. These ids will not match QEMU's - * representation (which follows the PowerISA). - * - * Store the position of the current register description so - * we can make the correspondence later. - */ - spr->gdb_id = num_regs; - num_regs++; } gdb_feature_builder_end(&builder); From 434531619fecd5ff9a08e548ed87b2b73c29cf3e Mon Sep 17 00:00:00 2001 From: Nicholas Piggin Date: Tue, 19 Mar 2024 16:01:46 +1000 Subject: [PATCH 6/8] target/ppc: Do not clear MSR[ME] on MCE interrupts to supervisor Hardware clears the MSR[ME] bit when delivering a machine check interrupt, so that is what QEMU does. The spapr environment runs in supervisor mode though, and receives machine check interrupts after they are processed by the hypervisor, and MSR[ME] must always be enabled in supervisor mode (otherwise it could checkstop the system). So MSR[ME] must not be cleared when delivering machine checks to the supervisor. The fix to prevent supervisor mode from modifying MSR[ME] also prevented it from re-enabling the incorrectly cleared MSR[ME] bit when returning from handling the interrupt. Before that fix, the problem was not very noticable with well-behaved code. So the Fixes tag is not strictly correct, but practically they go together. Found by kvm-unit-tests machine check tests (not yet upstream). Fixes: 678b6f1af75ef ("target/ppc: Prevent supervisor from modifying MSR[ME]") Reviewed-by: Harsh Prateek Bora Signed-off-by: Nicholas Piggin --- target/ppc/excp_helper.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c index 80f584f933..674c05a2ce 100644 --- a/target/ppc/excp_helper.c +++ b/target/ppc/excp_helper.c @@ -1345,9 +1345,10 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp) * clear (e.g., see FWNMI in PAPR). */ new_msr |= (target_ulong)MSR_HVB; + + /* HV machine check exceptions don't have ME set */ + new_msr &= ~((target_ulong)1 << MSR_ME); } - /* machine check exceptions don't have ME set */ - new_msr &= ~((target_ulong)1 << MSR_ME); msr |= env->error_code; break; From 74eb04af186457517b6bae8ec6ceac872bde822e Mon Sep 17 00:00:00 2001 From: Nicholas Piggin Date: Wed, 27 Mar 2024 13:28:11 +1000 Subject: [PATCH 7/8] tests/avocado: Fix ppc_hv_tests.py xorriso dependency guard For some reason the skipIf missing_deps() check fails to skip the test if it comes after the skipUnless lines, causing an error running on systems without xorriso. Avocado implements skipUnless is just an inverted skipIf, so it's not clear what the bug is or why this fixes it. For now it's enough to get things working. Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2246 Fixes: c9cb496710758 ("tests/avocado: ppc add hypervisor tests") Signed-off-by: Nicholas Piggin --- tests/avocado/ppc_hv_tests.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/avocado/ppc_hv_tests.py b/tests/avocado/ppc_hv_tests.py index 5080358e25..2c8ddd9257 100644 --- a/tests/avocado/ppc_hv_tests.py +++ b/tests/avocado/ppc_hv_tests.py @@ -42,10 +42,11 @@ def missing_deps(): # QEMU packages are downloaded and installed on each test. That's not a # large download, but it may be more polite to create qcow2 image with # QEMU already installed and use that. +# XXX: The order of these tests seems to matter, see git blame. +@skipIf(missing_deps(), 'dependencies (%s) not installed' % ','.join(deps)) @skipUnless(os.getenv('QEMU_TEST_FLAKY_TESTS'), 'Test sometimes gets stuck due to console handling problem') @skipUnless(os.getenv('AVOCADO_ALLOW_LARGE_STORAGE'), 'storage limited') @skipUnless(os.getenv('SPEED') == 'slow', 'runtime limited') -@skipIf(missing_deps(), 'dependencies (%s) not installed' % ','.join(deps)) class HypervisorTest(QemuSystemTest): timeout = 1000 From b07a5bb736ca08d55cc3ada8ca309943b55d4b70 Mon Sep 17 00:00:00 2001 From: Nicholas Piggin Date: Thu, 28 Mar 2024 12:00:15 +1000 Subject: [PATCH 8/8] tests/avocado: ppc_hv_tests.py set alpine time before setup-alpine If the time is wrong, setup-alpine SSL certificate checks can fail. setup-alpine is used to bring up the network, but it doesn't seem to to set NTP time before the failing SSL checks. This test has recently started failing presumably because the default time has now fallen too far behind. Fix this by setting time from the host time before running setup-alpine. Fixes: c9cb496710758 ("tests/avocado: ppc add hypervisor tests") Signed-off-by: Nicholas Piggin --- tests/avocado/ppc_hv_tests.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/avocado/ppc_hv_tests.py b/tests/avocado/ppc_hv_tests.py index 2c8ddd9257..bf8822bb97 100644 --- a/tests/avocado/ppc_hv_tests.py +++ b/tests/avocado/ppc_hv_tests.py @@ -14,6 +14,7 @@ from avocado_qemu import wait_for_console_pattern, exec_command import os import time import subprocess +from datetime import datetime deps = ["xorriso"] # dependent tools needed in the test setup/box. @@ -107,6 +108,8 @@ class HypervisorTest(QemuSystemTest): exec_command(self, 'root') wait_for_console_pattern(self, 'localhost login:') wait_for_console_pattern(self, 'You may change this message by editing /etc/motd.') + # If the time is wrong, SSL certificates can fail. + exec_command(self, 'date -s "' + datetime.utcnow().strftime('%Y-%m-%d %H:%M:%S' + '"')) exec_command(self, 'setup-alpine -qe') wait_for_console_pattern(self, 'Updating repository indexes... done.')