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: <65711eda-76da-882c-ee3e-a0f8805943ab@linux.ibm.com>
Date:   Wed, 20 Apr 2022 12:23:00 +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 v2 3/4] KVM: s390: selftests: Use TAP interface in the
 tprot test
Content-Language: en-US
To:     Thomas Huth <thuth@redhat.com>, kvm@vger.kernel.org,
        Christian Borntraeger <borntraeger@linux.ibm.com>,
        Claudio Imbrenda <imbrenda@linux.ibm.com>
Cc:     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>,
        Janis Schoetterl-Glausch <scgl@linux.ibm.com>,
        linux-s390@vger.kernel.org
References: <20220419185857.128351-1-thuth@redhat.com>
 <20220419185857.128351-4-thuth@redhat.com>
From:   Janosch Frank <frankja@linux.ibm.com>
In-Reply-To: <20220419185857.128351-4-thuth@redhat.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 4/19/22 20:58, Thomas Huth 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>

Reviewed-by: Janosch Frank <frankja@linux.ibm.com>

Some comments below.

> ---
>   tools/testing/selftests/kvm/s390x/tprot.c | 28 +++++++++++++++++++----
>   1 file changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/s390x/tprot.c b/tools/testing/selftests/kvm/s390x/tprot.c
> index c097b9db495e..baba883d7a6d 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)
> @@ -63,12 +64,12 @@ static enum permission test_protection(void *addr, uint8_t key)
>   }
>   
>   enum stage {
> -	STAGE_END,
>   	STAGE_INIT_SIMPLE,
>   	TEST_SIMPLE,
>   	STAGE_INIT_FETCH_PROT_OVERRIDE,
>   	TEST_FETCH_PROT_OVERRIDE,
>   	TEST_STORAGE_PROT_OVERRIDE,
> +	STAGE_END			/* this must be the last entry */

...so we can use it to calculate the test number

>   };
>   
>   struct test {
> @@ -182,7 +183,7 @@ static void guest_code(void)
>   	GUEST_SYNC(perform_next_stage(&i, mapped_0));
>   }
>   

> @@ -212,9 +222,13 @@ int main(int argc, char *argv[])
>   	HOST_SYNC(vm, TEST_SIMPLE);
>   
>   	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");
> -	HOST_SYNC(vm, STAGE_INIT_FETCH_PROT_OVERRIDE);
> +	if (guest_0_page != 0) {

Maybe add:
/* Use no_tap so we don't get a PASS print */

> +		HOST_SYNC_NO_TAP(vm, STAGE_INIT_FETCH_PROT_OVERRIDE);
> +		ksft_test_result_skip("STAGE_INIT_FETCH_PROT_OVERRIDE - "
> +				      "Did not allocate page at 0\n");
> +	} else {
> +		HOST_SYNC(vm, STAGE_INIT_FETCH_PROT_OVERRIDE);
> +	}

Otherwise this would look weird.

>   	if (guest_0_page == 0)
>   		mprotect(addr_gva2hva(vm, (vm_vaddr_t)0), PAGE_SIZE, PROT_READ);
>   	run->s.regs.crs[0] |= CR0_FETCH_PROTECTION_OVERRIDE;
> @@ -224,4 +238,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();
>   }