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/program...
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/specifi...
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.