Message ID | 20240208204844.119326-4-thuth@redhat.com |
---|---|
State | New |
Headers | show |
Series | Use TAP in some more x86 KVM selftests | expand |
On Thu, Feb 08, 2024 at 09:48:39PM +0100, Thomas Huth wrote: > From: Sean Christopherson <seanjc@google.com> > > Extract the code to set a vCPU's entry point out of vm_arch_vcpu_add() and > into a new API, vcpu_arch_set_entry_point(). Providing a separate API > will allow creating a KVM selftests hardness that can handle tests that > use different entry points for sub-tests, whereas *requiring* the entry > point to be specified at vCPU creation makes it difficult to create a > generic harness, e.g. the boilerplate setup/teardown can't easily create > and destroy the VM and vCPUs. With today's -next I'm seeing most of the KVM selftests failing on an arm64 defconfig with: # ==== Test Assertion Failure ==== # include/kvm_util_base.h:677: !ret # pid=735 tid=735 errno=9 - Bad file descriptor # 1 0x0000000000410937: vcpu_set_reg at kvm_util_base.h:677 (discriminator 4) # 2 (inlined by) vcpu_arch_set_entry_point at processor.c:370 (discriminator 4) # 3 0x0000000000407bab: vm_vcpu_add at kvm_util_base.h:981 # 4 (inlined by) __vm_create_with_vcpus at kvm_util.c:419 # 5 (inlined by) __vm_create_shape_with_one_vcpu at kvm_util.c:432 # 6 0x000000000040187b: __vm_create_with_one_vcpu at kvm_util_base.h:892 # 7 (inlined by) vm_create_with_one_vcpu at kvm_util_base.h:899 # 8 (inlined by) main at aarch32_id_regs.c:158 # 9 0x0000007fbcbe6dc3: ?? ??:0 # 10 0x0000007fbcbe6e97: ?? ??:0 # 11 0x0000000000401f2f: _start at ??:? # KVM_SET_ONE_REG failed, rc: -1 errno: 9 (Bad file descriptor) and a bisect pointed to this commit which does look plausibly relevant. Note that while this was bisected with plain arm64 defconfig and the KVM selftests fragment was not enabled, but enabling the KVM fragment gave the same result as would be expected based on the options enabled by the fragment. We're also seeing an alternative failure pattern where the tests segfault when run in a different environment, I'm also tracking that down but I suspect these are the same issue. A full log from a sample failing run can be seen here. https://lava.sirena.org.uk/scheduler/job/645026#L1581 Bisect log: git bisect start # good: [75d8cf735082728a5dfb7e46926ee184851cc519] Merge branch 'for-linux-next-fixes' of git://anongit.freedesktop.org/drm/drm-misc git bisect good 75d8cf735082728a5dfb7e46926ee184851cc519 # bad: [20af1ca418d2c0b11bc2a1fe8c0c88f67bcc2a7e] Add linux-next specific files for 20240228 git bisect bad 20af1ca418d2c0b11bc2a1fe8c0c88f67bcc2a7e # good: [1322f1801e59dddce10591d602d246c1bf49990c] Merge branch 'main' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git git bisect good 1322f1801e59dddce10591d602d246c1bf49990c # good: [f996a1cab1c3547a0bd2edf0daa7a71eddec9b58] Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/lsm.git git bisect good f996a1cab1c3547a0bd2edf0daa7a71eddec9b58 # bad: [22e19d7b30a88dc9e7b315935f44fb2a6c6bf7bf] Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/westeri/thunderbolt.git git bisect bad 22e19d7b30a88dc9e7b315935f44fb2a6c6bf7bf # good: [f9ad77051d5d45000848e87650a382995adf7e50] Merge branch 'master' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git git bisect good f9ad77051d5d45000848e87650a382995adf7e50 # bad: [6e9a1d8a249374b0c8ff9472f30f160c98881519] Merge branch 'next' of https://github.com/kvm-x86/linux.git git bisect bad 6e9a1d8a249374b0c8ff9472f30f160c98881519 # bad: [f3ac6b5aec49c3f8ced623802ee9efa6484263eb] Merge branch 'xen' git bisect bad f3ac6b5aec49c3f8ced623802ee9efa6484263eb # good: [938ccbf4327f38cec365986136e349486ddbb005] Merge branch 'pmu' git bisect good 938ccbf4327f38cec365986136e349486ddbb005 # bad: [f3750b0c7f6e48b0adfb9bd2419de4a3c604ca68] KVM: selftests: Add a basic SEV-ES smoke test git bisect bad f3750b0c7f6e48b0adfb9bd2419de4a3c604ca68 # bad: [992178c7219caa0bcdaa5c0ce59615b12da21662] KVM: selftests: Add a macro to define a test with one vcpu git bisect bad 992178c7219caa0bcdaa5c0ce59615b12da21662 # good: [71cd774ad2f98d4c78bc868e7e55397810be3540] KVM: s390: move s390-specific structs to uapi/asm/kvm.h git bisect good 71cd774ad2f98d4c78bc868e7e55397810be3540 # good: [db7d6fbc10447090bab8691a907a7c383ec66f58] KVM: remove unnecessary #ifdef git bisect good db7d6fbc10447090bab8691a907a7c383ec66f58 # good: [221d65449453846bbf6801d0ecf7dfdf4f413ad9] KVM: selftests: x86: sync_regs_test: Get regs structure before modifying it git bisect good 221d65449453846bbf6801d0ecf7dfdf4f413ad9 # bad: [8ef192609f14272b7bd6fc3a553ebe02d1133cd0] KVM: selftests: Move setting a vCPU's entry point to a dedicated API git bisect bad 8ef192609f14272b7bd6fc3a553ebe02d1133cd0 # first bad commit: [8ef192609f14272b7bd6fc3a553ebe02d1133cd0] KVM: selftests: Move setting a vCPU's entry point to a dedicated API
On Wed, Feb 28, 2024, Mark Brown wrote: > On Thu, Feb 08, 2024 at 09:48:39PM +0100, Thomas Huth wrote: > > From: Sean Christopherson <seanjc@google.com> > > > > Extract the code to set a vCPU's entry point out of vm_arch_vcpu_add() and > > into a new API, vcpu_arch_set_entry_point(). Providing a separate API > > will allow creating a KVM selftests hardness that can handle tests that > > use different entry points for sub-tests, whereas *requiring* the entry > > point to be specified at vCPU creation makes it difficult to create a > > generic harness, e.g. the boilerplate setup/teardown can't easily create > > and destroy the VM and vCPUs. > > With today's -next I'm seeing most of the KVM selftests failing on an > arm64 defconfig with: > > # ==== Test Assertion Failure ==== > # include/kvm_util_base.h:677: !ret > # pid=735 tid=735 errno=9 - Bad file descriptor > # 1 0x0000000000410937: vcpu_set_reg at kvm_util_base.h:677 (discriminator 4) > # 2 (inlined by) vcpu_arch_set_entry_point at processor.c:370 (discriminator 4) > # 3 0x0000000000407bab: vm_vcpu_add at kvm_util_base.h:981 > # 4 (inlined by) __vm_create_with_vcpus at kvm_util.c:419 > # 5 (inlined by) __vm_create_shape_with_one_vcpu at kvm_util.c:432 > # 6 0x000000000040187b: __vm_create_with_one_vcpu at kvm_util_base.h:892 > # 7 (inlined by) vm_create_with_one_vcpu at kvm_util_base.h:899 > # 8 (inlined by) main at aarch32_id_regs.c:158 > # 9 0x0000007fbcbe6dc3: ?? ??:0 > # 10 0x0000007fbcbe6e97: ?? ??:0 > # 11 0x0000000000401f2f: _start at ??:? > # KVM_SET_ONE_REG failed, rc: -1 errno: 9 (Bad file descriptor) > > and a bisect pointed to this commit which does look plausibly relevant. > > Note that while this was bisected with plain arm64 defconfig and the KVM > selftests fragment was not enabled, but enabling the KVM fragment gave > the same result as would be expected based on the options enabled by the > fragment. We're also seeing an alternative failure pattern where the > tests segfault when run in a different environment, I'm also tracking > that down but I suspect these are the same issue. Gah, my bad, I should have at least tested on ARM since I have easy access to such hardware. If I can't figure out what's going wrong in the next few hours, I'll drop this series and we can try again for 6.10. Sorry :-/ > A full log from a sample failing run can be seen here. > > https://lava.sirena.org.uk/scheduler/job/645026#L1581 > > Bisect log: > > git bisect start > # good: [75d8cf735082728a5dfb7e46926ee184851cc519] Merge branch 'for-linux-next-fixes' of git://anongit.freedesktop.org/drm/drm-misc > git bisect good 75d8cf735082728a5dfb7e46926ee184851cc519 > # bad: [20af1ca418d2c0b11bc2a1fe8c0c88f67bcc2a7e] Add linux-next specific files for 20240228 > git bisect bad 20af1ca418d2c0b11bc2a1fe8c0c88f67bcc2a7e > # good: [1322f1801e59dddce10591d602d246c1bf49990c] Merge branch 'main' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git > git bisect good 1322f1801e59dddce10591d602d246c1bf49990c > # good: [f996a1cab1c3547a0bd2edf0daa7a71eddec9b58] Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/lsm.git > git bisect good f996a1cab1c3547a0bd2edf0daa7a71eddec9b58 > # bad: [22e19d7b30a88dc9e7b315935f44fb2a6c6bf7bf] Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/westeri/thunderbolt.git > git bisect bad 22e19d7b30a88dc9e7b315935f44fb2a6c6bf7bf > # good: [f9ad77051d5d45000848e87650a382995adf7e50] Merge branch 'master' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git > git bisect good f9ad77051d5d45000848e87650a382995adf7e50 > # bad: [6e9a1d8a249374b0c8ff9472f30f160c98881519] Merge branch 'next' of https://github.com/kvm-x86/linux.git > git bisect bad 6e9a1d8a249374b0c8ff9472f30f160c98881519 > # bad: [f3ac6b5aec49c3f8ced623802ee9efa6484263eb] Merge branch 'xen' > git bisect bad f3ac6b5aec49c3f8ced623802ee9efa6484263eb > # good: [938ccbf4327f38cec365986136e349486ddbb005] Merge branch 'pmu' > git bisect good 938ccbf4327f38cec365986136e349486ddbb005 > # bad: [f3750b0c7f6e48b0adfb9bd2419de4a3c604ca68] KVM: selftests: Add a basic SEV-ES smoke test > git bisect bad f3750b0c7f6e48b0adfb9bd2419de4a3c604ca68 > # bad: [992178c7219caa0bcdaa5c0ce59615b12da21662] KVM: selftests: Add a macro to define a test with one vcpu > git bisect bad 992178c7219caa0bcdaa5c0ce59615b12da21662 > # good: [71cd774ad2f98d4c78bc868e7e55397810be3540] KVM: s390: move s390-specific structs to uapi/asm/kvm.h > git bisect good 71cd774ad2f98d4c78bc868e7e55397810be3540 > # good: [db7d6fbc10447090bab8691a907a7c383ec66f58] KVM: remove unnecessary #ifdef > git bisect good db7d6fbc10447090bab8691a907a7c383ec66f58 > # good: [221d65449453846bbf6801d0ecf7dfdf4f413ad9] KVM: selftests: x86: sync_regs_test: Get regs structure before modifying it > git bisect good 221d65449453846bbf6801d0ecf7dfdf4f413ad9 > # bad: [8ef192609f14272b7bd6fc3a553ebe02d1133cd0] KVM: selftests: Move setting a vCPU's entry point to a dedicated API > git bisect bad 8ef192609f14272b7bd6fc3a553ebe02d1133cd0 > # first bad commit: [8ef192609f14272b7bd6fc3a553ebe02d1133cd0] KVM: selftests: Move setting a vCPU's entry point to a dedicated API
Oliver and/or Marc, question for y'all towards the bottom. On Wed, Feb 28, 2024, Sean Christopherson wrote: > On Wed, Feb 28, 2024, Mark Brown wrote: > > On Thu, Feb 08, 2024 at 09:48:39PM +0100, Thomas Huth wrote: > > > From: Sean Christopherson <seanjc@google.com> > > > > > > Extract the code to set a vCPU's entry point out of vm_arch_vcpu_add() and > > > into a new API, vcpu_arch_set_entry_point(). Providing a separate API > > > will allow creating a KVM selftests hardness that can handle tests that > > > use different entry points for sub-tests, whereas *requiring* the entry > > > point to be specified at vCPU creation makes it difficult to create a > > > generic harness, e.g. the boilerplate setup/teardown can't easily create > > > and destroy the VM and vCPUs. > > > > With today's -next I'm seeing most of the KVM selftests failing on an > > arm64 defconfig with: > > > > # ==== Test Assertion Failure ==== > > # include/kvm_util_base.h:677: !ret > > # pid=735 tid=735 errno=9 - Bad file descriptor > > # 1 0x0000000000410937: vcpu_set_reg at kvm_util_base.h:677 (discriminator 4) > > # 2 (inlined by) vcpu_arch_set_entry_point at processor.c:370 (discriminator 4) > > # 3 0x0000000000407bab: vm_vcpu_add at kvm_util_base.h:981 > > # 4 (inlined by) __vm_create_with_vcpus at kvm_util.c:419 > > # 5 (inlined by) __vm_create_shape_with_one_vcpu at kvm_util.c:432 > > # 6 0x000000000040187b: __vm_create_with_one_vcpu at kvm_util_base.h:892 > > # 7 (inlined by) vm_create_with_one_vcpu at kvm_util_base.h:899 > > # 8 (inlined by) main at aarch32_id_regs.c:158 > > # 9 0x0000007fbcbe6dc3: ?? ??:0 > > # 10 0x0000007fbcbe6e97: ?? ??:0 > > # 11 0x0000000000401f2f: _start at ??:? > > # KVM_SET_ONE_REG failed, rc: -1 errno: 9 (Bad file descriptor) > > > > and a bisect pointed to this commit which does look plausibly relevant. > > > > Note that while this was bisected with plain arm64 defconfig and the KVM > > selftests fragment was not enabled, but enabling the KVM fragment gave > > the same result as would be expected based on the options enabled by the > > fragment. We're also seeing an alternative failure pattern where the > > tests segfault when run in a different environment, I'm also tracking > > that down but I suspect these are the same issue. > > Gah, my bad, I should have at least tested on ARM since I have easy access to > such hardware. If I can't figure out what's going wrong in the next few hours, > I'll drop this series and we can try again for 6.10. > > Sorry :-/ /facepalm The inner helper doesn't return the vCPU, and by dumb (bad) luck, selftests end up trying to use fd=0. diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c index ed4ab29f4fad..a9eb17295be4 100644 --- a/tools/testing/selftests/kvm/lib/aarch64/processor.c +++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c @@ -386,6 +386,7 @@ static struct kvm_vcpu *__aarch64_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id, aarch64_vcpu_setup(vcpu, init); vcpu_set_reg(vcpu, ARM64_CORE_REG(sp_el1), stack_vaddr + stack_size); + return vcpu; } struct kvm_vcpu *aarch64_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id, I'll squash the above and force push. In my defense, I would have caught this when build-testing, as the compiler does warn... lib/aarch64/processor.c -o /usr/local/google/home/seanjc/go/src/kernel.org/nox/tools/testing/selftests/kvm/lib/aarch64/processor.o lib/aarch64/processor.c: In function ‘__aarch64_vcpu_add’: lib/aarch64/processor.c:389:1: warning: no return statement in function returning non-void [-Wreturn-type] 389 | } | ^ At top level: cc1: note: unrecognized command-line option ‘-Wno-gnu-variable-sized-type-not-at-end’ may have been intended to silence earlier diagnostics but due to a different issue that is fixed in the kvm-arm tree[*], but not in mine, I built without -Werror and didn't see the new warn in the sea of GUEST_PRINTF warnings. Ugh, and I still can't enable -Werror, because there are unused functions in aarch64/vpmu_counter_access.c aarch64/vpmu_counter_access.c:96:20: error: unused function 'enable_counter' [-Werror,-Wunused-function] static inline void enable_counter(int idx) ^ aarch64/vpmu_counter_access.c:104:20: error: unused function 'disable_counter' [-Werror,-Wunused-function] static inline void disable_counter(int idx) ^ 2 errors generated. make: *** [Makefile:278: /usr/local/google/home/seanjc/go/src/kernel.org/nox/tools/testing/selftests/kvm/aarch64/vpmu_counter_access.o] Error 1 make: *** Waiting for unfinished jobs.... Commit 49f31cff9c533d264659356b90445023b04e10fb failed to build with 'make-clang make-arm make -j128'. Oliver/Marc, any thoughts on how you want to fix the unused function warnings? As evidenced by this goof, being able to compile with -Werror is super helpful. And another question: is there any reason to not force -Werror for selftests? [*] https://lore.kernel.org/all/20240202234603.366925-1-seanjc@google.com
+cc Raghavendra Hey, On Wed, Feb 28, 2024 at 01:19:48PM -0800, Sean Christopherson wrote: > but due to a different issue that is fixed in the kvm-arm tree[*], but not in mine, > I built without -Werror and didn't see the new warn in the sea of GUEST_PRINTF > warnings. > > Ugh, and I still can't enable -Werror, because there are unused functions in > aarch64/vpmu_counter_access.c > > aarch64/vpmu_counter_access.c:96:20: error: unused function 'enable_counter' [-Werror,-Wunused-function] > static inline void enable_counter(int idx) > ^ > aarch64/vpmu_counter_access.c:104:20: error: unused function 'disable_counter' [-Werror,-Wunused-function] > static inline void disable_counter(int idx) > ^ > 2 errors generated. > make: *** [Makefile:278: /usr/local/google/home/seanjc/go/src/kernel.org/nox/tools/testing/selftests/kvm/aarch64/vpmu_counter_access.o] Error 1 > make: *** Waiting for unfinished jobs.... > > Commit 49f31cff9c533d264659356b90445023b04e10fb failed to build with 'make-clang make-arm make -j128'. > > Oliver/Marc, any thoughts on how you want to fix the unused function warnings? > As evidenced by this goof, being able to compile with -Werror is super helpful. Are these the only remaining warnings we have in the arm64 selftests build? Faster than me paging this test back in: Raghu, are we missing any test cases upstream that these helpers were intended for? If no, mind sending a patch to get rid of them? > And another question: is there any reason to not force -Werror for selftests? Nothing comes to mind. We need to bite the bullet and make the switch. There might be breakage, but we can certainly handle that.
I really should fix the CC list _before_ drafting a reply... On Wed, Feb 28, 2024 at 09:29:56PM +0000, Oliver Upton wrote: > +cc Raghavendra See below :) > Hey, > > On Wed, Feb 28, 2024 at 01:19:48PM -0800, Sean Christopherson wrote: > > but due to a different issue that is fixed in the kvm-arm tree[*], but not in mine, > > I built without -Werror and didn't see the new warn in the sea of GUEST_PRINTF > > warnings. > > > > Ugh, and I still can't enable -Werror, because there are unused functions in > > aarch64/vpmu_counter_access.c > > > > aarch64/vpmu_counter_access.c:96:20: error: unused function 'enable_counter' [-Werror,-Wunused-function] > > static inline void enable_counter(int idx) > > ^ > > aarch64/vpmu_counter_access.c:104:20: error: unused function 'disable_counter' [-Werror,-Wunused-function] > > static inline void disable_counter(int idx) > > ^ > > 2 errors generated. > > make: *** [Makefile:278: /usr/local/google/home/seanjc/go/src/kernel.org/nox/tools/testing/selftests/kvm/aarch64/vpmu_counter_access.o] Error 1 > > make: *** Waiting for unfinished jobs.... > > > > Commit 49f31cff9c533d264659356b90445023b04e10fb failed to build with 'make-clang make-arm make -j128'. > > > > Oliver/Marc, any thoughts on how you want to fix the unused function warnings? > > As evidenced by this goof, being able to compile with -Werror is super helpful. > > Are these the only remaining warnings we have in the arm64 selftests > build? > > Faster than me paging this test back in: Raghu, are we missing any test > cases upstream that these helpers were intended for? If no, mind sending > a patch to get rid of them? > > > And another question: is there any reason to not force -Werror for selftests? > > Nothing comes to mind. We need to bite the bullet and make the switch. > There might be breakage, but we can certainly handle that. > > -- > Thanks, > Oliver
On Wed, Feb 28, 2024, Oliver Upton wrote: > +cc Raghavendra > > Hey, > > On Wed, Feb 28, 2024 at 01:19:48PM -0800, Sean Christopherson wrote: > > but due to a different issue that is fixed in the kvm-arm tree[*], but not in mine, > > I built without -Werror and didn't see the new warn in the sea of GUEST_PRINTF > > warnings. > > > > Ugh, and I still can't enable -Werror, because there are unused functions in > > aarch64/vpmu_counter_access.c > > > > aarch64/vpmu_counter_access.c:96:20: error: unused function 'enable_counter' [-Werror,-Wunused-function] > > static inline void enable_counter(int idx) > > ^ > > aarch64/vpmu_counter_access.c:104:20: error: unused function 'disable_counter' [-Werror,-Wunused-function] > > static inline void disable_counter(int idx) > > ^ > > 2 errors generated. > > make: *** [Makefile:278: /usr/local/google/home/seanjc/go/src/kernel.org/nox/tools/testing/selftests/kvm/aarch64/vpmu_counter_access.o] Error 1 > > make: *** Waiting for unfinished jobs.... > > > > Commit 49f31cff9c533d264659356b90445023b04e10fb failed to build with 'make-clang make-arm make -j128'. > > > > Oliver/Marc, any thoughts on how you want to fix the unused function warnings? > > As evidenced by this goof, being able to compile with -Werror is super helpful. > > Are these the only remaining warnings we have in the arm64 selftests > build? Yep, unless I've missed something, this is the only outstanding warning across all architectures that support selftests (sans LoongArch and PPC, which are pending).
On Wed, Feb 28, 2024 at 01:19:48PM -0800, Sean Christopherson wrote: > I'll squash the above and force push. Thanks, I'll give that a spin (likely tomorrow at this point). > And another question: is there any reason to not force -Werror for selftests? Enabling it by default can cause a bunch of fragility when people are running different compilers, there's things like all the different clang versions, and it's especially common to run into issues when you've got a shiny new compiler on a modern distro but need to go look at older kernels for some reason. You then get issues bisecting things since you get regions where the build was broken due to -Werror which may or may not be pointing at the runtime issue you were looking for (particularly likely to be hiding things if the build issue is in a different test).
Hey Oliver, +cc Shaoqin On Wed, Feb 28, 2024 at 1:30 PM Oliver Upton <oliver.upton@linux.dev> wrote: > > +cc Raghavendra > > Hey, > > On Wed, Feb 28, 2024 at 01:19:48PM -0800, Sean Christopherson wrote: > > but due to a different issue that is fixed in the kvm-arm tree[*], but not in mine, > > I built without -Werror and didn't see the new warn in the sea of GUEST_PRINTF > > warnings. > > > > Ugh, and I still can't enable -Werror, because there are unused functions in > > aarch64/vpmu_counter_access.c > > > > aarch64/vpmu_counter_access.c:96:20: error: unused function 'enable_counter' [-Werror,-Wunused-function] > > static inline void enable_counter(int idx) > > ^ > > aarch64/vpmu_counter_access.c:104:20: error: unused function 'disable_counter' [-Werror,-Wunused-function] > > static inline void disable_counter(int idx) > > ^ > > 2 errors generated. > > make: *** [Makefile:278: /usr/local/google/home/seanjc/go/src/kernel.org/nox/tools/testing/selftests/kvm/aarch64/vpmu_counter_access.o] Error 1 > > make: *** Waiting for unfinished jobs.... > > > > Commit 49f31cff9c533d264659356b90445023b04e10fb failed to build with 'make-clang make-arm make -j128'. > > > > Oliver/Marc, any thoughts on how you want to fix the unused function warnings? > > As evidenced by this goof, being able to compile with -Werror is super helpful. > > Are these the only remaining warnings we have in the arm64 selftests > build? > > Faster than me paging this test back in: Raghu, are we missing any test > cases upstream that these helpers were intended for? If no, mind sending > a patch to get rid of them? > I sent out a patch in the past to get rid of them [1], but Shaoqin is currently making an effort to (fix and) use them in their tests [2]. While we are still reviewing the series, we can apply [1] to unblock enabling -Werror and Shaqoqin can re-introduce the functions as needed. But, it's your call. Thank you. Raghavendra [1]: https://lore.kernel.org/lkml/d5cc3cf1-7b39-9ca3-adf2-224007c751fe@redhat.com/T/ [2]: https://lore.kernel.org/all/20240202025659.5065-3-shahuang@redhat.com/
On Wed, Feb 28, 2024 at 03:00:03PM -0800, Raghavendra Rao Ananta wrote: [...] > I sent out a patch in the past to get rid of them [1], but Shaoqin is > currently making an effort to (fix and) use them in their tests [2]. > While we are still reviewing the series, we can apply [1] to unblock > enabling -Werror and Shaqoqin can re-introduce the functions as > needed. But, it's your call. Thanks for the brief, now I remember :) Agreed, let's just delete these upstream. These accessors are simple anyway, and easy to re-review in Shaoqin's tests. Sean -- I'm going to pick up [1] and throw it on the branch with your cleanup.
On Wed, Feb 28, 2024 at 01:19:48PM -0800, Sean Christopherson wrote: > @@ -386,6 +386,7 @@ static struct kvm_vcpu *__aarch64_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id, > aarch64_vcpu_setup(vcpu, init); > > vcpu_set_reg(vcpu, ARM64_CORE_REG(sp_el1), stack_vaddr + stack_size); > + return vcpu; > } > > struct kvm_vcpu *aarch64_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id, > > I'll squash the above and force push. Confirmed that this fixes the originally reported issue.
diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h index 9e5afc472c142..a6e7738a8db73 100644 --- a/tools/testing/selftests/kvm/include/kvm_util_base.h +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h @@ -969,15 +969,18 @@ static inline void vcpu_dump(FILE *stream, struct kvm_vcpu *vcpu, * Input Args: * vm - Virtual Machine * vcpu_id - The id of the VCPU to add to the VM. - * guest_code - The vCPU's entry point */ -struct kvm_vcpu *vm_arch_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id, - void *guest_code); +struct kvm_vcpu *vm_arch_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id); +void vcpu_arch_set_entry_point(struct kvm_vcpu *vcpu, void *guest_code); static inline struct kvm_vcpu *vm_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id, void *guest_code) { - return vm_arch_vcpu_add(vm, vcpu_id, guest_code); + struct kvm_vcpu *vcpu = vm_arch_vcpu_add(vm, vcpu_id); + + vcpu_arch_set_entry_point(vcpu, guest_code); + + return vcpu; } /* Re-create a vCPU after restarting a VM, e.g. for state save/restore tests. */ diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c index 41c776b642c0c..05b6c40e3655f 100644 --- a/tools/testing/selftests/kvm/lib/aarch64/processor.c +++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c @@ -365,8 +365,13 @@ void vcpu_arch_dump(FILE *stream, struct kvm_vcpu *vcpu, uint8_t indent) indent, "", pstate, pc); } -struct kvm_vcpu *aarch64_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id, - struct kvm_vcpu_init *init, void *guest_code) +void vcpu_arch_set_entry_point(struct kvm_vcpu *vcpu, void *guest_code) +{ + vcpu_set_reg(vcpu, ARM64_CORE_REG(regs.pc), (uint64_t)guest_code); +} + +static struct kvm_vcpu *__aarch64_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id, + struct kvm_vcpu_init *init) { size_t stack_size; uint64_t stack_vaddr; @@ -381,15 +386,21 @@ struct kvm_vcpu *aarch64_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id, aarch64_vcpu_setup(vcpu, init); vcpu_set_reg(vcpu, ARM64_CORE_REG(sp_el1), stack_vaddr + stack_size); - vcpu_set_reg(vcpu, ARM64_CORE_REG(regs.pc), (uint64_t)guest_code); +} + +struct kvm_vcpu *aarch64_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id, + struct kvm_vcpu_init *init, void *guest_code) +{ + struct kvm_vcpu *vcpu = __aarch64_vcpu_add(vm, vcpu_id, init); + + vcpu_arch_set_entry_point(vcpu, guest_code); return vcpu; } -struct kvm_vcpu *vm_arch_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id, - void *guest_code) +struct kvm_vcpu *vm_arch_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id) { - return aarch64_vcpu_add(vm, vcpu_id, NULL, guest_code); + return __aarch64_vcpu_add(vm, vcpu_id, NULL); } void vcpu_args_set(struct kvm_vcpu *vcpu, unsigned int num, ...) diff --git a/tools/testing/selftests/kvm/lib/riscv/processor.c b/tools/testing/selftests/kvm/lib/riscv/processor.c index 7ca736fb41940..c993947f07823 100644 --- a/tools/testing/selftests/kvm/lib/riscv/processor.c +++ b/tools/testing/selftests/kvm/lib/riscv/processor.c @@ -277,8 +277,12 @@ static void __aligned(16) guest_unexp_trap(void) 0, 0, 0, 0, 0, 0); } -struct kvm_vcpu *vm_arch_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id, - void *guest_code) +void vcpu_arch_set_entry_point(struct kvm_vcpu *vcpu, void *guest_code) +{ + vcpu_set_reg(vcpu, RISCV_CORE_REG(regs.pc), (unsigned long)guest_code); +} + +struct kvm_vcpu *vm_arch_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id) { int r; size_t stack_size; @@ -312,7 +316,6 @@ struct kvm_vcpu *vm_arch_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id, /* Setup stack pointer and program counter of guest */ vcpu_set_reg(vcpu, RISCV_CORE_REG(regs.sp), stack_vaddr + stack_size); - vcpu_set_reg(vcpu, RISCV_CORE_REG(regs.pc), (unsigned long)guest_code); /* Setup default exception vector of guest */ vcpu_set_reg(vcpu, RISCV_GENERAL_CSR_REG(stvec), (unsigned long)guest_unexp_trap); diff --git a/tools/testing/selftests/kvm/lib/s390x/processor.c b/tools/testing/selftests/kvm/lib/s390x/processor.c index 15945121daf17..cd5301cc9788a 100644 --- a/tools/testing/selftests/kvm/lib/s390x/processor.c +++ b/tools/testing/selftests/kvm/lib/s390x/processor.c @@ -155,15 +155,18 @@ void virt_arch_dump(FILE *stream, struct kvm_vm *vm, uint8_t indent) virt_dump_region(stream, vm, indent, vm->pgd); } -struct kvm_vcpu *vm_arch_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id, - void *guest_code) +void vcpu_arch_set_entry_point(struct kvm_vcpu *vcpu, void *guest_code) +{ + vcpu->run->psw_addr = (uintptr_t)guest_code; +} + +struct kvm_vcpu *vm_arch_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id) { size_t stack_size = DEFAULT_STACK_PGS * getpagesize(); uint64_t stack_vaddr; struct kvm_regs regs; struct kvm_sregs sregs; struct kvm_vcpu *vcpu; - struct kvm_run *run; TEST_ASSERT(vm->page_size == 4096, "Unsupported page size: 0x%x", vm->page_size); @@ -184,9 +187,7 @@ struct kvm_vcpu *vm_arch_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id, sregs.crs[1] = vm->pgd | 0xf; /* Primary region table */ vcpu_sregs_set(vcpu, &sregs); - run = vcpu->run; - run->psw_mask = 0x0400000180000000ULL; /* DAT enabled + 64 bit mode */ - run->psw_addr = (uintptr_t)guest_code; + vcpu->run->psw_mask = 0x0400000180000000ULL; /* DAT enabled + 64 bit mode */ return vcpu; } diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c index d8288374078e4..b9b6cb730a088 100644 --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c @@ -562,8 +562,16 @@ void kvm_arch_vm_post_create(struct kvm_vm *vm) sync_global_to_guest(vm, host_cpu_is_amd); } -struct kvm_vcpu *vm_arch_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id, - void *guest_code) +void vcpu_arch_set_entry_point(struct kvm_vcpu *vcpu, void *guest_code) +{ + struct kvm_regs regs; + + vcpu_regs_get(vcpu, ®s); + regs.rip = (unsigned long) guest_code; + vcpu_regs_set(vcpu, ®s); +} + +struct kvm_vcpu *vm_arch_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id) { struct kvm_mp_state mp_state; struct kvm_regs regs; @@ -597,7 +605,6 @@ struct kvm_vcpu *vm_arch_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id, vcpu_regs_get(vcpu, ®s); regs.rflags = regs.rflags | 0x2; regs.rsp = stack_vaddr; - regs.rip = (unsigned long) guest_code; vcpu_regs_set(vcpu, ®s); /* Setup the MP state */