On 2/11/25 8:12 PM, Sean Christopherson wrote:
> On Mon, Feb 03, 2025, 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.
>>
>> Tested-by: Srikanth Aithal <sraithal@xxxxxxx>
>> Signed-off-by: Pratik R. Sampat <prsampat@xxxxxxx>
[..snip..]
>> +/*
>> + * A SEV-SNP VM requires the policy reserved bit to always be set.
>> + * The SMT policy bit is also required to be set based on SMT being
>> + * available and active on the system.
>> + */
>> +static inline u64 snp_default_policy(void)
>> +{
>> + bool smt_active = false;
>> + FILE *f;
>> +
>> + f = fopen("/sys/devices/system/cpu/smt/active", "r");
>
> Please add a helper to query if SMT is enabled. I doubt there will ever be many
> users of this, but it doesn't seem like something that should buried in SNP code.
>
> Ha! smt_possible() in tools/testing/selftests/kvm/x86/hyperv_cpuid.c is already
> guilty of burying a related helper, and it looks like it's a more robust version.
>
You're right, a more general helper is in order here.
Since the hyperv_cpuid selftest only seems to care about whether SMT is
possible (i.e., it may or may not be enabled) and we care about it being
enabled as well, for the flag to be set. I should make a more generic
variant(s) that can be accessible to both. Maybe I can implement it
within testing/selftests/kvm/include/x86_processor.h?
>> + if (f) {
>> + smt_active = fgetc(f) - '0';
>> + fclose(f);
>> + }
>> +
>> + return SNP_POLICY_RSVD_MBO | (smt_active ? SNP_POLICY_SMT : 0);
>> +}
>> +
>> /*
>> * The KVM_MEMORY_ENCRYPT_OP uAPI is utter garbage and takes an "unsigned long"
>> * instead of a proper struct. The size of the parameter is embedded in the
>> @@ -76,6 +109,7 @@ kvm_static_assert(SEV_RET_SUCCESS == 0);
>>
>> void sev_vm_init(struct kvm_vm *vm);
>> void sev_es_vm_init(struct kvm_vm *vm);
>> +void snp_vm_init(struct kvm_vm *vm);
>>
>> static inline void sev_register_encrypted_memory(struct kvm_vm *vm,
>> struct userspace_mem_region *region)
>> @@ -99,4 +133,17 @@ static inline void sev_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa,
>> vm_sev_ioctl(vm, KVM_SEV_LAUNCH_UPDATE_DATA, &update_data);
>> }
>>
>> +static inline void snp_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa,
>> + uint64_t hva, uint64_t size, uint8_t type)
>> +{
>> + struct kvm_sev_snp_launch_update update_data = {
>> + .uaddr = hva,
>> + .gfn_start = gpa >> PAGE_SHIFT,
>> + .len = size,
>> + .type = type,
>> + };
>> +
>> + vm_sev_ioctl(vm, KVM_SEV_SNP_LAUNCH_UPDATE, &update_data);
>> +}
>> +
>> #endif /* SELFTEST_KVM_SEV_H */
>> diff --git a/tools/testing/selftests/kvm/lib/x86/sev.c b/tools/testing/selftests/kvm/lib/x86/sev.c
>> index 280ec42e281b..17d493e9907a 100644
>> --- a/tools/testing/selftests/kvm/lib/x86/sev.c
>> +++ b/tools/testing/selftests/kvm/lib/x86/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,13 +42,35 @@ 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;
>>
>> - sev_launch_update_data(vm, gpa_base + offset, size);
>> + if (is_sev_snp_vm(vm)) {
>
> Curly braces are unnecessary.
Ack. Will be remove them.
>
>> + 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);
>> + }
>> + }
>> +}
>> +
>> +static void privatize_region(struct kvm_vm *vm, struct userspace_mem_region *region)
>
> Can't this just be a param to encrypt_region() that also says "make it private"?
>
Sure, I can parameterize encrypt_region() to have a private boolean.
The idea was essentially to ensure that privatizing and encryption can
be called separately if they wanted to. However, since this is
currently only used in tandem (by SNP) I can remove this to reduce
duplication.
>> +{
>> + const struct sparsebit *protected_phy_pages = region->protected_phy_pages;
>> + const vm_paddr_t gpa_base = region->region.guest_phys_addr;
>> + const sparsebit_idx_t lowest_page_in_region = gpa_base >> vm->page_shift;
>> + sparsebit_idx_t i, j;
>> +
>> + 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;
>> +
>> + vm_mem_set_private(vm, gpa_base + offset, size);
>> }
>> }
>>
>> @@ -77,6 +100,14 @@ void sev_es_vm_init(struct kvm_vm *vm)
>> }
>> }
>>
>> +void snp_vm_init(struct kvm_vm *vm)
>> +{
>> + struct kvm_sev_init init = { 0 };
>> +
>> + assert(vm->type == KVM_X86_SNP_VM);
>
> Use TEST_ASSERT(), or do nothing, don't use assert().
>
Alright, I can do that.
I will also send another cleanups patch to remove asserts() in the
SEV, SEV-ES counterparts too in that case!
>> + vm_sev_ioctl(vm, KVM_SEV_INIT2, &init);
>> +}
>> +
>> void sev_vm_launch(struct kvm_vm *vm, uint32_t policy)
>> {
>> struct kvm_sev_launch_start launch_start = {
>> @@ -93,7 +124,7 @@ void sev_vm_launch(struct kvm_vm *vm, uint32_t policy)
>> TEST_ASSERT_EQ(status.state, SEV_GUEST_STATE_LAUNCH_UPDATE);
>>
>> hash_for_each(vm->regions.slot_hash, ctr, region, slot_node)
>> - encrypt_region(vm, region);
>> + encrypt_region(vm, region, 0);
>
> Please add an enum/macro instead of open coding a literal '0'. I gotta assume
> there's an appropriate name for page type '0'.
>
For SNP, we supply this parameter to determine the page type for SNP
launch update defined as KVM_SEV_SNP_PAGE_TYPE_*. For SEV/SEV-ES,
however, the page type doesn't really get factored in and falls through
unaccounted in that case, so I had passed a zero to it.
Having said that, having a literal here is quite unclean. Maybe I can
pass one of the existing page types to it or, better yet, define a new
KVM_SEV_PAGE_TYPE_[RESERVED|UNUSED] type instead for our selftest
header?
>>
>> if (policy & SEV_POLICY_ES)
>> vm_sev_ioctl(vm, KVM_SEV_LAUNCH_UPDATE_VMSA, NULL);
