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/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. 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)