diff mbox series

[v3,3/8] KVM: selftests: Move setting a vCPU's entry point to a dedicated API

Message ID 20240208204844.119326-4-thuth@redhat.com (mailing list archive)
State New, archived
Headers show
Series Use TAP in some more x86 KVM selftests | expand

Commit Message

Thomas Huth Feb. 8, 2024, 8:48 p.m. UTC
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.

Signed-off-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 .../selftests/kvm/include/kvm_util_base.h     | 11 +++++----
 .../selftests/kvm/lib/aarch64/processor.c     | 23 ++++++++++++++-----
 .../selftests/kvm/lib/riscv/processor.c       |  9 +++++---
 .../selftests/kvm/lib/s390x/processor.c       | 13 ++++++-----
 .../selftests/kvm/lib/x86_64/processor.c      | 13 ++++++++---
 5 files changed, 47 insertions(+), 22 deletions(-)

Comments

Mark Brown Feb. 28, 2024, 7:04 p.m. UTC | #1
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
Sean Christopherson Feb. 28, 2024, 7:29 p.m. UTC | #2
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
Sean Christopherson Feb. 28, 2024, 9:19 p.m. UTC | #3
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
Oliver Upton Feb. 28, 2024, 9:29 p.m. UTC | #4
+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.
Oliver Upton Feb. 28, 2024, 9:34 p.m. UTC | #5
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
Sean Christopherson Feb. 28, 2024, 9:34 p.m. UTC | #6
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).
Mark Brown Feb. 28, 2024, 9:38 p.m. UTC | #7
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).
Raghavendra Rao Ananta Feb. 28, 2024, 11 p.m. UTC | #8
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/
Oliver Upton Feb. 29, 2024, 6:34 a.m. UTC | #9
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.
Mark Brown Feb. 29, 2024, 1:12 p.m. UTC | #10
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 mbox series

Patch

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, &regs);
+	regs.rip = (unsigned long) guest_code;
+	vcpu_regs_set(vcpu, &regs);
+}
+
+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, &regs);
 	regs.rflags = regs.rflags | 0x2;
 	regs.rsp = stack_vaddr;
-	regs.rip = (unsigned long) guest_code;
 	vcpu_regs_set(vcpu, &regs);
 
 	/* Setup the MP state */