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: <baa53445-b4de-7a05-24f5-46fa38e61666@linux.ibm.com>
Date:   Thu, 14 Apr 2022 14:33:55 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
Subject: Re: [PATCH 3/4] KVM: s390: selftests: Use TAP interface in the tprot
 test
Content-Language: en-US
To:     Thomas Huth <thuth@redhat.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>
From:   Janis Schoetterl-Glausch <scgl@linux.ibm.com>
In-Reply-To: <03f62ec7-2f7f-1f90-3029-d93713ab5afc@redhat.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

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.
> 
>  Thomas
> 
>>>       HOST_SYNC(vm, STAGE_INIT_FETCH_PROT_OVERRIDE);
>>>       if (guest_0_page == 0)
>>>           mprotect(addr_gva2hva(vm, (vm_vaddr_t)0), PAGE_SIZE, PROT_READ);
>>> @@ -224,4 +230,8 @@ int main(int argc, char *argv[])
>>>       run->s.regs.crs[0] |= CR0_STORAGE_PROTECTION_OVERRIDE;
>>>       run->kvm_dirty_regs = KVM_SYNC_CRS;
>>>       HOST_SYNC(vm, TEST_STORAGE_PROT_OVERRIDE);
>>> +
>>> +    kvm_vm_free(vm);
>>> +
>>> +    ksft_finished();
>>>   }
>>
>