linux-kernel - Re: [PATCH v3 2/9] KVM: selftests: Add a basic SNP smoke test

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZyI4cRLsaTQ3FMk7@google.com>
Date: Wed, 30 Oct 2024 06:46:24 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: "Pratik R. Sampat" <pratikrajesh.sampat@....com>
Cc: kvm@...r.kernel.org, pbonzini@...hat.com, pgonda@...gle.com, 
	thomas.lendacky@....com, michael.roth@....com, shuah@...nel.org, 
	linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/9] KVM: selftests: Add a basic SNP smoke test

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)