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)
