From: Sean Christopherson <seanjc@google.com>
To: Vipin Sharma <vipinsh@google.com>
Cc: pbonzini@redhat.com, vkuznets@redhat.com, dmatlack@google.com,
kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
Marc Zyngier <maz@kernel.org>, James Morse <james.morse@arm.com>,
m Suzuki K Poulose <suzuki.poulose@arm.com>,
Oliver Upton <oliver.upton@linux.dev>,
Zenghui Yu <yuzenghui@huawei.com>,
Anup Patel <anup@brainfault.org>,
Atish Patra <atishp@atishpatra.org>,
Christian Borntraeger <borntraeger@linux.ibm.com>,
Janosch Frank <frankja@linux.ibm.com>,
Claudio Imbrenda <imbrenda@linux.ibm.com>,
David Hildenbrand <david@redhat.com>
Subject: Re: [Patch v4 12/13] KVM: selftests: Make vCPU exit reason test assertion common.
Date: Wed, 1 Feb 2023 23:24:27 +0000 [thread overview]
Message-ID: <Y9r0q9cuK/ifu+OW@google.com> (raw)
In-Reply-To: <20221212183720.4062037-13-vipinsh@google.com>
+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@google.com>
> Signed-off-by: Vipin Sharma <vipinsh@google.com>
> Reviewed-by: David Matlack <dmatlack@google.com>
> ---
> .../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@google.com>
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@google.com>
---
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
--
next prev parent reply other threads:[~2023-02-01 23:25 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-12-12 18:37 [Patch v4 00/13] Add Hyper-v extended hypercall support in KVM Vipin Sharma
2022-12-12 18:37 ` [Patch v4 01/13] x86/hyperv: Add HV_EXPOSE_INVARIANT_TSC define Vipin Sharma
2022-12-12 18:37 ` [Patch v4 02/13] KVM: x86: Add a KVM-only leaf for CPUID_8000_0007_EDX Vipin Sharma
2022-12-27 20:20 ` Aaron Lewis
2022-12-12 18:37 ` [Patch v4 03/13] KVM: x86: Hyper-V invariant TSC control Vipin Sharma
2022-12-12 18:37 ` [Patch v4 04/13] KVM: selftests: Rename 'msr->available' to 'msr->fault_exepected' in hyperv_features test Vipin Sharma
2022-12-12 18:37 ` [Patch v4 05/13] KVM: selftests: Convert hyperv_features test to using KVM_X86_CPU_FEATURE() Vipin Sharma
2022-12-12 18:37 ` [Patch v4 06/13] KVM: selftests: Test that values written to Hyper-V MSRs are preserved Vipin Sharma
2022-12-12 18:37 ` [Patch v4 07/13] KVM: selftests: Test Hyper-V invariant TSC control Vipin Sharma
2022-12-12 18:37 ` [Patch v4 08/13] KVM: x86: hyper-v: Use common code for hypercall userspace exit Vipin Sharma
2022-12-12 18:37 ` [Patch v4 09/13] KVM: x86: hyper-v: Add extended hypercall support in Hyper-v Vipin Sharma
2022-12-12 18:37 ` [Patch v4 10/13] KVM: selftests: Test Hyper-V extended hypercall enablement Vipin Sharma
2022-12-12 18:37 ` [Patch v4 11/13] KVM: selftests: Replace hardcoded Linux OS id with HYPERV_LINUX_OS_ID Vipin Sharma
2022-12-12 18:37 ` [Patch v4 12/13] KVM: selftests: Make vCPU exit reason test assertion common Vipin Sharma
2023-02-01 23:24 ` Sean Christopherson [this message]
2023-02-02 18:31 ` Vipin Sharma
2023-02-02 18:51 ` Sean Christopherson
2023-02-02 18:59 ` Vipin Sharma
2023-02-03 22:08 ` Vipin Sharma
2023-02-03 22:57 ` Sean Christopherson
2022-12-12 18:37 ` [Patch v4 13/13] KVM: selftests: Test Hyper-V extended hypercall exit to userspace Vipin Sharma
2023-01-04 0:33 ` [Patch v4 00/13] Add Hyper-v extended hypercall support in KVM Vipin Sharma
2023-02-01 22:39 ` (subset) " Sean Christopherson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y9r0q9cuK/ifu+OW@google.com \
--to=seanjc@google.com \
--cc=anup@brainfault.org \
--cc=atishp@atishpatra.org \
--cc=borntraeger@linux.ibm.com \
--cc=david@redhat.com \
--cc=dmatlack@google.com \
--cc=frankja@linux.ibm.com \
--cc=imbrenda@linux.ibm.com \
--cc=james.morse@arm.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=maz@kernel.org \
--cc=oliver.upton@linux.dev \
--cc=pbonzini@redhat.com \
--cc=suzuki.poulose@arm.com \
--cc=vipinsh@google.com \
--cc=vkuznets@redhat.com \
--cc=yuzenghui@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).