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

From: Sampat, Pratik Rajesh
Date: Thu Jul 11 2024 - 12:27:23 EST



On 7/11/2024 10:57 AM, Peter Gonda wrote:
> On Wed, Jul 10, 2024 at 4:06 PM Pratik R. Sampat
> <pratikrajesh.sampat@xxxxxxx> 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@xxxxxxx>
>
> Tested-by: Peter Gonda <pgonda@xxxxxxxxxx>
>
>> ---
>> .../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);
>> + 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);
>> + TEST_ASSERT(!ret,
>> + "KVM_SEV_SNP_LAUNCH_START should not fail, 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.");
>> + }
>
> To save readers sometime do we want to comment that flags must be zero?
>

Ack. I can add that comment.

>> +
>> + ret = spawn_snp_launch_start(type, 0, 0);
>> + TEST_ASSERT(ret && errno == EINVAL,
>> + "KVM_SEV_SNP_LAUNCH_START should fail, invalid policy.");
>> +
>> + ret = spawn_snp_launch_start(type, SNP_POLICY_SMT, 0);
>> + TEST_ASSERT(ret && errno == EINVAL,
>> + "KVM_SEV_SNP_LAUNCH_START should fail, invalid policy.");
>> +
>> + ret = spawn_snp_launch_start(type, SNP_POLICY_RSVD_MBO, 0);
>> + TEST_ASSERT(ret && errno == EINVAL,
>> + "KVM_SEV_SNP_LAUNCH_START should fail, invalid policy.");
>
> Ditto on SMT comment, this could pass if SMT was disabled right?
>

Ack.
Yes, it could. Maybe the check I was speaking about earlier about SMT
can be made here as well and based on that we decide if this should fail
or pass.

>> +
>> + ret = spawn_snp_launch_start(type, SNP_POLICY_SMT | SNP_POLICY_RSVD_MBO |
>> + (255 * SNP_POLICY_ABI_MAJOR) |
>> + (255 * SNP_POLICY_ABI_MINOR), 0);
>> + TEST_ASSERT(ret && errno == EIO,
>> + "KVM_SEV_SNP_LAUNCH_START should fail, invalid version.");
>> +}
>> +
>> +static void test_snp_launch_update(uint32_t type, uint64_t policy)
>> +{
>> + struct kvm_vcpu *vcpu;
>> + struct kvm_vm *vm;
>> + int ret;
>> +
>> + for (int pgtype = 0; pgtype <= KVM_SEV_SNP_PAGE_TYPE_CPUID; pgtype++) {
>
> Do we want to test KVM_SEV_SNP_PAGE_TYPE_CPUID+1 to make sure that fails?
>

We could. Looking at loop however, we also go through 0x2 which is
undefined so I thought we were already taking care of the negative test
case here. Having said that, I have no issues in adding one more case
that fails.

>> + vm = vm_sev_create_with_one_vcpu(type, NULL, &vcpu);
>> + snp_vm_launch(vm, policy, 0);
>> + ret = snp_vm_launch_update(vm, pgtype);
>> +
>> + switch (pgtype) {
>> + case KVM_SEV_SNP_PAGE_TYPE_NORMAL:
>> + case KVM_SEV_SNP_PAGE_TYPE_ZERO:
>> + case KVM_SEV_SNP_PAGE_TYPE_UNMEASURED:
>> + case KVM_SEV_SNP_PAGE_TYPE_SECRETS:
>> + TEST_ASSERT(!ret,
>> + "KVM_SEV_SNP_LAUNCH_UPDATE should not fail, invalid Page type.");
>
> Double negative maybe: "KVM_SEV_SNP_LAUNCH_UPDATE should succeed..."
>

Ack. This double negative is used in a couple of more places. Will clean
them up too.

>> + break;
>> + case KVM_SEV_SNP_PAGE_TYPE_CPUID:
>> + TEST_ASSERT(ret && errno == EIO,
>> + "KVM_SEV_SNP_LAUNCH_UPDATE should fail, invalid Page type.");
>
> This is a valid page type right? But I think the error is from the ASP
> due to the page being malformed for a CPUID page.
>

Yes you're absolutely right. It's technically a correct page type just
not set up correctly to be used this way so we should see it fail.

>> + break;
>> + default:
>> + TEST_ASSERT(ret && errno == EINVAL,
>> + "KVM_SEV_SNP_LAUNCH_UPDATE should fail, invalid Page type.");
>> + }
>> +
>> + kvm_vm_free(vm);
>> + }
>> +}
>> +
>> +void test_snp_launch_finish(uint32_t type, uint64_t policy)
>> +{
>> + struct kvm_vcpu *vcpu;
>> + struct kvm_vm *vm;
>> + int ret;
>> +
>> + vm = vm_sev_create_with_one_vcpu(type, NULL, &vcpu);
>> + snp_vm_launch(vm, policy, 0);
>> + snp_vm_launch_update(vm, KVM_SEV_SNP_PAGE_TYPE_NORMAL);
>> + ret = snp_vm_launch_finish(vm, 0);
>> + TEST_ASSERT(!ret,
>> + "KVM_SEV_SNP_LAUNCH_FINISH should not fail, invalid flag.");
>
> Comment is wrong, maybe: "KVM_SEV_SNP_LAUNCH_FINISH should not fail."
>

Thanks for catching this. Will fix the comment.

>> + kvm_vm_free(vm);
>> +
>> + for (int i = 1; i < 16; i++) {
>> + vm = vm_sev_create_with_one_vcpu(type, NULL, &vcpu);
>> + snp_vm_launch(vm, policy, 0);
>> + snp_vm_launch_update(vm, KVM_SEV_SNP_PAGE_TYPE_NORMAL);
>> + ret = snp_vm_launch_finish(vm, BIT(i));
>> + TEST_ASSERT(ret && errno == EINVAL,
>> + "KVM_SEV_SNP_LAUNCH_FINISH should fail, invalid flag.");
>> + kvm_vm_free(vm);
>
> To save readers sometime do we want to comment that flags must be zero?
>

Ack.

Thanks again for the review