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

["Pratik R. Sampat" <pratikrajesh.sampat@amd.com>]

Hi Sean,

On 10/28/2024 12:55 PM, Sean Christopherson wrote:
> 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.

That's fair. It is related to the FW supplied to it from userspace and
naming it with kvm prefix is misleading. I'll change that.

> 
>>>> +		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.
> 

Right, SMT should match the host configuration. Would a
SNP_DEFAULT_POLICY work if we made it check for SMT too in the macro?

Instead of,
#define SNP_POLICY	(SNP_POLICY_SMT | SNP_POLICY_RSVD_MBO)

Have something like this instead to make it generic and less ambiguous?
#define SNP_DEFAULT_POLICY()		 			       \
({								       \
	SNP_POLICY_RSVD_MBO | (is_smt_active() ? SNP_POLICY_SMT : 0);  \
})

> 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?

SEV_POLICY_NO_DBG is mainly for the guest policy structure of SEV and
SEV-ES - pg 31, Table 2
https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/programmer-references/55766_SEV-KM_API_Specification.pdf

and, SNP_POLICY_DBG is a bit in the guest policy structure of SNP - pg
27, Table 9
https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/specifications/56860.pdf

In the former, a SEV guest disables debugging if SEV_POLICY_NO_DBG is
set. Similarly, a SNP guest enables debugging if SNP_POLICY_DBG is set.

An SNP guest can certainly just have the policy SNP_POLICY_RSVD_MBO,
barring the case on a SMT system where that bit must be set too for a
successful launch.