Received-SPF: pass (google.com: domain of linux-kernel+bounces-199850-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Mon, 3 Jun 2024 16:55:37 -0700
In-Reply-To: <ZhkKb+lgPDNRsYXa@chao-email>
Precedence: bulk
Mime-Version: 1.0
References: <20240410143446.797262-1-chao.gao@intel.com> <20240410143446.797262-2-chao.gao@intel.com>
 <CALMp9eR294v_2-yXagKR8HM_WbqihJ5JcRwD1NTGvJxsOFsnyw@mail.gmail.com> <ZhkKb+lgPDNRsYXa@chao-email>
Message-ID: <Zl5X-YHQL-tIUb4h@google.com>
Subject: Re: [RFC PATCH v3 01/10] KVM: VMX: Virtualize Intel IA32_SPEC_CTRL
From: Sean Christopherson <seanjc@google.com>
To: Chao Gao <chao.gao@intel.com>
Cc: Jim Mattson <jmattson@google.com>, kvm@vger.kernel.org, linux-kernel@vger.kernel.org, 
	daniel.sneddon@linux.intel.com, pawan.kumar.gupta@linux.intel.com, 
	Paolo Bonzini <pbonzini@redhat.com>, Jonathan Corbet <corbet@lwn.net>, 
	Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>, 
	Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org, 
	"H. Peter Anvin" <hpa@zytor.com>, linux-doc@vger.kernel.org
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Apr 12, 2024, Chao Gao wrote:
> On Thu, Apr 11, 2024 at 09:07:31PM -0700, Jim Mattson wrote:
> >On Wed, Apr 10, 2024 at 7:35=E2=80=AFAM Chao Gao <chao.gao@intel.com> wr=
ote:
> >>
> >> From: Daniel Sneddon <daniel.sneddon@linux.intel.com>
> >>
> >> Currently KVM disables interception of IA32_SPEC_CTRL after a non-0 is
> >> written to IA32_SPEC_CTRL by guest. The guest is allowed to write any
> >> value directly to hardware. There is a tertiary control for
> >> IA32_SPEC_CTRL. This control allows for bits in IA32_SPEC_CTRL to be
> >> masked to prevent guests from changing those bits.
> >>
> >> Add controls setting the mask for IA32_SPEC_CTRL and desired value for
> >> masked bits.
> >>
> >> These new controls are especially helpful for protecting guests that
> >> don't know about BHI_DIS_S and that are running on hardware that
> >> supports it. This allows the hypervisor to set BHI_DIS_S to fully
> >> protect the guest.
> >>
> >> Suggested-by: Sean Christopherson <seanjc@google.com>
> >> Signed-off-by: Daniel Sneddon <daniel.sneddon@linux.intel.com>
> >> Signed-off-by: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
> >> [ add a new ioctl to report supported bits. Fix the inverted check ]
> >> Signed-off-by: Chao Gao <chao.gao@intel.com>
> >
> >This looks quite Intel-centric. Isn't this feature essentially the
> >same as AMD's V_SPEC_CTRL?

In spirit, yes.  In practice, not really.  The implementations required for=
 each
end up being quite different.  I think the only bit of code that could be r=
eused
by SVM, and isn't already, is the generation of supported_force_spec_ctrl.

+       kvm_caps.supported_force_spec_ctrl =3D 0;
+
+       if (cpu_has_spec_ctrl_shadow()) {
+               kvm_caps.supported_force_spec_ctrl |=3D SPEC_CTRL_IBRS;
+
+               if (boot_cpu_has(X86_FEATURE_STIBP))
+                       kvm_caps.supported_force_spec_ctrl |=3D SPEC_CTRL_S=
TIBP;
+
+               if (boot_cpu_has(X86_FEATURE_SSBD))
+                       kvm_caps.supported_force_spec_ctrl |=3D SPEC_CTRL_S=
SBD;
+
+               if (boot_cpu_has(X86_FEATURE_RRSBA_CTRL) &&
+                   (host_arch_capabilities & ARCH_CAP_RRSBA))
+                       kvm_caps.supported_force_spec_ctrl |=3D SPEC_CTRL_R=
RSBA_DIS_S;
+
+               if (boot_cpu_has(X86_FEATURE_BHI_CTRL))
+                       kvm_caps.supported_force_spec_ctrl |=3D SPEC_CTRL_B=
HI_DIS_S;
+       }

> Yes. they are almost the same. one small difference is intel's version ca=
n
> force some bits off though I don't see how forcing bits off can be useful=
.

Another not-so-small difference is that Intel's version can also force bits=
 *on*,
and force them on only for the guest with minimal overhead.

> >Can't we consolidate the code, rather than
> >having completely independent implementations for AMD and Intel?
>=20
> We surely can consolidate the code. I will do this.
>=20
> I have a question about V_SPEC_CTRL. w/ V_SPEC_CTRL, the SPEC_CTRL MSR re=
tains
> the host's value on VM-enter:
>=20
> .macro RESTORE_GUEST_SPEC_CTRL
>         /* No need to do anything if SPEC_CTRL is unset or V_SPEC_CTRL is=
 set */
>         ALTERNATIVE_2 "", \
>                 "jmp 800f", X86_FEATURE_MSR_SPEC_CTRL, \
>                 "", X86_FEATURE_V_SPEC_CTRL
>=20
> Does this mean all mitigations used by the host will be enabled for the g=
uest
> and guests cannot disable them?

Yes.

> Is this intentional? this looks suboptimal. Why not set SPEC_CTRL value t=
o 0 and
> let guest decide which features to enable? On the VMX side, we need host =
to
> apply certain hardware mitigations (i.e., BHI_DIS_S and RRSBA_DIS_S) for =
guest
> because BHI's software mitigation may be ineffective. I am not sure why S=
VM is
> enabling all mitigations used by the host for guests. Wouldn't it be bett=
er to
> enable them on an as-needed basis?

AMD's V_SPEC_CTRL doesn't provide a fast context switch of SPEC_CTRL, it pe=
rforms
a bitwise-OR of the host and guest values.  So to load a subset (or superse=
t) of
the host protections, KVM would need to do an extra WRMSR before VMRUN, and=
 again
after VMRUN.

That said, I have no idea whether or not avoiding WRMSR on AMD is optimal.