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: <615ce4b8-1b82-ba6a-0546-f77e8a93bf3f@redhat.com>
Date:   Thu, 3 Aug 2023 07:17:51 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Content-Language: en-US
To:     Sean Christopherson <seanjc@google.com>
Cc:     kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        Paolo Bonzini <pbonzini@redhat.com>,
        linux-kselftest@vger.kernel.org,
        David Matlack <dmatlack@google.com>
References: <20230712075910.22480-1-thuth@redhat.com>
 <20230712075910.22480-3-thuth@redhat.com> <ZMq0nYYDbOX1cOKN@google.com>
From:   Thomas Huth <thuth@redhat.com>
Subject: Re: [PATCH 2/4] KVM: selftests: x86: Use TAP interface in the
 sync_regs test
In-Reply-To: <ZMq0nYYDbOX1cOKN@google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 02/08/2023 21.55, Sean Christopherson wrote:
> On Wed, Jul 12, 2023, Thomas Huth wrote:
>> The sync_regs test currently does not have any output (unless one
>> of the TEST_ASSERT statement fails), so it's hard to say for a user
>> whether a certain new sub-test has been included in the binary or
>> not. Let's make this a little bit more user-friendly and include
>> some TAP output via the kselftest_harness.h interface.
>> To be able to use the interface, we have to break up the huge main()
>> function here in more fine grained parts - then we can use the
>> TEST_F() macro to define the individual tests. Since these are run
>> with a separate VM now, we have also to make sure to create the
>> expected state at the beginning of each test, so some parts grow
>> a little bit - which should be OK considering that the individual
>> tests are more self-contained now.
>>
>> Suggested-by: David Matlack <dmatlack@google.com>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   .../selftests/kvm/x86_64/sync_regs_test.c     | 113 +++++++++++++++---
> 
> FYI, there's an in-flight patch[*] to expand this test's coverage, and I plan on
> grabbing that in some form before this one (sorry).  Let me know if there are
> any tweaks that can be done to Michal's patch to make it easier to convert the
> test to tap.
> 
> I'll also try to get Michal's patch into kvm-x86/next sooner than later so that
> you can use that as the basic.

Ok, I'll wait 'til the dust settles and then redo my patch (there is no 
hurry for this, and I'm only doing it in my spare minutes).

Any chance that you could already take at least the other conversion patches 
from my series?

...
>> +TEST_F(sync_regs_test, req_and_verify_all_valid)
>> +{
>> +	struct kvm_vcpu *vcpu = self->vcpu;
>> +	struct kvm_run *run = vcpu->run;
>> +	struct kvm_vcpu_events events;
>> +	struct kvm_sregs sregs;
>> +	struct kvm_regs regs;
>> +	int rv;
>>   
>>   	/* Request and verify all valid register sets. */
>>   	/* TODO: BUILD TIME CHECK: TEST_ASSERT(KVM_SYNC_X86_NUM_FIELDS != 3); */
>>   	run->kvm_valid_regs = TEST_SYNC_FIELDS;
>>   	rv = _vcpu_run(vcpu);
>> +	TEST_ASSERT(rv == 0, "vcpu_run failed: %d\n", rv);
> 
> Just use vcpu_run() instead of _vcpu_run().  And please post that as a separate
> patch, I think/hope it will make the conversion-to-tap patch smaller.

Ok, makes sense.

>> +TEST_F(sync_regs_test, clear_kvm_valid_and_dirty_regs)
>> +{
>> +	struct kvm_vcpu *vcpu = self->vcpu;
>> +	struct kvm_run *run = vcpu->run;
>> +	struct kvm_regs regs;
>> +	int rv;
>> +
>> +	/* Run once to get register set */
>> +	run->kvm_valid_regs = TEST_SYNC_FIELDS;
>> +	rv = _vcpu_run(vcpu);
>> +	TEST_ASSERT(rv == 0, "vcpu_run failed: %d\n", rv);
> 
> At least you're consistent :-)

Just copy-n-pasting the preexistent code ;-)

>> +	TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_IO);
>>   
>>   	/* Clear kvm_valid_regs bits and kvm_dirty_bits.
>>   	 * Verify s.regs values are not overwritten with existing guest values
>> @@ -187,9 +246,11 @@ int main(int argc, char *argv[])
>>   	run->kvm_valid_regs = 0;
>>   	run->kvm_dirty_regs = 0;
>>   	run->s.regs.regs.rbx = 0xAAAA;
>> +	vcpu_regs_get(vcpu, &regs);
> 
> Can you split this change to its own patch too?  I'm pretty sure that change
> stands on its own, and slotting it in here made me do a double-take.

Ok.

  Thomas