Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <060d927e-59f0-1e10-06ac-ed323b2a5433@redhat.com>
Date:   Thu, 10 Nov 2022 08:45:11 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.13.0
Subject: Re: [RFC PATCH 0/3] Use TAP in some more KVM selftests
Content-Language: en-US
To:     David Matlack <dmatlack@google.com>,
        Sean Christopherson <seanjc@google.com>
Cc:     Paolo Bonzini <pbonzini@redhat.com>, kvm@vger.kernel.org,
        Shuah Khan <shuah@kernel.org>, linux-kselftest@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20221004093131.40392-1-thuth@redhat.com>
 <Y0nOv6fqTe2NnPuu@google.com> <Y2mrh7h1jrZSPU5l@google.com>
From:   Thomas Huth <thuth@redhat.com>
In-Reply-To: <Y2mrh7h1jrZSPU5l@google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 08/11/2022 02.06, David Matlack wrote:
> On Fri, Oct 14, 2022 at 09:03:59PM +0000, Sean Christopherson wrote:
>> On Tue, Oct 04, 2022, Thomas Huth wrote:
>>> Many KVM selftests are completely silent. This has the disadvantage
>>> for the users that they do not know what's going on here. For example,
>>> some time ago, a tester asked me how to know whether a certain new
>>> sub-test has been added to one of the s390x test binaries or not (which
>>> he didn't compile on his own), which is hard to judge when there is no
>>> output. So I finally went ahead and implemented TAP output in the
>>> s390x-specific tests some months ago.
>>>
>>> Now I wonder whether that could be a good strategy for the x86 and
>>> generic tests, too?
>>
>> Taking Andrew's thoughts a step further, I'm in favor of adding TAP output, but
>> only if we implement it in such a way that it reduces the burden on writing new
>> tests.  I _really_ like that sync_regs_test's subtests are split into consumable
>> chunks, but I worry that the amount of boilerplate needed will deter test writes
>> and increase the maintenance cost.
>>
>> And my experience with KVM-unit-tests is that letting test writers specify strings
>> for test names is a bad idea, e.g. using an arbitrary string creates a disconnect
>> between what the user sees and what code is running, and makes it unnecessarily
>> difficult to connect a failure back to code.  And if we ever support running
>> specific testcases by name (I'm still not sure this is a net positive), arbitrary
>> strings get really annoying because inevitably an arbitrary string will contain
>> characters that need to be escaped in the shell.
>>
>> Adding a macro or three to let tests define and run testscases with minimal effort
>> would more or less eliminate the boilerplate.  And in theory providing semi-rigid
>> macros would help force simple tests to conform to standard patterns, which should
>> reduce the cost of someone new understanding the test, and would likely let us do
>> more automagic things in the future.
>>
>> E.g. something like this in the test:
>>
>> 	KVM_RUN_TESTCASES(vcpu,
>> 		test_clear_kvm_dirty_regs_bits,
>> 		test_set_invalid,
>> 		test_req_and_verify_all_valid_regs,
>> 		test_set_and_verify_various_reg_values,
>> 		test_clear_kvm_dirty_regs_bits,
>> 	);
> 
> There is an existing framework in
> tools/testing/selftests/kselftest_harness.h that provides macros for
> setting up and running tests cases. I converted sync_regs_test to use it
> below as an example [1].
> 
> The harness runs each subtest in a child process, so sharing a VM/VCPU
> across test cases is not possible. This means setting up and tearing
> down a VM for every test case, but the harness makes this pretty easy
> with FIXTURE_{SETUP,TEARDOWN}(). With this harness, we can keep using
> TEST_ASSERT() as-is, and still run all test cases even if one fails.
> Plus no need for the hard-coded ksft_*() calls in main().

  Hi!

Sorry for not getting back to this earlier - I'm pretty much busy with other 
stuff right now. But your suggestion looks really cool, I like it - so if 
you've got some spare time to work on the conversion, please go ahead (I 
won't have much time to work on this in the next weeks, I think)!

  Thomas