On Mon, Oct 28, 2024, Pratik R. Sampat wro4te:
On 10/28/2024 12:55 PM, Sean Christopherson wrote:
On Mon, Oct 21, 2024, Pratik R. Sampat wrote:
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); \ })
No, unless it's the least awful option, don't hide dynamic functionality in a macro that looks like it holds static data. The idea is totally fine, but put it in an actual helper, not a macro, _if_ there's actually a need for a default policy. If there's only ever one main path that creates SNP VMs, then I don't see the point in specifying a default policy.
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.
Ugh, one is SEV_xxx, the other is SNP_xxx. Argh! And IIUC, they are mutually exclusive (totally separate thigns?), because SNP guests use an 8-byte structure, whereas SEV/SEV-ES use a 4-byte structure, and with different layouts.
That means this is _extremely_ confusing. Separate the SEV_xxx defines from the SNP_xxx defines, because other than a name, they have nothing in common.
+/* Minimum firmware version required for the SEV-SNP support */ +#define SNP_FW_REQ_VER_MAJOR 1 +#define SNP_FW_REQ_VER_MINOR 51
Side topic, why are these hardcoded? And where did they come from? If they're arbitrary KVM selftests values, make that super duper clear.
+#define SNP_POLICY_MINOR_BIT 0 +#define SNP_POLICY_MAJOR_BIT 8
s/BIT/SHIFT. "BIT" implies they are a single bit, which is obviously not the case. But I vote to omit the extra #define entirely and just open code the shift in the SNP_FW_VER_{MAJOR,MINOR} macros.
#define SEV_POLICY_NO_DBG (1UL << 0) #define SEV_POLICY_ES (1UL << 2) +#define SNP_POLICY_SMT (1ULL << 16) +#define SNP_POLICY_RSVD_MBO (1ULL << 17) +#define SNP_POLICY_DBG (1ULL << 19) +#define SNP_POLICY (SNP_POLICY_SMT | SNP_POLICY_RSVD_MBO) + +#define SNP_FW_VER_MAJOR(maj) ((uint8_t)(maj) << SNP_POLICY_MAJOR_BIT) +#define SNP_FW_VER_MINOR(min) ((uint8_t)(min) << SNP_POLICY_MINOR_BIT)