From: Thomas Huth <thuth@redhat.com>
To: Sean Christopherson <seanjc@google.com>
Cc: kvm@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>,
linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org,
Shuah Khan <shuah@kernel.org>,
Muhammad Usama Anjum <usama.anjum@collabora.com>
Subject: Re: [PATCH v2] KVM: selftests: Use TAP interface in the set_memory_region test
Date: Fri, 3 May 2024 09:30:57 +0200 [thread overview]
Message-ID: <25cc89b7-822f-4735-bec5-59458ec18a49@redhat.com> (raw)
In-Reply-To: <ZjPrlLNNGNh2mOmW@google.com>
On 02/05/2024 21.37, Sean Christopherson wrote:
> On Fri, Apr 26, 2024, Thomas Huth wrote:
>> Use the kselftest_harness.h interface in this test to get TAP
>> output, so that it is easier for the user to see what the test
>> is doing. (Note: We are not using the KVM_ONE_VCPU_TEST_SUITE()
>> macro here since these tests are creating their VMs with the
>> vm_create_barebones() function, not with vm_create_with_one_vcpu())
>>
>> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>> v2:
>> - Rebase to linux-next branch
>> - Make "loops" variable static
>> - Added Andrew's Reviewed-by
>>
>> .../selftests/kvm/set_memory_region_test.c | 86 +++++++++----------
>> 1 file changed, 42 insertions(+), 44 deletions(-)
>>
>> diff --git a/tools/testing/selftests/kvm/set_memory_region_test.c b/tools/testing/selftests/kvm/set_memory_region_test.c
>> index 68c899d27561..a5c9bee5235a 100644
>> --- a/tools/testing/selftests/kvm/set_memory_region_test.c
>> +++ b/tools/testing/selftests/kvm/set_memory_region_test.c
>> @@ -16,6 +16,7 @@
>> #include <test_util.h>
>> #include <kvm_util.h>
>> #include <processor.h>
>> +#include "kselftest_harness.h"
>>
>> /*
>> * s390x needs at least 1MB alignment, and the x86_64 MOVE/DELETE tests need a
>> @@ -38,6 +39,8 @@ extern const uint64_t final_rip_end;
>>
>> static sem_t vcpu_ready;
>>
>> +static int loops;
>
> ...
>
>> -static void test_add_overlapping_private_memory_regions(void)
>> +TEST(add_overlapping_private_memory_regions)
>> {
>> struct kvm_vm *vm;
>> int memfd;
>> int r;
>>
>> - pr_info("Testing ADD of overlapping KVM_MEM_GUEST_MEMFD memory regions\n");
>> + if (!has_cap_guest_memfd())
>> + SKIP(return, "Missing KVM_MEM_GUEST_MEMFD / KVM_X86_SW_PROTECTED_VM");
>
> I like that we can actually report sub-tests as being skipped, but I don't like
> having multiple ways to express requirements. And IMO, this is much less readable
> than TEST_REQUIRE(has_cap_guest_memfd());
>
> AIUI, each test runs in a child process, so TEST_REQUIRE() can simply exit(), it
> just needs to avoid ksft_exit_skip() so that a sub-test doesn't spit out the full
> test summary.
>
> And if using exit() isn't an option, setjmp()+longjmp() will do the trick (I got
> that working for KVM_ONE_VCPU_TEST() before I realized tests run as a child).
>
> The below is lightly tested, but I think it does what we want?
Not quite ... for example, if I force vmx_pmu_caps_test to skip the last
test, I get:
TAP version 13
1..5
# Starting 5 tests from 1 test cases.
# RUN vmx_pmu_caps.guest_wrmsr_perf_capabilities ...
# OK vmx_pmu_caps.guest_wrmsr_perf_capabilities
ok 1 vmx_pmu_caps.guest_wrmsr_perf_capabilities
# RUN vmx_pmu_caps.basic_perf_capabilities ...
# OK vmx_pmu_caps.basic_perf_capabilities
ok 2 vmx_pmu_caps.basic_perf_capabilities
# RUN vmx_pmu_caps.fungible_perf_capabilities ...
# OK vmx_pmu_caps.fungible_perf_capabilities
ok 3 vmx_pmu_caps.fungible_perf_capabilities
# RUN vmx_pmu_caps.immutable_perf_capabilities ...
# OK vmx_pmu_caps.immutable_perf_capabilities
ok 4 vmx_pmu_caps.immutable_perf_capabilities
# RUN vmx_pmu_caps.lbr_perf_capabilities ...
ok 5 # SKIP - Requirement not met: host_cap.lbr_format && 0
# OK vmx_pmu_caps.lbr_perf_capabilities
ok 5 vmx_pmu_caps.lbr_perf_capabilities
# PASSED: 5 / 5 tests passed.
# Totals: pass:5 fail:0 xfail:0 xpass:0 skip:0 error:0
As you can see, the "ok 5" line is duplicated now, once marked with "# SKIP"
and once as successfull. I don't think that this is valid TAP anymore?
> I also think we would effectively forbid direct use of TEST(). Partly because
> it's effectively necessary to use TEST_REQUIRE(), but also so that all tests will
> have an existing single point of contact if we need/want to make similar changes
> in the future.
Ok, but I wrote in the patch description, KVM_ONE_VCPU_TEST_SUITE() does not
work for the set_memory_region test since it does not like to have a
pre-defined vcpu ... so if we want to forbid TEST(), I assume we'd need
another macro like KVM_BAREBONE_TEST_SUITE() ?
Not sure whether I really like it, though, since I'd prefer if we could keep
the possibility to use the original selftest macros (for people who are
already used to those macros from other selftests).
Thomas
next prev parent reply other threads:[~2024-05-03 7:31 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-26 11:45 [PATCH v2] KVM: selftests: Use TAP interface in the set_memory_region test Thomas Huth
2024-04-26 14:03 ` Muhammad Usama Anjum
2024-05-02 19:37 ` Sean Christopherson
2024-05-03 7:30 ` Thomas Huth [this message]
2024-05-03 18:44 ` 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=25cc89b7-822f-4735-bec5-59458ec18a49@redhat.com \
--to=thuth@redhat.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=pbonzini@redhat.com \
--cc=seanjc@google.com \
--cc=shuah@kernel.org \
--cc=usama.anjum@collabora.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 read-only IMAP folder(s) and NNTP newsgroup(s).