Re: [kvm-unit-tests PATCH v2 2/4] Makefile: Prepare for clang EFI builds
On 04/09/2024 12.50, Andrew Jones wrote: clang complains about GNU extensions such as variable sized types not being at the end of structs unless -Wno-gnu is used. We may eventually want -Wno-gnu, but for now let's just handle the warnings as they come. Add -Wno-gnu-variable-sized-type-not-at-end to avoid the warning issued for the initrd_dev_path struct. Signed-off-by: Andrew Jones --- Makefile | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Makefile b/Makefile index 3d51cb726120..7471f7285b78 100644 --- a/Makefile +++ b/Makefile @@ -50,6 +50,8 @@ EFI_CFLAGS += -fshort-wchar # EFI applications use PIC as they are loaded to dynamic addresses, not a fixed # starting address EFI_CFLAGS += -fPIC +# Avoid error with the initrd_dev_path struct +EFI_CFLAGS += -Wno-gnu-variable-sized-type-not-at-end # Create shared library EFI_LDFLAGS := -Bsymbolic -shared -nostdlib endif Reviewed-by: Thomas Huth
Re: [kvm-unit-tests PATCH 1/3] riscv: Drop mstrict-align
On 03/09/2024 18.30, Andrew Jones wrote: The spec says unaligned accesses are supported, so this isn't required and clang doesn't support it. A platform might have slow unaligned accesses, but kvm-unit-tests isn't about speed anyway. Signed-off-by: Andrew Jones --- riscv/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/riscv/Makefile b/riscv/Makefile index 179a373dbacf..2ee7c5bb5ad8 100644 --- a/riscv/Makefile +++ b/riscv/Makefile @@ -76,7 +76,7 @@ LDFLAGS += -melf32lriscv endif CFLAGS += -DCONFIG_RELOC CFLAGS += -mcmodel=medany -CFLAGS += -mstrict-align +#CFLAGS += -mstrict-align CFLAGS += -std=gnu99 CFLAGS += -ffreestanding CFLAGS += -O2 Reviewed-by: Thomas Huth
Re: [kvm-unit-tests PATCH 3/3] riscv: gitlab-ci: Add clang build tests
On 03/09/2024 18.30, Andrew Jones wrote: Test building 32 and 64-bit with clang. Throw a test of in- and out- of-tree building in too by swapping which is done to which (32-bit vs. 64-bit) with respect to the gcc build tests. Signed-off-by: Andrew Jones --- .gitlab-ci.yml | 28 1 file changed, 28 insertions(+) Acked-by: Thomas Huth
Re: [kvm-unit-tests PATCH 2/3] configure: Support cross compiling with clang
On 03/09/2024 18.30, Andrew Jones wrote: When a user specifies the compiler with --cc assume it's already fully named, even if the user also specifies a cross-prefix. This allows clang to be selected for the compiler, which doesn't use prefixes, but also still provide a cross prefix for binutils. If a user needs a prefix on the compiler that they specify with --cc, then they'll just have to specify it with the prefix prepended. Also ensure user provided cflags are used when testing the compiler, since the flags may drastically change behavior, such as the --target flag for clang. With these changes it's possible to cross compile for riscv with clang after configuring with ./configure --arch=riscv64 --cc=clang --cflags='--target=riscv64' \ --cross-prefix=riscv64-linux-gnu- Signed-off-by: Andrew Jones --- configure | 11 --- 1 file changed, 8 insertions(+), 3 deletions(-) Reviewed-by: Thomas Huth
Re: [kvm-unit-tests PATCH] build: retain intermediate .aux.o targets
On 26/07/2024 06.15, Nicholas Piggin wrote: On Fri Jun 14, 2024 at 6:38 PM AEST, Nicholas Piggin wrote: On Fri Jun 14, 2024 at 11:08 AM AEST, Segher Boessenkool wrote: On Fri, Jun 14, 2024 at 10:43:39AM +1000, Nicholas Piggin wrote: On Wed Jun 12, 2024 at 6:28 PM AEST, Segher Boessenkool wrote: On Wed, Jun 12, 2024 at 02:42:32PM +1000, Nicholas Piggin wrote: arm, powerpc, riscv, build .aux.o targets with implicit pattern rules in dependency chains that cause them to be made as intermediate files, which get removed when make finishes. This results in unnecessary partial rebuilds. If make is run again, this time the .aux.o targets are not intermediate, possibly due to being made via different dependencies. Adding .aux.o files to .PRECIOUS prevents them being removed and solves the rebuild problem. s390x does not have the problem because .SECONDARY prevents dependancies from being built as intermediate. However the same change is made for s390x, for consistency. This is exactly what .SECONDARY is for, as its documentation says, even. Wouldn't it be better to just add a .SECONDARY to the other targets as well? Yeah we were debating that and agreed .PRECIOUS may not be the cleanest fix but since we already use that it's okay for a minimal fix. But why add it to s390x then? It is not a fix there at all! Eh, not a big deal. I mentioned that in the changelog it doesn't seem to pracicaly fix something. And I rather the makefiles converge as much as possible rather than diverge more. .SECONDARY was added independently and not to fix this problem in s390x. And s390x has .SECONDARY slightly wrong AFAIKS. It mentions .SECONDARY: twice in a way that looks like it was meant to depend on specific targets, it actually gives it no dependencies and the resulting semantics are that all intermediate files in the build are treated as secondary. So somethig there should be cleaned up. If the .SECONDARY was changed to only depend on the .gobj and .hdr.obj then suddenly that would break .aux.o if I don't make the change. So I'm meaning to work out what to do with all that, i.e., whether to add blanket .SECONDARY for all and trim or remove the .PRECIOUS files, or remove s390x's secondary, or make it more specific, or something else. But it takes a while for me to do makefile work. Hi Thomas, Ping on this patch? I assumed that Marc would chime in on the s390x part here? Thomas
Re: [kvm-unit-tests PATCH v10 15/15] powerpc/gitlab-ci: Enable more tests with Fedora 40
On 12/06/2024 07.23, Nicholas Piggin wrote: With Fedora 40 (QEMU 8.2), more tests can be enabled. Signed-off-by: Nicholas Piggin --- .gitlab-ci.yml| 2 +- powerpc/unittests.cfg | 17 - 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index ffb3767ec..ee14330a3 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -110,7 +110,7 @@ build-ppc64le: extends: .intree_template image: fedora:40 script: - - dnf install -y qemu-system-ppc gcc-powerpc64-linux-gnu nmap-ncat + - dnf install -y qemu-system-ppc gcc-powerpc64-linux-gnu nmap-ncat jq Please mention the addition of jq in the patch description (why it is needed). Thanks, Thomas
Re: [kvm-unit-tests PATCH v10 13/15] powerpc: Add a panic test
On 12/06/2024 07.23, Nicholas Piggin wrote: This adds a simple panic test for pseries and powernv that works with TCG (unlike the s390x panic tests), making it easier to test this part of the harness code. Signed-off-by: Nicholas Piggin --- lib/powerpc/asm/rtas.h | 1 + lib/powerpc/rtas.c | 16 powerpc/run| 2 +- powerpc/selftest.c | 18 -- powerpc/unittests.cfg | 5 + 5 files changed, 39 insertions(+), 3 deletions(-) Reviewed-by: Thomas Huth
Re: [kvm-unit-tests PATCH v10 10/15] powerpc: Remove remnants of ppc64 directory and build structure
On 12/06/2024 07.23, Nicholas Piggin wrote: This moves merges ppc64 directories and files into powerpc, and merges the 3 makefiles into one. The configure --arch=powerpc option is aliased to ppc64 for good measure. Acked-by: Thomas Huth Signed-off-by: Nicholas Piggin --- Seems like this patch does not apply cleanly anymore, could you please rebase? Thanks, Thomas
Re: [kvm-unit-tests PATCH v10 14/15] powerpc/gitlab-ci: Upgrade powerpc to Fedora 40
On 12/06/2024 07.23, Nicholas Piggin wrote: QEMU has fixed a number of powerpc test fails in Fedora 40, so upgrade to that image. Other architectures seem to be okay with Fedora 40 except for x86-64, which fails some xsave and realmode tests, so only change powerpc to start with. Signed-off-by: Nicholas Piggin --- .gitlab-ci.yml | 2 ++ 1 file changed, 2 insertions(+) FYI, I've pushed now the generic patch to bump all jobs to F40, so I think you can drop this one here from your queue now. Thomas
Re: [kvm-unit-tests PATCH v10 08/15] powerpc: add pmu tests
On 12/06/2024 07.23, Nicholas Piggin wrote: Add some initial PMU testing. - PMC5/6 tests - PMAE / PMI test - BHRB basic tests Signed-off-by: Nicholas Piggin --- ... diff --git a/powerpc/pmu.c b/powerpc/pmu.c new file mode 100644 index 0..bdc45e167 --- /dev/null +++ b/powerpc/pmu.c @@ -0,0 +1,562 @@ ... +static void test_pmc5_with_ldat(void) +{ + unsigned long pmc5_1, pmc5_2; + register unsigned long r4 asm("r4"); + register unsigned long r5 asm("r5"); + register unsigned long r6 asm("r6"); + uint64_t val; + + reset_mmcr0(); + mtspr(SPR_PMC5, 0); + mtspr(SPR_MMCR0, mfspr(SPR_MMCR0) & ~(MMCR0_FC | MMCR0_FC56)); + asm volatile(".rep 20 ; nop ; .endr" ::: "memory"); + mtspr(SPR_MMCR0, mfspr(SPR_MMCR0) | (MMCR0_FC | MMCR0_FC56)); + pmc5_1 = mfspr(SPR_PMC5); + + val = 0xdeadbeef; + r4 = 0; + r5 = 0xdeadbeef; + r6 = 100; + reset_mmcr0(); + mtspr(SPR_PMC5, 0); + mtspr(SPR_MMCR0, mfspr(SPR_MMCR0) & ~(MMCR0_FC | MMCR0_FC56)); + asm volatile(".rep 10 ; nop ; .endr ; ldat %0,%3,0x10 ; .rep 10 ; nop ; .endr" : "=r"(r4), "+r"(r5), "+r"(r6) : "r"(&val) :"memory"); Looks like older versions of Clang do not like this instruction: /tmp/pmu-4fda98.s: Assembler messages: /tmp/pmu-4fda98.s:1685: Error: unrecognized opcode: `ldat' clang-13: error: assembler command failed with exit code 1 (use -v to see invocation) Could you please work-around that issue? Also, please break the very long line here. Thanks! + mtspr(SPR_MMCR0, mfspr(SPR_MMCR0) | (MMCR0_FC | MMCR0_FC56)); + pmc5_2 = mfspr(SPR_PMC5); + assert(r4 == 0xdeadbeef); + assert(val == 0xdeadbeef); + + /* TCG does not count instructions around syscalls correctly */ + report_kfail(host_is_tcg, pmc5_1 != pmc5_2 + 1, +"PMC5 counts instructions with ldat"); +} Thomas
Re: [kvm-unit-tests PATCH v10 08/15] powerpc: add pmu tests
On 12/06/2024 07.23, Nicholas Piggin wrote: Add some initial PMU testing. - PMC5/6 tests - PMAE / PMI test - BHRB basic tests Signed-off-by: Nicholas Piggin --- lib/powerpc/asm/processor.h | 2 + lib/powerpc/asm/reg.h | 9 + lib/powerpc/asm/setup.h | 1 + lib/powerpc/setup.c | 20 ++ powerpc/Makefile.common | 3 +- powerpc/pmu.c | 562 powerpc/unittests.cfg | 3 + 7 files changed, 599 insertions(+), 1 deletion(-) create mode 100644 powerpc/pmu.c Hi Nicholas! Something in this patch breaks compiling with Clang in the Travis-CI: https://app.travis-ci.com/github/huth/kvm-unit-tests/builds/271013764 Not sure what exactly it is, the log is not very helpful. Do you have a ppc64 host where you could test your patches with Clang? Thomas
[PATCH] Documentation: Remove the unused "topology_updates" from kernel-parameters.txt
The "topology_updates" switch has been removed four years ago in commit c30f931e891e ("powerpc/numa: remove ability to enable topology updates"), so let's remove this from the documentation, too. Signed-off-by: Thomas Huth --- Documentation/admin-guide/kernel-parameters.txt | 6 -- 1 file changed, 6 deletions(-) diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt index f58001338860..b75852f1a789 100644 --- a/Documentation/admin-guide/kernel-parameters.txt +++ b/Documentation/admin-guide/kernel-parameters.txt @@ -6600,12 +6600,6 @@ e.g. base its process migration decisions on it. Default is on. - topology_updates= [KNL, PPC, NUMA] - Format: {off} - Specify if the kernel should ignore (off) - topology updates sent by the hypervisor to this - LPAR. - torture.disable_onoff_at_boot= [KNL] Prevent the CPU-hotplug component of torturing until after init has spawned. -- 2.45.2
Re: [kvm-unit-tests PATCH v9 31/31] powerpc: gitlab CI update
On 04/05/2024 14.28, Nicholas Piggin wrote: This adds testing for the powernv machine, and adds a gitlab-ci test group instead of specifying all tests in .gitlab-ci.yml, and adds a few new tests (smp, atomics) that are known to work in CI. Signed-off-by: Nicholas Piggin --- .gitlab-ci.yml| 30 -- powerpc/unittests.cfg | 32 ++-- 2 files changed, 34 insertions(+), 28 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 23bb69e24..31a2a4e34 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -97,17 +97,10 @@ build-ppc64be: - cd build - ../configure --arch=ppc64 --endian=big --cross-prefix=powerpc64-linux-gnu- - make -j2 - - ACCEL=tcg ./run_tests.sh - selftest-setup - selftest-migration - selftest-migration-skip - spapr_hcall - rtas-get-time-of-day - rtas-get-time-of-day-base - rtas-set-time-of-day - emulator - | tee results.txt - - if grep -q FAIL results.txt ; then exit 1 ; fi + - ACCEL=tcg MAX_SMP=8 ./run_tests.sh -g gitlab-ci | tee results.txt + - grep -q PASS results.txt && ! grep -q FAIL results.txt + - ACCEL=tcg MAX_SMP=8 MACHINE=powernv ./run_tests.sh -g gitlab-ci | tee results.txt + - grep -q PASS results.txt && ! grep -q FAIL results.txt build-ppc64le: extends: .intree_template @@ -115,17 +108,10 @@ build-ppc64le: - dnf install -y qemu-system-ppc gcc-powerpc64-linux-gnu nmap-ncat - ./configure --arch=ppc64 --endian=little --cross-prefix=powerpc64-linux-gnu- - make -j2 - - ACCEL=tcg ./run_tests.sh - selftest-setup - selftest-migration - selftest-migration-skip - spapr_hcall - rtas-get-time-of-day - rtas-get-time-of-day-base - rtas-set-time-of-day - emulator - | tee results.txt - - if grep -q FAIL results.txt ; then exit 1 ; fi + - ACCEL=tcg MAX_SMP=8 ./run_tests.sh -g gitlab-ci | tee results.txt + - grep -q PASS results.txt && ! grep -q FAIL results.txt + - ACCEL=tcg MAX_SMP=8 MACHINE=powernv ./run_tests.sh -g gitlab-ci | tee results.txt + - grep -q PASS results.txt && ! grep -q FAIL results.txt # build-riscv32: # Fedora doesn't package a riscv32 compiler for QEMU. Oh, well. diff --git a/powerpc/unittests.cfg b/powerpc/unittests.cfg index d767f5d68..6fae688a8 100644 --- a/powerpc/unittests.cfg +++ b/powerpc/unittests.cfg @@ -16,17 +16,25 @@ file = selftest.elf smp = 2 extra_params = -m 1g -append 'setup smp=2 mem=1024' -groups = selftest +groups = selftest gitlab-ci [selftest-migration] file = selftest-migration.elf machine = pseries groups = selftest migration +# QEMU 7.0 (Fedora 37) in gitlab CI has known migration bugs in TCG, so +# make a kvm-only version for CI +[selftest-migration-ci] +file = selftest-migration.elf +machine = pseries +groups = nodefault selftest migration gitlab-ci +accel = kvm + [selftest-migration-skip] file = selftest-migration.elf machine = pseries -groups = selftest migration +groups = selftest migration gitlab-ci extra_params = -append "skip" [migration-memory] @@ -37,6 +45,7 @@ groups = migration [spapr_hcall] file = spapr_hcall.elf machine = pseries +groups = gitlab-ci [spapr_vpa] file = spapr_vpa.elf @@ -47,38 +56,43 @@ file = rtas.elf machine = pseries timeout = 5 extra_params = -append "get-time-of-day date=$(date +%s)" -groups = rtas +groups = rtas gitlab-ci [rtas-get-time-of-day-base] file = rtas.elf machine = pseries timeout = 5 extra_params = -rtc base="2006-06-17" -append "get-time-of-day date=$(date --date="2006-06-17 UTC" +%s)" -groups = rtas +groups = rtas gitlab-ci [rtas-set-time-of-day] file = rtas.elf machine = pseries extra_params = -append "set-time-of-day" timeout = 5 -groups = rtas +groups = rtas gitlab-ci [emulator] file = emulator.elf +groups = gitlab-ci +# QEMU 7.0 (Fedora 37) in gitlab CI fails this [interrupts] file = interrupts.elf +# QEMU 7.0 (Fedora 37) in gitlab CI fails this [mmu] file = mmu.elf smp = $MAX_SMP +# QEMU 7.0 (Fedora 37) in gitlab CI fails this [pmu] file = pmu.elf [smp] file = smp.elf smp = 2 +groups = gitlab-ci [smp-smt] file = smp.elf @@ -92,16 +106,19 @@ accel = tcg,thread=single [atomics] file = atomics.elf +groups = gitlab-ci [atomics-migration] file = atomics.elf machine = pseries extra_params = -append "migration -m" -groups = migration +groups = migration gitlab-ci +# QEMU 7.0 (Fedora 37) in gitlab CI fails this [timebase] file = timebase.elf +# QEMU 7.0 (Fedora 37) in gitlab CI fails this [timebase-icount] file = timebase.elf accel = tcg @@ -115,14 +132,17 @@ smp = 2,threads=2 extra_params = -machine cap-htm=on -append "h_cede_tm" groups = h_cede_tm +# QEMU 7.0 (Fedora 37) in gitlab CI fails this [sprs] file = sprs.elf +# QEMU 7.0 (Fedora 37) in gitlab CI fails this [sprs-migration] file = sprs.elf machine = pseries
Re: [kvm-unit-tests PATCH v9 30/31] powerpc: Add facility to query TCG or KVM host
On 04/05/2024 14.28, Nicholas Piggin wrote: Use device tree properties to determine whether KVM or TCG is in use. Logically these are not the inverse of one another, because KVM can be used on top of a TCG processor (if TCG is emulating HV mode, or if it provides a nested hypervisor interface with spapr). This can be a problem because some issues relate to TCG CPU emulation, and some to the spapr hypervisor implementation. At the moment there is no way to determine TCG is running a KVM host that is running the tests, but the two independent variables are added in case that is able to be determined in future. For now that case is just incorrectly considered to be kvm && !tcg. Use this facility to restrict some of the known test failures to TCG. Signed-off-by: Nicholas Piggin --- lib/powerpc/asm/processor.h | 3 +++ lib/powerpc/setup.c | 25 + powerpc/atomics.c | 2 +- powerpc/interrupts.c| 6 -- powerpc/mmu.c | 2 +- powerpc/pmu.c | 6 +++--- powerpc/sprs.c | 2 +- powerpc/timebase.c | 4 ++-- powerpc/tm.c| 2 +- 9 files changed, 41 insertions(+), 11 deletions(-) As mentioned elsewhere, it would be nice to have this earlier in the series so you could use the conditions in the earlier patches already (but if it is too cumbersome to rework, I don't insist on that). Reviewed-by: Thomas Huth
Re: [kvm-unit-tests PATCH v9 29/31] powerpc: Remove remnants of ppc64 directory and build structure
On 04/05/2024 14.28, Nicholas Piggin wrote: This moves merges ppc64 directories and files into powerpc, and merges the 3 makefiles into one. The configure --arch=powerpc option is aliased to ppc64 for good measure. Signed-off-by: Nicholas Piggin --- ... diff --git a/powerpc/Makefile b/powerpc/Makefile index 8a007ab54..e4b5312a2 100644 --- a/powerpc/Makefile +++ b/powerpc/Makefile @@ -1 +1,111 @@ -include $(SRCDIR)/$(TEST_DIR)/Makefile.$(ARCH) +# +# powerpc makefile +# +# Authors: Andrew Jones I'd maybe drop that e-mail address now since it it not valid anymore. Andrew, do want to see your new mail address here? Apart from that: Acked-by: Thomas Huth
Re: [kvm-unit-tests PATCH v9 27/31] powerpc: add pmu tests
On 04/05/2024 14.28, Nicholas Piggin wrote: Add some initial PMU testing. - PMC5/6 tests - PMAE / PMI test - BHRB basic tests Signed-off-by: Nicholas Piggin --- ... diff --git a/lib/powerpc/setup.c b/lib/powerpc/setup.c index a4ff678ce..8ff4939e2 100644 --- a/lib/powerpc/setup.c +++ b/lib/powerpc/setup.c @@ -33,6 +33,7 @@ u32 initrd_size; u32 cpu_to_hwid[NR_CPUS] = { [0 ... NR_CPUS-1] = (~0U) }; int nr_cpus_present; uint64_t tb_hz; +uint64_t cpu_hz; struct mem_region mem_regions[NR_MEM_REGIONS]; phys_addr_t __physical_start, __physical_end; @@ -42,6 +43,7 @@ struct cpu_set_params { unsigned icache_bytes; unsigned dcache_bytes; uint64_t tb_hz; + uint64_t cpu_hz; }; static void cpu_set(int fdtnode, u64 regval, void *info) @@ -95,6 +97,22 @@ static void cpu_set(int fdtnode, u64 regval, void *info) data = (u32 *)prop->data; params->tb_hz = fdt32_to_cpu(*data); + prop = fdt_get_property(dt_fdt(), fdtnode, + "ibm,extended-clock-frequency", NULL); + if (prop) { + data = (u32 *)prop->data; + params->cpu_hz = fdt32_to_cpu(*data); + params->cpu_hz <<= 32; + data = (u32 *)prop->data + 1; + params->cpu_hz |= fdt32_to_cpu(*data); Why don't you simply cast to (u64 *) and use fdt64_to_cpu() here instead? ... diff --git a/powerpc/pmu.c b/powerpc/pmu.c new file mode 100644 index 0..8b13ee4cd --- /dev/null +++ b/powerpc/pmu.c @@ -0,0 +1,403 @@ ... +static void test_pmc5_with_fault(void) +{ + unsigned long pmc5_1, pmc5_2; + + handle_exception(0x700, &illegal_handler, NULL); + handle_exception(0xe40, &illegal_handler, NULL); + + reset_mmcr0(); + mtspr(SPR_PMC5, 0); + mtspr(SPR_MMCR0, mfspr(SPR_MMCR0) & ~(MMCR0_FC | MMCR0_FC56)); + asm volatile(".long 0x0" ::: "memory"); + mtspr(SPR_MMCR0, mfspr(SPR_MMCR0) | (MMCR0_FC | MMCR0_FC56)); + assert(got_interrupt); + got_interrupt = false; + pmc5_1 = mfspr(SPR_PMC5); + + reset_mmcr0(); + mtspr(SPR_PMC5, 0); + mtspr(SPR_MMCR0, mfspr(SPR_MMCR0) & ~(MMCR0_FC | MMCR0_FC56)); + asm volatile(".rep 20 ; nop ; .endr ; .long 0x0" ::: "memory"); + mtspr(SPR_MMCR0, mfspr(SPR_MMCR0) | (MMCR0_FC | MMCR0_FC56)); + assert(got_interrupt); + got_interrupt = false; + pmc5_2 = mfspr(SPR_PMC5); + + /* TCG and POWER9 do not count instructions around faults correctly */ + report_kfail(true, pmc5_1 + 20 == pmc5_2, "PMC5 counts instructions with fault"); It would be nice to have the TCG detection patch before this patch, so you could use the right condition here right from the start. + handle_exception(0x700, NULL, NULL); + handle_exception(0xe40, NULL, NULL); +} + +static void test_pmc5_with_sc(void) +{ + unsigned long pmc5_1, pmc5_2; + + handle_exception(0xc00, &sc_handler, NULL); + + reset_mmcr0(); + mtspr(SPR_PMC5, 0); + mtspr(SPR_MMCR0, mfspr(SPR_MMCR0) & ~(MMCR0_FC | MMCR0_FC56)); + asm volatile("sc 0" ::: "memory"); + mtspr(SPR_MMCR0, mfspr(SPR_MMCR0) | (MMCR0_FC | MMCR0_FC56)); + assert(got_interrupt); + got_interrupt = false; + pmc5_1 = mfspr(SPR_PMC5); + + reset_mmcr0(); + mtspr(SPR_PMC5, 0); + mtspr(SPR_MMCR0, mfspr(SPR_MMCR0) & ~(MMCR0_FC | MMCR0_FC56)); + asm volatile(".rep 20 ; nop ; .endr ; sc 0" ::: "memory"); + mtspr(SPR_MMCR0, mfspr(SPR_MMCR0) | (MMCR0_FC | MMCR0_FC56)); + assert(got_interrupt); + got_interrupt = false; + pmc5_2 = mfspr(SPR_PMC5); + + /* TCG does not count instructions around syscalls correctly */ + report_kfail(true, pmc5_1 + 20 == pmc5_2, "PMC5 counts instructions with syscall"); dito + handle_exception(0xc00, NULL, NULL); +} + +static void test_pmc56(void) +{ + unsigned long tmp; + + report_prefix_push("pmc56"); + + reset_mmcr0(); + mtspr(SPR_PMC5, 0); + mtspr(SPR_PMC6, 0); + report(mfspr(SPR_PMC5) == 0, "PMC5 zeroed"); + report(mfspr(SPR_PMC6) == 0, "PMC6 zeroed"); + mtspr(SPR_MMCR0, mfspr(SPR_MMCR0) & ~MMCR0_FC); + msleep(100); + report(mfspr(SPR_PMC5) == 0, "PMC5 frozen"); + report(mfspr(SPR_PMC6) == 0, "PMC6 frozen"); + mtspr(SPR_MMCR0, mfspr(SPR_MMCR0) & ~MMCR0_FC56); + mdelay(100); + mtspr(SPR_MMCR0, mfspr(SPR_MMCR0) | (MMCR0_FC | MMCR0_FC56)); + report(mfspr(SPR_PMC5) != 0, "PMC5 counting"); + report(mfspr(SPR_PMC6) != 0, "PMC6 counting"); + + /* Dynamic frequency scaling could cause to be out, so don't fail. */ + tmp = mfspr(SPR_PMC6); + report(true, "PMC6 ratio to reported clock frequency is %ld%%", tmp * 1000 / cpu_hz); report(true, ...) looks weird. Use report_info() instead? Thomas
Re: [kvm-unit-tests PATCH v9 26/31] powerpc: add usermode support
On 04/05/2024 14.28, Nicholas Piggin wrote: The biggest difficulty for user mode is MMU support. Otherwise it is a simple matter of setting and clearing MSR[PR] with rfid and sc respectively. Some common harness operations will fail in usermode, so some workarounds are reqiured (e.g., puts() can't be used directly). A usermode privileged instruction interrupt test is added. Signed-off-by: Nicholas Piggin --- Reviewed-by: Thomas Huth
Re: [kvm-unit-tests PATCH v9 25/31] powerpc: Add sieve.c common test
On 04/05/2024 14.28, Nicholas Piggin wrote: Now that sieve copes with lack of MMU support, it can be run by powerpc. Signed-off-by: Nicholas Piggin --- powerpc/Makefile.common | 1 + powerpc/sieve.c | 1 + powerpc/unittests.cfg | 3 +++ 3 files changed, 5 insertions(+) create mode 12 powerpc/sieve.c Reviewed-by: Thomas Huth
Re: [kvm-unit-tests PATCH v9 24/31] common/sieve: Support machines without MMU
On 04/05/2024 14.28, Nicholas Piggin wrote: Not all powerpc CPUs provide MMU support. Define vm_available() that is true by default but archs can override it. Use this to run VM tests. Cc: Paolo Bonzini Cc: Thomas Huth Cc: k...@vger.kernel.org Reviewed-by: Andrew Jones Signed-off-by: Nicholas Piggin --- Reviewed-by: Thomas Huth
Re: [kvm-unit-tests PATCH v9 23/31] common/sieve: Use vmalloc.h for setup_mmu definition
On 04/05/2024 14.28, Nicholas Piggin wrote: There is no good reason to put setup_vm in libcflat.h when it's defined in vmalloc.h. Cc: Paolo Bonzini Cc: Thomas Huth Cc: Janosch Frank Cc: Claudio Imbrenda Cc: Nico Böhr Cc: David Hildenbrand Cc: k...@vger.kernel.org Cc: linux-s...@vger.kernel.org Acked-by: Andrew Jones Signed-off-by: Nicholas Piggin --- Reviewed-by: Thomas Huth
Re: [kvm-unit-tests PATCH v9 22/31] powerpc: Add MMU support
On 04/05/2024 14.28, Nicholas Piggin wrote: Add support for radix MMU, 4kB and 64kB pages. This also adds MMU interrupt test cases, and runs the interrupts test entirely with MMU enabled if it is available (aside from machine check tests). Acked-by: Andrew Jones (configure changes) Signed-off-by: Nicholas Piggin --- ... diff --git a/lib/ppc64/mmu.c b/lib/ppc64/mmu.c new file mode 100644 index 0..5307cd862 --- /dev/null +++ b/lib/ppc64/mmu.c @@ -0,0 +1,281 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Radix MMU support + * + * Copyright (C) 2024, IBM Inc, Nicholas Piggin + * + * Derived from Linux kernel MMU code. + */ +#include +#include +#include +#include +#include +#include +#include + +#include "alloc_page.h" +#include "vmalloc.h" +#include +#include + +#include + +static pgd_t *identity_pgd; + +bool vm_available(void) +{ + return cpu_has_radix; +} + +bool mmu_enabled(void) +{ + return current_cpu()->pgtable != NULL; +} + +void mmu_enable(pgd_t *pgtable) +{ + struct cpu *cpu = current_cpu(); + + if (!pgtable) + pgtable = identity_pgd; + + cpu->pgtable = pgtable; + + mtmsr(mfmsr() | (MSR_IR|MSR_DR)); +} + +void mmu_disable(void) +{ + struct cpu *cpu = current_cpu(); + + cpu->pgtable = NULL; + + mtmsr(mfmsr() & ~(MSR_IR|MSR_DR)); +} + +static inline void tlbie(unsigned long rb, unsigned long rs, int ric, int prs, int r) +{ + asm volatile(".machine push ; .machine power9; ptesync ; tlbie %0,%1,%2,%3,%4 ; eieio ; tlbsync ; ptesync ; .machine pop" :: "r"(rb), "r"(rs), "i"(ric), "i"(prs), "i"(r) : "memory"); That's a very long line, please split it up after every assembly instruction (using \n for new lines). +} ... diff --git a/powerpc/mmu.c b/powerpc/mmu.c new file mode 100644 index 0..fef790506 --- /dev/null +++ b/powerpc/mmu.c @@ -0,0 +1,283 @@ +/* SPDX-License-Identifier: LGPL-2.0-only */ +/* + * MMU Tests + * + * Copyright 2024 Nicholas Piggin, IBM Corp. + */ +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +static inline void tlbie(unsigned long rb, unsigned long rs, int ric, int prs, int r) +{ + asm volatile(".machine push ; .machine power9; ptesync ; tlbie %0,%1,%2,%3,%4 ; eieio ; tlbsync ; ptesync ; .machine pop" :: "r"(rb), "r"(rs), "i"(ric), "i"(prs), "i"(r) : "memory"); +} Same function again? Maybe it could go into mmu.h instead? +static inline void tlbiel(unsigned long rb, unsigned long rs, int ric, int prs, int r) +{ + asm volatile(".machine push ; .machine power9; ptesync ; tlbiel %0,%1,%2,%3,%4 ; ptesync ; .machine pop" :: "r"(rb), "r"(rs), "i"(ric), "i"(prs), "i"(r) : "memory"); +} Please also split up the above long line. It would also be cool if you could get one of the other ppc guys at IBM to review this patch, since I don't have a clue about this MMU stuff at all. Thanks, Thomas
Re: [kvm-unit-tests PATCH v9 21/31] powerpc: Add timebase tests
On 04/05/2024 14.28, Nicholas Piggin wrote: This has a known failure on QEMU TCG machines where the decrementer interrupt is not lowered when the DEC wraps from -ve to +ve. Would it then make sense to mark the test with accel = kvm to avoid the test failure when running with TCG? diff --git a/powerpc/timebase.c b/powerpc/timebase.c new file mode 100644 index 0..02a4e33c0 --- /dev/null +++ b/powerpc/timebase.c @@ -0,0 +1,331 @@ +/* SPDX-License-Identifier: LGPL-2.0-only */ +/* + * Test Timebase + * + * Copyright 2024 Nicholas Piggin, IBM Corp. + * + * This contains tests of timebase facility, TB, DEC, etc. + */ +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +static int dec_bits = 0; + +static void cpu_dec_bits(int fdtnode, u64 regval __unused, void *arg __unused) +{ + const struct fdt_property *prop; + int plen; + + prop = fdt_get_property(dt_fdt(), fdtnode, "ibm,dec-bits", &plen); + if (!prop) { + dec_bits = 32; + return; + } + + /* Sanity check for the property layout (first two bytes are header) */ + assert(plen == 4); + + dec_bits = fdt32_to_cpu(*(uint32_t *)prop->data); +} + +/* Check amount of CPUs nodes that have the TM flag */ +static int find_dec_bits(void) +{ + int ret; + + ret = dt_for_each_cpu_node(cpu_dec_bits, NULL); What sense does it make to run this for each CPU node if the cpu_dec_bits function always overwrites the global dec_bits variable? Wouldn't it be sufficient to run this for the first node only? Or should the cpu_dec_bits function maybe check that all nodes have the same value? + if (ret < 0) + return ret; + + return dec_bits; +} + + +static bool do_migrate = false; +static volatile bool got_interrupt; +static volatile struct pt_regs recorded_regs; + +static uint64_t dec_max; +static uint64_t dec_min; + +static void test_tb(int argc, char **argv) +{ + uint64_t tb; + + tb = get_tb(); + if (do_migrate) + migrate(); + report(get_tb() >= tb, "timebase is incrementing"); If you use >= for testing, it could also mean that the TB stays at the same value, so "timebase is incrementing" sounds misleading. Maybe rather "timebase is not decreasing" ? Or wait a little bit, then check with ">" only ? +} + +static void dec_stop_handler(struct pt_regs *regs, void *data) +{ + mtspr(SPR_DEC, dec_max); +} + +static void dec_handler(struct pt_regs *regs, void *data) +{ + got_interrupt = true; + memcpy((void *)&recorded_regs, regs, sizeof(struct pt_regs)); + regs->msr &= ~MSR_EE; +} + +static void test_dec(int argc, char **argv) +{ + uint64_t tb1, tb2, dec; + int i; + + handle_exception(0x900, &dec_handler, NULL); + + for (i = 0; i < 100; i++) { + tb1 = get_tb(); + mtspr(SPR_DEC, dec_max); + dec = mfspr(SPR_DEC); + tb2 = get_tb(); + if (tb2 - tb1 < dec_max - dec) + break; + } + /* POWER CPUs can have a slight (few ticks) variation here */ + report_kfail(true, tb2 - tb1 >= dec_max - dec, "decrementer remains within TB after mtDEC"); + + tb1 = get_tb(); + mtspr(SPR_DEC, dec_max); + mdelay(1000); + dec = mfspr(SPR_DEC); + tb2 = get_tb(); + report(tb2 - tb1 >= dec_max - dec, "decrementer remains within TB after 1s"); + + mtspr(SPR_DEC, dec_max); + local_irq_enable(); + local_irq_disable(); + if (mfspr(SPR_DEC) <= dec_max) { + report(!got_interrupt, "no interrupt on decrementer positive"); + } + got_interrupt = false; + + mtspr(SPR_DEC, 1); + mdelay(100); /* Give the timer a chance to run */ + if (do_migrate) + migrate(); + local_irq_enable(); + local_irq_disable(); + report(got_interrupt, "interrupt on decrementer underflow"); + got_interrupt = false; + + if (do_migrate) + migrate(); + local_irq_enable(); + local_irq_disable(); + report(got_interrupt, "interrupt on decrementer still underflown"); + got_interrupt = false; + + mtspr(SPR_DEC, 0); + mdelay(100); /* Give the timer a chance to run */ + if (do_migrate) + migrate(); + local_irq_enable(); + local_irq_disable(); + report(got_interrupt, "DEC deal with set to 0"); + got_interrupt = false; + + /* Test for level-triggered decrementer */ + mtspr(SPR_DEC, -1ULL); + if (do_migrate) + migrate(); + local_irq_enable(); + local_irq_disable(); + report(got_interrupt, "interrupt on decrementer write MSB"); + got_interrupt = false; + + mtspr(SPR_DEC, dec_max); + local_irq_enable(); + if (do_migrate) + migrate(); + mtspr(SPR_DEC, -1);
Re: [kvm-unit-tests PATCH v9 20/31] powerpc: Add atomics tests
On 04/05/2024 14.28, Nicholas Piggin wrote: Signed-off-by: Nicholas Piggin --- Please provide at least a short patch description about what is being tested here! Thanks, Thomas
Re: [kvm-unit-tests PATCH v9 19/31] powerpc: Avoid using larx/stcx. in spinlocks when only one CPU is running
On 04/05/2024 14.28, Nicholas Piggin wrote: The test harness uses spinlocks if they are implemented with larx/stcx. it can prevent some test scenarios such as testing migration of a reservation. I'm having a hard time to understand that patch description. Maybe you could rephrase it / elaborate what's the exact problem here? Thanks, Thomas Signed-off-by: Nicholas Piggin --- lib/powerpc/asm/smp.h| 1 + lib/powerpc/smp.c| 5 + lib/powerpc/spinlock.c | 29 + lib/ppc64/asm/spinlock.h | 7 ++- powerpc/Makefile.common | 1 + 5 files changed, 42 insertions(+), 1 deletion(-) create mode 100644 lib/powerpc/spinlock.c diff --git a/lib/powerpc/asm/smp.h b/lib/powerpc/asm/smp.h index c45988bfa..66188b9dd 100644 --- a/lib/powerpc/asm/smp.h +++ b/lib/powerpc/asm/smp.h @@ -15,6 +15,7 @@ struct cpu { extern int nr_cpus_present; extern int nr_cpus_online; +extern bool multithreaded; extern struct cpu cpus[]; register struct cpu *__current_cpu asm("r13"); diff --git a/lib/powerpc/smp.c b/lib/powerpc/smp.c index 27b169841..73c0ef214 100644 --- a/lib/powerpc/smp.c +++ b/lib/powerpc/smp.c @@ -276,6 +276,8 @@ static void start_each_secondary(int fdtnode, u64 regval __unused, void *info) start_core(fdtnode, datap->entry); } +bool multithreaded = false; + /* * Start all stopped cpus on the guest at entry with register 3 set to r3 * We expect that we come in with only one thread currently started @@ -290,6 +292,7 @@ bool start_all_cpus(secondary_entry_fn entry) assert(nr_cpus_online == 1); assert(nr_started == 1); + multithreaded = true; ret = dt_for_each_cpu_node(start_each_secondary, &data); assert(ret == 0); assert(nr_started == nr_cpus_present); @@ -361,10 +364,12 @@ static void wait_each_secondary(int fdtnode, u64 regval __unused, void *info) void stop_all_cpus(void) { + assert(multithreaded); while (nr_cpus_online > 1) cpu_relax(); dt_for_each_cpu_node(wait_each_secondary, NULL); mb(); nr_started = 1; + multithreaded = false; } diff --git a/lib/powerpc/spinlock.c b/lib/powerpc/spinlock.c new file mode 100644 index 0..623a1f2c1 --- /dev/null +++ b/lib/powerpc/spinlock.c @@ -0,0 +1,29 @@ +/* SPDX-License-Identifier: LGPL-2.0 */ +#include +#include + +/* + * Skip the atomic when single-threaded, which helps avoid larx/stcx. in + * the harness when testing tricky larx/stcx. sequences (e.g., migration + * vs reservation). + */ +void spin_lock(struct spinlock *lock) +{ + if (!multithreaded) { + assert(lock->v == 0); + lock->v = 1; + } else { + while (__sync_lock_test_and_set(&lock->v, 1)) + ; + } +} + +void spin_unlock(struct spinlock *lock) +{ + assert(lock->v == 1); + if (!multithreaded) { + lock->v = 0; + } else { + __sync_lock_release(&lock->v); + } +} diff --git a/lib/ppc64/asm/spinlock.h b/lib/ppc64/asm/spinlock.h index f59eed191..b952386da 100644 --- a/lib/ppc64/asm/spinlock.h +++ b/lib/ppc64/asm/spinlock.h @@ -1,6 +1,11 @@ #ifndef _ASMPPC64_SPINLOCK_H_ #define _ASMPPC64_SPINLOCK_H_ -#include +struct spinlock { + unsigned int v; +}; + +void spin_lock(struct spinlock *lock); +void spin_unlock(struct spinlock *lock); #endif /* _ASMPPC64_SPINLOCK_H_ */ diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common index b98f71c2f..1ee9c25d6 100644 --- a/powerpc/Makefile.common +++ b/powerpc/Makefile.common @@ -49,6 +49,7 @@ cflatobjs += lib/powerpc/rtas.o cflatobjs += lib/powerpc/processor.o cflatobjs += lib/powerpc/handlers.o cflatobjs += lib/powerpc/smp.o +cflatobjs += lib/powerpc/spinlock.o OBJDIRS += lib/powerpc
Re: [kvm-unit-tests PATCH v9 18/31] powerpc: Permit ACCEL=tcg,thread=single
On 04/05/2024 14.28, Nicholas Piggin wrote: Modify run script to permit single vs mttcg threading, add a thread=single smp case to unittests.cfg. Signed-off-by: Nicholas Piggin --- powerpc/run | 4 ++-- powerpc/unittests.cfg | 6 ++ 2 files changed, 8 insertions(+), 2 deletions(-) Reviewed-by: Thomas Huth
Re: [kvm-unit-tests PATCH v9 16/31] powerpc: add SMP and IPI support
On 04/05/2024 14.28, Nicholas Piggin wrote: powerpc SMP support is very primitive and does not set up a first-class runtime environment for secondary CPUs. This reworks SMP support, and provides a complete C and harness environment for the secondaries, including interrupt handling, as well as IPI support. Signed-off-by: Nicholas Piggin I now skimmed through the patch and it looks fine so far: Acked-by: Thomas Huth
Re: [kvm-unit-tests PATCH v9 15/31] powerpc: Enable page alloc operations
On 04/05/2024 14.28, Nicholas Piggin wrote: These will be used for stack allocation for secondary CPUs. Signed-off-by: Nicholas Piggin --- lib/powerpc/setup.c | 8 powerpc/Makefile.common | 1 + 2 files changed, 9 insertions(+) Reviewed-by: Thomas Huth
Re: [kvm-unit-tests PATCH v9 14/31] powerpc: Remove broken SMP exception stack setup
On 04/05/2024 14.28, Nicholas Piggin wrote: The exception stack setup does not work correctly for SMP, because it is the boot processor that calls cpu_set() which sets SPRG2 to the exception stack, not the target CPU itself. So secondaries never got their SPRG2 set to a valid exception stack. So secondary CPUs currently must not run into any exceptions? Remove the SMP code and just set an exception stack for the boot processor. Make the stack 64kB while we're here, to match the size of the regular stack. Signed-off-by: Nicholas Piggin --- lib/powerpc/setup.c | 16 1 file changed, 8 insertions(+), 8 deletions(-) Reviewed-by: Thomas Huth
Re: [kvm-unit-tests PATCH v9 07/31] scripts: allow machine option to be specified in unittests.cfg
On 08/05/2024 14.58, Thomas Huth wrote: On 08/05/2024 14.55, Thomas Huth wrote: On 08/05/2024 14.27, Nicholas Piggin wrote: On Wed May 8, 2024 at 1:08 AM AEST, Thomas Huth wrote: On 04/05/2024 14.28, Nicholas Piggin wrote: This allows different machines with different requirements to be supported by run_tests.sh, similarly to how different accelerators are handled. Acked-by: Thomas Huth Acked-by: Andrew Jones Signed-off-by: Nicholas Piggin --- docs/unittests.txt | 7 +++ scripts/common.bash | 8 ++-- scripts/runtime.bash | 16 3 files changed, 25 insertions(+), 6 deletions(-) diff --git a/docs/unittests.txt b/docs/unittests.txt index 7cf2c55ad..6449efd78 100644 --- a/docs/unittests.txt +++ b/docs/unittests.txt @@ -42,6 +42,13 @@ For / directories that support multiple architectures, this restricts the test to the specified arch. By default, the test will run on any architecture. +machine +--- +For those architectures that support multiple machine types, this restricts +the test to the specified machine. By default, the test will run on +any machine type. (Note, the machine can be specified with the MACHINE= +environment variable, and defaults to the architecture's default.) + smp --- smp = diff --git a/scripts/common.bash b/scripts/common.bash index 5e9ad53e2..3aa557c8c 100644 --- a/scripts/common.bash +++ b/scripts/common.bash @@ -10,6 +10,7 @@ function for_each_unittest() local opts local groups local arch + local machine local check local accel local timeout @@ -21,7 +22,7 @@ function for_each_unittest() if [[ "$line" =~ ^\[(.*)\]$ ]]; then rematch=${BASH_REMATCH[1]} if [ -n "${testname}" ]; then - $(arch_cmd) "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" + $(arch_cmd) "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$machine" "$check" "$accel" "$timeout" fi testname=$rematch smp=1 @@ -29,6 +30,7 @@ function for_each_unittest() opts="" groups="" arch="" + machine="" check="" accel="" timeout="" @@ -58,6 +60,8 @@ function for_each_unittest() groups=${BASH_REMATCH[1]} elif [[ $line =~ ^arch\ *=\ *(.*)$ ]]; then arch=${BASH_REMATCH[1]} + elif [[ $line =~ ^machine\ *=\ *(.*)$ ]]; then + machine=${BASH_REMATCH[1]} elif [[ $line =~ ^check\ *=\ *(.*)$ ]]; then check=${BASH_REMATCH[1]} elif [[ $line =~ ^accel\ *=\ *(.*)$ ]]; then @@ -67,7 +71,7 @@ function for_each_unittest() fi done if [ -n "${testname}" ]; then - $(arch_cmd) "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" + $(arch_cmd) "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$machine" "$check" "$accel" "$timeout" fi exec {fd}<&- } diff --git a/scripts/runtime.bash b/scripts/runtime.bash index 177b62166..0c96d6ea2 100644 --- a/scripts/runtime.bash +++ b/scripts/runtime.bash @@ -32,7 +32,7 @@ premature_failure() get_cmdline() { local kernel=$1 - echo "TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel $RUNTIME_arch_run $kernel -smp $smp $opts" + echo "TESTNAME=$testname TIMEOUT=$timeout MACHINE=$machine ACCEL=$accel $RUNTIME_arch_run $kernel -smp $smp $opts" } skip_nodefault() @@ -80,9 +80,10 @@ function run() local kernel="$4" local opts="$5" local arch="$6" - local check="${CHECK:-$7}" - local accel="$8" - local timeout="${9:-$TIMEOUT}" # unittests.cfg overrides the default + local machine="$7" + local check="${CHECK:-$8}" + local accel="$9" + local timeout="${10:-$TIMEOUT}" # unittests.cfg overrides the default if [ "${CONFIG_EFI}" == "y" ]; then kernel=${kernel/%.flat/.efi} @@ -116,6 +117,13 @@ function run() return 2 fi + if [ -n "$machine" ] && [ -n "$MACHINE" ] && [ "$machine" != "$MACHINE" ]; then + print_result &quo
Re: [kvm-unit-tests PATCH v9 07/31] scripts: allow machine option to be specified in unittests.cfg
On 08/05/2024 14.55, Thomas Huth wrote: On 08/05/2024 14.27, Nicholas Piggin wrote: On Wed May 8, 2024 at 1:08 AM AEST, Thomas Huth wrote: On 04/05/2024 14.28, Nicholas Piggin wrote: This allows different machines with different requirements to be supported by run_tests.sh, similarly to how different accelerators are handled. Acked-by: Thomas Huth Acked-by: Andrew Jones Signed-off-by: Nicholas Piggin --- docs/unittests.txt | 7 +++ scripts/common.bash | 8 ++-- scripts/runtime.bash | 16 3 files changed, 25 insertions(+), 6 deletions(-) diff --git a/docs/unittests.txt b/docs/unittests.txt index 7cf2c55ad..6449efd78 100644 --- a/docs/unittests.txt +++ b/docs/unittests.txt @@ -42,6 +42,13 @@ For / directories that support multiple architectures, this restricts the test to the specified arch. By default, the test will run on any architecture. +machine +--- +For those architectures that support multiple machine types, this restricts +the test to the specified machine. By default, the test will run on +any machine type. (Note, the machine can be specified with the MACHINE= +environment variable, and defaults to the architecture's default.) + smp --- smp = diff --git a/scripts/common.bash b/scripts/common.bash index 5e9ad53e2..3aa557c8c 100644 --- a/scripts/common.bash +++ b/scripts/common.bash @@ -10,6 +10,7 @@ function for_each_unittest() local opts local groups local arch + local machine local check local accel local timeout @@ -21,7 +22,7 @@ function for_each_unittest() if [[ "$line" =~ ^\[(.*)\]$ ]]; then rematch=${BASH_REMATCH[1]} if [ -n "${testname}" ]; then - $(arch_cmd) "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" + $(arch_cmd) "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$machine" "$check" "$accel" "$timeout" fi testname=$rematch smp=1 @@ -29,6 +30,7 @@ function for_each_unittest() opts="" groups="" arch="" + machine="" check="" accel="" timeout="" @@ -58,6 +60,8 @@ function for_each_unittest() groups=${BASH_REMATCH[1]} elif [[ $line =~ ^arch\ *=\ *(.*)$ ]]; then arch=${BASH_REMATCH[1]} + elif [[ $line =~ ^machine\ *=\ *(.*)$ ]]; then + machine=${BASH_REMATCH[1]} elif [[ $line =~ ^check\ *=\ *(.*)$ ]]; then check=${BASH_REMATCH[1]} elif [[ $line =~ ^accel\ *=\ *(.*)$ ]]; then @@ -67,7 +71,7 @@ function for_each_unittest() fi done if [ -n "${testname}" ]; then - $(arch_cmd) "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" + $(arch_cmd) "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$machine" "$check" "$accel" "$timeout" fi exec {fd}<&- } diff --git a/scripts/runtime.bash b/scripts/runtime.bash index 177b62166..0c96d6ea2 100644 --- a/scripts/runtime.bash +++ b/scripts/runtime.bash @@ -32,7 +32,7 @@ premature_failure() get_cmdline() { local kernel=$1 - echo "TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel $RUNTIME_arch_run $kernel -smp $smp $opts" + echo "TESTNAME=$testname TIMEOUT=$timeout MACHINE=$machine ACCEL=$accel $RUNTIME_arch_run $kernel -smp $smp $opts" } skip_nodefault() @@ -80,9 +80,10 @@ function run() local kernel="$4" local opts="$5" local arch="$6" - local check="${CHECK:-$7}" - local accel="$8" - local timeout="${9:-$TIMEOUT}" # unittests.cfg overrides the default + local machine="$7" + local check="${CHECK:-$8}" + local accel="$9" + local timeout="${10:-$TIMEOUT}" # unittests.cfg overrides the default if [ "${CONFIG_EFI}" == "y" ]; then kernel=${kernel/%.flat/.efi} @@ -116,6 +117,13 @@ function run() return 2 fi + if [ -n "$machine" ] && [ -n "$MACHINE" ] && [ "$machine" != "$MACHINE" ]; then + print_result "SKIP" $testname "" &qu
Re: [kvm-unit-tests PATCH v9 07/31] scripts: allow machine option to be specified in unittests.cfg
On 08/05/2024 14.27, Nicholas Piggin wrote: On Wed May 8, 2024 at 1:08 AM AEST, Thomas Huth wrote: On 04/05/2024 14.28, Nicholas Piggin wrote: This allows different machines with different requirements to be supported by run_tests.sh, similarly to how different accelerators are handled. Acked-by: Thomas Huth Acked-by: Andrew Jones Signed-off-by: Nicholas Piggin --- docs/unittests.txt | 7 +++ scripts/common.bash | 8 ++-- scripts/runtime.bash | 16 3 files changed, 25 insertions(+), 6 deletions(-) diff --git a/docs/unittests.txt b/docs/unittests.txt index 7cf2c55ad..6449efd78 100644 --- a/docs/unittests.txt +++ b/docs/unittests.txt @@ -42,6 +42,13 @@ For / directories that support multiple architectures, this restricts the test to the specified arch. By default, the test will run on any architecture. +machine +--- +For those architectures that support multiple machine types, this restricts +the test to the specified machine. By default, the test will run on +any machine type. (Note, the machine can be specified with the MACHINE= +environment variable, and defaults to the architecture's default.) + smp --- smp = diff --git a/scripts/common.bash b/scripts/common.bash index 5e9ad53e2..3aa557c8c 100644 --- a/scripts/common.bash +++ b/scripts/common.bash @@ -10,6 +10,7 @@ function for_each_unittest() local opts local groups local arch + local machine local check local accel local timeout @@ -21,7 +22,7 @@ function for_each_unittest() if [[ "$line" =~ ^\[(.*)\]$ ]]; then rematch=${BASH_REMATCH[1]} if [ -n "${testname}" ]; then - $(arch_cmd) "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" + $(arch_cmd) "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$machine" "$check" "$accel" "$timeout" fi testname=$rematch smp=1 @@ -29,6 +30,7 @@ function for_each_unittest() opts="" groups="" arch="" + machine="" check="" accel="" timeout="" @@ -58,6 +60,8 @@ function for_each_unittest() groups=${BASH_REMATCH[1]} elif [[ $line =~ ^arch\ *=\ *(.*)$ ]]; then arch=${BASH_REMATCH[1]} + elif [[ $line =~ ^machine\ *=\ *(.*)$ ]]; then + machine=${BASH_REMATCH[1]} elif [[ $line =~ ^check\ *=\ *(.*)$ ]]; then check=${BASH_REMATCH[1]} elif [[ $line =~ ^accel\ *=\ *(.*)$ ]]; then @@ -67,7 +71,7 @@ function for_each_unittest() fi done if [ -n "${testname}" ]; then - $(arch_cmd) "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" + $(arch_cmd) "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$machine" "$check" "$accel" "$timeout" fi exec {fd}<&- } diff --git a/scripts/runtime.bash b/scripts/runtime.bash index 177b62166..0c96d6ea2 100644 --- a/scripts/runtime.bash +++ b/scripts/runtime.bash @@ -32,7 +32,7 @@ premature_failure() get_cmdline() { local kernel=$1 -echo "TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel $RUNTIME_arch_run $kernel -smp $smp $opts" +echo "TESTNAME=$testname TIMEOUT=$timeout MACHINE=$machine ACCEL=$accel $RUNTIME_arch_run $kernel -smp $smp $opts" } skip_nodefault() @@ -80,9 +80,10 @@ function run() local kernel="$4" local opts="$5" local arch="$6" -local check="${CHECK:-$7}" -local accel="$8" -local timeout="${9:-$TIMEOUT}" # unittests.cfg overrides the default +local machine="$7" +local check="${CHECK:-$8}" +local accel="$9" +local timeout="${10:-$TIMEOUT}" # unittests.cfg overrides the default if [ "${CONFIG_EFI}" == "y" ]; then kernel=${kernel/%.flat/.efi} @@ -116,6 +117,13 @@ function run() return 2 fi
Re: [kvm-unit-tests PATCH v9 07/31] scripts: allow machine option to be specified in unittests.cfg
On 04/05/2024 14.28, Nicholas Piggin wrote: This allows different machines with different requirements to be supported by run_tests.sh, similarly to how different accelerators are handled. Acked-by: Thomas Huth Acked-by: Andrew Jones Signed-off-by: Nicholas Piggin --- docs/unittests.txt | 7 +++ scripts/common.bash | 8 ++-- scripts/runtime.bash | 16 3 files changed, 25 insertions(+), 6 deletions(-) diff --git a/docs/unittests.txt b/docs/unittests.txt index 7cf2c55ad..6449efd78 100644 --- a/docs/unittests.txt +++ b/docs/unittests.txt @@ -42,6 +42,13 @@ For / directories that support multiple architectures, this restricts the test to the specified arch. By default, the test will run on any architecture. +machine +--- +For those architectures that support multiple machine types, this restricts +the test to the specified machine. By default, the test will run on +any machine type. (Note, the machine can be specified with the MACHINE= +environment variable, and defaults to the architecture's default.) + smp --- smp = diff --git a/scripts/common.bash b/scripts/common.bash index 5e9ad53e2..3aa557c8c 100644 --- a/scripts/common.bash +++ b/scripts/common.bash @@ -10,6 +10,7 @@ function for_each_unittest() local opts local groups local arch + local machine local check local accel local timeout @@ -21,7 +22,7 @@ function for_each_unittest() if [[ "$line" =~ ^\[(.*)\]$ ]]; then rematch=${BASH_REMATCH[1]} if [ -n "${testname}" ]; then - $(arch_cmd) "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" + $(arch_cmd) "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$machine" "$check" "$accel" "$timeout" fi testname=$rematch smp=1 @@ -29,6 +30,7 @@ function for_each_unittest() opts="" groups="" arch="" + machine="" check="" accel="" timeout="" @@ -58,6 +60,8 @@ function for_each_unittest() groups=${BASH_REMATCH[1]} elif [[ $line =~ ^arch\ *=\ *(.*)$ ]]; then arch=${BASH_REMATCH[1]} + elif [[ $line =~ ^machine\ *=\ *(.*)$ ]]; then + machine=${BASH_REMATCH[1]} elif [[ $line =~ ^check\ *=\ *(.*)$ ]]; then check=${BASH_REMATCH[1]} elif [[ $line =~ ^accel\ *=\ *(.*)$ ]]; then @@ -67,7 +71,7 @@ function for_each_unittest() fi done if [ -n "${testname}" ]; then - $(arch_cmd) "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" + $(arch_cmd) "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$machine" "$check" "$accel" "$timeout" fi exec {fd}<&- } diff --git a/scripts/runtime.bash b/scripts/runtime.bash index 177b62166..0c96d6ea2 100644 --- a/scripts/runtime.bash +++ b/scripts/runtime.bash @@ -32,7 +32,7 @@ premature_failure() get_cmdline() { local kernel=$1 -echo "TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel $RUNTIME_arch_run $kernel -smp $smp $opts" +echo "TESTNAME=$testname TIMEOUT=$timeout MACHINE=$machine ACCEL=$accel $RUNTIME_arch_run $kernel -smp $smp $opts" } skip_nodefault() @@ -80,9 +80,10 @@ function run() local kernel="$4" local opts="$5" local arch="$6" -local check="${CHECK:-$7}" -local accel="$8" -local timeout="${9:-$TIMEOUT}" # unittests.cfg overrides the default +local machine="$7" +local check="${CHECK:-$8}" +local accel="$9" +local timeout="${10:-$TIMEOUT}" # unittests.cfg overrides the default if [ "${CONFIG_EFI}" == "y" ]; then kernel=${kernel/%.flat/.efi} @@ -116,6 +117,13 @@ function run() return 2 fi +if [ -n "$machine" ] && [ -n "$MACHINE" ] && [ "$machine" != "$MACHIN
Re: [kvm-unit-tests PATCH v9 17/31] powerpc: Add cpu_relax
On 04/05/2024 14.28, Nicholas Piggin wrote: Add a cpu_relax variant that uses SMT priority nop instructions like Linux. This was split out of the SMP patch because it affects the sprs test case. Signed-off-by: Nicholas Piggin --- lib/ppc64/asm/barrier.h | 1 + powerpc/sprs.c | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) Reviewed-by: Thomas Huth
Re: [kvm-unit-tests PATCH v9 12/31] powerpc: general interrupt tests
On 04/05/2024 14.28, Nicholas Piggin wrote: Add basic testing of various kinds of interrupts, machine check, page fault, illegal, decrementer, trace, syscall, etc. This has a known failure on QEMU TCG pseries machines where MSR[ME] can be incorrectly set to 0. Signed-off-by: Nicholas Piggin --- lib/powerpc/asm/processor.h | 4 + lib/powerpc/asm/reg.h | 17 ++ lib/powerpc/setup.c | 11 + lib/ppc64/asm/ptrace.h | 16 ++ powerpc/Makefile.common | 3 +- powerpc/interrupts.c| 414 powerpc/unittests.cfg | 3 + 7 files changed, 467 insertions(+), 1 deletion(-) create mode 100644 powerpc/interrupts.c Acked-by: Thomas Huth
Re: [kvm-unit-tests PATCH v9 03/31] powerpc: Mark known failing tests as kfail
On 07/05/2024 06.07, Nicholas Piggin wrote: On Mon May 6, 2024 at 5:37 PM AEST, Thomas Huth wrote: On 04/05/2024 14.28, Nicholas Piggin wrote: Mark the failing h_cede_tm and spapr_vpa tests as kfail. Signed-off-by: Nicholas Piggin --- powerpc/spapr_vpa.c | 3 ++- powerpc/tm.c| 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/powerpc/spapr_vpa.c b/powerpc/spapr_vpa.c index c2075e157..46fa0485c 100644 --- a/powerpc/spapr_vpa.c +++ b/powerpc/spapr_vpa.c @@ -150,7 +150,8 @@ static void test_vpa(void) report_fail("Could not deregister after registration"); disp_count1 = be32_to_cpu(vpa->vp_dispatch_count); - report(disp_count1 % 2 == 1, "Dispatch count is odd after deregister"); + /* TCG known fail, could be wrong test, must verify against PowerVM */ + report_kfail(true, disp_count1 % 2 == 1, "Dispatch count is odd after deregister"); Using "true" as first argument looks rather pointless - then you could also simply delete the test completely if it can never be tested reliably. Thus could you please introduce a helper function is_tcg() that could be used to check whether we run under TCG (and not KVM)? I think you could check for "linux,kvm" in the "compatible" property in /hypervisor in the device tree to see whether we're running in KVM mode or in TCG mode. This I added in patch 30. The reason for the suboptimal patch ordering was just me being lazy and avoiding rebasing annoyance. I'd written a bunch of failing test cases for QEMU work, but hadn't done the kvm/tcg test yet. It had a few conflicts so I put it at the end... can rebase if you'd really prefer. Ah, ok, no need to rebase then, as long it's there in the end, it's fine. Thanks, Thomas
Re: [kvm-unit-tests PATCH v9 02/31] report: Add known failure reporting option
On 06/05/2024 10.01, Andrew Jones wrote: On Mon, May 06, 2024 at 09:25:37AM GMT, Thomas Huth wrote: On 04/05/2024 14.28, Nicholas Piggin wrote: There are times we would like to test a function that is known to fail in some conditions due to a bug in implementation (QEMU, KVM, or even hardware). It would be nice to count these as known failures and not report a summary failure. xfail is not the same thing, xfail means failure is required and a pass causes the test to fail. So add kfail for known failures. Actually, I wonder whether that's not rather a bug in report_xfail() instead. Currently, when you call report_xfail(true, ...), the result is *always* counted as a failure, either as an expected failure (if the test really failed), or as a normal failure (if the test succeeded). What's the point of counting a successful test as a failure?? Andrew, you've originally introduced report_xfail in commit a5af7b8a67e, could you please comment on this? An expected failure passes when the test fails and fails when the test passes, i.e. XFAIL == PASS (but separately accounted with 'xfailures') XPASS == FAIL If we expect something to fail and it passes then this may be due to the thing being fixed, so we should change the test to expect success, or due to the test being written incorrectly for our expectations. Either way, when an expected failure doesn't fail, it means our expectations are wrong and we need to be alerted to that, hence a FAIL is reported. Ok, so this was on purpose, indeed. Maybe we should add this information in a comment right in front of the function, so that others don't scratch their head, too? Anyway, this patch here is fine then: Reviewed-by: Thomas Huth
Re: [kvm-unit-tests PATCH v9 03/31] powerpc: Mark known failing tests as kfail
On 04/05/2024 14.28, Nicholas Piggin wrote: Mark the failing h_cede_tm and spapr_vpa tests as kfail. Signed-off-by: Nicholas Piggin --- powerpc/spapr_vpa.c | 3 ++- powerpc/tm.c| 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/powerpc/spapr_vpa.c b/powerpc/spapr_vpa.c index c2075e157..46fa0485c 100644 --- a/powerpc/spapr_vpa.c +++ b/powerpc/spapr_vpa.c @@ -150,7 +150,8 @@ static void test_vpa(void) report_fail("Could not deregister after registration"); disp_count1 = be32_to_cpu(vpa->vp_dispatch_count); - report(disp_count1 % 2 == 1, "Dispatch count is odd after deregister"); + /* TCG known fail, could be wrong test, must verify against PowerVM */ + report_kfail(true, disp_count1 % 2 == 1, "Dispatch count is odd after deregister"); Using "true" as first argument looks rather pointless - then you could also simply delete the test completely if it can never be tested reliably. Thus could you please introduce a helper function is_tcg() that could be used to check whether we run under TCG (and not KVM)? I think you could check for "linux,kvm" in the "compatible" property in /hypervisor in the device tree to see whether we're running in KVM mode or in TCG mode. report_prefix_pop(); } diff --git a/powerpc/tm.c b/powerpc/tm.c index 6b1ceeb6e..d9e7f455d 100644 --- a/powerpc/tm.c +++ b/powerpc/tm.c @@ -133,7 +133,8 @@ int main(int argc, char **argv) report_skip("TM is not available"); goto done; } - report(cpus_with_tm == nr_cpus, + /* KVM does not report TM in secondary threads in POWER9 */ + report_kfail(true, cpus_with_tm == nr_cpus, "TM available in all 'ibm,pa-features' properties"); Could you check the PVR for POWER9 here instead of using "true" as first parameter? Thomas
Re: [kvm-unit-tests PATCH v9 02/31] report: Add known failure reporting option
On 04/05/2024 14.28, Nicholas Piggin wrote: There are times we would like to test a function that is known to fail in some conditions due to a bug in implementation (QEMU, KVM, or even hardware). It would be nice to count these as known failures and not report a summary failure. xfail is not the same thing, xfail means failure is required and a pass causes the test to fail. So add kfail for known failures. Actually, I wonder whether that's not rather a bug in report_xfail() instead. Currently, when you call report_xfail(true, ...), the result is *always* counted as a failure, either as an expected failure (if the test really failed), or as a normal failure (if the test succeeded). What's the point of counting a successful test as a failure?? Andrew, you've originally introduced report_xfail in commit a5af7b8a67e, could you please comment on this? IMHO we should rather do something like this instead: diff --git a/lib/report.c b/lib/report.c --- a/lib/report.c +++ b/lib/report.c @@ -98,7 +98,7 @@ static void va_report(const char *msg_fmt, skipped++; else if (xfail && !pass) xfailures++; - else if (xfail || !pass) + else if (!xfail && !pass) failures++; spin_unlock(&lock); Thomas
Re: [kvm-unit-tests PATCH v9 01/31] doc: update unittests doc
On 04/05/2024 14.28, Nicholas Piggin wrote: This adds a few minor fixes. Signed-off-by: Nicholas Piggin --- docs/unittests.txt | 12 +++- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/docs/unittests.txt b/docs/unittests.txt index 3192a60ec..7cf2c55ad 100644 --- a/docs/unittests.txt +++ b/docs/unittests.txt @@ -15,8 +15,8 @@ unittests.cfg format # is the comment symbol, all following contents of the line is ignored. -Each unit test is defined with a [unit-test-name] line, followed by -a set of parameters that control how the test case is run. The name is +Each unit test is defined with a [unit-test-name] line, followed by a +set of parameters that control how the test case is run. The name is arbitrary and appears in the status reporting output. Parameters appear on their own lines under the test name, and have a @@ -62,8 +62,8 @@ groups groups = ... Used to group the test cases for the `run_tests.sh -g ...` run group -option. Adding a test to the nodefault group will cause it to not be -run by default. +option. The group name is arbitrary, aside from the nodefault group +which makes the test to not be run by default. accel - @@ -82,8 +82,10 @@ Optional timeout in seconds, after which the test will be killed and fail. check - -check = =< +check = = Check a file for a particular value before running a test. The check line can contain multiple files to check separated by a space, but each check parameter needs to be of the form = + +The path and value can not contain space, =, or shell wildcard characters. Could you comment on my feedback here, please: https://lore.kernel.org/kvm/951ccd88-0e39-4379-8d86-718e72594...@redhat.com/ Thanks, Thomas
Re: [kvm-unit-tests PATCH v8 06/35] gitlab-ci: Run migration selftest on s390x and powerpc
On 16/04/2024 09.55, Thomas Huth wrote: On 16/04/2024 09.18, Thomas Huth wrote: On 11/04/2024 21.22, Thomas Huth wrote: On 08/04/2024 18.06, Nico Boehr wrote: Quoting Nicholas Piggin (2024-04-05 10:35:07) The migration harness is complicated and easy to break so CI will be helpful. Signed-off-by: Nicholas Piggin --- .gitlab-ci.yml | 32 +++- s390x/unittests.cfg | 8 2 files changed, 31 insertions(+), 9 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index ff34b1f50..60b3cdfd2 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml [...] @@ -135,7 +147,7 @@ build-riscv64: build-s390x: extends: .outoftree_template script: - - dnf install -y qemu-system-s390x gcc-s390x-linux-gnu + - dnf install -y qemu-system-s390x gcc-s390x-linux-gnu nmap-ncat - mkdir build - cd build - ../configure --arch=s390x --cross-prefix=s390x-linux-gnu- @@ -161,6 +173,8 @@ build-s390x: sclp-1g sclp-3g selftest-setup + selftest-migration-kvm We're running under TCG in the Gitlab CI. I'm a little bit confused why we're running a KVM-only test here. The build-s390x job is TCG, indeed, but we have the "s390x-kvm" job that runs on a KVM-capable s390x host, so it could be added there? I now gave it a try and it seems to work, so I updated this patch and pushed it to the repository now. Hmm, "selftest-migration" now was failing once here: https://gitlab.com/kvm-unit-tests/kvm-unit-tests/-/jobs/6633865591 Let's keep an eye on it, and if it is not stable enough, we might need to disable it in the CI again... And it just failed again: https://gitlab.com/kvm-unit-tests/kvm-unit-tests/-/jobs/6635395811 ... not sure whether this is due to the slow CI machine or whether there is still a bug lurking around somewhere, but anyway, I disabled it now for the CI again to avoid that other MRs get affected. Thomas
Re: [kvm-unit-tests PATCH v8 11/35] powerpc/sprs: Specify SPRs with data rather than code
On 05/04/2024 10.35, Nicholas Piggin wrote: A significant rework that builds an array of 'struct spr', where each element describes an SPR. This makes various metadata about the SPR like name and access type easier to carry and use. Hypervisor privileged registers are described despite not being used at the moment for completeness, but also the code might one day be reused for a hypervisor-privileged test. Acked-by: Thomas Huth Signed-off-by: Nicholas Piggin --- lib/powerpc/asm/reg.h | 2 + powerpc/sprs.c| 647 +- 2 files changed, 457 insertions(+), 192 deletions(-) I tried to merge this patch, but it seems to break running the tests on Ubuntu Focal with Clang 11 in the Travis-CI: https://app.travis-ci.com/github/huth/kvm-unit-tests/jobs/620577246 Could you please have a look? Thanks, Thomas
Re: [kvm-unit-tests PATCH v8 06/35] gitlab-ci: Run migration selftest on s390x and powerpc
On 16/04/2024 09.18, Thomas Huth wrote: On 11/04/2024 21.22, Thomas Huth wrote: On 08/04/2024 18.06, Nico Boehr wrote: Quoting Nicholas Piggin (2024-04-05 10:35:07) The migration harness is complicated and easy to break so CI will be helpful. Signed-off-by: Nicholas Piggin --- .gitlab-ci.yml | 32 +++- s390x/unittests.cfg | 8 2 files changed, 31 insertions(+), 9 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index ff34b1f50..60b3cdfd2 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml [...] @@ -135,7 +147,7 @@ build-riscv64: build-s390x: extends: .outoftree_template script: - - dnf install -y qemu-system-s390x gcc-s390x-linux-gnu + - dnf install -y qemu-system-s390x gcc-s390x-linux-gnu nmap-ncat - mkdir build - cd build - ../configure --arch=s390x --cross-prefix=s390x-linux-gnu- @@ -161,6 +173,8 @@ build-s390x: sclp-1g sclp-3g selftest-setup + selftest-migration-kvm We're running under TCG in the Gitlab CI. I'm a little bit confused why we're running a KVM-only test here. The build-s390x job is TCG, indeed, but we have the "s390x-kvm" job that runs on a KVM-capable s390x host, so it could be added there? I now gave it a try and it seems to work, so I updated this patch and pushed it to the repository now. Hmm, "selftest-migration" now was failing once here: https://gitlab.com/kvm-unit-tests/kvm-unit-tests/-/jobs/6633865591 Let's keep an eye on it, and if it is not stable enough, we might need to disable it in the CI again... Thomas
Re: [kvm-unit-tests PATCH v8 10/35] powerpc: interrupt stack backtracing
On 05/04/2024 10.35, Nicholas Piggin wrote: Add support for backtracing across interrupt stacks, and add interrupt frame backtrace for unhandled interrupts. This requires a back-chain created from initial interrupt stack frame to the r1 value of the interrupted context. A label is added at the return location of the exception handler call, so the unwinder can recognize the initial interrupt frame. The additional cstart entry-frame is no longer required because the unwinder now looks for frame == 0 as well as address == 0. Signed-off-by: Nicholas Piggin --- Acked-by: Thomas Huth
Re: [kvm-unit-tests PATCH v8 06/35] gitlab-ci: Run migration selftest on s390x and powerpc
On 11/04/2024 21.22, Thomas Huth wrote: On 08/04/2024 18.06, Nico Boehr wrote: Quoting Nicholas Piggin (2024-04-05 10:35:07) The migration harness is complicated and easy to break so CI will be helpful. Signed-off-by: Nicholas Piggin --- .gitlab-ci.yml | 32 +++- s390x/unittests.cfg | 8 2 files changed, 31 insertions(+), 9 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index ff34b1f50..60b3cdfd2 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml [...] @@ -135,7 +147,7 @@ build-riscv64: build-s390x: extends: .outoftree_template script: - - dnf install -y qemu-system-s390x gcc-s390x-linux-gnu + - dnf install -y qemu-system-s390x gcc-s390x-linux-gnu nmap-ncat - mkdir build - cd build - ../configure --arch=s390x --cross-prefix=s390x-linux-gnu- @@ -161,6 +173,8 @@ build-s390x: sclp-1g sclp-3g selftest-setup + selftest-migration-kvm We're running under TCG in the Gitlab CI. I'm a little bit confused why we're running a KVM-only test here. The build-s390x job is TCG, indeed, but we have the "s390x-kvm" job that runs on a KVM-capable s390x host, so it could be added there? I now gave it a try and it seems to work, so I updated this patch and pushed it to the repository now. Thomas
Re: [kvm-unit-tests PATCH v8 09/35] powerpc: Fix stack backtrace termination
On 05/04/2024 10.35, Nicholas Piggin wrote: The backtrace handler terminates when it sees a NULL caller address, but the powerpc stack setup does not keep such a NULL caller frame at the start of the stack. This happens to work on pseries because the memory at 0 is mapped and it contains 0 at the location of the return address pointer if it were a stack frame. But this is fragile, and does not work with powernv where address 0 contains firmware instructions. Use the existing dummy frame on stack as the NULL caller, and create a new frame on stack for the entry code. Signed-off-by: Nicholas Piggin --- powerpc/cstart64.S | 15 +++ 1 file changed, 15 insertions(+) Reviewed-by: Thomas Huth
Re: [kvm-unit-tests PATCH v8 05/35] arch-run: Add a "continuous" migration option for tests
On 05/04/2024 10.35, Nicholas Piggin wrote: The cooperative migration protocol is very good to control precise pre and post conditions for a migration event. However in some cases its intrusiveness to the test program, can mask problems and make analysis more difficult. For example to stress test migration vs concurrent complicated memory access, including TLB refill, ram dirtying, etc., then the tight spin at getchar() and resumption of the workload after migration is unhelpful. This adds a continuous migration mode that directs the harness to perform migrations continually. This is added to the migration selftests, which also sees cooperative migration iterations reduced to avoid increasing test time too much. Signed-off-by: Nicholas Piggin --- common/selftest-migration.c | 16 +-- lib/migrate.c | 18 lib/migrate.h | 3 ++ scripts/arch-run.bash | 55 - 4 files changed, 82 insertions(+), 10 deletions(-) Reviewed-by: Thomas Huth
Re: [kvm-unit-tests PATCH v8 03/35] migration: Add a migrate_skip command
On 16/04/2024 05.22, Nicholas Piggin wrote: On Tue Apr 9, 2024 at 1:59 AM AEST, Nico Boehr wrote: Quoting Nicholas Piggin (2024-04-05 10:35:04) [...] diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash index 39419d4e2..4a1aab48d 100644 --- a/scripts/arch-run.bash +++ b/scripts/arch-run.bash [...] @@ -179,8 +189,11 @@ run_migration () # Wait for test exit or further migration messages. if ! seen_migrate_msg ${src_out} ; then sleep 0.1 - else + elif grep -q "Now migrate the VM" < ${src_out} ; then do_migration || return $? + elif [ $skip_migration -eq 0 ] && grep -q "Skipped VM migration" < ${src_out} ; then + echo > ${src_infifo} # Resume src and carry on. + break; If I understand the code correctly, this simply makes the test PASS when migration is skipped, am I wrong? This just gets the harness past the wait-for-migration phase, it otherwise should not change behaviour. If so, can we set ret=77 here so we get a nice SKIP? The harness _should_ still scan the status value printed by the test case when it exits. Is it not working as expected? We certainly should be able to make it SKIP. I just gave it a try (by modifying the selftest-migration-skip test accordingly), and it seems to work fine, so I think this patch here should be ok. Thomas
Re: [kvm-unit-tests PATCH v8 05/35] arch-run: Add a "continuous" migration option for tests
On 05/04/2024 10.35, Nicholas Piggin wrote: The cooperative migration protocol is very good to control precise pre and post conditions for a migration event. However in some cases its intrusiveness to the test program, can mask problems and make analysis more difficult. For example to stress test migration vs concurrent complicated memory access, including TLB refill, ram dirtying, etc., then the tight spin at getchar() and resumption of the workload after migration is unhelpful. This adds a continuous migration mode that directs the harness to perform migrations continually. This is added to the migration selftests, which also sees cooperative migration iterations reduced to avoid increasing test time too much. Signed-off-by: Nicholas Piggin --- common/selftest-migration.c | 16 +-- lib/migrate.c | 18 lib/migrate.h | 3 ++ scripts/arch-run.bash | 55 - 4 files changed, 82 insertions(+), 10 deletions(-) Reviewed-by: Thomas Huth
Re: [kvm-unit-tests PATCH v8 06/35] gitlab-ci: Run migration selftest on s390x and powerpc
On 08/04/2024 18.06, Nico Boehr wrote: Quoting Nicholas Piggin (2024-04-05 10:35:07) The migration harness is complicated and easy to break so CI will be helpful. Signed-off-by: Nicholas Piggin --- .gitlab-ci.yml | 32 +++- s390x/unittests.cfg | 8 2 files changed, 31 insertions(+), 9 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index ff34b1f50..60b3cdfd2 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml [...] @@ -135,7 +147,7 @@ build-riscv64: build-s390x: extends: .outoftree_template script: - - dnf install -y qemu-system-s390x gcc-s390x-linux-gnu + - dnf install -y qemu-system-s390x gcc-s390x-linux-gnu nmap-ncat - mkdir build - cd build - ../configure --arch=s390x --cross-prefix=s390x-linux-gnu- @@ -161,6 +173,8 @@ build-s390x: sclp-1g sclp-3g selftest-setup + selftest-migration-kvm We're running under TCG in the Gitlab CI. I'm a little bit confused why we're running a KVM-only test here. The build-s390x job is TCG, indeed, but we have the "s390x-kvm" job that runs on a KVM-capable s390x host, so it could be added there? Thomas
Re: [kvm-unit-tests PATCH v7 07/35] common: add memory dirtying vs migration test
On 19/03/2024 08.58, Nicholas Piggin wrote: This test stores to a bunch of pages and verifies previous stores, while being continually migrated. Default runtime is 5 seconds. Add this test to ppc64 and s390x builds. This can fail due to a QEMU TCG physical memory dirty bitmap bug, so it is not enabled in unittests for TCG yet. The selftest-migration test time is reduced significantly because this test Signed-off-by: Nicholas Piggin --- common/memory-verify.c | 67 + common/selftest-migration.c | 8 ++--- powerpc/Makefile.common | 1 + powerpc/memory-verify.c | 1 + powerpc/unittests.cfg | 7 s390x/Makefile | 1 + s390x/memory-verify.c | 1 + s390x/unittests.cfg | 6 8 files changed, 88 insertions(+), 4 deletions(-) create mode 100644 common/memory-verify.c create mode 12 powerpc/memory-verify.c create mode 12 s390x/memory-verify.c diff --git a/common/memory-verify.c b/common/memory-verify.c new file mode 100644 index 0..e78fb4338 --- /dev/null +++ b/common/memory-verify.c @@ -0,0 +1,67 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Simple memory verification test, used to exercise dirty memory migration. + */ +#include +#include +#include +#include +#include + +#define NR_PAGES 32 + +static unsigned time_sec = 5; + +static void do_getopts(int argc, char **argv) +{ + int i; + + for (i = 0; i < argc; ++i) { + if (strcmp(argv[i], "-t") == 0) { + i++; + if (i == argc) + break; + time_sec = atol(argv[i]); + } + } + + printf("running for %d secs\n", time_sec); +} + +int main(int argc, char **argv) +{ + void *mem = malloc(NR_PAGES*PAGE_SIZE); Use alloc_pages(5) instead ? Or add at least some white spaces around "*". Apart from that this patch looks sane to me, so with that line fixed: Reviewed-by: Thomas Huth
Re: [kvm-unit-tests PATCH v7 06/35] gitlab-ci: Run migration selftest on s390x and powerpc
On 19/03/2024 08.58, Nicholas Piggin wrote: The migration harness is complicated and easy to break so CI will be helpful. Signed-off-by: Nicholas Piggin --- .gitlab-ci.yml | 18 +++--- s390x/unittests.cfg | 8 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index ff34b1f50..bd34da04f 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -92,26 +92,28 @@ build-arm: build-ppc64be: extends: .outoftree_template script: - - dnf install -y qemu-system-ppc gcc-powerpc64-linux-gnu + - dnf install -y qemu-system-ppc gcc-powerpc64-linux-gnu nmap-ncat - mkdir build - cd build - ../configure --arch=ppc64 --endian=big --cross-prefix=powerpc64-linux-gnu- - make -j2 - ACCEL=tcg ./run_tests.sh - selftest-setup spapr_hcall rtas-get-time-of-day rtas-get-time-of-day-base - rtas-set-time-of-day emulator + selftest-setup selftest-migration selftest-migration-skip spapr_hcall + rtas-get-time-of-day rtas-get-time-of-day-base rtas-set-time-of-day I used to squash as much as possible into one line in the past, but nowadays I rather prefer one test per line (like it is done for s390x below), so that it is easier to identify the changes ... So if you like, I think you could also put each test on a separate line here now (since you're touching all lines with tests here anyway). + emulator | tee results.txt - if grep -q FAIL results.txt ; then exit 1 ; fi build-ppc64le: extends: .intree_template script: - - dnf install -y qemu-system-ppc gcc-powerpc64-linux-gnu + - dnf install -y qemu-system-ppc gcc-powerpc64-linux-gnu nmap-ncat - ./configure --arch=ppc64 --endian=little --cross-prefix=powerpc64-linux-gnu- - make -j2 - ACCEL=tcg ./run_tests.sh - selftest-setup spapr_hcall rtas-get-time-of-day rtas-get-time-of-day-base - rtas-set-time-of-day emulator + selftest-setup selftest-migration selftest-migration-skip spapr_hcall + rtas-get-time-of-day rtas-get-time-of-day-base rtas-set-time-of-day + emulator | tee results.txt - if grep -q FAIL results.txt ; then exit 1 ; fi @@ -135,7 +137,7 @@ build-riscv64: build-s390x: extends: .outoftree_template script: - - dnf install -y qemu-system-s390x gcc-s390x-linux-gnu + - dnf install -y qemu-system-s390x gcc-s390x-linux-gnu nmap-ncat - mkdir build - cd build - ../configure --arch=s390x --cross-prefix=s390x-linux-gnu- @@ -161,6 +163,8 @@ build-s390x: sclp-1g sclp-3g selftest-setup + selftest-migration-kvm + selftest-migration-skip sieve smp stsi diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg index 49e3e4608..b79b99416 100644 --- a/s390x/unittests.cfg +++ b/s390x/unittests.cfg @@ -31,6 +31,14 @@ groups = selftest migration # https://lore.kernel.org/qemu-devel/20240219061731.232570-1-npig...@gmail.com/ accel = kvm +[selftest-migration-kvm] +file = selftest-migration.elf +groups = nodefault +accel = kvm +# This is a special test for gitlab-ci that can must not use TCG until the "can" or "must"? +# TCG migration fix has made its way into CI environment's QEMU. +# https://lore.kernel.org/qemu-devel/20240219061731.232570-1-npig...@gmail.com/ + [selftest-migration-skip] file = selftest-migration.elf groups = selftest migration Thomas
Re: [kvm-unit-tests PATCH v7 01/35] arch-run: Add functions to help handle migration directives from test
On 19/03/2024 08.58, Nicholas Piggin wrote: The migration harness will be expanded to deal with more commands from the test, moving these checks into functions helps keep things managable. Signed-off-by: Nicholas Piggin --- scripts/arch-run.bash | 20 +++- 1 file changed, 15 insertions(+), 5 deletions(-) Reviewed-by: Thomas Huth
Re: [kvm-unit-tests PATCH 03/32] powerpc: Fix stack backtrace termination
On 05/03/2024 07.29, Nicholas Piggin wrote: On Tue Feb 27, 2024 at 6:50 PM AEST, Thomas Huth wrote: On 26/02/2024 11.11, Nicholas Piggin wrote: ... /* save DTB pointer */ - std r3, 56(r1) + SAVE_GPR(3,r1) Isn't SAVE_GPR rather meant for the interrupt frame, not for the normal C calling convention frames? Sorry for asking dumb questions ... I still have a hard time understanding the changes here... :-/ Ah, that was me being lazy and using an interrupt frame for the new frame. Ah, ok. It's super-confusing (at least for me) to see an interrupt frame here out of no reason... could you please either add proper comments here explaining this, or even better switch to a normal stack frame, please? Thanks, Thomas
Re: [kvm-unit-tests PATCH 7/7] common: add memory dirtying vs migration test
On 05/03/2024 03.50, Nicholas Piggin wrote: On Mon Mar 4, 2024 at 4:22 PM AEST, Thomas Huth wrote: On 26/02/2024 10.38, Nicholas Piggin wrote: This test stores to a bunch of pages and verifies previous stores, while being continually migrated. This can fail due to a QEMU TCG physical memory dirty bitmap bug. Good idea, but could we then please drop "continuous" test from selftest-migration.c again? ... having two common tests to exercise the continuous migration that take quite a bunch of seconds to finish sounds like a waste of time in the long run to me. Yeah if you like. I could shorten them up a bit. I did want to have the selftests for just purely testing the harness with as little "test" code as possible. Ok, but then please shorten the selftest to ~ 2 seconds if possible, please. Thomas
Re: [kvm-unit-tests PATCH 6/7] gitlab-ci: Run migration selftest on s390x and powerpc
On 05/03/2024 03.38, Nicholas Piggin wrote: On Sat Mar 2, 2024 at 12:16 AM AEST, Thomas Huth wrote: On 26/02/2024 10.38, Nicholas Piggin wrote: The migration harness is complicated and easy to break so CI will be helpful. Signed-off-by: Nicholas Piggin --- .gitlab-ci.yml | 18 +++--- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 71d986e98..61f196d5d 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -64,26 +64,28 @@ build-arm: build-ppc64be: extends: .outoftree_template script: - - dnf install -y qemu-system-ppc gcc-powerpc64-linux-gnu + - dnf install -y qemu-system-ppc gcc-powerpc64-linux-gnu nmap-ncat - mkdir build - cd build - ../configure --arch=ppc64 --endian=big --cross-prefix=powerpc64-linux-gnu- - make -j2 - ACCEL=tcg ./run_tests.sh - selftest-setup spapr_hcall rtas-get-time-of-day rtas-get-time-of-day-base - rtas-set-time-of-day emulator + selftest-setup selftest-migration selftest-migration-skip spapr_hcall + rtas-get-time-of-day rtas-get-time-of-day-base rtas-set-time-of-day + emulator | tee results.txt - if grep -q FAIL results.txt ; then exit 1 ; fi build-ppc64le: extends: .intree_template script: - - dnf install -y qemu-system-ppc gcc-powerpc64-linux-gnu + - dnf install -y qemu-system-ppc gcc-powerpc64-linux-gnu nmap-ncat - ./configure --arch=ppc64 --endian=little --cross-prefix=powerpc64-linux-gnu- - make -j2 - ACCEL=tcg ./run_tests.sh - selftest-setup spapr_hcall rtas-get-time-of-day rtas-get-time-of-day-base - rtas-set-time-of-day emulator + selftest-setup selftest-migration selftest-migration-skip spapr_hcall + rtas-get-time-of-day rtas-get-time-of-day-base rtas-set-time-of-day + emulator | tee results.txt - if grep -q FAIL results.txt ; then exit 1 ; fi @@ -107,7 +109,7 @@ build-riscv64: build-s390x: extends: .outoftree_template script: - - dnf install -y qemu-system-s390x gcc-s390x-linux-gnu + - dnf install -y qemu-system-s390x gcc-s390x-linux-gnu nmap-ncat - mkdir build - cd build - ../configure --arch=s390x --cross-prefix=s390x-linux-gnu- @@ -133,6 +135,8 @@ build-s390x: sclp-1g sclp-3g selftest-setup + selftest-migration + selftest-migration-skip sieve smp stsi While I can update the qemu binary for the s390x-kvm job, the build-* jobs run in a container with a normal QEMU from the corresponding distros, so I think this has to wait 'til we get distros that contain your QEMU TCG migration fix. Okay. powerpc *could* run into the TCG bug too, in practice it has not. We could try enable it there to get migration into CI, and revert it if it starts showing random failures? Fine for me. Thomas
Re: [kvm-unit-tests PATCH 14/32] powerpc: general interrupt tests
On 05/03/2024 03.19, Nicholas Piggin wrote: On Fri Mar 1, 2024 at 10:41 PM AEST, Thomas Huth wrote: On 26/02/2024 11.12, Nicholas Piggin wrote: Add basic testing of various kinds of interrupts, machine check, page fault, illegal, decrementer, trace, syscall, etc. This has a known failure on QEMU TCG pseries machines where MSR[ME] can be incorrectly set to 0. Two questions out of curiosity: Any chance that this could be fixed easily in QEMU? Yes I have a fix on the mailing list. It should get into 9.0 and probably stable. Ok, then it's IMHO not worth the effort to make the k-u-t work around this bug in older QEMU versions. Or is there a way to detect TCG from within the test? (for example, we have a host_is_tcg() function for s390x so we can e.g. use report_xfail() for tests that are known to fail on TCG there) I do have a half-done patch which adds exactly this. One (minor) annoyance is that it doesn't seem possible to detect QEMU version to add workarounds. E.g., we would like to test the fixed functionality, but older qemu should not. Maybe that's going too much into a rabbit hole. We *could* put a QEMU version into device tree to deal with this though... No, let's better not do this - hardwired version checks are often a bad idea, e.g. when a bug is in QEMU 8.0.0 and 8.1.0, it could be fixed in 8.0.1 and then it could get really messy with the version checks... Thomas
Re: [kvm-unit-tests PATCH 14/32] powerpc: general interrupt tests
On 05/03/2024 03.30, Nicholas Piggin wrote: On Fri Mar 1, 2024 at 11:45 PM AEST, Andrew Jones wrote: On Fri, Mar 01, 2024 at 01:41:22PM +0100, Thomas Huth wrote: On 26/02/2024 11.12, Nicholas Piggin wrote: Add basic testing of various kinds of interrupts, machine check, page fault, illegal, decrementer, trace, syscall, etc. This has a known failure on QEMU TCG pseries machines where MSR[ME] can be incorrectly set to 0. Two questions out of curiosity: Any chance that this could be fixed easily in QEMU? Or is there a way to detect TCG from within the test? (for example, we have a host_is_tcg() function for s390x so we can e.g. use report_xfail() for tests that are known to fail on TCG there) If there's nothing better, then it should be possible to check the QEMU_ACCEL environment variable which will be there with the default environ. @@ -0,0 +1,415 @@ +/* + * Test interrupts + * + * Copyright 2024 Nicholas Piggin, IBM Corp. + * + * This work is licensed under the terms of the GNU LGPL, version 2. I know, we're using this line in a lot of source files ... but maybe we should do better for new files at least: "LGPL, version 2" is a little bit ambiguous: Does it mean the "Library GPL version 2.0" or the "Lesser GPL version 2.1"? Maybe you could clarify by additionally providing a SPDX identifier here, or by explicitly writing 2.0 or 2.1. Let's only add SPDX identifiers to new files. +1 Speaking of which, a bunch of these just got inherited from the file that was copied to begin with (I tried not to remove copyright notices unless there was really nothing of the original remaining). So for new code/files, is there any particular preference for the license to use? I don't much mind between the *GPL*. Looks like almost all the SPDX code use GPL 2.0 only, but that could be just from coming from Linux. I might just go with that. k-u-t were originally licensed under the LGPL, but in the course of time, code has been copied from the Linux kernel here and there, so we updated the license to GPL 2. So yes, for code that might have been copied from the kernel, we have to use GPL 2. For other code, it's up to the author to chose. (IIRC we once discussed that LGPL would be nice for the files in the lib/ directory, while GPL is fine for the other folders, but my memory might fail here and it was never written in stone anyway) Thomas
Re: [kvm-unit-tests PATCH 7/7] common: add memory dirtying vs migration test
On 26/02/2024 10.38, Nicholas Piggin wrote: This test stores to a bunch of pages and verifies previous stores, while being continually migrated. This can fail due to a QEMU TCG physical memory dirty bitmap bug. Good idea, but could we then please drop "continuous" test from selftest-migration.c again? ... having two common tests to exercise the continuous migration that take quite a bunch of seconds to finish sounds like a waste of time in the long run to me. Signed-off-by: Nicholas Piggin --- common/memory-verify.c | 48 + powerpc/Makefile.common | 1 + powerpc/memory-verify.c | 1 + powerpc/unittests.cfg | 7 ++ s390x/Makefile | 1 + s390x/memory-verify.c | 1 + s390x/unittests.cfg | 6 ++ 7 files changed, 65 insertions(+) create mode 100644 common/memory-verify.c create mode 12 powerpc/memory-verify.c create mode 12 s390x/memory-verify.c diff --git a/common/memory-verify.c b/common/memory-verify.c new file mode 100644 index 0..7c4ec087b --- /dev/null +++ b/common/memory-verify.c @@ -0,0 +1,48 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Simple memory verification test, used to exercise dirty memory migration. + * + */ +#include +#include +#include +#include +#include + +#define NR_PAGES 32 + +int main(int argc, char **argv) +{ + void *mem = malloc(NR_PAGES*PAGE_SIZE); + bool success = true; + uint64_t ms; + long i; + + report_prefix_push("memory"); + + memset(mem, 0, NR_PAGES*PAGE_SIZE); + + migrate_begin_continuous(); + ms = get_clock_ms(); + i = 0; + do { + int j; + + for (j = 0; j < NR_PAGES*PAGE_SIZE; j += PAGE_SIZE) { + if (*(volatile long *)(mem + j) != i) { + success = false; + goto out; + } + *(volatile long *)(mem + j) = i + 1; + } + i++; + } while (get_clock_ms() - ms < 5000); Maybe add a parameter so that the user can use different values for the runtime than always doing 5 seconds? Thomas +out: + migrate_end_continuous(); + + report(success, "memory verification stress test"); + + report_prefix_pop(); + + return report_summary(); +}
Re: [kvm-unit-tests PATCH 5/7] arch-run: Add a "continuous" migration option for tests
On 26/02/2024 10.38, Nicholas Piggin wrote: The cooperative migration protocol is very good to control precise pre and post conditions for a migration event. However in some cases its intrusiveness to the test program, can mask problems and make analysis more difficult. For example to stress test migration vs concurrent complicated memory access, including TLB refill, ram dirtying, etc., then the tight spin at getchar() and resumption of the workload after migration is unhelpful. This adds a continuous migration mode that directs the harness to perform migrations continually. This is added to the migration selftests, which also sees cooperative migration iterations reduced to avoid increasing test time too much. Signed-off-by: Nicholas Piggin --- common/selftest-migration.c | 16 +-- lib/migrate.c | 18 lib/migrate.h | 3 ++ scripts/arch-run.bash | 55 - 4 files changed, 82 insertions(+), 10 deletions(-) diff --git a/common/selftest-migration.c b/common/selftest-migration.c index 0afd8581c..9a9b61835 100644 --- a/common/selftest-migration.c +++ b/common/selftest-migration.c @@ -9,12 +9,13 @@ */ #include #include +#include -#define NR_MIGRATIONS 30 +#define NR_MIGRATIONS 15 int main(int argc, char **argv) { - report_prefix_push("migration"); + report_prefix_push("migration harness"); if (argc > 1 && !strcmp(argv[1], "skip")) { migrate_skip(); @@ -24,7 +25,16 @@ int main(int argc, char **argv) for (i = 0; i < NR_MIGRATIONS; i++) migrate_quiet(); - report(true, "simple harness stress"); + report(true, "cooperative migration"); + + migrate_begin_continuous(); + mdelay(2000); + migrate_end_continuous(); + mdelay(1000); + migrate_begin_continuous(); + mdelay(2000); + migrate_end_continuous(); + report(true, "continuous migration"); } report_prefix_pop(); diff --git a/lib/migrate.c b/lib/migrate.c index 1d22196b7..770f76d5c 100644 --- a/lib/migrate.c +++ b/lib/migrate.c @@ -60,3 +60,21 @@ void migrate_skip(void) puts("Skipped VM migration (quiet)\n"); (void)getchar(); } + +void migrate_begin_continuous(void) +{ + puts("Begin continuous migration\n"); + (void)getchar(); +} + +void migrate_end_continuous(void) +{ + /* +* Migration can split this output between source and dest QEMU +* output files, print twice and match once to always cope with +* a split. +*/ + puts("End continuous migration\n"); + puts("End continuous migration (quiet)\n"); + (void)getchar(); +} diff --git a/lib/migrate.h b/lib/migrate.h index db6e0c501..35b6703a2 100644 --- a/lib/migrate.h +++ b/lib/migrate.h @@ -11,3 +11,6 @@ void migrate_quiet(void); void migrate_once(void); void migrate_skip(void); + +void migrate_begin_continuous(void); +void migrate_end_continuous(void); diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash index d0f6f098f..5c7e72036 100644 --- a/scripts/arch-run.bash +++ b/scripts/arch-run.bash @@ -125,15 +125,17 @@ qmp_events () filter_quiet_msgs () { grep -v "Now migrate the VM (quiet)" | + grep -v "Begin continuous migration (quiet)" | + grep -v "End continuous migration (quiet)" | grep -v "Skipped VM migration (quiet)" } seen_migrate_msg () { if [ $skip_migration -eq 1 ]; then - grep -q -e "Now migrate the VM" < $1 + grep -q -e "Now migrate the VM" -e "Begin continuous migration" < $1 else - grep -q -e "Now migrate the VM" -e "Skipped VM migration" < $1 + grep -q -e "Now migrate the VM" -e "Begin continuous migration" -e "Skipped VM migration" < $1 fi } @@ -161,6 +163,7 @@ run_migration () src_qmpout=/dev/null dst_qmpout=/dev/null skip_migration=0 + continuous_migration=0 mkfifo ${src_outfifo} mkfifo ${dst_outfifo} @@ -186,9 +189,12 @@ run_migration () do_migration || return $? while ps -p ${live_pid} > /dev/null ; do - # Wait for test exit or further migration messages. - if ! seen_migrate_msg ${src_out} ; then + if [[ ${continuous_migration} -eq 1 ]] ; then Here you're using "[[" for testing ... + do_migration || return $? + elif ! seen_migrate_msg ${src_out} ; then sleep 0.1 + elif grep -q "Begin continuous migration" < ${src_out} ; then + do_migration || return $? elif grep -q "Now migrate the VM" < ${src_out} ; then do_migration || return $? elif [ $skip_migration -eq 0 ] && grep -q "Skipped V
Re: [kvm-unit-tests PATCH 2/7] migration: Add a migrate_skip command
On 26/02/2024 10.38, Nicholas Piggin wrote: Tests that are run with MIGRATION=yes but skip due to some requirement not being met will show as a failure due to the harness requirement to see one successful migration. The workaround for this is to migrate in test's skip path. Add a new command that just tells the harness to not expect a migration. Signed-off-by: Nicholas Piggin --- common/selftest-migration.c | 14 - lib/migrate.c | 19 - lib/migrate.h | 2 ++ powerpc/unittests.cfg | 6 ++ s390x/unittests.cfg | 5 + scripts/arch-run.bash | 41 + 6 files changed, 73 insertions(+), 14 deletions(-) Reviewed-by: Thomas Huth
Re: [kvm-unit-tests PATCH 6/7] gitlab-ci: Run migration selftest on s390x and powerpc
On 26/02/2024 10.38, Nicholas Piggin wrote: The migration harness is complicated and easy to break so CI will be helpful. Signed-off-by: Nicholas Piggin --- .gitlab-ci.yml | 18 +++--- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 71d986e98..61f196d5d 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -64,26 +64,28 @@ build-arm: build-ppc64be: extends: .outoftree_template script: - - dnf install -y qemu-system-ppc gcc-powerpc64-linux-gnu + - dnf install -y qemu-system-ppc gcc-powerpc64-linux-gnu nmap-ncat - mkdir build - cd build - ../configure --arch=ppc64 --endian=big --cross-prefix=powerpc64-linux-gnu- - make -j2 - ACCEL=tcg ./run_tests.sh - selftest-setup spapr_hcall rtas-get-time-of-day rtas-get-time-of-day-base - rtas-set-time-of-day emulator + selftest-setup selftest-migration selftest-migration-skip spapr_hcall + rtas-get-time-of-day rtas-get-time-of-day-base rtas-set-time-of-day + emulator | tee results.txt - if grep -q FAIL results.txt ; then exit 1 ; fi build-ppc64le: extends: .intree_template script: - - dnf install -y qemu-system-ppc gcc-powerpc64-linux-gnu + - dnf install -y qemu-system-ppc gcc-powerpc64-linux-gnu nmap-ncat - ./configure --arch=ppc64 --endian=little --cross-prefix=powerpc64-linux-gnu- - make -j2 - ACCEL=tcg ./run_tests.sh - selftest-setup spapr_hcall rtas-get-time-of-day rtas-get-time-of-day-base - rtas-set-time-of-day emulator + selftest-setup selftest-migration selftest-migration-skip spapr_hcall + rtas-get-time-of-day rtas-get-time-of-day-base rtas-set-time-of-day + emulator | tee results.txt - if grep -q FAIL results.txt ; then exit 1 ; fi @@ -107,7 +109,7 @@ build-riscv64: build-s390x: extends: .outoftree_template script: - - dnf install -y qemu-system-s390x gcc-s390x-linux-gnu + - dnf install -y qemu-system-s390x gcc-s390x-linux-gnu nmap-ncat - mkdir build - cd build - ../configure --arch=s390x --cross-prefix=s390x-linux-gnu- @@ -133,6 +135,8 @@ build-s390x: sclp-1g sclp-3g selftest-setup + selftest-migration + selftest-migration-skip sieve smp stsi While I can update the qemu binary for the s390x-kvm job, the build-* jobs run in a container with a normal QEMU from the corresponding distros, so I think this has to wait 'til we get distros that contain your QEMU TCG migration fix. Thomas
Re: [kvm-unit-tests PATCH 4/7] powerpc: add asm/time.h header with delay and get_clock_us/ms
On 26/02/2024 10.38, Nicholas Piggin wrote: This matches s390x clock and delay APIs, so common test code can start using time facilities. Signed-off-by: Nicholas Piggin --- lib/powerpc/asm/processor.h | 21 - lib/powerpc/asm/time.h | 30 ++ lib/powerpc/processor.c | 11 +++ lib/powerpc/smp.c | 1 + lib/ppc64/asm/time.h| 1 + powerpc/spapr_vpa.c | 1 + powerpc/sprs.c | 1 + powerpc/tm.c| 1 + 8 files changed, 46 insertions(+), 21 deletions(-) create mode 100644 lib/powerpc/asm/time.h create mode 100644 lib/ppc64/asm/time.h Reviewed-by: Thomas Huth
Re: [kvm-unit-tests PATCH 14/32] powerpc: general interrupt tests
On 01/03/2024 14.45, Andrew Jones wrote: On Fri, Mar 01, 2024 at 01:41:22PM +0100, Thomas Huth wrote: On 26/02/2024 11.12, Nicholas Piggin wrote: Add basic testing of various kinds of interrupts, machine check, page fault, illegal, decrementer, trace, syscall, etc. This has a known failure on QEMU TCG pseries machines where MSR[ME] can be incorrectly set to 0. Two questions out of curiosity: Any chance that this could be fixed easily in QEMU? Or is there a way to detect TCG from within the test? (for example, we have a host_is_tcg() function for s390x so we can e.g. use report_xfail() for tests that are known to fail on TCG there) If there's nothing better, then it should be possible to check the QEMU_ACCEL environment variable which will be there with the default environ. Well, but that's only available from the host side, not within the test (i.e. the guest). So that does not help much with report_xfail... I was rather thinking of something like checking the device tree, e.g. for the compatible property in /hypervisor to see whether it's KVM or TCG...? Thomas
Re: [kvm-unit-tests PATCH 3/7] (arm|s390): Use migrate_skip in test cases
On 26/02/2024 10.38, Nicholas Piggin wrote: Have tests use the new migrate_skip command in skip paths, rather than calling migrate_once to prevent harness reporting an error. s390x/migration.c adds a new command that looks like it was missing previously. Signed-off-by: Nicholas Piggin --- arm/gic.c | 21 - s390x/migration-cmm.c | 8 s390x/migration-skey.c | 4 +++- s390x/migration.c | 1 + 4 files changed, 20 insertions(+), 14 deletions(-) Reviewed-by: Thomas Huth
Re: [kvm-unit-tests PATCH 1/7] arch-run: Keep infifo open
On 26/02/2024 10.38, Nicholas Piggin wrote: The infifo fifo that is used to send characters to QEMU console is only able to receive one character before the cat process exits. Supporting interactions between test and harness involving multiple characters requires the fifo to remain open. This also allows us to let the cat out of the bag, simplifying the input pipeline. LOL, we rather let the cat out of the subshell now, but I like the play on words :-) Signed-off-by: Nicholas Piggin --- scripts/arch-run.bash | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash index 6daef3218..e5b36a07b 100644 --- a/scripts/arch-run.bash +++ b/scripts/arch-run.bash @@ -158,6 +158,11 @@ run_migration () mkfifo ${src_outfifo} mkfifo ${dst_outfifo} + # Holding both ends of the input fifo open prevents opens from + # blocking and readers getting EOF when a writer closes it. + mkfifo ${dst_infifo} + exec {dst_infifo_fd}<>${dst_infifo} + eval "$migcmdline" \ -chardev socket,id=mon,path=${src_qmp},server=on,wait=off \ -mon chardev=mon,mode=control > ${src_outfifo} & @@ -191,14 +196,10 @@ run_migration () do_migration () { - # We have to use cat to open the named FIFO, because named FIFO's, - # unlike pipes, will block on open() until the other end is also - # opened, and that totally breaks QEMU... - mkfifo ${dst_infifo} eval "$migcmdline" \ -chardev socket,id=mon,path=${dst_qmp},server=on,wait=off \ -mon chardev=mon,mode=control -incoming unix:${dst_incoming} \ - < <(cat ${dst_infifo}) > ${dst_outfifo} & + < ${dst_infifo} > ${dst_outfifo} & incoming_pid=$! cat ${dst_outfifo} | tee ${dst_out} | filter_quiet_msgs & @@ -245,7 +246,6 @@ do_migration () # keypress to dst so getchar completes and test continues echo > ${dst_infifo} - rm ${dst_infifo} I assume it will not get deleted by the trap handler? ... sounds fine to me, so I dare to say: Reviewed-by: Thomas Huth
Re: [kvm-unit-tests PATCH 14/32] powerpc: general interrupt tests
On 26/02/2024 11.12, Nicholas Piggin wrote: Add basic testing of various kinds of interrupts, machine check, page fault, illegal, decrementer, trace, syscall, etc. This has a known failure on QEMU TCG pseries machines where MSR[ME] can be incorrectly set to 0. Two questions out of curiosity: Any chance that this could be fixed easily in QEMU? Or is there a way to detect TCG from within the test? (for example, we have a host_is_tcg() function for s390x so we can e.g. use report_xfail() for tests that are known to fail on TCG there) Signed-off-by: Nicholas Piggin --- lib/powerpc/asm/processor.h | 4 + lib/powerpc/asm/reg.h | 17 ++ lib/powerpc/setup.c | 11 + lib/ppc64/asm/ptrace.h | 16 ++ powerpc/Makefile.common | 3 +- powerpc/interrupts.c| 415 powerpc/unittests.cfg | 3 + 7 files changed, 468 insertions(+), 1 deletion(-) create mode 100644 powerpc/interrupts.c diff --git a/lib/powerpc/asm/processor.h b/lib/powerpc/asm/processor.h index cf1b9d8ff..eed37d1f4 100644 --- a/lib/powerpc/asm/processor.h +++ b/lib/powerpc/asm/processor.h @@ -11,7 +11,11 @@ void do_handle_exception(struct pt_regs *regs); #endif /* __ASSEMBLY__ */ extern bool cpu_has_hv; +extern bool cpu_has_power_mce; +extern bool cpu_has_siar; extern bool cpu_has_heai; +extern bool cpu_has_prefix; +extern bool cpu_has_sc_lev; static inline uint64_t mfspr(int nr) { diff --git a/lib/powerpc/asm/reg.h b/lib/powerpc/asm/reg.h index 782e75527..d6097f48f 100644 --- a/lib/powerpc/asm/reg.h +++ b/lib/powerpc/asm/reg.h @@ -5,8 +5,15 @@ #define UL(x) _AC(x, UL) +#define SPR_DSISR 0x012 +#define SPR_DAR0x013 +#define SPR_DEC0x016 #define SPR_SRR0 0x01a #define SPR_SRR1 0x01b +#define SRR1_PREFIX UL(0x2000) +#define SPR_FSCR 0x099 +#define FSCR_PREFIX UL(0x2000) +#define SPR_HFSCR 0x0be #define SPR_TB0x10c #define SPR_SPRG0 0x110 #define SPR_SPRG1 0x111 @@ -22,12 +29,17 @@ #define PVR_VER_POWER8 UL(0x004d) #define PVR_VER_POWER9 UL(0x004e) #define PVR_VER_POWER10 UL(0x0080) +#define SPR_HDEC 0x136 #define SPR_HSRR0 0x13a #define SPR_HSRR1 0x13b +#define SPR_LPCR 0x13e +#define LPCR_HDICE UL(0x1) +#define SPR_HEIR 0x153 #define SPR_MMCR0 0x31b #define MMCR0_FCUL(0x8000) #define MMCR0_PMAE UL(0x0400) #define MMCR0_PMAO UL(0x0080) +#define SPR_SIAR 0x31c /* Machine State Register definitions: */ #define MSR_LE_BIT0 @@ -35,6 +47,11 @@ #define MSR_HV_BIT60 /* Hypervisor mode */ #define MSR_SF_BIT63 /* 64-bit mode */ +#define MSR_DR UL(0x0010) +#define MSR_IR UL(0x0020) +#define MSR_BE UL(0x0200) /* Branch Trace Enable */ +#define MSR_SE UL(0x0400) /* Single Step Enable */ +#define MSR_EE UL(0x8000) #define MSR_MEUL(0x1000) #endif diff --git a/lib/powerpc/setup.c b/lib/powerpc/setup.c index 3c81aee9e..9b665f59c 100644 --- a/lib/powerpc/setup.c +++ b/lib/powerpc/setup.c @@ -87,7 +87,11 @@ static void cpu_set(int fdtnode, u64 regval, void *info) } bool cpu_has_hv; +bool cpu_has_power_mce; /* POWER CPU machine checks */ +bool cpu_has_siar; bool cpu_has_heai; +bool cpu_has_prefix; +bool cpu_has_sc_lev; /* sc interrupt has LEV field in SRR1 */ static void cpu_init(void) { @@ -112,15 +116,22 @@ static void cpu_init(void) switch (mfspr(SPR_PVR) & PVR_VERSION_MASK) { case PVR_VER_POWER10: + cpu_has_prefix = true; + cpu_has_sc_lev = true; case PVR_VER_POWER9: case PVR_VER_POWER8E: case PVR_VER_POWER8NVL: case PVR_VER_POWER8: + cpu_has_power_mce = true; cpu_has_heai = true; + cpu_has_siar = true; break; default: break; } + + if (!cpu_has_hv) /* HEIR is HV register */ + cpu_has_heai = false; } static void mem_init(phys_addr_t freemem_start) diff --git a/lib/ppc64/asm/ptrace.h b/lib/ppc64/asm/ptrace.h index 12de7499b..db263a59e 100644 --- a/lib/ppc64/asm/ptrace.h +++ b/lib/ppc64/asm/ptrace.h @@ -5,6 +5,9 @@ #define STACK_FRAME_OVERHEAD112 /* size of minimum stack frame */ #ifndef __ASSEMBLY__ + +#include + struct pt_regs { unsigned long gpr[32]; unsigned long nip; @@ -17,6 +20,19 @@ struct pt_regs { unsigned long _pad; /* stack must be 16-byte aligned */ }; +static inline bool regs_is_prefix(volatile struct pt_regs *regs) +{ + return regs->msr & SRR1_PREFIX; +} + +static inline void regs_advance_insn(struct pt_regs *regs) +{ + if (regs_is_prefix(regs)) +
Re: [kvm-unit-tests PATCH 12/32] powerpc: Fix emulator illegal instruction test for powernv
On 26/02/2024 11.11, Nicholas Piggin wrote: Illegal instructions cause 0xe40 (HEAI) interrupts rather than program interrupts. Acked-by: Thomas Huth Signed-off-by: Nicholas Piggin --- lib/powerpc/asm/processor.h | 1 + lib/powerpc/setup.c | 13 + powerpc/emulator.c | 21 - 3 files changed, 34 insertions(+), 1 deletion(-) diff --git a/lib/powerpc/asm/processor.h b/lib/powerpc/asm/processor.h index 9d8061962..cf1b9d8ff 100644 --- a/lib/powerpc/asm/processor.h +++ b/lib/powerpc/asm/processor.h @@ -11,6 +11,7 @@ void do_handle_exception(struct pt_regs *regs); #endif /* __ASSEMBLY__ */ extern bool cpu_has_hv; +extern bool cpu_has_heai; static inline uint64_t mfspr(int nr) { diff --git a/lib/powerpc/setup.c b/lib/powerpc/setup.c index 89e5157f2..3c81aee9e 100644 --- a/lib/powerpc/setup.c +++ b/lib/powerpc/setup.c @@ -87,6 +87,7 @@ static void cpu_set(int fdtnode, u64 regval, void *info) } bool cpu_has_hv; +bool cpu_has_heai; static void cpu_init(void) { @@ -108,6 +109,18 @@ static void cpu_init(void) hcall(H_SET_MODE, 0, 4, 0, 0); #endif } + + switch (mfspr(SPR_PVR) & PVR_VERSION_MASK) { + case PVR_VER_POWER10: + case PVR_VER_POWER9: + case PVR_VER_POWER8E: + case PVR_VER_POWER8NVL: + case PVR_VER_POWER8: + cpu_has_heai = true; + break; + default: + break; + } } static void mem_init(phys_addr_t freemem_start) diff --git a/powerpc/emulator.c b/powerpc/emulator.c index 39dd59645..c9b17f742 100644 --- a/powerpc/emulator.c +++ b/powerpc/emulator.c @@ -31,6 +31,20 @@ static void program_check_handler(struct pt_regs *regs, void *opaque) regs->nip += 4; } +static void heai_handler(struct pt_regs *regs, void *opaque) +{ + int *data = opaque; + + if (verbose) { + printf("Detected invalid instruction %#018lx: %08x\n", + regs->nip, *(uint32_t*)regs->nip); + } + + *data = 8; /* Illegal instruction */ + + regs->nip += 4; +} + static void alignment_handler(struct pt_regs *regs, void *opaque) { int *data = opaque; @@ -362,7 +376,12 @@ int main(int argc, char **argv) { int i; - handle_exception(0x700, program_check_handler, (void *)&is_invalid); + if (cpu_has_heai) { + handle_exception(0xe40, heai_handler, (void *)&is_invalid); + handle_exception(0x700, program_check_handler, (void *)&is_invalid); + } else { + handle_exception(0x700, program_check_handler, (void *)&is_invalid); The 0x700 line looks identical to the other part of the if-statement ... I'd suggest to leave it outside of the if-statement, drop the else-part and just set 0xe40 if cpu_has_heai. Thomas + } handle_exception(0x600, alignment_handler, (void *)&alignment); for (i = 1; i < argc; i++) {
Re: [kvm-unit-tests PATCH 08/32] powerpc/sprs: Avoid taking PMU interrupts caused by register fuzzing
On 26/02/2024 11.11, Nicholas Piggin wrote: Storing certain values in MMCR0 can cause PMU interrupts when msleep enables MSR[EE], and this crashes the test. Freeze the PMU counters and clear any PMU exception before calling msleep. Signed-off-by: Nicholas Piggin --- lib/powerpc/asm/reg.h | 4 powerpc/sprs.c| 17 +++-- 2 files changed, 15 insertions(+), 6 deletions(-) Reviewed-by: Thomas Huth
Re: [kvm-unit-tests PATCH 07/32] powerpc/sprs: Don't fail changed SPRs that are used by the test harness
On 26/02/2024 11.11, Nicholas Piggin wrote: SPRs annotated with SPR_HARNESS can change between consecutive reads because the test harness code has changed them. Avoid failing the test in this case. Signed-off-by: Nicholas Piggin --- powerpc/sprs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/powerpc/sprs.c b/powerpc/sprs.c index 8253ea971..44edd0d7b 100644 --- a/powerpc/sprs.c +++ b/powerpc/sprs.c @@ -563,7 +563,7 @@ int main(int argc, char **argv) if (before[i] >> 32) pass = false; } - if (!(sprs[i].type & SPR_ASYNC) && (before[i] != after[i])) + if (!(sprs[i].type & (SPR_HARNESS|SPR_ASYNC)) && (before[i] != after[i])) pass = false; if (sprs[i].width == 32 && !(before[i] >> 32) && !(after[i] >> 32)) I guess you could also squash this into the previous patch (to avoid problems with bisecting later?) ... Anyway: Reviewed-by: Thomas Huth
Re: [kvm-unit-tests PATCH 05/32] powerpc: Cleanup SPR and MSR definitions
On 26/02/2024 11.11, Nicholas Piggin wrote: Move SPR and MSR defines out of ppc_asm.h and processor.h and into a new include, asm/reg.h. Add a define for the PVR SPR and various processor versions, and replace the open coded numbers in the sprs.c test case. Signed-off-by: Nicholas Piggin --- lib/powerpc/asm/ppc_asm.h | 8 +--- lib/powerpc/asm/processor.h | 7 +-- lib/powerpc/asm/reg.h | 30 ++ lib/powerpc/asm/time.h | 1 + lib/ppc64/asm/reg.h | 1 + powerpc/sprs.c | 21 ++--- 6 files changed, 44 insertions(+), 24 deletions(-) create mode 100644 lib/powerpc/asm/reg.h create mode 100644 lib/ppc64/asm/reg.h Reviewed-by: Thomas Huth
Re: [kvm-unit-tests PATCH 04/32] powerpc: interrupt stack backtracing
On 26/02/2024 11.11, Nicholas Piggin wrote: Add support for backtracing across interrupt stacks, and add interrupt frame backtrace for unhandled interrupts. Signed-off-by: Nicholas Piggin --- lib/powerpc/processor.c | 4 ++- lib/ppc64/asm/stack.h | 3 +++ lib/ppc64/stack.c | 55 + powerpc/Makefile.ppc64 | 1 + powerpc/cstart64.S | 7 -- 5 files changed, 67 insertions(+), 3 deletions(-) create mode 100644 lib/ppc64/stack.c diff --git a/lib/powerpc/processor.c b/lib/powerpc/processor.c index ad0d95666..114584024 100644 --- a/lib/powerpc/processor.c +++ b/lib/powerpc/processor.c @@ -51,7 +51,9 @@ void do_handle_exception(struct pt_regs *regs) return; } - printf("unhandled cpu exception %#lx at NIA:0x%016lx MSR:0x%016lx\n", regs->trap, regs->nip, regs->msr); + printf("Unhandled cpu exception %#lx at NIA:0x%016lx MSR:0x%016lx\n", + regs->trap, regs->nip, regs->msr); + dump_frame_stack((void *)regs->nip, (void *)regs->gpr[1]); abort(); } diff --git a/lib/ppc64/asm/stack.h b/lib/ppc64/asm/stack.h index 9734bbb8f..94fd1021c 100644 --- a/lib/ppc64/asm/stack.h +++ b/lib/ppc64/asm/stack.h @@ -5,4 +5,7 @@ #error Do not directly include . Just use . #endif +#define HAVE_ARCH_BACKTRACE +#define HAVE_ARCH_BACKTRACE_FRAME + #endif diff --git a/lib/ppc64/stack.c b/lib/ppc64/stack.c new file mode 100644 index 0..fcb7fa860 --- /dev/null +++ b/lib/ppc64/stack.c @@ -0,0 +1,55 @@ +#include +#include +#include + +extern char exception_stack_marker[]; + +int backtrace_frame(const void *frame, const void **return_addrs, int max_depth) +{ + static int walking; + int depth = 0; + const unsigned long *bp = (unsigned long *)frame; + void *return_addr; + + asm volatile("" ::: "lr"); /* Force it to save LR */ + + if (walking) { + printf("RECURSIVE STACK WALK!!!\n"); + return 0; + } + walking = 1; + + bp = (unsigned long *)bp[0]; + return_addr = (void *)bp[2]; + + for (depth = 0; bp && depth < max_depth; depth++) { + return_addrs[depth] = return_addr; + if (return_addrs[depth] == 0) + break; + if (return_addrs[depth] == exception_stack_marker) { + struct pt_regs *regs; + + regs = (void *)bp + STACK_FRAME_OVERHEAD; + bp = (unsigned long *)bp[0]; + /* Represent interrupt frame with vector number */ + return_addr = (void *)regs->trap; + if (depth + 1 < max_depth) { + depth++; + return_addrs[depth] = return_addr; + return_addr = (void *)regs->nip; + } + } else { + bp = (unsigned long *)bp[0]; + return_addr = (void *)bp[2]; + } + } + + walking = 0; + return depth; +} + +int backtrace(const void **return_addrs, int max_depth) +{ + return backtrace_frame(__builtin_frame_address(0), return_addrs, + max_depth); +} diff --git a/powerpc/Makefile.ppc64 b/powerpc/Makefile.ppc64 index b0ed2b104..eb682c226 100644 --- a/powerpc/Makefile.ppc64 +++ b/powerpc/Makefile.ppc64 @@ -17,6 +17,7 @@ cstart.o = $(TEST_DIR)/cstart64.o reloc.o = $(TEST_DIR)/reloc64.o OBJDIRS += lib/ppc64 +cflatobjs += lib/ppc64/stack.o # ppc64 specific tests tests = $(TEST_DIR)/spapr_vpa.elf diff --git a/powerpc/cstart64.S b/powerpc/cstart64.S index 14ab0c6c8..278af84a6 100644 --- a/powerpc/cstart64.S +++ b/powerpc/cstart64.S @@ -188,6 +188,7 @@ call_handler: .endr mfsprg1 r0 std r0,GPR1(r1) + std r0,0(r1) /* lr, xer, ccr */ @@ -206,12 +207,12 @@ call_handler: subir31, r31, 0b - start_text ld r2, (p_toc_text - start_text)(r31) - /* FIXME: build stack frame */ - /* call generic handler */ addi r3,r1,STACK_FRAME_OVERHEAD bl do_handle_exception + .global exception_stack_marker +exception_stack_marker: /* restore context */ @@ -321,6 +322,7 @@ handler_trampoline: /* nip and msr */ mfsrr0 r0 std r0, _NIP(r1) + std r0, INT_FRAME_SIZE+16(r1) So if I got that right, INT_FRAME_SIZE+16 points to the stack frame of the function that was running before the interrupt handler? Is it such a good idea to change that here? Please add a comment here explaining this line. Thanks! mfsrr1 r0 std r0, _MSR(r1) @@ -337,6 +339,7 @@ handler_htrampoline: /* nip and msr */ mfspr r0, SPR_HSRR0 std r0, _NIP(r1) + std r0, INT_FRAME_SIZE+16(r1) dito. mfspr r0, SPR_HSRR1
Re: [kvm-unit-tests PATCH 03/32] powerpc: Fix stack backtrace termination
On 27/02/2024 09.50, Thomas Huth wrote: On 26/02/2024 11.11, Nicholas Piggin wrote: The backtrace handler terminates when it sees a NULL caller address, but the powerpc stack setup does not keep such a NULL caller frame at the start of the stack. This happens to work on pseries because the memory at 0 is mapped and it contains 0 at the location of the return address pointer if it were a stack frame. But this is fragile, and does not work with powernv where address 0 contains firmware instructions. Use the existing dummy frame on stack as the NULL caller, and create a new frame on stack for the entry code. Signed-off-by: Nicholas Piggin --- powerpc/cstart64.S | 12 ++-- 1 file changed, 10 insertions(+), 2 deletions(-) Thanks for tackling this! ... however, not doing powerpc work since years anymore, I have some ignorant questions below... diff --git a/powerpc/cstart64.S b/powerpc/cstart64.S index e18ae9a22..14ab0c6c8 100644 --- a/powerpc/cstart64.S +++ b/powerpc/cstart64.S @@ -46,8 +46,16 @@ start: add r1, r1, r31 add r2, r2, r31 + /* Zero backpointers in initial stack frame so backtrace() stops */ + li r0,0 + std r0,0(r1) 0(r1) is the back chain pointer ... + std r0,16(r1) ... but what is 16(r1) ? I suppose that should be the "LR save word" ? But isn't that at 8(r1) instead?? (not sure whether I'm looking at the right ELF abi spec right now...) Ok, I was looking at the wrong ELF spec, indeed (it was an ancient 32-bit spec, not the 64-bit ABI). Sorry for the confusion. Having a proper #define or a comment for the 16 here would still be helpful, though. Thomas
Re: [kvm-unit-tests PATCH 03/32] powerpc: Fix stack backtrace termination
On 26/02/2024 11.11, Nicholas Piggin wrote: The backtrace handler terminates when it sees a NULL caller address, but the powerpc stack setup does not keep such a NULL caller frame at the start of the stack. This happens to work on pseries because the memory at 0 is mapped and it contains 0 at the location of the return address pointer if it were a stack frame. But this is fragile, and does not work with powernv where address 0 contains firmware instructions. Use the existing dummy frame on stack as the NULL caller, and create a new frame on stack for the entry code. Signed-off-by: Nicholas Piggin --- powerpc/cstart64.S | 12 ++-- 1 file changed, 10 insertions(+), 2 deletions(-) Thanks for tackling this! ... however, not doing powerpc work since years anymore, I have some ignorant questions below... diff --git a/powerpc/cstart64.S b/powerpc/cstart64.S index e18ae9a22..14ab0c6c8 100644 --- a/powerpc/cstart64.S +++ b/powerpc/cstart64.S @@ -46,8 +46,16 @@ start: add r1, r1, r31 add r2, r2, r31 + /* Zero backpointers in initial stack frame so backtrace() stops */ + li r0,0 + std r0,0(r1) 0(r1) is the back chain pointer ... + std r0,16(r1) ... but what is 16(r1) ? I suppose that should be the "LR save word" ? But isn't that at 8(r1) instead?? (not sure whether I'm looking at the right ELF abi spec right now...) Anyway, a comment in the source would be helpful here. + + /* Create entry frame */ + stdur1,-INT_FRAME_SIZE(r1) Since we already create an initial frame via stackptr from powerpc/flat.lds, do we really need to create this additional one here? Or does the one from flat.lds have to be completely empty, i.e. also no DTB pointer in it? /* save DTB pointer */ - std r3, 56(r1) + SAVE_GPR(3,r1) Isn't SAVE_GPR rather meant for the interrupt frame, not for the normal C calling convention frames? Sorry for asking dumb questions ... I still have a hard time understanding the changes here... :-/ /* * Call relocate. relocate is C code, but careful to not use @@ -101,7 +109,7 @@ start: stw r4, 0(r3) /* complete setup */ -1: ld r3, 56(r1) +1: REST_GPR(3, r1) bl setup /* run the test */ Thomas
Re: [kvm-unit-tests PATCH 02/32] powerpc: Fix pseries getchar return value
On 26/02/2024 11.11, Nicholas Piggin wrote: getchar() didn't get the shift value correct and never returned the first character. This never really mattered since it was only ever used for press-a-key-to-continue prompts. but it tripped me up when debugging a QEMU console output problem. Signed-off-by: Nicholas Piggin --- lib/powerpc/hcall.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/powerpc/hcall.c b/lib/powerpc/hcall.c index 711cb1b0f..b4d39ac65 100644 --- a/lib/powerpc/hcall.c +++ b/lib/powerpc/hcall.c @@ -43,5 +43,5 @@ int __getchar(void) asm volatile (" sc 1 " : "+r"(r3), "+r"(r4), "=r"(r5) : "r"(r3), "r"(r4)); - return r3 == H_SUCCESS && r4 > 0 ? r5 >> 48 : -1; + return r3 == H_SUCCESS && r4 > 0 ? r5 >> 56 : -1; } Reviewed-by: Thomas Huth
Re: [kvm-unit-tests PATCH 01/32] powerpc: Fix KVM caps on POWER9 hosts
On 26/02/2024 11.11, Nicholas Piggin wrote: KVM does not like to run on POWER9 hosts without cap-ccf-assist=off. Signed-off-by: Nicholas Piggin --- powerpc/run | 2 ++ 1 file changed, 2 insertions(+) diff --git a/powerpc/run b/powerpc/run index e469f1eb3..5cdb94194 100755 --- a/powerpc/run +++ b/powerpc/run @@ -24,6 +24,8 @@ M+=",accel=$ACCEL$ACCEL_PROPS" if [[ "$ACCEL" == "tcg" ]] ; then M+=",cap-cfpc=broken,cap-sbbc=broken,cap-ibs=broken,cap-ccf-assist=off" +elif [[ "$ACCEL" == "kvm" ]] ; then + M+=",cap-ccf-assist=off" fi Since it is needed in both cases, you could also move it out of the if-statement and remove it from the tcg part. Anyway, Reviewed-by: Thomas Huth
Re: [kvm-unit-tests PATCH v5 0/8] Multi-migration support
On 26/02/2024 09.10, Nicholas Piggin wrote: On Fri Feb 23, 2024 at 5:06 PM AEST, Thomas Huth wrote: On 21/02/2024 04.27, Nicholas Piggin wrote: Now that strange arm64 hang is found to be QEMU bug, I'll repost. Since arm64 requires Thomas's uart patch and it is worse affected by the QEMU bug, I will just not build it on arm. The QEMU bug still affects powerpc (and presumably s390x) but it's not causing so much trouble for this test case. I have another test case that can hit it reliably and doesn't cause crashes but that takes some harness and common lib work so I'll send that another time. Since v4: - Don't build selftest-migration on arm. - Reduce selftest-migration iterations from 100 to 30 to make the test run faster (it's ~0.5s per migration). Thanks, I think the series is ready to go now ... we just have to wait for your QEMU TCG migration fix to get merged first. Or should we maybe mark the selftest-migration with "accel = kvm" for now and remove that line later once QEMU has been fixed? Could we merge it? I'm juggling a bunch of different things and prone to lose track of something :\ I'll need to drum up a bit of interest to review the QEMU fixes from those who know the code too, so that may take some time. Ok, I merged it, but with "accel = kvm" for the time being (otherwise this would be quite a pitfall for people trying to run the k-u-t with TCG when they don't know that they have to fetch a patch from the mailing list to get it working). I left it out of arm unittests.cfg entirely, and s390 and powerpc seems to work by luck enough to be useful for gitlab CI so I don't think there is a chnage needed really unless you're paranoid. At least the s390x test does not work reliably at all when running with TCG without your QEMU patch, so I think we really need the "accel = kvm" for the time being here. I do have a later patch that adds a memory tester that does trigger it right away on powerpc. I'll send that out after this series is merged... but we do still have the issue that the gitlab CI image has the old QEMU don't we? Until we update distro. We only run selected tests in the gitlab-CI, so unless you add it to .gitlab-ci.yml, the selftest-migration test won't be run there. Thomas
Re: [kvm-unit-tests PATCH v5 0/8] Multi-migration support
On 21/02/2024 04.27, Nicholas Piggin wrote: Now that strange arm64 hang is found to be QEMU bug, I'll repost. Since arm64 requires Thomas's uart patch and it is worse affected by the QEMU bug, I will just not build it on arm. The QEMU bug still affects powerpc (and presumably s390x) but it's not causing so much trouble for this test case. I have another test case that can hit it reliably and doesn't cause crashes but that takes some harness and common lib work so I'll send that another time. Since v4: - Don't build selftest-migration on arm. - Reduce selftest-migration iterations from 100 to 30 to make the test run faster (it's ~0.5s per migration). Thanks, I think the series is ready to go now ... we just have to wait for your QEMU TCG migration fix to get merged first. Or should we maybe mark the selftest-migration with "accel = kvm" for now and remove that line later once QEMU has been fixed? Thomas
Re: [kvm-unit-tests PATCH v4 8/8] migration: add a migration selftest
On 17/02/2024 08.19, Nicholas Piggin wrote: On Fri Feb 16, 2024 at 9:15 PM AEST, Thomas Huth wrote: On 09/02/2024 10.11, Nicholas Piggin wrote: Add a selftest for migration support in guest library and test harness code. It performs migrations in a tight loop to irritate races and bugs in the test harness code. Include the test in arm, s390, powerpc. Acked-by: Claudio Imbrenda (s390x) Reviewed-by: Thomas Huth Signed-off-by: Nicholas Piggin --- arm/Makefile.common | 1 + arm/selftest-migration.c | 1 + arm/unittests.cfg| 6 ++ Hi Nicholas, I just gave the patches a try, but the arm test seems to fail for me: Only the first getchar() seems to wait for a character, all the subsequent ones don't wait anymore and just continue immediately ... is this working for you? Or do I need another patch on top? Hey sorry missed this comment It does seem to work for me, I've mostly tested pseries but I did test others too (that's how I saw the arm getchar limit). How are you observing it not waiting for migration? According to you other mail, I think you figured it out already, but just for the records: You can see it when running the guest manually, e.g. something like: qemu-system-aarch64 -nodefaults -machine virt -accel tcg -cpu cortex-a57 \ -device virtio-serial-device -device virtconsole,chardev=ctd \ -chardev testdev,id=ctd -device pci-testdev -display none \ -serial mon:stdio -kernel arm/selftest-migration.flat -smp 1 Without my "lib/arm/io: Fix calling getchar() multiple times" patch, the guest only waits during the first getchar(), all the others simply return immediately. Thomas
Re: [kvm-unit-tests PATCH v4 8/8] migration: add a migration selftest
On 09/02/2024 10.11, Nicholas Piggin wrote: Add a selftest for migration support in guest library and test harness code. It performs migrations in a tight loop to irritate races and bugs in the test harness code. Include the test in arm, s390, powerpc. Acked-by: Claudio Imbrenda (s390x) Reviewed-by: Thomas Huth Signed-off-by: Nicholas Piggin --- arm/Makefile.common | 1 + arm/selftest-migration.c | 1 + arm/unittests.cfg| 6 ++ common/selftest-migration.c | 34 ++ powerpc/Makefile.common | 1 + powerpc/selftest-migration.c | 1 + powerpc/unittests.cfg| 4 s390x/Makefile | 1 + s390x/selftest-migration.c | 1 + s390x/unittests.cfg | 4 10 files changed, 54 insertions(+) create mode 12 arm/selftest-migration.c create mode 100644 common/selftest-migration.c create mode 12 powerpc/selftest-migration.c create mode 12 s390x/selftest-migration.c diff --git a/arm/Makefile.common b/arm/Makefile.common index f828dbe0..f107c478 100644 --- a/arm/Makefile.common +++ b/arm/Makefile.common @@ -5,6 +5,7 @@ # tests-common = $(TEST_DIR)/selftest.$(exe) +tests-common += $(TEST_DIR)/selftest-migration.$(exe) tests-common += $(TEST_DIR)/spinlock-test.$(exe) tests-common += $(TEST_DIR)/pci-test.$(exe) tests-common += $(TEST_DIR)/pmu.$(exe) diff --git a/arm/selftest-migration.c b/arm/selftest-migration.c new file mode 12 index ..bd1eb266 --- /dev/null +++ b/arm/selftest-migration.c @@ -0,0 +1 @@ +../common/selftest-migration.c \ No newline at end of file diff --git a/arm/unittests.cfg b/arm/unittests.cfg index fe601cbb..db0e4c9b 100644 --- a/arm/unittests.cfg +++ b/arm/unittests.cfg @@ -55,6 +55,12 @@ smp = $MAX_SMP extra_params = -append 'smp' groups = selftest +# Test migration +[selftest-migration] +file = selftest-migration.flat +groups = selftest migration +arch = arm64 + # Test PCI emulation [pci-test] file = pci-test.flat diff --git a/common/selftest-migration.c b/common/selftest-migration.c new file mode 100644 index ..f70c505f --- /dev/null +++ b/common/selftest-migration.c @@ -0,0 +1,34 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Machine independent migration tests + * + * This is just a very simple test that is intended to stress the migration + * support in the test harness. This could be expanded to test more guest + * library code, but architecture-specific tests should be used to test + * migration of tricky machine state. + */ +#include +#include + +#if defined(__arm__) || defined(__aarch64__) +/* arm can only call getchar 15 times */ +#define NR_MIGRATIONS 15 +#else +#define NR_MIGRATIONS 100 +#endif FYI, I just wrote a patch that will hopefully fix the limitation to 15 times on arm: https://lore.kernel.org/kvm/20240216140210.70280-1-th...@redhat.com/T/#u Thomas
Re: [kvm-unit-tests PATCH v4 8/8] migration: add a migration selftest
On 09/02/2024 10.11, Nicholas Piggin wrote: Add a selftest for migration support in guest library and test harness code. It performs migrations in a tight loop to irritate races and bugs in the test harness code. Include the test in arm, s390, powerpc. Acked-by: Claudio Imbrenda (s390x) Reviewed-by: Thomas Huth Signed-off-by: Nicholas Piggin --- arm/Makefile.common | 1 + arm/selftest-migration.c | 1 + arm/unittests.cfg| 6 ++ Hi Nicholas, I just gave the patches a try, but the arm test seems to fail for me: Only the first getchar() seems to wait for a character, all the subsequent ones don't wait anymore and just continue immediately ... is this working for you? Or do I need another patch on top? Thanks, Thomas
Re: [kvm-unit-tests PATCH v4 6/8] migration: Add quiet migration support
On 09/02/2024 10.11, Nicholas Piggin wrote: Console output required to support migration becomes quite noisy when doing lots of migrations. Provide a migrate_quiet() call that suppresses console output and doesn't log a message. Signed-off-by: Nicholas Piggin --- lib/migrate.c | 11 +++ lib/migrate.h | 1 + scripts/arch-run.bash | 4 ++-- 3 files changed, 14 insertions(+), 2 deletions(-) Reviewed-by: Thomas Huth
Re: [kvm-unit-tests PATCH v4 4/8] migration: Support multiple migrations
On 09/02/2024 10.11, Nicholas Piggin wrote: Support multiple migrations by flipping dest file/socket variables to source after the migration is complete, ready to start again. A new destination is created if the test outputs the migrate line again. Test cases may now switch to calling migrate() one or more times. Signed-off-by: Nicholas Piggin --- lib/migrate.c | 8 ++-- lib/migrate.h | 1 + scripts/arch-run.bash | 86 --- 3 files changed, 77 insertions(+), 18 deletions(-) Reviewed-by: Thomas Huth
Re: [kvm-unit-tests PATCH v4 2/8] arch-run: Clean up initrd cleanup
On 09/02/2024 10.11, Nicholas Piggin wrote: Rather than put a big script into the trap handler, have it call a function. Signed-off-by: Nicholas Piggin --- scripts/arch-run.bash | 13 - 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash index 11d47a85..c1dd67ab 100644 --- a/scripts/arch-run.bash +++ b/scripts/arch-run.bash @@ -269,10 +269,21 @@ search_qemu_binary () export PATH=$save_path } +initrd_cleanup () +{ + rm -f $KVM_UNIT_TESTS_ENV + if [ "$KVM_UNIT_TESTS_ENV_OLD" ]; then + export KVM_UNIT_TESTS_ENV="$KVM_UNIT_TESTS_ENV_OLD" + else + unset KVM_UNIT_TESTS_ENV + fi + unset KVM_UNIT_TESTS_ENV_OLD +} + initrd_create () { if [ "$ENVIRON_DEFAULT" = "yes" ]; then - trap_exit_push 'rm -f $KVM_UNIT_TESTS_ENV; [ "$KVM_UNIT_TESTS_ENV_OLD" ] && export KVM_UNIT_TESTS_ENV="$KVM_UNIT_TESTS_ENV_OLD" || unset KVM_UNIT_TESTS_ENV; unset KVM_UNIT_TESTS_ENV_OLD' + trap_exit_push 'initrd_cleanup' [ -f "$KVM_UNIT_TESTS_ENV" ] && export KVM_UNIT_TESTS_ENV_OLD="$KVM_UNIT_TESTS_ENV" export KVM_UNIT_TESTS_ENV=$(mktemp) env_params Reviewed-by: Thomas Huth
Re: [kvm-unit-tests PATCH v3 4/8] migration: Support multiple migrations
On 09/02/2024 09.39, Nicholas Piggin wrote: On Fri Feb 9, 2024 at 6:19 PM AEST, Thomas Huth wrote: On 09/02/2024 08.01, Nicholas Piggin wrote: Support multiple migrations by flipping dest file/socket variables to source after the migration is complete, ready to start again. A new destination is created if the test outputs the migrate line again. Test cases may now switch to calling migrate() one or more times. Signed-off-by: Nicholas Piggin --- ... diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash index 3689d7c2..a914ba17 100644 --- a/scripts/arch-run.bash +++ b/scripts/arch-run.bash @@ -129,12 +129,16 @@ run_migration () return 77 fi + migcmdline=$@ + trap 'trap - TERM ; kill 0 ; exit 2' INT TERM - trap 'rm -f ${migout1} ${migout_fifo1} ${migsock} ${qmp1} ${qmp2} ${fifo}' RETURN EXIT + trap 'rm -f ${migout1} ${migout2} ${migout_fifo1} ${migout_fifo2} ${migsock} ${qmp1} ${qmp2} ${fifo}' RETURN EXIT migsock=$(mktemp -u -t mig-helper-socket.XX) migout1=$(mktemp -t mig-helper-stdout1.XX) migout_fifo1=$(mktemp -u -t mig-helper-fifo-stdout1.XX) + migout2=$(mktemp -t mig-helper-stdout2.XX) + migout_fifo2=$(mktemp -u -t mig-helper-fifo-stdout2.XX) qmp1=$(mktemp -u -t mig-helper-qmp1.XX) qmp2=$(mktemp -u -t mig-helper-qmp2.XX) fifo=$(mktemp -u -t mig-helper-fifo.XX) @@ -142,18 +146,61 @@ run_migration () qmpout2=/dev/null mkfifo ${migout_fifo1} - eval "$@" -chardev socket,id=mon1,path=${qmp1},server=on,wait=off \ + mkfifo ${migout_fifo2} + + eval "$migcmdline" \ + -chardev socket,id=mon1,path=${qmp1},server=on,wait=off \ -mon chardev=mon1,mode=control > ${migout_fifo1} & live_pid=$! cat ${migout_fifo1} | tee ${migout1} & - # We have to use cat to open the named FIFO, because named FIFO's, unlike - # pipes, will block on open() until the other end is also opened, and that - # totally breaks QEMU... + # The test must prompt the user to migrate, so wait for the "migrate" + # keyword + while ! grep -q -i "Now migrate the VM" < ${migout1} ; do + if ! ps -p ${live_pid} > /dev/null ; then + echo "ERROR: Test exit before migration point." >&2 + qmp ${qmp1} '"quit"'> ${qmpout1} 2>/dev/null + return 3 + fi + sleep 0.1 + done + + # This starts the first source QEMU in advance of the test reaching the + # migration point, since we expect at least one migration. Subsequent + # sources are started as the test hits migrate keywords. + do_migration || return $? + + while ps -p ${live_pid} > /dev/null ; do + # Wait for EXIT or further migrations + if ! grep -q -i "Now migrate the VM" < ${migout1} ; then + sleep 0.1 + else + do_migration || return $? + fi + done + + wait ${live_pid} + ret=$? + + while (( $(jobs -r | wc -l) > 0 )); do + sleep 0.1 + done + + return $ret +} + +do_migration () +{ + # We have to use cat to open the named FIFO, because named FIFO's, + # unlike pipes, will block on open() until the other end is also + # opened, and that totally breaks QEMU... mkfifo ${fifo} - eval "$@" -chardev socket,id=mon2,path=${qmp2},server=on,wait=off \ - -mon chardev=mon2,mode=control -incoming unix:${migsock} < <(cat ${fifo}) & + eval "$migcmdline" \ + -chardev socket,id=mon2,path=${qmp2},server=on,wait=off \ + -mon chardev=mon2,mode=control -incoming unix:${migsock} \ + < <(cat ${fifo}) > ${migout_fifo2} & incoming_pid=$! + cat ${migout_fifo2} | tee ${migout2} & # The test must prompt the user to migrate, so wait for the "migrate" keyword while ! grep -q -i "Now migrate the VM" < ${migout1} ; do So the old check for the "migrate" keyword is also still around? It's just the comment is staleish, it only checks "Now migrate...". Why do we need to wait on two spots for the "Now mirgrate..." string now? So that the it ensures we do one migration, subsequent ones are optional. I was thinking we could just remove that, and possibly even remove the MIGRATION=yes/no paths and always just use the same code here. But that's for another time. Actually there is some weirdness here. There are *three* spots where it waits for migration. Yes, that'
Re: [kvm-unit-tests PATCH v3 8/8] migration: add a migration selftest
On 09/02/2024 08.01, Nicholas Piggin wrote: Add a selftest for migration support in guest library and test harness code. It performs migrations a tight loop to irritate races and bugs in "*in* a tight loop" ? the test harness code. Include the test in arm, s390, powerpc. Acked-by: Claudio Imbrenda (s390x) Signed-off-by: Nicholas Piggin --- This has flushed out several bugs in developing the multi migration test harness code already. Thanks, Nick arm/Makefile.common | 1 + arm/selftest-migration.c | 1 + arm/unittests.cfg| 6 ++ common/selftest-migration.c | 34 ++ powerpc/Makefile.common | 1 + powerpc/selftest-migration.c | 1 + powerpc/unittests.cfg| 4 s390x/Makefile | 1 + s390x/selftest-migration.c | 1 + s390x/unittests.cfg | 4 10 files changed, 54 insertions(+) create mode 12 arm/selftest-migration.c create mode 100644 common/selftest-migration.c create mode 12 powerpc/selftest-migration.c create mode 12 s390x/selftest-migration.c diff --git a/arm/Makefile.common b/arm/Makefile.common index f828dbe0..f107c478 100644 --- a/arm/Makefile.common +++ b/arm/Makefile.common @@ -5,6 +5,7 @@ # tests-common = $(TEST_DIR)/selftest.$(exe) +tests-common += $(TEST_DIR)/selftest-migration.$(exe) tests-common += $(TEST_DIR)/spinlock-test.$(exe) tests-common += $(TEST_DIR)/pci-test.$(exe) tests-common += $(TEST_DIR)/pmu.$(exe) diff --git a/arm/selftest-migration.c b/arm/selftest-migration.c new file mode 12 index ..bd1eb266 --- /dev/null +++ b/arm/selftest-migration.c @@ -0,0 +1 @@ +../common/selftest-migration.c \ No newline at end of file diff --git a/arm/unittests.cfg b/arm/unittests.cfg index fe601cbb..1ffd9a82 100644 --- a/arm/unittests.cfg +++ b/arm/unittests.cfg @@ -55,6 +55,12 @@ smp = $MAX_SMP extra_params = -append 'smp' groups = selftest +# Test migration +[selftest-migration] +file = selftest-migration.flat +groups = selftest migration + +arch = arm64 Please swap the last two lines! # Test PCI emulation [pci-test] file = pci-test.flat With the nits fixed: Reviewed-by: Thomas Huth
Re: [kvm-unit-tests PATCH v3 7/8] Add common/ directory for architecture-independent tests
On 09/02/2024 08.01, Nicholas Piggin wrote: x86/sieve.c is used by s390x, arm, and riscv via symbolic link. Make a new directory common/ for architecture-independent tests and move sieve.c here. Signed-off-by: Nicholas Piggin --- arm/sieve.c| 2 +- common/sieve.c | 51 + riscv/sieve.c | 2 +- s390x/sieve.c | 2 +- x86/sieve.c| 52 +- 5 files changed, 55 insertions(+), 54 deletions(-) create mode 100644 common/sieve.c mode change 100644 => 12 x86/sieve.c Reviewed-by: Thomas Huth
Re: [kvm-unit-tests PATCH v3 5/8] arch-run: rename migration variables
On 09/02/2024 08.01, Nicholas Piggin wrote: Using 1 and 2 for source and destination is confusing, particularly now with multiple migrations that flip between them. Do a rename pass to tidy things up. Signed-off-by: Nicholas Piggin --- scripts/arch-run.bash | 115 +- 1 file changed, 58 insertions(+), 57 deletions(-) Acked-by: Thomas Huth
Re: [kvm-unit-tests PATCH v3 4/8] migration: Support multiple migrations
On 09/02/2024 08.01, Nicholas Piggin wrote: Support multiple migrations by flipping dest file/socket variables to source after the migration is complete, ready to start again. A new destination is created if the test outputs the migrate line again. Test cases may now switch to calling migrate() one or more times. Signed-off-by: Nicholas Piggin --- ... diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash index 3689d7c2..a914ba17 100644 --- a/scripts/arch-run.bash +++ b/scripts/arch-run.bash @@ -129,12 +129,16 @@ run_migration () return 77 fi + migcmdline=$@ + trap 'trap - TERM ; kill 0 ; exit 2' INT TERM - trap 'rm -f ${migout1} ${migout_fifo1} ${migsock} ${qmp1} ${qmp2} ${fifo}' RETURN EXIT + trap 'rm -f ${migout1} ${migout2} ${migout_fifo1} ${migout_fifo2} ${migsock} ${qmp1} ${qmp2} ${fifo}' RETURN EXIT migsock=$(mktemp -u -t mig-helper-socket.XX) migout1=$(mktemp -t mig-helper-stdout1.XX) migout_fifo1=$(mktemp -u -t mig-helper-fifo-stdout1.XX) + migout2=$(mktemp -t mig-helper-stdout2.XX) + migout_fifo2=$(mktemp -u -t mig-helper-fifo-stdout2.XX) qmp1=$(mktemp -u -t mig-helper-qmp1.XX) qmp2=$(mktemp -u -t mig-helper-qmp2.XX) fifo=$(mktemp -u -t mig-helper-fifo.XX) @@ -142,18 +146,61 @@ run_migration () qmpout2=/dev/null mkfifo ${migout_fifo1} - eval "$@" -chardev socket,id=mon1,path=${qmp1},server=on,wait=off \ + mkfifo ${migout_fifo2} + + eval "$migcmdline" \ + -chardev socket,id=mon1,path=${qmp1},server=on,wait=off \ -mon chardev=mon1,mode=control > ${migout_fifo1} & live_pid=$! cat ${migout_fifo1} | tee ${migout1} & - # We have to use cat to open the named FIFO, because named FIFO's, unlike - # pipes, will block on open() until the other end is also opened, and that - # totally breaks QEMU... + # The test must prompt the user to migrate, so wait for the "migrate" + # keyword + while ! grep -q -i "Now migrate the VM" < ${migout1} ; do + if ! ps -p ${live_pid} > /dev/null ; then + echo "ERROR: Test exit before migration point." >&2 + qmp ${qmp1} '"quit"'> ${qmpout1} 2>/dev/null + return 3 + fi + sleep 0.1 + done + + # This starts the first source QEMU in advance of the test reaching the + # migration point, since we expect at least one migration. Subsequent + # sources are started as the test hits migrate keywords. + do_migration || return $? + + while ps -p ${live_pid} > /dev/null ; do + # Wait for EXIT or further migrations + if ! grep -q -i "Now migrate the VM" < ${migout1} ; then + sleep 0.1 + else + do_migration || return $? + fi + done + + wait ${live_pid} + ret=$? + + while (( $(jobs -r | wc -l) > 0 )); do + sleep 0.1 + done + + return $ret +} + +do_migration () +{ + # We have to use cat to open the named FIFO, because named FIFO's, + # unlike pipes, will block on open() until the other end is also + # opened, and that totally breaks QEMU... mkfifo ${fifo} - eval "$@" -chardev socket,id=mon2,path=${qmp2},server=on,wait=off \ - -mon chardev=mon2,mode=control -incoming unix:${migsock} < <(cat ${fifo}) & + eval "$migcmdline" \ + -chardev socket,id=mon2,path=${qmp2},server=on,wait=off \ + -mon chardev=mon2,mode=control -incoming unix:${migsock} \ + < <(cat ${fifo}) > ${migout_fifo2} & incoming_pid=$! + cat ${migout_fifo2} | tee ${migout2} & # The test must prompt the user to migrate, so wait for the "migrate" keyword while ! grep -q -i "Now migrate the VM" < ${migout1} ; do So the old check for the "migrate" keyword is also still around? Why do we need to wait on two spots for the "Now mirgrate..." string now? Thomas @@ -164,7 +211,7 @@ run_migration () qmp ${qmp2} '"quit"'> ${qmpout2} 2>/dev/null return 3 fi - sleep 1 + sleep 0.1 done # Wait until the destination has created the incoming and qmp sockets @@ -176,7 +223,7 @@ run_migration () # Wait for the migration to complete migstatus=`qmp ${qmp1} '"query-migrate"' | grep return` while ! grep -q '"completed"' <<<"$migstatus" ; do - sleep 1 + sleep 0.1 if ! migstatus=`qmp ${qmp1} '"query-migrate"'`; then echo "ERROR: Querying migration state failed." >&2 echo > ${fifo} @@ -192,14 +239,34 @@ run_migration ()
Re: [kvm-unit-tests PATCH v3 6/8] migration: Add quiet migration support
On 09/02/2024 08.01, Nicholas Piggin wrote: Console output required to support migration becomes quite noisy when doing lots of migrations. Provide a migrate_quiet() call that suppresses console output and doesn't log a message. Signed-off-by: Nicholas Piggin --- lib/migrate.c | 12 lib/migrate.h | 1 + scripts/arch-run.bash | 4 ++-- 3 files changed, 15 insertions(+), 2 deletions(-) diff --git a/lib/migrate.c b/lib/migrate.c index b7721659..4e0ab516 100644 --- a/lib/migrate.c +++ b/lib/migrate.c @@ -18,6 +18,18 @@ void migrate(void) report_info("Migration complete"); } +/* + * Like migrate() but supporess output and logs, useful for intensive s/supporess/suppress/ + * migration stress testing without polluting logs. Test cases should + * provide relevant information about migration in failure reports. + */ +void migrate_quiet(void) +{ + puts("Now migrate the VM (quiet)\n"); + (void)getchar(); +} + + Remove one empty line, please! /* * Initiate migration and wait for it to complete. * If this function is called more than once, it is a no-op. diff --git a/lib/migrate.h b/lib/migrate.h index 2af06a72..95b9102b 100644 --- a/lib/migrate.h +++ b/lib/migrate.h @@ -7,4 +7,5 @@ */ void migrate(void); +void migrate_quiet(void); void migrate_once(void); diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash index 0b45eb61..29cf9b0c 100644 --- a/scripts/arch-run.bash +++ b/scripts/arch-run.bash @@ -152,7 +152,7 @@ run_migration () -chardev socket,id=mon,path=${src_qmp},server=on,wait=off \ -mon chardev=mon,mode=control > ${src_outfifo} & live_pid=$! - cat ${src_outfifo} | tee ${src_out} & + cat ${src_outfifo} | tee ${src_out} | grep -v "Now migrate the VM (quiet)" & # The test must prompt the user to migrate, so wait for the "migrate" # keyword @@ -200,7 +200,7 @@ do_migration () -mon chardev=mon,mode=control -incoming unix:${dst_incoming} \ < <(cat ${dst_infifo}) > ${dst_outfifo} & incoming_pid=$! - cat ${dst_outfifo} | tee ${dst_out} & + cat ${dst_outfifo} | tee ${dst_out} | grep -v "Now migrate the VM (quiet)" & # The test must prompt the user to migrate, so wait for the "migrate" keyword while ! grep -q -i "Now migrate the VM" < ${src_out} ; do Thomas
Re: [kvm-unit-tests PATCH v3 3/8] migration: use a more robust way to wait for background job
On 09/02/2024 08.01, Nicholas Piggin wrote: Starting a pipeline of jobs in the background does not seem to have a simple way to reliably find the pid of a particular process in the pipeline (because not all processes are started when the shell continues to execute). The way PID of QEMU is derived can result in a failure waiting on a PID that is not running. This is easier to hit with subsequent multiple-migration support. Changing this to use $! by swapping the pipeline for a fifo is more robust. Signed-off-by: Nicholas Piggin --- scripts/arch-run.bash | 15 +++ 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash index 1e903e83..3689d7c2 100644 --- a/scripts/arch-run.bash +++ b/scripts/arch-run.bash @@ -130,19 +130,22 @@ run_migration () fi trap 'trap - TERM ; kill 0 ; exit 2' INT TERM - trap 'rm -f ${migout1} ${migsock} ${qmp1} ${qmp2} ${fifo}' RETURN EXIT + trap 'rm -f ${migout1} ${migout_fifo1} ${migsock} ${qmp1} ${qmp2} ${fifo}' RETURN EXIT migsock=$(mktemp -u -t mig-helper-socket.XX) migout1=$(mktemp -t mig-helper-stdout1.XX) + migout_fifo1=$(mktemp -u -t mig-helper-fifo-stdout1.XX) qmp1=$(mktemp -u -t mig-helper-qmp1.XX) qmp2=$(mktemp -u -t mig-helper-qmp2.XX) fifo=$(mktemp -u -t mig-helper-fifo.XX) qmpout1=/dev/null qmpout2=/dev/null + mkfifo ${migout_fifo1} eval "$@" -chardev socket,id=mon1,path=${qmp1},server=on,wait=off \ - -mon chardev=mon1,mode=control | tee ${migout1} & - live_pid=`jobs -l %+ | grep "eval" | awk '{print$2}'` + -mon chardev=mon1,mode=control > ${migout_fifo1} & + live_pid=$! + cat ${migout_fifo1} | tee ${migout1} & # We have to use cat to open the named FIFO, because named FIFO's, unlike # pipes, will block on open() until the other end is also opened, and that @@ -150,7 +153,7 @@ run_migration () mkfifo ${fifo} eval "$@" -chardev socket,id=mon2,path=${qmp2},server=on,wait=off \ -mon chardev=mon2,mode=control -incoming unix:${migsock} < <(cat ${fifo}) & - incoming_pid=`jobs -l %+ | awk '{print$2}'` + incoming_pid=$! # The test must prompt the user to migrate, so wait for the "migrate" keyword while ! grep -q -i "Now migrate the VM" < ${migout1} ; do @@ -164,6 +167,10 @@ run_migration () sleep 1 done + # Wait until the destination has created the incoming and qmp sockets + while ! [ -S ${migsock} ] ; do sleep 0.1 ; done + while ! [ -S ${qmp2} ] ; do sleep 0.1 ; done + qmp ${qmp1} '"migrate", "arguments": { "uri": "unix:'${migsock}'" }' > ${qmpout1} # Wait for the migration to complete Reviewed-by: Thomas Huth
Re: [kvm-unit-tests PATCH v3 2/8] arch-run: Clean up initrd cleanup
On 09/02/2024 08.01, Nicholas Piggin wrote: Rather than put a big script into the trap handler, have it call a function. Signed-off-by: Nicholas Piggin --- scripts/arch-run.bash | 13 - 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash index 11d47a85..1e903e83 100644 --- a/scripts/arch-run.bash +++ b/scripts/arch-run.bash @@ -269,10 +269,21 @@ search_qemu_binary () export PATH=$save_path } +initrd_cleanup () +{ + rm -f $KVM_UNIT_TESTS_ENV + if [ "$KVM_UNIT_TESTS_ENV_OLD" ]; then + export KVM_UNIT_TESTS_ENV="$KVM_UNIT_TESTS_ENV_OLD" + else + unset KVM_UNIT_TESTS_ENV + unset KVM_UNIT_TESTS_ENV_OLD + fi +} Looking at the original code below, shouldn't this rather unset KVM_UNIT_TESTS_ENV_OLD after the "fi" statement? Thomas initrd_create () { if [ "$ENVIRON_DEFAULT" = "yes" ]; then - trap_exit_push 'rm -f $KVM_UNIT_TESTS_ENV; [ "$KVM_UNIT_TESTS_ENV_OLD" ] && export KVM_UNIT_TESTS_ENV="$KVM_UNIT_TESTS_ENV_OLD" || unset KVM_UNIT_TESTS_ENV; unset KVM_UNIT_TESTS_ENV_OLD' + trap_exit_push 'initrd_cleanup' [ -f "$KVM_UNIT_TESTS_ENV" ] && export KVM_UNIT_TESTS_ENV_OLD="$KVM_UNIT_TESTS_ENV" export KVM_UNIT_TESTS_ENV=$(mktemp) env_params
Re: [kvm-unit-tests PATCH v3 1/8] arch-run: Fix TRAP handler recursion to remove temporary files properly
On 09/02/2024 08.01, Nicholas Piggin wrote: Migration files were not being removed when the QEMU process is interrupted (e.g., with ^C). This is becaus the SIGINT propagates to the bash TRAP handler, which recursively TRAPs due to the 'kill 0' in the handler. This eventually crashes bash. This can be observed by interrupting a long-running test program that is run with MIGRATION=yes, /tmp/mig-helper-* files remain afterwards. Removing TRAP recursion solves this problem and allows the EXIT handler to run and clean up the files. This also moves the trap handler before temp file creation, and expands the name variables at trap-time rather than install-time, which closes the small race between creation trap handler install. Reviewed-by: Thomas Huth Signed-off-by: Nicholas Piggin --- scripts/arch-run.bash | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash index d0864360..11d47a85 100644 --- a/scripts/arch-run.bash +++ b/scripts/arch-run.bash @@ -129,6 +129,9 @@ run_migration () return 77 fi + trap 'trap - TERM ; kill 0 ; exit 2' INT TERM + trap 'rm -f ${migout1} ${migsock} ${qmp1} ${qmp2} ${fifo}' RETURN EXIT + migsock=$(mktemp -u -t mig-helper-socket.XX) migout1=$(mktemp -t mig-helper-stdout1.XX) qmp1=$(mktemp -u -t mig-helper-qmp1.XX) @@ -137,9 +140,6 @@ run_migration () qmpout1=/dev/null qmpout2=/dev/null - trap 'kill 0; exit 2' INT TERM - trap 'rm -f ${migout1} ${migsock} ${qmp1} ${qmp2} ${fifo}' RETURN EXIT - eval "$@" -chardev socket,id=mon1,path=${qmp1},server=on,wait=off \ -mon chardev=mon1,mode=control | tee ${migout1} & live_pid=`jobs -l %+ | grep "eval" | awk '{print$2}'` @@ -209,11 +209,11 @@ run_panic () return 77 fi - qmp=$(mktemp -u -t panic-qmp.XX) - - trap 'kill 0; exit 2' INT TERM + trap 'trap - TERM ; kill 0 ; exit 2' INT TERM trap 'rm -f ${qmp}' RETURN EXIT + qmp=$(mktemp -u -t panic-qmp.XX) + # start VM stopped so we don't miss any events eval "$@" -chardev socket,id=mon1,path=${qmp},server=on,wait=off \ -mon chardev=mon1,mode=control -S & Reviewed-by: Thomas Huth
Re: [kvm-unit-tests PATCH v2 2/9] arch-run: Clean up temporary files properly
On 02/02/2024 07.57, Nicholas Piggin wrote: Migration files weren't being removed when tests were interrupted. This improves the situation. Signed-off-by: Nicholas Piggin --- scripts/arch-run.bash | 12 +++- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash index d0864360..f22ead6f 100644 --- a/scripts/arch-run.bash +++ b/scripts/arch-run.bash @@ -134,12 +134,14 @@ run_migration () qmp1=$(mktemp -u -t mig-helper-qmp1.XX) qmp2=$(mktemp -u -t mig-helper-qmp2.XX) fifo=$(mktemp -u -t mig-helper-fifo.XX) + + # race here between file creation and trap + trap "trap - TERM ; kill 0 ; exit 2" INT TERM + trap "rm -f ${migout1} ${migsock} ${qmp1} ${qmp2} ${fifo}" RETURN EXIT + qmpout1=/dev/null qmpout2=/dev/null - trap 'kill 0; exit 2' INT TERM - trap 'rm -f ${migout1} ${migsock} ${qmp1} ${qmp2} ${fifo}' RETURN EXIT - eval "$@" -chardev socket,id=mon1,path=${qmp1},server=on,wait=off \ -mon chardev=mon1,mode=control | tee ${migout1} & live_pid=`jobs -l %+ | grep "eval" | awk '{print$2}'` @@ -211,8 +213,8 @@ run_panic () qmp=$(mktemp -u -t panic-qmp.XX) - trap 'kill 0; exit 2' INT TERM - trap 'rm -f ${qmp}' RETURN EXIT + trap "trap - TERM ; kill 0 ; exit 2" INT TERM + trap "rm -f ${qmp}" RETURN EXIT # start VM stopped so we don't miss any events eval "$@" -chardev socket,id=mon1,path=${qmp},server=on,wait=off \ So the point is that the "EXIT" trap wasn't executed without the "trap - TERM" in the other trap? ... ok, then your patch certainly makes sense. Reviewed-by: Thomas Huth
Re: [kvm-unit-tests PATCH v2 3/9] arch-run: Clean up initrd cleanup
On 02/02/2024 07.57, Nicholas Piggin wrote: Rather than put a big script into the trap handler, have it call a function. Signed-off-by: Nicholas Piggin --- scripts/arch-run.bash | 12 +++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash index f22ead6f..cc7da7c5 100644 --- a/scripts/arch-run.bash +++ b/scripts/arch-run.bash @@ -271,10 +271,20 @@ search_qemu_binary () export PATH=$save_path } +initrd_cleanup () +{ + if [ "$KVM_UNIT_TESTS_ENV_OLD" ]; then + export KVM_UNIT_TESTS_ENV="$KVM_UNIT_TESTS_ENV_OLD" + else + unset KVM_UNIT_TESTS_ENV + unset KVM_UNIT_TESTS_ENV_OLD + fi +} + initrd_create () { if [ "$ENVIRON_DEFAULT" = "yes" ]; then - trap_exit_push 'rm -f $KVM_UNIT_TESTS_ENV; [ "$KVM_UNIT_TESTS_ENV_OLD" ] && export KVM_UNIT_TESTS_ENV="$KVM_UNIT_TESTS_ENV_OLD" || unset KVM_UNIT_TESTS_ENV; unset KVM_UNIT_TESTS_ENV_OLD' + trap_exit_push 'rm -f $KVM_UNIT_TESTS_ENV; initrd_cleanup' Why don't you move the 'rm -f $KVM_UNIT_TESTS_ENV' into the initrd_cleanup() function, too? ... that would IMHO make more sense for a function that is called *_cleanup() ? Thomas
Re: [kvm-unit-tests PATCH v2 1/9] (arm|powerpc|s390x): Makefile: Fix .aux.o generation
On 02/02/2024 10.30, Andrew Jones wrote: On Fri, Feb 02, 2024 at 04:57:32PM +1000, Nicholas Piggin wrote: Using all prerequisites for the source file results in the build dying on the second time around with: gcc: fatal error: cannot specify ‘-o’ with ‘-c’, ‘-S’ or ‘-E’ with multiple files This is due to auxinfo.h becoming a prerequisite after the first build recorded the dependency. D'oh, of course I only tried to run "make" once when testing that patch :-/ diff --git a/arm/Makefile.common b/arm/Makefile.common index 54cb4a63..c2ee568c 100644 --- a/arm/Makefile.common +++ b/arm/Makefile.common @@ -71,7 +71,7 @@ FLATLIBS = $(libcflat) $(LIBFDT_archive) $(libeabi) ifeq ($(CONFIG_EFI),y) %.aux.o: $(SRCDIR)/lib/auxinfo.c - $(CC) $(CFLAGS) -c -o $@ $^ \ + $(CC) $(CFLAGS) -c -o $@ $< \ -DPROGNAME=\"$(@:.aux.o=.efi)\" -DAUXFLAGS=$(AUXFLAGS) There are two instances of the %.aux.o target in arm/Makefile.common. We need to fix both. We can actually pull the target out of the two arms of the CONFIG_EFI if-else, though, by changing the .efi/.flat to .$(exe). I went ahead and pushed this patch with the trivial fix for the else-branch to the repo to unbreak the build. If you think it's worthwhile to unify the target, please provide a patch to do so, thanks! Thomas
Re: [kvm-unit-tests PATCH v5 29/29] powerpc: Add timebase tests
On 16/12/2023 14.42, Nicholas Piggin wrote: This has a known failure on QEMU TCG machines where the decrementer interrupt is not lowered when the DEC wraps from -ve to +ve. Signed-off-by: Nicholas Piggin --- lib/powerpc/asm/ppc_asm.h | 1 + lib/powerpc/asm/processor.h | 22 +++ powerpc/Makefile.common | 1 + powerpc/smp.c | 22 --- powerpc/timebase.c | 328 powerpc/unittests.cfg | 8 + 6 files changed, 360 insertions(+), 22 deletions(-) create mode 100644 powerpc/timebase.c ... diff --git a/powerpc/timebase.c b/powerpc/timebase.c new file mode 100644 index ..4d80ea09 --- /dev/null +++ b/powerpc/timebase.c @@ -0,0 +1,328 @@ +/* + * Test Timebase + * + * Copyright 2017 Thomas Huth, Red Hat Inc. No, not really. Please update ;-) Thomas
Re: [kvm-unit-tests PATCH v5 25/29] powerpc: Add rtas stop-self support
On 16/12/2023 14.42, Nicholas Piggin wrote: In preparation for improved SMP support, add stop-self support to the harness. This is non-trivial because it requires an unlocked rtas call: a CPU can't be holding a spin lock when it goes offline or it will deadlock other CPUs. rtas permits stop-self to be called without serialising all other rtas operations. Signed-off-by: Nicholas Piggin --- lib/powerpc/asm/rtas.h | 2 ++ lib/powerpc/rtas.c | 78 +- 2 files changed, 64 insertions(+), 16 deletions(-) ... +void rtas_stop_self(void) +{ + struct rtas_args args; + uint32_t token; + int ret; + + ret = rtas_token("stop-self", &token); + if (ret) { + puts("RTAS stop-self not available\n"); + return; + } + + ret = rtas_call_unlocked(&args, token, 0, 1, NULL); + printf("RTAS stop-self returnd %d\n", ret); s/returnd/returned/ +} With the typo fixed: Reviewed-by: Thomas Huth
Re: [kvm-unit-tests PATCH v5 24/29] powerpc: interrupt tests
On 16/12/2023 14.42, Nicholas Piggin wrote: Add basic testing of various kinds of interrupts, machine check, page fault, illegal, decrementer, trace, syscall, etc. This has a known failure on QEMU TCG pseries machines where MSR[ME] can be incorrectly set to 0. Signed-off-by: Nicholas Piggin --- lib/powerpc/asm/ppc_asm.h | 21 +- powerpc/Makefile.common | 3 +- powerpc/interrupts.c | 422 ++ powerpc/unittests.cfg | 3 + 4 files changed, 445 insertions(+), 4 deletions(-) create mode 100644 powerpc/interrupts.c diff --git a/lib/powerpc/asm/ppc_asm.h b/lib/powerpc/asm/ppc_asm.h index ef2d91dd..778e78ee 100644 --- a/lib/powerpc/asm/ppc_asm.h +++ b/lib/powerpc/asm/ppc_asm.h @@ -35,17 +35,32 @@ #endif /* __BYTE_ORDER__ */ +#define SPR_DSISR 0x012 +#define SPR_DAR0x013 +#define SPR_DEC0x016 +#define SPR_SRR0 0x01A +#define SPR_SRR1 0x01B +#define SPR_FSCR 0x099 +#define FSCR_PREFIX 0x2000 +#define SPR_HDEC 0x136 #define SPR_HSRR0 0x13A #define SPR_HSRR1 0x13B +#define SPR_LPCR 0x13E +#define LPCR_HDICE 0x1UL +#define SPR_HEIR 0x153 +#define SPR_SIAR 0x31C /* Machine State Register definitions: */ #define MSR_LE_BIT0 #define MSR_EE_BIT15 /* External Interrupts Enable */ #define MSR_HV_BIT60 /* Hypervisor mode */ #define MSR_SF_BIT63 /* 64-bit mode */ -#define MSR_ME 0x1000ULL -#define SPR_HSRR0 0x13A -#define SPR_HSRR1 0x13B +#define MSR_DR 0x0010ULL +#define MSR_IR 0x0020ULL +#define MSR_BE 0x0200ULL /* Branch Trace Enable */ +#define MSR_SE 0x0400ULL /* Single Step Enable */ +#define MSR_EE 0x8000ULL +#define MSR_ME 0x1000ULL #endif /* _ASMPOWERPC_PPC_ASM_H */ diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common index a7af225b..b340a53b 100644 --- a/powerpc/Makefile.common +++ b/powerpc/Makefile.common @@ -11,7 +11,8 @@ tests-common = \ $(TEST_DIR)/rtas.elf \ $(TEST_DIR)/emulator.elf \ $(TEST_DIR)/tm.elf \ - $(TEST_DIR)/sprs.elf + $(TEST_DIR)/sprs.elf \ + $(TEST_DIR)/interrupts.elf tests-all = $(tests-common) $(tests) all: directories $(TEST_DIR)/boot_rom.bin $(tests-all) diff --git a/powerpc/interrupts.c b/powerpc/interrupts.c new file mode 100644 index ..3217b15e --- /dev/null +++ b/powerpc/interrupts.c @@ -0,0 +1,422 @@ +/* + * Test interrupts + * + * This work is licensed under the terms of the GNU LGPL, version 2. + */ +#include +#include +#include +#include +#include +#include +#include +#include + +#define SPR_LPCR 0x13E +#define LPCR_HDICE 0x1UL +#define SPR_DEC0x016 +#define SPR_HDEC 0x136 + +#define MSR_DR 0x0010ULL +#define MSR_IR 0x0020ULL +#define MSR_EE 0x8000ULL +#define MSR_ME 0x1000ULL Why don't you use the definitions from ppc_asm.h above? +static bool cpu_has_heir(void) +{ + uint32_t pvr = mfspr(287); /* Processor Version Register */ + + if (!machine_is_powernv()) + return false; + + /* POWER6 has HEIR, but QEMU powernv support does not go that far */ + switch (pvr >> 16) { + case 0x4b: /* POWER8E */ + case 0x4c: /* POWER8NVL */ + case 0x4d: /* POWER8 */ + case 0x4e: /* POWER9 */ + case 0x80: /* POWER10 */ I'd suggest to introduce some #defines for those PVR values instead of using magic numbers all over the place? + return true; + default: + return false; + } +} + +static bool cpu_has_prefix(void) +{ + uint32_t pvr = mfspr(287); /* Processor Version Register */ + switch (pvr >> 16) { + case 0x80: /* POWER10 */ + return true; + default: + return false; + } +} + +static bool cpu_has_lev_in_srr1(void) +{ + uint32_t pvr = mfspr(287); /* Processor Version Register */ + switch (pvr >> 16) { + case 0x80: /* POWER10 */ + return true; + default: + return false; + } +} + +static bool regs_is_prefix(volatile struct pt_regs *regs) +{ + return (regs->msr >> (63-34)) & 1; You introduced a bunch of new #define MSR_xx statements ... why not for this one, too? +} + +static void regs_advance_insn(struct pt_regs *regs) +{ + if (regs_is_prefix(regs)) + regs->nip += 8; + else + regs->nip += 4; +} + +static volatile bool got_interrupt; +static volatile struct pt_regs recorded_regs; + +static void mce_handler(struct pt_regs *regs, void *opaque) +{ + got_interrupt = true; + m
Re: [kvm-unit-tests PATCH v5 22/29] powerpc: Fix emulator illegal instruction test for powernv
On 16/12/2023 14.42, Nicholas Piggin wrote: Illegal instructions cause 0xe40 (HEAI) interrupts rather than program interrupts. Signed-off-by: Nicholas Piggin --- powerpc/emulator.c | 19 ++- 1 file changed, 18 insertions(+), 1 deletion(-) Acked-by: Thomas Huth
Re: [kvm-unit-tests PATCH v5 20/29] scripts: Accommodate powerpc powernv machine differences
On 16/12/2023 14.42, Nicholas Piggin wrote: The QEMU powerpc powernv machine has minor differences that must be accommodated for in output parsing: - Summary parsing must search more lines of output for the summary line, to accommodate OPAL message on shutdown. - Premature failure testing must tolerate case differences in kernel load error message. Signed-off-by: Nicholas Piggin --- scripts/runtime.bash | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) Acked-by: Thomas Huth