Re: [PATCH v6 6/9] KVM: selftests: Add library support for interacting with SNP

[Sean Christopherson <seanjc@google.com>]

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@amd.com>
> >> Signed-off-by: Pratik R. Sampat <prsampat@amd.com>
> 
> [..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.