Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Subject: Re: [RFC V2 PATCH 3/8] selftests: kvm: priv_memfd_test: Add support
 for memory conversion
To:     Vishal Annapurve <vannapurve@google.com>, x86@kernel.org,
        kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-kselftest@vger.kernel.org
Cc:     pbonzini@redhat.com, vkuznets@redhat.com, wanpengli@tencent.com,
        jmattson@google.com, joro@8bytes.org, tglx@linutronix.de,
        mingo@redhat.com, bp@alien8.de, dave.hansen@linux.intel.com,
        hpa@zytor.com, shauh@kernel.org, yang.zhong@intel.com,
        drjones@redhat.com, ricarkol@google.com, aaronlewis@google.com,
        wei.w.wang@intel.com, kirill.shutemov@linux.intel.com,
        corbet@lwn.net, hughd@google.com, jlayton@kernel.org,
        bfields@fieldses.org, akpm@linux-foundation.org,
        chao.p.peng@linux.intel.com, yu.c.zhang@linux.intel.com,
        jun.nakajima@intel.com, dave.hansen@intel.com,
        michael.roth@amd.com, qperret@google.com, steven.price@arm.com,
        ak@linux.intel.com, david@redhat.com, luto@kernel.org,
        vbabka@suse.cz, marcorr@google.com, erdemaktas@google.com,
        pgonda@google.com, nikunj@amd.com, seanjc@google.com,
        diviness@google.com, Shuah Khan <skhan@linuxfoundation.org>
References: <20220511000811.384766-1-vannapurve@google.com>
 <20220511000811.384766-4-vannapurve@google.com>
From:   Shuah Khan <skhan@linuxfoundation.org>
Message-ID: <934a07be-ce72-7916-5614-e78af8293b5c@linuxfoundation.org>
Date:   Thu, 12 May 2022 11:40:57 -0600
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: <20220511000811.384766-4-vannapurve@google.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 5/10/22 6:08 PM, Vishal Annapurve wrote:
> Add handling of explicit private/shared memory conversion using
> KVM_HC_MAP_GPA_RANGE and implicit memory conversion by handling
> KVM_EXIT_MEMORY_ERROR.
> 
> Signed-off-by: Vishal Annapurve <vannapurve@google.com>
> ---
>   tools/testing/selftests/kvm/priv_memfd_test.c | 87 +++++++++++++++++++
>   1 file changed, 87 insertions(+)
> 
> diff --git a/tools/testing/selftests/kvm/priv_memfd_test.c b/tools/testing/selftests/kvm/priv_memfd_test.c
> index bbb58c62e186..55e24c893b07 100644
> --- a/tools/testing/selftests/kvm/priv_memfd_test.c
> +++ b/tools/testing/selftests/kvm/priv_memfd_test.c
> @@ -155,6 +155,83 @@ static struct test_run_helper priv_memfd_testsuite[] = {
>   	},
>   };
>   
> +static void handle_vm_exit_hypercall(struct kvm_run *run,
> +	uint32_t test_id)
> +{
> +	uint64_t gpa, npages, attrs;
> +	int priv_memfd =
> +		priv_memfd_testsuite[test_id].priv_memfd;

Do you need this on a separate line? Doesn't looks like it will exceed
the limit with the tab?

> +	int ret;
> +	int fallocate_mode;
> +
> +	if (run->hypercall.nr != KVM_HC_MAP_GPA_RANGE) {
> +		TEST_FAIL("Unhandled Hypercall %lld\n",
> +					run->hypercall.nr);

Is this considered test fail or skip because of unmet dependency?
Also do you need run->hypercall.nr os a separate line?

> +	}
> +
> +	gpa = run->hypercall.args[0];
> +	npages = run->hypercall.args[1];
> +	attrs = run->hypercall.args[2];
> +
> +	if ((gpa < TEST_MEM_GPA) || ((gpa +
> +		(npages << MIN_PAGE_SHIFT)) > TEST_MEM_END)) {
> +		TEST_FAIL("Unhandled gpa 0x%lx npages %ld\n",
> +			gpa, npages);

Same question here about gpa, npages on a separate line? Also
align it with the previous line for readability.

TEST_FAIL("Unhandled gpa 0x%lx npages %ld\n",
	  gpa, npages);
  
> +	}
> +
> +	if (attrs & KVM_MAP_GPA_RANGE_ENCRYPTED)
> +		fallocate_mode = 0;
> +	else {
> +		fallocate_mode = (FALLOC_FL_PUNCH_HOLE |
> +			FALLOC_FL_KEEP_SIZE);
> +	}
> +	pr_info("Converting off 0x%lx pages 0x%lx to %s\n",
> +		(gpa - TEST_MEM_GPA), npages,
> +		fallocate_mode ?
> +			"shared" : "private");
> +	ret = fallocate(priv_memfd, fallocate_mode,
> +		(gpa - TEST_MEM_GPA),
> +		npages << MIN_PAGE_SHIFT);
> +	TEST_ASSERT(ret != -1,
> +		"fallocate failed in hc handling");
> +	run->hypercall.ret = 0;
> +}
> +
> +static void handle_vm_exit_memory_error(struct kvm_run *run,
> +	uint32_t test_id)
> +{
> +	uint64_t gpa, size, flags;
> +	int ret;
> +	int priv_memfd =
> +		priv_memfd_testsuite[test_id].priv_memfd;
> +	int fallocate_mode;
> +
> +	gpa = run->memory.gpa;
> +	size = run->memory.size;
> +	flags = run->memory.flags;
> +
> +	if ((gpa < TEST_MEM_GPA) || ((gpa + size)
> +					> TEST_MEM_END)) {
> +		TEST_FAIL("Unhandled gpa 0x%lx size 0x%lx\n",
> +			gpa, size);
> +	}
> +
> +	if (flags & KVM_MEMORY_EXIT_FLAG_PRIVATE)
> +		fallocate_mode = 0;
> +	else {
> +		fallocate_mode = (FALLOC_FL_PUNCH_HOLE |
> +				FALLOC_FL_KEEP_SIZE);
> +	}
> +	pr_info("Converting off 0x%lx size 0x%lx to %s\n",
> +		(gpa - TEST_MEM_GPA), size,
> +		fallocate_mode ?
> +			"shared" : "private");
> +	ret = fallocate(priv_memfd, fallocate_mode,
> +		(gpa - TEST_MEM_GPA), size);
> +	TEST_ASSERT(ret != -1,
> +		"fallocate failed in memory error handling");
> +}
> +
>   static void vcpu_work(struct kvm_vm *vm, uint32_t test_id)
>   {
>   	struct kvm_run *run;
> @@ -181,6 +258,16 @@ static void vcpu_work(struct kvm_vm *vm, uint32_t test_id)
>   			continue;
>   		}
>   
> +		if (run->exit_reason == KVM_EXIT_HYPERCALL) {
> +			handle_vm_exit_hypercall(run, test_id);
> +			continue;
> +		}
> +
> +		if (run->exit_reason == KVM_EXIT_MEMORY_ERROR) {
> +			handle_vm_exit_memory_error(run, test_id);
> +			continue;
> +		}
> +
>   		TEST_FAIL("Unhandled VCPU exit reason %d\n", run->exit_reason);
>   		break;
>   	}
> 

Looks like you can easily combine lines without running into # chars limit
for several lines of code in this patch. If you haven't already, run
checkpatch to make sure coding guidelines are honored.

thanks,
-- Shuah