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

["Sampat, Pratik Rajesh" <pratikrajesh.sampat@amd.com>]

On 7/11/2024 10:57 AM, Peter Gonda wrote:
> On Wed, Jul 10, 2024 at 4:06 PM Pratik R. Sampat
> <pratikrajesh.sampat@amd.com> 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@amd.com>
> 
> Tested-by: Peter Gonda <pgonda@google.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);
>> +       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