Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <c44de70a-60e3-65b6-14a7-3e4400f3084b@redhat.com>
Date:   Tue, 19 Apr 2022 19:45:05 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.8.0
Subject: Re: [PATCH 3/4] KVM: s390: selftests: Use TAP interface in the tprot
 test
Content-Language: en-US
To:     Janis Schoetterl-Glausch <scgl@linux.ibm.com>,
        Claudio Imbrenda <imbrenda@linux.ibm.com>
Cc:     kvm@vger.kernel.org,
        Christian Borntraeger <borntraeger@linux.ibm.com>,
        Janosch Frank <frankja@linux.ibm.com>,
        linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org,
        David Hildenbrand <david@redhat.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Shuah Khan <shuah@kernel.org>
References: <20220414105322.577439-1-thuth@redhat.com>
 <20220414105322.577439-4-thuth@redhat.com>
 <20220414135110.6b2baead@p-imbrenda>
 <03f62ec7-2f7f-1f90-3029-d93713ab5afc@redhat.com>
 <baa53445-b4de-7a05-24f5-46fa38e61666@linux.ibm.com>
From:   Thomas Huth <thuth@redhat.com>
In-Reply-To: <baa53445-b4de-7a05-24f5-46fa38e61666@linux.ibm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 14/04/2022 14.33, Janis Schoetterl-Glausch wrote:
> On 4/14/22 14:08, Thomas Huth wrote:
>> On 14/04/2022 13.51, Claudio Imbrenda wrote:
>>> On Thu, 14 Apr 2022 12:53:21 +0200
>>> Thomas Huth <thuth@redhat.com> wrote:
>>>
>>>> The tprot 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 kselftests.h interface.
>>>>
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>>    tools/testing/selftests/kvm/s390x/tprot.c | 12 +++++++++++-
>>>>    1 file changed, 11 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/tools/testing/selftests/kvm/s390x/tprot.c b/tools/testing/selftests/kvm/s390x/tprot.c
>>>> index c097b9db495e..a714b4206e95 100644
>>>> --- a/tools/testing/selftests/kvm/s390x/tprot.c
>>>> +++ b/tools/testing/selftests/kvm/s390x/tprot.c
>>>> @@ -8,6 +8,7 @@
>>>>    #include <sys/mman.h>
>>>>    #include "test_util.h"
>>>>    #include "kvm_util.h"
>>>> +#include "kselftest.h"
>>>>      #define PAGE_SHIFT 12
>>>>    #define PAGE_SIZE (1 << PAGE_SHIFT)
>>>> @@ -69,6 +70,7 @@ enum stage {
>>>>        STAGE_INIT_FETCH_PROT_OVERRIDE,
>>>>        TEST_FETCH_PROT_OVERRIDE,
>>>>        TEST_STORAGE_PROT_OVERRIDE,
>>>> +    NUM_STAGES            /* this must be the last entry */
> 
> You could move STAGE_END down and use that instead.
> 
>>>>    };
>>>>      struct test {
>>>> @@ -196,6 +198,7 @@ static void guest_code(void)
>>>>        }                                    \
>>>>        ASSERT_EQ(uc.cmd, UCALL_SYNC);                        \
>>>>        ASSERT_EQ(uc.args[1], __stage);                        \
>>>> +    ksft_test_result_pass("" #stage "\n");                    \
>>>>    })
>>>>      int main(int argc, char *argv[])
>>>> @@ -204,6 +207,9 @@ int main(int argc, char *argv[])
>>>>        struct kvm_run *run;
>>>>        vm_vaddr_t guest_0_page;
>>>>    +    ksft_print_header();
>>>> +    ksft_set_plan(NUM_STAGES - 1);    /* STAGE_END is not counted, thus - 1 */
>>>> +
>>>>        vm = vm_create_default(VCPU_ID, 0, guest_code);
>>>>        run = vcpu_state(vm, VCPU_ID);
>>>>    @@ -213,7 +219,7 @@ int main(int argc, char *argv[])
>>>>          guest_0_page = vm_vaddr_alloc(vm, PAGE_SIZE, 0);
>>>>        if (guest_0_page != 0)
>>>> -        print_skip("Did not allocate page at 0 for fetch protection override tests");
>>>> +        ksft_print_msg("Did not allocate page at 0 for fetch protection override tests\n");
>>>
>>> will this print a skip, though?
>>
>> No, it's now only a message.
>>
>>> or you don't want to print a skip because then the numbering in the
>>> planning doesn't match anymore?
>>
>> Right.
>>
>>> in which case, is there an easy way to fix it?
>>
>> Honestly, this part of the code is a little bit of a riddle to me - I wonder why this was using "print_skip()" at all, since the HOST_SYNC below is executed anyway... so this sounds rather like a warning message to me that says that the following test might not work as expected, instead of a real test-is-skipped message?
>>
>> Janis, could you please clarify the intention here?
> 
> Both the host and the guest check the same condition independently, the host just to print the message,
> then the guest is run and skips those stages.

Ok.

However, I'm not sure how to make this use ksft_test_result_skip() in a nice 
way now, though, without makeing the macro way uglier ...
I'll have a try, but if that does not work out I'd suggest to simply keep 
the ksft_print_msg() here instead.

  Thomas