Received-SPF: pass (google.com: domain of linux-kernel+bounces-167319-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <25cc89b7-822f-4735-bec5-59458ec18a49@redhat.com>
Date: Fri, 3 May 2024 09:30:57 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2] KVM: selftests: Use TAP interface in the
 set_memory_region test
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>
References: <20240426114552.667346-1-thuth@redhat.com>
 <ZjPrlLNNGNh2mOmW@google.com>
From: Thomas Huth <thuth@redhat.com>
Content-Language: en-US
Autocrypt: addr=thuth@redhat.com; keydata=
 xsFNBFH7eUwBEACzyOXKU+5Pcs6wNpKzrlJwzRl3VGZt95VCdb+FgoU9g11m7FWcOafrVRwU
 yYkTm9+7zBUc0sW5AuPGR/dp3pSLX/yFWsA/UB4nJsHqgDvDU7BImSeiTrnpMOTXb7Arw2a2
 4CflIyFqjCpfDM4MuTmzTjXq4Uov1giGE9X6viNo1pxyEpd7PanlKNnf4PqEQp06X4IgUacW
 tSGj6Gcns1bCuHV8OPWLkf4hkRnu8hdL6i60Yxz4E6TqlrpxsfYwLXgEeswPHOA6Mn4Cso9O
 0lewVYfFfsmokfAVMKWzOl1Sr0KGI5T9CpmRfAiSHpthhHWnECcJFwl72NTi6kUcUzG4se81
 O6n9d/kTj7pzTmBdfwuOZ0YUSqcqs0W+l1NcASSYZQaDoD3/SLk+nqVeCBB4OnYOGhgmIHNW
 0CwMRO/GK+20alxzk//V9GmIM2ACElbfF8+Uug3pqiHkVnKqM7W9/S1NH2qmxB6zMiJUHlTH
 gnVeZX0dgH27mzstcF786uPcdEqS0KJuxh2kk5IvUSL3Qn3ZgmgdxBMyCPciD/1cb7/Ahazr
 3ThHQXSHXkH/aDXdfLsKVuwDzHLVSkdSnZdt5HHh75/NFHxwaTlydgfHmFFwodK8y/TjyiGZ
 zg2Kje38xnz8zKn9iesFBCcONXS7txENTzX0z80WKBhK+XSFJwARAQABzR5UaG9tYXMgSHV0
 aCA8dGh1dGhAcmVkaGF0LmNvbT7CwXgEEwECACIFAlVgX6oCGwMGCwkIBwMCBhUIAgkKCwQW
 AgMBAh4BAheAAAoJEC7Z13T+cC21EbIP/ii9cvT2HHGbFRl8HqGT6+7Wkb+XLMqJBMAIGiQK
 QIP3xk1HPTsLfVG0ao4hy/oYkGNOP8+ubLnZen6Yq3zAFiMhQ44lvgigDYJo3Ve59gfe99KX
 EbtB+X95ODARkq0McR6OAsPNJ7gpEUzfkQUUJTXRDQXfG/FX303Gvk+YU0spm2tsIKPl6AmV
 1CegDljzjycyfJbk418MQmMu2T82kjrkEofUO2a24ed3VGC0/Uz//XCR2ZTo+vBoBUQl41BD
 eFFtoCSrzo3yPFS+w5fkH9NT8ChdpSlbNS32NhYQhJtr9zjWyFRf0Zk+T/1P7ECn6gTEkp5k
 ofFIA4MFBc/fXbaDRtBmPB0N9pqTFApIUI4vuFPPO0JDrII9dLwZ6lO9EKiwuVlvr1wwzsgq
 zJTPBU3qHaUO4d/8G+gD7AL/6T4zi8Jo/GmjBsnYaTzbm94lf0CjXjsOX3seMhaE6WAZOQQG
 tZHAO1kAPWpaxne+wtgMKthyPLNwelLf+xzGvrIKvLX6QuLoWMnWldu22z2ICVnLQChlR9d6
 WW8QFEpo/FK7omuS8KvvopFcOOdlbFMM8Y/8vBgVMSsK6fsYUhruny/PahprPbYGiNIhKqz7
 UvgyZVl4pBFjTaz/SbimTk210vIlkDyy1WuS8Zsn0htv4+jQPgo9rqFE4mipJjy/iboDzsFN
 BFH7eUwBEAC2nzfUeeI8dv0C4qrfCPze6NkryUflEut9WwHhfXCLjtvCjnoGqFelH/PE9NF4
 4VPSCdvD1SSmFVzu6T9qWdcwMSaC+e7G/z0/AhBfqTeosAF5XvKQlAb9ZPkdDr7YN0a1XDfa
 +NgA+JZB4ROyBZFFAwNHT+HCnyzy0v9Sh3BgJJwfpXHH2l3LfncvV8rgFv0bvdr70U+On2XH
 5bApOyW1WpIG5KPJlDdzcQTyptOJ1dnEHfwnABEfzI3dNf63rlxsGouX/NFRRRNqkdClQR3K
 gCwciaXfZ7ir7fF0u1N2UuLsWA8Ei1JrNypk+MRxhbvdQC4tyZCZ8mVDk+QOK6pyK2f4rMf/
 WmqxNTtAVmNuZIwnJdjRMMSs4W4w6N/bRvpqtykSqx7VXcgqtv6eqoDZrNuhGbekQA0sAnCJ
 VPArerAZGArm63o39me/bRUQeQVSxEBmg66yshF9HkcUPGVeC4B0TPwz+HFcVhheo6hoJjLq
 knFOPLRj+0h+ZL+D0GenyqD3CyuyeTT5dGcNU9qT74bdSr20k/CklvI7S9yoQje8BeQAHtdV
 cvO8XCLrpGuw9SgOS7OP5oI26a0548M4KldAY+kqX6XVphEw3/6U1KTf7WxW5zYLTtadjISB
 X9xsRWSU+Yqs3C7oN5TIPSoj9tXMoxZkCIHWvnqGwZ7JhwARAQABwsFfBBgBAgAJBQJR+3lM
 AhsMAAoJEC7Z13T+cC21hPAQAIsBL9MdGpdEpvXs9CYrBkd6tS9mbaSWj6XBDfA1AEdQkBOn
 ZH1Qt7HJesk+qNSnLv6+jP4VwqK5AFMrKJ6IjE7jqgzGxtcZnvSjeDGPF1h2CKZQPpTw890k
 fy18AvgFHkVk2Oylyexw3aOBsXg6ukN44vIFqPoc+YSU0+0QIdYJp/XFsgWxnFIMYwDpxSHS
 5fdDxUjsk3UBHZx+IhFjs2siVZi5wnHIqM7eK9abr2cK2weInTBwXwqVWjsXZ4tq5+jQrwDK
 cvxIcwXdUTLGxc4/Z/VRH1PZSvfQxdxMGmNTGaXVNfdFZjm4fz0mz+OUi6AHC4CZpwnsliGV
 ODqwX8Y1zic9viSTbKS01ZNp175POyWViUk9qisPZB7ypfSIVSEULrL347qY/hm9ahhqmn17
 Ng255syASv3ehvX7iwWDfzXbA0/TVaqwa1YIkec+/8miicV0zMP9siRcYQkyTqSzaTFBBmqD
 oiT+z+/E59qj/EKfyce3sbC9XLjXv3mHMrq1tKX4G7IJGnS989E/fg6crv6NHae9Ckm7+lSs
 IQu4bBP2GxiRQ+NV3iV/KU3ebMRzqIC//DCOxzQNFNJAKldPe/bKZMCxEqtVoRkuJtNdp/5a
 yXFZ6TfE1hGKrDBYAm4vrnZ4CXFSBDllL59cFFOJCkn4Xboj/aVxxJxF30bn
In-Reply-To: <ZjPrlLNNGNh2mOmW@google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

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