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?
