Re: [Patch v4 12/13] KVM: selftests: Make vCPU exit reason test assertion common.

From: Sean Christopherson
Date: Wed Feb 01 2023 - 18:25:06 EST

+all the other KVM selftests maintainers and reviewers

On Mon, Dec 12, 2022, Vipin Sharma wrote:
> Make TEST_ASSERT_KVM_EXIT_REASON() macro and replace all exit reason
> test assert statements with it.
>
> No functional changes intended.
>
> Suggested-by: David Matlack <dmatlack@xxxxxxxxxx>
> Signed-off-by: Vipin Sharma <vipinsh@xxxxxxxxxx>
> Reviewed-by: David Matlack <dmatlack@xxxxxxxxxx>
> ---
> .../testing/selftests/kvm/aarch64/psci_test.c | 4 +--
> .../testing/selftests/kvm/include/test_util.h | 10 ++++++++
> .../kvm/lib/s390x/diag318_test_handler.c | 3 +--
> .../selftests/kvm/s390x/sync_regs_test.c | 15 +++--------
> .../selftests/kvm/set_memory_region_test.c | 6 +----
> tools/testing/selftests/kvm/x86_64/amx_test.c | 8 +-----
> .../kvm/x86_64/cr4_cpuid_sync_test.c | 8 +-----
> .../testing/selftests/kvm/x86_64/debug_regs.c | 2 +-
> .../selftests/kvm/x86_64/flds_emulation.h | 5 +---
> .../selftests/kvm/x86_64/hyperv_clock.c | 7 +-----
> .../selftests/kvm/x86_64/hyperv_evmcs.c | 8 +-----
> .../selftests/kvm/x86_64/hyperv_features.c | 14 ++---------
> .../testing/selftests/kvm/x86_64/hyperv_ipi.c | 6 +----
> .../selftests/kvm/x86_64/hyperv_svm_test.c | 7 +-----
> .../selftests/kvm/x86_64/hyperv_tlb_flush.c | 14 ++---------
> .../selftests/kvm/x86_64/kvm_clock_test.c | 5 +---
> .../selftests/kvm/x86_64/kvm_pv_test.c | 5 +---
> .../selftests/kvm/x86_64/monitor_mwait_test.c | 9 +------
> .../kvm/x86_64/nested_exceptions_test.c | 5 +---
> .../selftests/kvm/x86_64/platform_info_test.c | 14 +++--------
> .../kvm/x86_64/pmu_event_filter_test.c | 6 +----
> tools/testing/selftests/kvm/x86_64/smm_test.c | 9 +------
> .../testing/selftests/kvm/x86_64/state_test.c | 8 +-----
> .../selftests/kvm/x86_64/svm_int_ctl_test.c | 8 +-----
> .../kvm/x86_64/svm_nested_shutdown_test.c | 7 +-----
> .../kvm/x86_64/svm_nested_soft_inject_test.c | 6 +----
> .../selftests/kvm/x86_64/svm_vmcall_test.c | 6 +----
> .../selftests/kvm/x86_64/sync_regs_test.c | 25 ++++---------------
> .../kvm/x86_64/triple_fault_event_test.c | 9 ++-----
> .../selftests/kvm/x86_64/tsc_scaling_sync.c | 6 +----
> .../kvm/x86_64/ucna_injection_test.c | 22 +++-------------
> .../selftests/kvm/x86_64/userspace_io_test.c | 6 +----
> .../kvm/x86_64/userspace_msr_exit_test.c | 22 +++-------------
> .../kvm/x86_64/vmx_apic_access_test.c | 11 ++------
> .../kvm/x86_64/vmx_close_while_nested_test.c | 5 +---
> .../selftests/kvm/x86_64/vmx_dirty_log_test.c | 7 +-----
> .../vmx_exception_with_invalid_guest_state.c | 4 +--
> .../x86_64/vmx_invalid_nested_guest_state.c | 4 +--
> .../kvm/x86_64/vmx_nested_tsc_scaling_test.c | 6 +----
> .../kvm/x86_64/vmx_preemption_timer_test.c | 8 +-----
> .../kvm/x86_64/vmx_tsc_adjust_test.c | 6 +----
> .../selftests/kvm/x86_64/xapic_ipi_test.c | 6 +----
> .../selftests/kvm/x86_64/xen_shinfo_test.c | 7 +-----
> .../selftests/kvm/x86_64/xen_vmcall_test.c | 5 +---
> 44 files changed, 71 insertions(+), 293 deletions(-)

