When running as an SEV-ES guest, the PL0_SSP, PL1_SSP, PL2_SSP, PL3_SSP,
and U_CET fields in the VMCB save area are type B, meaning the host
state is automatically loaded on a VMEXIT, but is not saved on a VMRUN.
The other shadow stack MSRs, S_CET, SSP, and ISST_ADDR are type A,
meaning they are loaded on VMEXIT and saved on VMRUN. Manually save the
type B host MSR values before VMRUN.
Signed-off-by: John Allen <[email protected]>
---
arch/x86/kvm/svm/sev.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index c25aeb550cd9..03dd68bddd51 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -3028,6 +3028,19 @@ void sev_es_prepare_switch_to_guest(struct sev_es_save_area *hostsa)
/* MSR_IA32_XSS is restored on VMEXIT, save the currnet host value */
hostsa->xss = host_xss;
+
+ if (boot_cpu_has(X86_FEATURE_SHSTK)) {
+ /*
+ * MSR_IA32_U_CET, MSR_IA32_PL0_SSP, MSR_IA32_PL1_SSP,
+ * MSR_IA32_PL2_SSP, and MSR_IA32_PL3_SSP are restored on
+ * VMEXIT, save the current host values.
+ */
+ rdmsrl(MSR_IA32_U_CET, hostsa->u_cet);
+ rdmsrl(MSR_IA32_PL0_SSP, hostsa->vmpl0_ssp);
+ rdmsrl(MSR_IA32_PL1_SSP, hostsa->vmpl1_ssp);
+ rdmsrl(MSR_IA32_PL2_SSP, hostsa->vmpl2_ssp);
+ rdmsrl(MSR_IA32_PL3_SSP, hostsa->vmpl3_ssp);
+ }
}
void sev_vcpu_deliver_sipi_vector(struct kvm_vcpu *vcpu, u8 vector)
--
2.39.1
On Wed, May 24, 2023, John Allen wrote:
> When running as an SEV-ES guest, the PL0_SSP, PL1_SSP, PL2_SSP, PL3_SSP,
> and U_CET fields in the VMCB save area are type B, meaning the host
> state is automatically loaded on a VMEXIT, but is not saved on a VMRUN.
> The other shadow stack MSRs, S_CET, SSP, and ISST_ADDR are type A,
> meaning they are loaded on VMEXIT and saved on VMRUN. Manually save the
> type B host MSR values before VMRUN.
>
> Signed-off-by: John Allen <[email protected]>
> ---
> arch/x86/kvm/svm/sev.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index c25aeb550cd9..03dd68bddd51 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -3028,6 +3028,19 @@ void sev_es_prepare_switch_to_guest(struct sev_es_save_area *hostsa)
>
> /* MSR_IA32_XSS is restored on VMEXIT, save the currnet host value */
> hostsa->xss = host_xss;
> +
> + if (boot_cpu_has(X86_FEATURE_SHSTK)) {
> + /*
> + * MSR_IA32_U_CET, MSR_IA32_PL0_SSP, MSR_IA32_PL1_SSP,
> + * MSR_IA32_PL2_SSP, and MSR_IA32_PL3_SSP are restored on
> + * VMEXIT, save the current host values.
> + */
> + rdmsrl(MSR_IA32_U_CET, hostsa->u_cet);
> + rdmsrl(MSR_IA32_PL0_SSP, hostsa->vmpl0_ssp);
> + rdmsrl(MSR_IA32_PL1_SSP, hostsa->vmpl1_ssp);
> + rdmsrl(MSR_IA32_PL2_SSP, hostsa->vmpl2_ssp);
> + rdmsrl(MSR_IA32_PL3_SSP, hostsa->vmpl3_ssp);
Heh, can you send a patch to fix the names for the PLx_SSP fields? They should
be ->plN_ssp, not ->vmplN_ssp.
As for the values themselves, the kernel doesn't support Supervisor Shadow Stacks
(SSS), so PL0-2_SSP are guaranteed to be zero. And if/when SSS support is added,
I doubt the kernel will ever use PL1_SSP or PL2_SSP, so those can probably be
ignored entirely, and PL0_SSP might be constant per task? In other words, I don't
see any reason to try and track the host values for support that doesn't exist,
just do what VMX does for BNDCFGS and yell if the MSRs are non-zero. Though for
SSS it probably makes sense for KVM to refuse to load (KVM continues on for BNDCFGS
because it's a pretty safe assumption that the kernel won't regain MPX supported).
E.g. in rough pseudocode
if (boot_cpu_has(X86_FEATURE_SHSTK)) {
rdmsrl(MSR_IA32_PLx_SSP, host_plx_ssp);
if (WARN_ON_ONCE(host_pl0_ssp || host_pl1_ssp || host_pl2_ssp))
return -EIO;
}
On Fri, Jun 23, 2023 at 02:11:46PM -0700, Sean Christopherson wrote:
> On Wed, May 24, 2023, John Allen wrote:
> > When running as an SEV-ES guest, the PL0_SSP, PL1_SSP, PL2_SSP, PL3_SSP,
> > and U_CET fields in the VMCB save area are type B, meaning the host
> > state is automatically loaded on a VMEXIT, but is not saved on a VMRUN.
> > The other shadow stack MSRs, S_CET, SSP, and ISST_ADDR are type A,
> > meaning they are loaded on VMEXIT and saved on VMRUN. Manually save the
> > type B host MSR values before VMRUN.
> >
> > Signed-off-by: John Allen <[email protected]>
> > ---
> > arch/x86/kvm/svm/sev.c | 13 +++++++++++++
> > 1 file changed, 13 insertions(+)
> >
> > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> > index c25aeb550cd9..03dd68bddd51 100644
> > --- a/arch/x86/kvm/svm/sev.c
> > +++ b/arch/x86/kvm/svm/sev.c
> > @@ -3028,6 +3028,19 @@ void sev_es_prepare_switch_to_guest(struct sev_es_save_area *hostsa)
> >
> > /* MSR_IA32_XSS is restored on VMEXIT, save the currnet host value */
> > hostsa->xss = host_xss;
> > +
> > + if (boot_cpu_has(X86_FEATURE_SHSTK)) {
> > + /*
> > + * MSR_IA32_U_CET, MSR_IA32_PL0_SSP, MSR_IA32_PL1_SSP,
> > + * MSR_IA32_PL2_SSP, and MSR_IA32_PL3_SSP are restored on
> > + * VMEXIT, save the current host values.
> > + */
> > + rdmsrl(MSR_IA32_U_CET, hostsa->u_cet);
> > + rdmsrl(MSR_IA32_PL0_SSP, hostsa->vmpl0_ssp);
> > + rdmsrl(MSR_IA32_PL1_SSP, hostsa->vmpl1_ssp);
> > + rdmsrl(MSR_IA32_PL2_SSP, hostsa->vmpl2_ssp);
> > + rdmsrl(MSR_IA32_PL3_SSP, hostsa->vmpl3_ssp);
>
> Heh, can you send a patch to fix the names for the PLx_SSP fields? They should
> be ->plN_ssp, not ->vmplN_ssp.
Yes, I will include a patch to address this in the next version of the
series.
>
> As for the values themselves, the kernel doesn't support Supervisor Shadow Stacks
> (SSS), so PL0-2_SSP are guaranteed to be zero. And if/when SSS support is added,
> I doubt the kernel will ever use PL1_SSP or PL2_SSP, so those can probably be
> ignored entirely, and PL0_SSP might be constant per task? In other words, I don't
> see any reason to try and track the host values for support that doesn't exist,
> just do what VMX does for BNDCFGS and yell if the MSRs are non-zero. Though for
> SSS it probably makes sense for KVM to refuse to load (KVM continues on for BNDCFGS
> because it's a pretty safe assumption that the kernel won't regain MPX supported).
>
> E.g. in rough pseudocode
>
> if (boot_cpu_has(X86_FEATURE_SHSTK)) {
> rdmsrl(MSR_IA32_PLx_SSP, host_plx_ssp);
>
> if (WARN_ON_ONCE(host_pl0_ssp || host_pl1_ssp || host_pl2_ssp))
> return -EIO;
> }
The function in question returns void and wouldn't be able to return a
failure code to callers. We would have to rework this path in order to
fail in this way. Is it sufficient to just WARN_ON_ONCE here or is there
some other way we can cause KVM to fail to load here?
On Tue, Aug 01, 2023, John Allen wrote:
> On Fri, Jun 23, 2023 at 02:11:46PM -0700, Sean Christopherson wrote:
> > On Wed, May 24, 2023, John Allen wrote:
> > As for the values themselves, the kernel doesn't support Supervisor Shadow Stacks
> > (SSS), so PL0-2_SSP are guaranteed to be zero. And if/when SSS support is added,
> > I doubt the kernel will ever use PL1_SSP or PL2_SSP, so those can probably be
> > ignored entirely, and PL0_SSP might be constant per task? In other words, I don't
> > see any reason to try and track the host values for support that doesn't exist,
> > just do what VMX does for BNDCFGS and yell if the MSRs are non-zero. Though for
> > SSS it probably makes sense for KVM to refuse to load (KVM continues on for BNDCFGS
> > because it's a pretty safe assumption that the kernel won't regain MPX supported).
> >
> > E.g. in rough pseudocode
> >
> > if (boot_cpu_has(X86_FEATURE_SHSTK)) {
> > rdmsrl(MSR_IA32_PLx_SSP, host_plx_ssp);
> >
> > if (WARN_ON_ONCE(host_pl0_ssp || host_pl1_ssp || host_pl2_ssp))
> > return -EIO;
> > }
>
> The function in question returns void and wouldn't be able to return a
> failure code to callers. We would have to rework this path in order to
> fail in this way. Is it sufficient to just WARN_ON_ONCE here or is there
> some other way we can cause KVM to fail to load here?
Sorry, I should have been more explicit than "it probably make sense for KVM to
refuse to load". The above would go somewhere in __kvm_x86_vendor_init().
On Tue, Aug 01, 2023 at 09:28:11AM -0700, Sean Christopherson wrote:
> On Tue, Aug 01, 2023, John Allen wrote:
> > On Fri, Jun 23, 2023 at 02:11:46PM -0700, Sean Christopherson wrote:
> > > On Wed, May 24, 2023, John Allen wrote:
> > > As for the values themselves, the kernel doesn't support Supervisor Shadow Stacks
> > > (SSS), so PL0-2_SSP are guaranteed to be zero. And if/when SSS support is added,
> > > I doubt the kernel will ever use PL1_SSP or PL2_SSP, so those can probably be
> > > ignored entirely, and PL0_SSP might be constant per task? In other words, I don't
> > > see any reason to try and track the host values for support that doesn't exist,
> > > just do what VMX does for BNDCFGS and yell if the MSRs are non-zero. Though for
> > > SSS it probably makes sense for KVM to refuse to load (KVM continues on for BNDCFGS
> > > because it's a pretty safe assumption that the kernel won't regain MPX supported).
> > >
> > > E.g. in rough pseudocode
> > >
> > > if (boot_cpu_has(X86_FEATURE_SHSTK)) {
> > > rdmsrl(MSR_IA32_PLx_SSP, host_plx_ssp);
> > >
> > > if (WARN_ON_ONCE(host_pl0_ssp || host_pl1_ssp || host_pl2_ssp))
> > > return -EIO;
> > > }
> >
> > The function in question returns void and wouldn't be able to return a
> > failure code to callers. We would have to rework this path in order to
> > fail in this way. Is it sufficient to just WARN_ON_ONCE here or is there
> > some other way we can cause KVM to fail to load here?
>
> Sorry, I should have been more explicit than "it probably make sense for KVM to
> refuse to load". The above would go somewhere in __kvm_x86_vendor_init().
I see, in that case that change should probably go up with:
"KVM:x86: Enable CET virtualization for VMX and advertise to userspace"
in Weijiang Yang's series with the rest of the changes to
__kvm_x86_vendor_init(). Though I can tack it on in my series if
needed.
On 8/2/2023 1:03 AM, John Allen wrote:
> On Tue, Aug 01, 2023 at 09:28:11AM -0700, Sean Christopherson wrote:
>> On Tue, Aug 01, 2023, John Allen wrote:
>>> On Fri, Jun 23, 2023 at 02:11:46PM -0700, Sean Christopherson wrote:
>>>> On Wed, May 24, 2023, John Allen wrote:
>>>> As for the values themselves, the kernel doesn't support Supervisor Shadow Stacks
>>>> (SSS), so PL0-2_SSP are guaranteed to be zero. And if/when SSS support is added,
>>>> I doubt the kernel will ever use PL1_SSP or PL2_SSP, so those can probably be
>>>> ignored entirely, and PL0_SSP might be constant per task? In other words, I don't
>>>> see any reason to try and track the host values for support that doesn't exist,
>>>> just do what VMX does for BNDCFGS and yell if the MSRs are non-zero. Though for
>>>> SSS it probably makes sense for KVM to refuse to load (KVM continues on for BNDCFGS
>>>> because it's a pretty safe assumption that the kernel won't regain MPX supported).
>>>>
>>>> E.g. in rough pseudocode
>>>>
>>>> if (boot_cpu_has(X86_FEATURE_SHSTK)) {
>>>> rdmsrl(MSR_IA32_PLx_SSP, host_plx_ssp);
>>>>
>>>> if (WARN_ON_ONCE(host_pl0_ssp || host_pl1_ssp || host_pl2_ssp))
>>>> return -EIO;
>>>> }
>>> The function in question returns void and wouldn't be able to return a
>>> failure code to callers. We would have to rework this path in order to
>>> fail in this way. Is it sufficient to just WARN_ON_ONCE here or is there
>>> some other way we can cause KVM to fail to load here?
>> Sorry, I should have been more explicit than "it probably make sense for KVM to
>> refuse to load". The above would go somewhere in __kvm_x86_vendor_init().
> I see, in that case that change should probably go up with:
> "KVM:x86: Enable CET virtualization for VMX and advertise to userspace"
> in Weijiang Yang's series with the rest of the changes to
> __kvm_x86_vendor_init(). Though I can tack it on in my series if
> needed.
The downside with above WARN_ON check is, KVM has to clear PL{0,1,2}_SSP
for all CPUs when
SVM/VMX module is unloaded given guest would use them, otherwise, it may
hit the check next
time the module is reloaded.
Can we add check as below to make it easier?
@@ -9616,6 +9618,24 @@ static int __kvm_x86_vendor_init(struct
kvm_x86_init_ops *ops)
return -EIO;
}
+ if (boot_cpu_has(X86_FEATURE_SHSTK)) {
+ rdmsrl(MSR_IA32_S_CET, host_s_cet);
+ if (host_s_cet & CET_SHSTK_EN) {
+ /*
+ * Current CET KVM solution assumes host supervisor
+ * shadow stack is always disable. If it's enabled
+ * on host side, the guest supervisor states would
+ * conflict with that of host's. When host
supervisor
+ * shadow stack is enabled one day, part of
guest CET
+ * enabling code should be refined to make both
parties
+ * work properly. Right now stop KVM module loading
+ * once host supervisor shadow stack is detected on.
+ */
+ pr_err("Host supervisor shadow stack is not
compatible with KVM!\n");
+ return -EIO;
+ }
+ }
+
x86_emulator_cache = kvm_alloc_emulator_cache();
if (!x86_emulator_cache) {
pr_err("failed to allocate cache for x86 emulator\n");
Anyway, these PLx_SSP only takes effect when SSS is enabled on host,
otherwise,
they can used as scratch registers when SHSTK is available.
On Wed, Aug 02, 2023, Weijiang Yang wrote:
>
> On 8/2/2023 1:03 AM, John Allen wrote:
> > On Tue, Aug 01, 2023 at 09:28:11AM -0700, Sean Christopherson wrote:
> > > On Tue, Aug 01, 2023, John Allen wrote:
> > > > On Fri, Jun 23, 2023 at 02:11:46PM -0700, Sean Christopherson wrote:
> > > > > On Wed, May 24, 2023, John Allen wrote:
> > > > > As for the values themselves, the kernel doesn't support Supervisor Shadow Stacks
> > > > > (SSS), so PL0-2_SSP are guaranteed to be zero. And if/when SSS support is added,
> > > > > I doubt the kernel will ever use PL1_SSP or PL2_SSP, so those can probably be
> > > > > ignored entirely, and PL0_SSP might be constant per task? In other words, I don't
> > > > > see any reason to try and track the host values for support that doesn't exist,
> > > > > just do what VMX does for BNDCFGS and yell if the MSRs are non-zero. Though for
> > > > > SSS it probably makes sense for KVM to refuse to load (KVM continues on for BNDCFGS
> > > > > because it's a pretty safe assumption that the kernel won't regain MPX supported).
> > > > >
> > > > > E.g. in rough pseudocode
> > > > >
> > > > > if (boot_cpu_has(X86_FEATURE_SHSTK)) {
> > > > > rdmsrl(MSR_IA32_PLx_SSP, host_plx_ssp);
> > > > >
> > > > > if (WARN_ON_ONCE(host_pl0_ssp || host_pl1_ssp || host_pl2_ssp))
> > > > > return -EIO;
> > > > > }
> > > > The function in question returns void and wouldn't be able to return a
> > > > failure code to callers. We would have to rework this path in order to
> > > > fail in this way. Is it sufficient to just WARN_ON_ONCE here or is there
> > > > some other way we can cause KVM to fail to load here?
> > > Sorry, I should have been more explicit than "it probably make sense for KVM to
> > > refuse to load". The above would go somewhere in __kvm_x86_vendor_init().
> > I see, in that case that change should probably go up with:
> > "KVM:x86: Enable CET virtualization for VMX and advertise to userspace"
> > in Weijiang Yang's series with the rest of the changes to
> > __kvm_x86_vendor_init(). Though I can tack it on in my series if
> > needed.
>
> The downside with above WARN_ON check is, KVM has to clear PL{0,1,2}_SSP for
> all CPUs when SVM/VMX module is unloaded given guest would use them, otherwise,
> it may hit the check next time the module is reloaded.
Off topic, can you please try to fix your mail client? Almost of your replies
have extra newlines. I'm guessing something is auto-wrapping at 80 chars, and
doing it poorly.
> Can we add check as below to make it easier?
Hmm, yeah, that makes sense. I based my suggestion off of what KVM does for MPX,
but I forgot that KVM clears MSR_IA32_BNDCFGS on VM-Exit via the VMCS, i.e.
effectively does preserve the host value so long as the host value is zero.
Not clearing the MSRs on module exit is a bit uncouth, but this is more or less
the same situation/argument for not doing INVEPT on module exit. It's unsafe for
a module to assume that there aren't TLB entries for a given EP4TA, because even
if all sources of EPTPs (hypervisor/KVM modules) are well-intentioned and *try*
to clean up after themselves, it's always possible that a module crashed or was
buggy. I.e. asserting the the PLx_SSP MSRs are zero is simply wrong, whereas
asserting that SSS is not enabled is correct.
> @@ -9616,6 +9618,24 @@ static int __kvm_x86_vendor_init(struct
> kvm_x86_init_ops *ops)
> return -EIO;
> }
>
> + if (boot_cpu_has(X86_FEATURE_SHSTK)) {
> + rdmsrl(MSR_IA32_S_CET, host_s_cet);
> + if (host_s_cet & CET_SHSTK_EN) {
Make this a WARN_ON_ONCE() and drop the pr_err(). Hitting this would very much
be a kernel bug, e.g. either someone added SSS support and neglected to update
KVM, or the kernel's MSR_IA32_S_CET is corrupted.
> + /*
> + * Current CET KVM solution assumes host supervisor
> + * shadow stack is always disable. If it's enabled
> + * on host side, the guest supervisor states would
> + * conflict with that of host's. When host
> supervisor
> + * shadow stack is enabled one day, part of guest
> CET
> + * enabling code should be refined to make both
> parties
> + * work properly. Right now stop KVM module loading
> + * once host supervisor shadow stack is detected on.
I don't think we need to have a super elaborate comment, stating that SSS isn't
support and so KVM doesn't save/restore PLx_SSP MSRs should suffice.
> + */
Put the comment above the if-statment that has the WARN. That reduces the
indentation, and avoids the question of whether or not a comment above a single
line is supposed to have curly braces.
E.g. something like this, though I think even the below comment is probably
unnecessarily verbose.
/*
* Linux doesn't yet support supervisor shadow stacks (SSS), so
* so KVM doesn't save/restore the associated MSRs, i.e. KVM
* may clobber the host values. Yell and refuse to load if SSS
* is unexpectedly enabled, e.g. to avoid crashing the host.
*/
if (WARN_ON_ONCE(host_s_cet & CET_SHSTK_EN))
Thanks!
On 8/3/2023 12:38 AM, Sean Christopherson wrote:
> On Wed, Aug 02, 2023, Weijiang Yang wrote:
>> On 8/2/2023 1:03 AM, John Allen wrote:
>>> On Tue, Aug 01, 2023 at 09:28:11AM -0700, Sean Christopherson wrote:
>>>> On Tue, Aug 01, 2023, John Allen wrote:
>>>>> On Fri, Jun 23, 2023 at 02:11:46PM -0700, Sean Christopherson wrote:
>>>>>> On Wed, May 24, 2023, John Allen wrote:
>>>>>> As for the values themselves, the kernel doesn't support Supervisor Shadow Stacks
>>>>>> (SSS), so PL0-2_SSP are guaranteed to be zero. And if/when SSS support is added,
>>>>>> I doubt the kernel will ever use PL1_SSP or PL2_SSP, so those can probably be
>>>>>> ignored entirely, and PL0_SSP might be constant per task? In other words, I don't
>>>>>> see any reason to try and track the host values for support that doesn't exist,
>>>>>> just do what VMX does for BNDCFGS and yell if the MSRs are non-zero. Though for
>>>>>> SSS it probably makes sense for KVM to refuse to load (KVM continues on for BNDCFGS
>>>>>> because it's a pretty safe assumption that the kernel won't regain MPX supported).
>>>>>>
>>>>>> E.g. in rough pseudocode
>>>>>>
>>>>>> if (boot_cpu_has(X86_FEATURE_SHSTK)) {
>>>>>> rdmsrl(MSR_IA32_PLx_SSP, host_plx_ssp);
>>>>>>
>>>>>> if (WARN_ON_ONCE(host_pl0_ssp || host_pl1_ssp || host_pl2_ssp))
>>>>>> return -EIO;
>>>>>> }
>>>>> The function in question returns void and wouldn't be able to return a
>>>>> failure code to callers. We would have to rework this path in order to
>>>>> fail in this way. Is it sufficient to just WARN_ON_ONCE here or is there
>>>>> some other way we can cause KVM to fail to load here?
>>>> Sorry, I should have been more explicit than "it probably make sense for KVM to
>>>> refuse to load". The above would go somewhere in __kvm_x86_vendor_init().
>>> I see, in that case that change should probably go up with:
>>> "KVM:x86: Enable CET virtualization for VMX and advertise to userspace"
>>> in Weijiang Yang's series with the rest of the changes to
>>> __kvm_x86_vendor_init(). Though I can tack it on in my series if
>>> needed.
>> The downside with above WARN_ON check is, KVM has to clear PL{0,1,2}_SSP for
>> all CPUs when SVM/VMX module is unloaded given guest would use them, otherwise,
>> it may hit the check next time the module is reloaded.
> Off topic, can you please try to fix your mail client? Almost of your replies
> have extra newlines. I'm guessing something is auto-wrapping at 80 chars, and
> doing it poorly.
Sorry for that. Some of the blank lines are added by me, and some are auto-added by the
editor. I changed the settings and will avoid to do so.
>> Can we add check as below to make it easier?
> Hmm, yeah, that makes sense. I based my suggestion off of what KVM does for MPX,
> but I forgot that KVM clears MSR_IA32_BNDCFGS on VM-Exit via the VMCS, i.e.
> effectively does preserve the host value so long as the host value is zero.
>
> Not clearing the MSRs on module exit is a bit uncouth, but this is more or less
> the same situation/argument for not doing INVEPT on module exit. It's unsafe for
> a module to assume that there aren't TLB entries for a given EP4TA, because even
> if all sources of EPTPs (hypervisor/KVM modules) are well-intentioned and *try*
> to clean up after themselves, it's always possible that a module crashed or was
> buggy. I.e. asserting the the PLx_SSP MSRs are zero is simply wrong, whereas
> asserting that SSS is not enabled is correct.
OK.
>> @@ -9616,6 +9618,24 @@ static int __kvm_x86_vendor_init(struct
>> kvm_x86_init_ops *ops)
>> return -EIO;
>> }
>>
>> + if (boot_cpu_has(X86_FEATURE_SHSTK)) {
>> + rdmsrl(MSR_IA32_S_CET, host_s_cet);
>> + if (host_s_cet & CET_SHSTK_EN) {
> Make this a WARN_ON_ONCE() and drop the pr_err(). Hitting this would very much
> be a kernel bug, e.g. either someone added SSS support and neglected to update
> KVM, or the kernel's MSR_IA32_S_CET is corrupted.
OK, will change it.
>> + /*
>> + * Current CET KVM solution assumes host supervisor
>> + * shadow stack is always disable. If it's enabled
>> + * on host side, the guest supervisor states would
>> + * conflict with that of host's. When host
>> supervisor
>> + * shadow stack is enabled one day, part of guest
>> CET
>> + * enabling code should be refined to make both
>> parties
>> + * work properly. Right now stop KVM module loading
>> + * once host supervisor shadow stack is detected on.
> I don't think we need to have a super elaborate comment, stating that SSS isn't
> support and so KVM doesn't save/restore PLx_SSP MSRs should suffice.
>
>> + */
> Put the comment above the if-statment that has the WARN. That reduces the
> indentation, and avoids the question of whether or not a comment above a single
> line is supposed to have curly braces.
>
> E.g. something like this, though I think even the below comment is probably
> unnecessarily verbose.
>
> /*
> * Linux doesn't yet support supervisor shadow stacks (SSS), so
> * so KVM doesn't save/restore the associated MSRs, i.e. KVM
> * may clobber the host values. Yell and refuse to load if SSS
> * is unexpectedly enabled, e.g. to avoid crashing the host.
> */
> if (WARN_ON_ONCE(host_s_cet & CET_SHSTK_EN))
I will keep these comments as some hints to end users when something unexpected happens!
Thanks a lot!