On Fri, Feb 14, 2025, Pratik Rajesh Sampat wrote: > 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? It should go in kvm_util.h, /sys/devices/system/cpu/smt/active is a generic interface provided by kernel/cpu.c. > >> @@ -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? Ya, define something new and arbitrary. I vote for either KVM_SEV_PAGE_TYPE_NONE or KVM_SEV_PAGE_TYPE_INVALID. RESERVED suggests the page is "valid" but reserved for some entity. Ditto for UNUSED; valid, but not yet claimed.