Re: [PATCH v3 2/9] KVM: selftests: Add a basic SNP smoke test

[Sean Christopherson <seanjc@google.com>]

On Mon, Oct 21, 2024, Pratik R. Sampat wrote:
> >> +		test_sev(guest_sev_es_code, KVM_X86_SEV_ES_VM, SEV_POLICY_ES);
> >>  
> >>  		test_sev_es_shutdown();
> >>  
> >>  		if (kvm_has_cap(KVM_CAP_XCRS) &&
> >>  		    (xgetbv(0) & XFEATURE_MASK_X87_AVX) == XFEATURE_MASK_X87_AVX) {
> >> -			test_sync_vmsa(0);
> >> -			test_sync_vmsa(SEV_POLICY_NO_DBG);
> >> +			test_sync_vmsa(KVM_X86_SEV_ES_VM, SEV_POLICY_ES);
> >> +			test_sync_vmsa(KVM_X86_SEV_ES_VM, SEV_POLICY_ES | SEV_POLICY_NO_DBG);
> >> +		}
> >> +	}
> >> +
> >> +	if (kvm_cpu_has(X86_FEATURE_SNP) && is_kvm_snp_supported()) {
> > 
> > Why do we need both?  KVM shouldn't advertise SNP if it's not supported.
> 
> My rationale behind needing this was that the feature can be advertised
> but it may not have the right API major or minor release which could be
> updated post boot and could determine it's support during runtime.

KVM will never determine support after KVM has been loaded.  If *KVM* has a
dependency on the API major.minor, then X86_FEATURE_SNP must be set if and only
if the supported API version is available.

If the API major.minor is purely a userspace thing, then is_kvm_snp_supported()
is misnamed, because the check has nothing to do with KVM.  E.g. something like
is_snp_api_version_supported() would be more appropriate.

> >> +		unsigned long snp_policy = SNP_POLICY;
> > 
> > u64, no?
> 
> Yes, sorry for the oversight. Will change it to u64.
> 
> > 
> >> +
> >> +		if (unlikely(!is_smt_active()))
> >> +			snp_policy &= ~SNP_POLICY_SMT;
> > 
> > Why does SNP_POLICY assume SMT?  And what is RSVD_MBO?  E.g. why not this?
> > 
> > 		u64 policy = is_smt_active() ? SNP_POLICY_SMT : SNP_POLICY;
> > 
> 
> I think most systems support SMT so I enabled the bit in by default and
> only unset it when there isn't any support.

That's confusing though, because you're mixing architectural defines with semi-
arbitrary selftests behavior.  RSVD_MBO on the other is apparently tightly coupled
with SNP, i.e. SNP can't exist without that bit, so it makes sense that RSVD_MBO
needs to be part of SNP_POLICY

If you want to have a *software*-defined default policy, then make it obvious that
it's software defined.  E.g. name the #define SNP_DEFAULT_POLICY, not simply
SNP_POLICY, because the latter is too easily misconstrued as the base SNP policy,
which it is not.  That said, IIUC, SMT *must* match the host configuration, i.e.
whether or not SMT is set is non-negotiable.  In that case, there's zero value in
defining SNP_DEFAULT_POLICY, because it can't be a sane default for all systems.

Side topic, I assume one of SEV_POLICY_NO_DBG or SNP_POLICY_DBG *must* be specified, 
and that they are mutualy exclusive?  E.g. what happens if the full policy is simply
SNP_POLICY_RSVD_MBO?