linux-kernel - Re: [PATCH 2/2] KVM: s390: selftests: Test vm and vcpu memop with keys

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 17 Feb 2022 10:54:41 -0700
From:   Shuah Khan <skhan@...uxfoundation.org>
To:     Janis Schoetterl-Glausch <scgl@...ux.ibm.com>,
        Christian Borntraeger <borntraeger@...ux.ibm.com>,
        Janosch Frank <frankja@...ux.ibm.com>,
        Claudio Imbrenda <imbrenda@...ux.ibm.com>
Cc:     Thomas Huth <thuth@...hat.com>,
        David Hildenbrand <david@...hat.com>, kvm@...r.kernel.org,
        linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org,
        Shuah Khan <skhan@...uxfoundation.org>
Subject: Re: [PATCH 2/2] KVM: s390: selftests: Test vm and vcpu memop with
 keys

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@...ux.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