Re: [PATCH v4 5/8] KVM: selftests: Add library support for interacting with SNP

Pratik Rajesh Sampat
2025-01-13T20:10:24Z

On 1/13/25 2:32 AM, Nikunj A. Dadhania wrote:
>
>
> On 11/15/2024 5:11 AM, Pratik R. Sampat wrote:
>> Extend the SEV library to include support for SNP ioctl() wrappers,
>> which aid in launching and interacting with a SEV-SNP guest.
>>
>> Signed-off-by: Pratik R. Sampat <pratikrajesh.sampat@amd.com>
>> ---
>> .../selftests/kvm/include/x86_64/sev.h | 49 ++++++++++-
>> tools/testing/selftests/kvm/lib/x86_64/sev.c | 81 ++++++++++++++++++-
>> 2 files changed, 125 insertions(+), 5 deletions(-)
>
>
>> diff --git a/tools/testing/selftests/kvm/lib/x86_64/sev.c b/tools/testing/selftests/kvm/lib/x86_64/sev.c
>> index d6e7a422b69d..40b90d3a5769 100644
>> --- a/tools/testing/selftests/kvm/lib/x86_64/sev.c
>> +++ b/tools/testing/selftests/kvm/lib/x86_64/sev.c
>> @@ -31,7 +31,8 @@ bool is_sev_vm(struct kvm_vm *vm)
>> * and find the first range, but that's correct because the condition
>> * expression would cause us to quit the loop.
>> */
>> -static void encrypt_region(struct kvm_vm *vm, struct userspace_mem_region *region)
>> +static void encrypt_region(struct kvm_vm *vm, struct userspace_mem_region *region,
>> + uint8_t page_type)
>> {
>> const struct sparsebit *protected_phy_pages = region->protected_phy_pages;
>> const vm_paddr_t gpa_base = region->region.guest_phys_addr;
>> @@ -41,16 +42,39 @@ static void encrypt_region(struct kvm_vm *vm, struct userspace_mem_region *regio
>> if (!sparsebit_any_set(protected_phy_pages))
>> return;
>>
>> - sev_register_encrypted_memory(vm, region);
>> + if (!is_sev_snp_vm(vm))
>> + sev_register_encrypted_memory(vm, region);
>>
>> sparsebit_for_each_set_range(protected_phy_pages, i, j) {
>> const uint64_t size = (j - i + 1) * vm->page_size;
>> const uint64_t offset = (i - lowest_page_in_region) * vm->page_size;
>>
>> + if (is_sev_snp_vm(vm)) {
>> + snp_launch_update_data(vm, gpa_base + offset,
>> + (uint64_t)addr_gpa2hva(vm, gpa_base + offset),
>> + size, page_type);
>> + continue;
>> + }
>> +
>
> Instead of using continue, if/else is the better code flow:
>
> if (is_sev_snp_vm(vm))
> snp_launch_update_data(vm, gpa_base + offset,
> (uint64_t)addr_gpa2hva(vm, gpa_base + offset),
> size, page_type);
> else
> sev_launch_update_data(vm, gpa_base + offset, size);
>
> Right?

Sure, I can change that. That's definitely cleaner.

Thanks!
Pratik