I love the cleanup, but in the future, please don't squeeze KVM-wide changes in
the middle of an otherwise arch-specific series unless it's absolutely necessary.
I get why you added the macro before copy-pasting more code into a new test, but
the unfortunate side effect is that complicates grabbing the entire series.

And incorporate ./scripts/get_maintainer.pl into your workflow, the other KVM
selftests folks need to be in the loop for these types of changes.

> diff --git a/tools/testing/selftests/kvm/include/test_util.h b/tools/testing/selftests/kvm/include/test_util.h
> index 80d6416f3012..3f15f216d2a6 100644
> --- a/tools/testing/selftests/kvm/include/test_util.h
> +++ b/tools/testing/selftests/kvm/include/test_util.h
> @@ -63,6 +63,16 @@ void test_assert(bool exp, const char *exp_str,
> #a, #b, #a, (unsigned long) __a, #b, (unsigned long) __b); \
> } while (0)
>
> +#define TEST_ASSERT_KVM_EXIT_REASON(vcpu, expected_exit_reason) \
> +({ \

Unless the macro needs to "return" a value, do-while(0) is generally preferred.

> + __u32 exit_reason = (vcpu)->run->exit_reason; \
> + \
> + TEST_ASSERT(exit_reason == (expected_exit_reason), \
> + "Unexpected exit reason: %u (%s)", \

This "needs" to opportunistically enhance the message to spit out the expected
reason, and to clarify that it's a KVM exit reason. In the open coded form, the
expected reason is _usually_ captured in the assertion, but that's not guaranteed,
e.g. if it's not hardcoded. But with the common code, the expected exit reason
will generally get resolved into its literal, which isn't very human friendly.

And even when it is provided, I find it annoying to have to search back a few
lines to understand what failed.

E.g. the new macro yields "x86_64/hyperv_evmcs.c:269: exit_reason == (2)".

> + exit_reason, \
> + exit_reason_str(exit_reason)); \

No need to put these on separate lines.

How about this?

#define TEST_ASSERT_KVM_EXIT_REASON(vcpu, expected) \
do { \
__u32 exit_reason = (vcpu)->run->exit_reason; \
\
TEST_ASSERT(exit_reason == (expected), \
"Wanted KVM exit reason: %u (%s), got: %u (%s)", \
expected, exit_reason_str(expected), \
exit_reason, exit_reason_str(exit_reason)); \
} while (0)

which yields errors like:

==== Test Assertion Failure ====
x86_64/hyperv_extended_hypercalls.c:71: exit_reason == (2)
pid=108104 tid=108104 errno=0 - Success
1 0x0000000000401793: main at hyperv_extended_hypercalls.c:71
2 0x00000000004148b3: __libc_start_call_main at libc-start.o:?
3 0x0000000000415eff: __libc_start_main_impl at ??:?
4 0x00000000004018f0: _start at ??:?
Wanted KVM exit reason: 2 (IO), got: 27 (HYPERV)

On a related topic, exit_reason_str() is a bit stale and also annoying to update.
Can you fold in the below when you send v2 of this patch? And then if you're
feeling ambititous, add another patch to update the array?

--
From: Sean Christopherson <seanjc@xxxxxxxxxx>
Date: Wed, 1 Feb 2023 23:17:19 +0000
Subject: [PATCH] KVM: selftests: Add macro to generate KVM exit reason strings

Add and use a macro to generate the KVM exit reason strings array instead
of relying on developers to correctly copy+paste+edit each string.

Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
---
tools/testing/selftests/kvm/lib/kvm_util.c | 55 ++++++++++++----------
1 file changed, 29 insertions(+), 26 deletions(-)

diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index f25b3e9b5a07..b3682b25eedf 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -1815,38 +1815,41 @@ void vm_dump(FILE *stream, struct kvm_vm *vm, uint8_t indent)
vcpu_dump(stream, vcpu, indent + 2);
}

+#define KVM_EXIT_STRING(x) {KVM_EXIT_##x, #x}
+
/* Known KVM exit reasons */
static struct exit_reason {
unsigned int reason;
const char *name;
} exit_reasons_known[] = {
- {KVM_EXIT_UNKNOWN, "UNKNOWN"},
- {KVM_EXIT_EXCEPTION, "EXCEPTION"},
- {KVM_EXIT_IO, "IO"},
- {KVM_EXIT_HYPERCALL, "HYPERCALL"},
- {KVM_EXIT_DEBUG, "DEBUG"},
- {KVM_EXIT_HLT, "HLT"},
- {KVM_EXIT_MMIO, "MMIO"},
- {KVM_EXIT_IRQ_WINDOW_OPEN, "IRQ_WINDOW_OPEN"},
- {KVM_EXIT_SHUTDOWN, "SHUTDOWN"},
- {KVM_EXIT_FAIL_ENTRY, "FAIL_ENTRY"},
- {KVM_EXIT_INTR, "INTR"},
- {KVM_EXIT_SET_TPR, "SET_TPR"},
- {KVM_EXIT_TPR_ACCESS, "TPR_ACCESS"},
- {KVM_EXIT_S390_SIEIC, "S390_SIEIC"},
- {KVM_EXIT_S390_RESET, "S390_RESET"},
- {KVM_EXIT_DCR, "DCR"},
- {KVM_EXIT_NMI, "NMI"},
- {KVM_EXIT_INTERNAL_ERROR, "INTERNAL_ERROR"},
- {KVM_EXIT_OSI, "OSI"},
- {KVM_EXIT_PAPR_HCALL, "PAPR_HCALL"},
- {KVM_EXIT_DIRTY_RING_FULL, "DIRTY_RING_FULL"},
- {KVM_EXIT_X86_RDMSR, "RDMSR"},
- {KVM_EXIT_X86_WRMSR, "WRMSR"},
- {KVM_EXIT_XEN, "XEN"},
- {KVM_EXIT_HYPERV, "HYPERV"},
+ KVM_EXIT_STRING(UNKNOWN),
+ KVM_EXIT_STRING(EXCEPTION),
+ KVM_EXIT_STRING(IO),
+ KVM_EXIT_STRING(HYPERCALL),
+ KVM_EXIT_STRING(DEBUG),
+ KVM_EXIT_STRING(HLT),
+ KVM_EXIT_STRING(MMIO),
+ KVM_EXIT_STRING(IRQ_WINDOW_OPEN),
+ KVM_EXIT_STRING(SHUTDOWN),
+ KVM_EXIT_STRING(FAIL_ENTRY),
+ KVM_EXIT_STRING(INTR),
+ KVM_EXIT_STRING(SET_TPR),
+ KVM_EXIT_STRING(TPR_ACCESS),
+ KVM_EXIT_STRING(S390_SIEIC),
+ KVM_EXIT_STRING(S390_RESET),
+ KVM_EXIT_STRING(DCR),
+ KVM_EXIT_STRING(NMI),
+ KVM_EXIT_STRING(INTERNAL_ERROR),
+ KVM_EXIT_STRING(OSI),
+ KVM_EXIT_STRING(PAPR_HCALL),
+ KVM_EXIT_STRING(DIRTY_RING_FULL),
+ KVM_EXIT_STRING(X86_RDMSR),
+ KVM_EXIT_STRING(X86_WRMSR),
+ KVM_EXIT_STRING(XEN),
+ KVM_EXIT_STRING(HYPERV),
+
#ifdef KVM_EXIT_MEMORY_NOT_PRESENT
- {KVM_EXIT_MEMORY_NOT_PRESENT, "MEMORY_NOT_PRESENT"},
+ KVM_EXIT_STRING(MEMORY_NOT_PRESENT),
#endif
};


base-commit: b20015517a2c6b45bafa09aee45d1698f91428d6
--