Message-ID: <Zd-jdAtI_C_d_fp4@google.com>
Date: Wed, 28 Feb 2024 13:19:48 -0800
From: Sean Christopherson <seanjc@...gle.com>
To: Mark Brown <broonie@...nel.org>
Cc: Thomas Huth <thuth@...hat.com>, kvm@...r.kernel.org, linux-kselftest@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Paolo Bonzini <pbonzini@...hat.com>, 
	Andrew Jones <ajones@...tanamicro.com>, Marc Zyngier <maz@...nel.org>, 
	Oliver Upton <oliver.upton@...ux.dev>, Aishwarya TCV <aishwarya.tcv@....com>
Subject: Re: [PATCH v3 3/8] 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@...gle.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_baseh: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

Powered by blists - more mailing lists