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;
Subject: Re: [PATCH 2/2] KVM: s390: selftests: Test vm and vcpu memop with
 keys
To:     Janis Schoetterl-Glausch <scgl@linux.ibm.com>,
        Christian Borntraeger <borntraeger@linux.ibm.com>,
        Janosch Frank <frankja@linux.ibm.com>,
        Claudio Imbrenda <imbrenda@linux.ibm.com>
Cc:     Thomas Huth <thuth@redhat.com>,
        David Hildenbrand <david@redhat.com>, kvm@vger.kernel.org,
        linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org,
        Shuah Khan <skhan@linuxfoundation.org>
References: <20220211182215.2730017-11-scgl@linux.ibm.com>
 <20220217145336.1794778-1-scgl@linux.ibm.com>
 <20220217145336.1794778-3-scgl@linux.ibm.com>
From:   Shuah Khan <skhan@linuxfoundation.org>
Message-ID: <7d0b5b03-21f4-0402-779a-788d4bd58071@linuxfoundation.org>
Date:   Thu, 17 Feb 2022 10:54:41 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.8.1
MIME-Version: 1.0
In-Reply-To: <20220217145336.1794778-3-scgl@linux.ibm.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2/17/22 7:53 AM, Janis Schoetterl-Glausch wrote:
> Test storage key checking for both vm and vcpu MEM_OP ioctls.
> Test both error and non error conditions.
> 

This patch seems to combine restructuring the code and new code.
e,g test_errors() was added in the last patch, only to be redone
in this patch with test_errors split into test_common_errors()

Doing restructure in a separate patch and then adding new code
makes it easier to review and also keep them simpler patches.

Please split the code in these two patches to just do restructure
and then add new code.

I also would like to have good reasons to change existing code and
make them into macros.
  
> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> ---
>   tools/testing/selftests/kvm/s390x/memop.c | 342 +++++++++++++++++++++-
>   1 file changed, 328 insertions(+), 14 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/s390x/memop.c b/tools/testing/selftests/kvm/s390x/memop.c
> index 4510418d73e6..bc12a9238967 100644
> --- a/tools/testing/selftests/kvm/s390x/memop.c
> +++ b/tools/testing/selftests/kvm/s390x/memop.c
> @@ -201,6 +201,8 @@ static int err_memop_ioctl(struct test_vcpu vcpu, struct kvm_s390_mem_op *ksmo)
>   #define PAGE_SHIFT 12
>   #define PAGE_SIZE (1ULL << PAGE_SHIFT)
>   #define PAGE_MASK (~(PAGE_SIZE - 1))
> +#define CR0_FETCH_PROTECTION_OVERRIDE	(1UL << (63 - 38))
> +#define CR0_STORAGE_PROTECTION_OVERRIDE	(1UL << (63 - 39))
>   
>   #define ASSERT_MEM_EQ(p1, p2, size) \
>   	TEST_ASSERT(!memcmp(p1, p2, size), "Memory contents do not match!")
> @@ -235,6 +237,11 @@ static struct test_default test_default_init(void *guest_code)
>   	return t;
>   }
>   
> +static vm_vaddr_t test_vaddr_alloc(struct test_vcpu vm, size_t size, vm_vaddr_t vaddr_min)
> +{
> +	return vm_vaddr_alloc(vm.vm, size, vaddr_min);
> +}
> +

What is the value of adding a new routine that simply calls another?
Do you see this routine changing in the future to do more?

thanks,
-- Shuah