linux-kernel - Re: [RFC 4/5] selftests: KVM: SNP IOCTL test

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]

Message-ID: <5556f872-22fa-44dc-8c25-037b30797fed@amd.com>
Date: Tue, 13 Aug 2024 10:23:15 -0500
From: "Pratik R. Sampat" <pratikrajesh.sampat@....com>
To: Sean Christopherson <seanjc@...gle.com>
CC: <kvm@...r.kernel.org>, <shuah@...nel.org>, <thomas.lendacky@....com>,
	<michael.roth@....com>, <pbonzini@...hat.com>, <pgonda@...gle.com>,
	<linux-kselftest@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [RFC 4/5] selftests: KVM: SNP IOCTL test



On 8/9/2024 10:48 AM, Sean Christopherson wrote:
> On Wed, Jul 10, 2024, Pratik R. Sampat wrote:
>> Introduce testing of SNP ioctl calls. This patch includes both positive
>> and negative tests of various parameters such as flags, page types and
>> policies.
>>
>> Signed-off-by: Pratik R. Sampat <pratikrajesh.sampat@....com>
>> ---
>>  .../selftests/kvm/x86_64/sev_smoke_test.c     | 119 +++++++++++++++++-
>>  1 file changed, 118 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c b/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c
>> index 500c67b3793b..1d5c275c11b3 100644
>> --- a/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c
>> +++ b/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c
>> @@ -186,13 +186,130 @@ static void test_sev_launch(void *guest_code, uint32_t type, uint64_t policy)
>>  	kvm_vm_free(vm);
>>  }
>>  
>> +static int spawn_snp_launch_start(uint32_t type, uint64_t policy, uint8_t flags)
>> +{
>> +	struct kvm_vcpu *vcpu;
>> +	struct kvm_vm *vm;
>> +	int ret;
>> +
>> +	vm = vm_sev_create_with_one_vcpu(type, NULL, &vcpu);
> 
> Is a vCPU actually necessary/interesting?
> 

Yes, vcpu is not really needed here.
I could get away with a simple vm_create variant where I can pass the type.

>> +	ret = snp_vm_launch(vm, policy, flags);
>> +	kvm_vm_free(vm);
>> +
>> +	return ret;
>> +}
>> +
>> +static void test_snp_launch_start(uint32_t type, uint64_t policy)
>> +{
>> +	uint8_t i;
>> +	int ret;
>> +
>> +	ret = spawn_snp_launch_start(type, policy, 0);
> 
> s/spawn/__test, because "spawn" implies there's something living after this.
> 

Ack. Changed.

>> +	TEST_ASSERT(!ret,
>> +		    "KVM_SEV_SNP_LAUNCH_START should not fail, invalid flag.");
> 
> This should go away once vm_sev_ioctl() handles the assertion, but this assert
> message is bad (there's no invalid flag).
> 
>> +
>> +	for (i = 1; i < 8; i++) {
>> +		ret = spawn_snp_launch_start(type, policy, BIT(i));
>> +		TEST_ASSERT(ret && errno == EINVAL,
>> +			    "KVM_SEV_SNP_LAUNCH_START should fail, invalid flag.");
> 
> Print the flag, type, and policy.  In general, please think about what information
> would be helpful if this fails.

Right, I'll be more mindful of the error messages shown and try to
display more useful information from them for the overall series.

Thanks!
Pratik