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.
