[RFC,v2,6/6] KVM: SVM: Add CET features to supported_xss

  If the CPU supports CET, add CET XSAVES feature bits to the
supported_xss mask.

Signed-off-by: John Allen <john.allen@amd.com>
---
v2:
  - Remove curly braces around if statement
---
 arch/x86/kvm/svm/svm.c | 4 ++++
 1 file changed, 4 insertions(+)
  

On Wed, 2023-05-24 at 15:53 +0000, John Allen wrote:
> If the CPU supports CET, add CET XSAVES feature bits to the
> supported_xss mask.
> 
> Signed-off-by: John Allen <john.allen@amd.com>
> ---
> v2:
>   - Remove curly braces around if statement
> ---
>  arch/x86/kvm/svm/svm.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 6afd2c44fdb6..cee496bee0a9 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -5070,6 +5070,10 @@ static __init void svm_set_cpu_caps(void)
>             boot_cpu_has(X86_FEATURE_AMD_SSBD))
>                 kvm_cpu_cap_set(X86_FEATURE_VIRT_SSBD);
>  
> +       if (kvm_cpu_cap_has(X86_FEATURE_SHSTK))
> +               kvm_caps.supported_xss |= XFEATURE_MASK_CET_USER |
> +                                         XFEATURE_MASK_CET_KERNEL;
> +

Is setting XFEATURE_MASK_CET_KERNEL here ok? The host kernel will not
support XFEATURE_MASK_CET_KERNEL. I guess after this there is a small
window of time where host IA32_XSS could have non-host supported
supervisor state.

Sort of separately, how does SVM work with respect to saving and
restoring guest supervisor CET state (I mean the CET_S stuff)?

I'm not sure there is any problem, but just wondering how it all works.
Thanks.
  

On Wed, May 24, 2023 at 05:24:23PM +0000, Edgecombe, Rick P wrote:
> On Wed, 2023-05-24 at 15:53 +0000, John Allen wrote:
> > If the CPU supports CET, add CET XSAVES feature bits to the
> > supported_xss mask.
> > 
> > Signed-off-by: John Allen <john.allen@amd.com>
> > ---
> > v2:
> >   - Remove curly braces around if statement
> > ---
> >  arch/x86/kvm/svm/svm.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > index 6afd2c44fdb6..cee496bee0a9 100644
> > --- a/arch/x86/kvm/svm/svm.c
> > +++ b/arch/x86/kvm/svm/svm.c
> > @@ -5070,6 +5070,10 @@ static __init void svm_set_cpu_caps(void)
> >             boot_cpu_has(X86_FEATURE_AMD_SSBD))
> >                 kvm_cpu_cap_set(X86_FEATURE_VIRT_SSBD);
> >  
> > +       if (kvm_cpu_cap_has(X86_FEATURE_SHSTK))
> > +               kvm_caps.supported_xss |= XFEATURE_MASK_CET_USER |
> > +                                         XFEATURE_MASK_CET_KERNEL;
> > +
> 
> Is setting XFEATURE_MASK_CET_KERNEL here ok? The host kernel will not
> support XFEATURE_MASK_CET_KERNEL. I guess after this there is a small
> window of time where host IA32_XSS could have non-host supported
> supervisor state.
> 
> Sort of separately, how does SVM work with respect to saving and
> restoring guest supervisor CET state (I mean the CET_S stuff)?

Apart from a minor exception involving SEV-ES, we are piggybacking on
the state saving/restoring in Yang Weijiang's x86/VMX series. So by
inspection, it looks like guest supervisor support is broken as the
supervisor XSAVES state and MSRs are not included in that series. I
currently don't have a way to test this case, but I think there are
operating systems that support it. I'll work on getting a guest set up
that can actually test this and hopefully have working guest supervisor
support in the next version of the series.

Thanks,
John

> 
> I'm not sure there is any problem, but just wondering how it all works.
> Thanks.
  

On Fri, 2023-06-09 at 10:34 -0500, John Allen wrote:
> > Is setting XFEATURE_MASK_CET_KERNEL here ok? The host kernel will
> > not
> > support XFEATURE_MASK_CET_KERNEL. I guess after this there is a
> > small
> > window of time where host IA32_XSS could have non-host supported
> > supervisor state.
> > 
> > Sort of separately, how does SVM work with respect to saving and
> > restoring guest supervisor CET state (I mean the CET_S stuff)?
> 
> Apart from a minor exception involving SEV-ES, we are piggybacking on
> the state saving/restoring in Yang Weijiang's x86/VMX series. So by
> inspection, it looks like guest supervisor support is broken as the
> supervisor XSAVES state and MSRs are not included in that series. I
> currently don't have a way to test this case, but I think there are
> operating systems that support it. I'll work on getting a guest set
> up
> that can actually test this and hopefully have working guest
> supervisor
> support in the next version of the series.

Hmm, interesting. VMX has some separate non-xsaves thing to save and
restore the guests supervisor CET state, so Weijiang's series doesn't
use the xsaves supervisor CET support. Also, since the host might have
CR4.CET set for its own reasons, if the host handled an exit with the
the guests MSR_IA32_S_CET set it could suddenly be subjected to CET
enforcement that it doesn't expect. Waiting to restore it until
returning to the guest is too late.

At least that's the reasoning on the VMX side as I understand it. If
you need to add XFEATURE_CET_KERNEL generally, we'll have to think
about how it works with host IBT. Probably easiest to leave it disabled
on the Intel side.
  

On Fri, Jun 09, 2023, Rick P Edgecombe wrote:
> On Fri, 2023-06-09 at 10:34 -0500, John Allen wrote:
> > > Is setting XFEATURE_MASK_CET_KERNEL here ok? The host kernel will not
> > > support XFEATURE_MASK_CET_KERNEL. I guess after this there is a small
> > > window of time where host IA32_XSS could have non-host supported
> > > supervisor state.
> > > 
> > > Sort of separately, how does SVM work with respect to saving and
> > > restoring guest supervisor CET state (I mean the CET_S stuff)?
> > 
> > Apart from a minor exception involving SEV-ES, we are piggybacking on the
> > state saving/restoring in Yang Weijiang's x86/VMX series. So by inspection,
> > it looks like guest supervisor support is broken as the supervisor XSAVES
> > state and MSRs are not included in that series. I currently don't have a
> > way to test this case, but I think there are operating systems that support
> > it. I'll work on getting a guest set up that can actually test this and
> > hopefully have working guest supervisor support in the next version of the
> > series.
> 
> Hmm, interesting. VMX has some separate non-xsaves thing to save and
> restore the guests supervisor CET state, so Weijiang's series doesn't
> use the xsaves supervisor CET support.

Heh, that and Weijiang's series is a wee bit incomplete.

> Also, since the host might have CR4.CET set for its own reasons, if the host
> handled an exit with the the guests MSR_IA32_S_CET set it could suddenly be
> subjected to CET enforcement that it doesn't expect. Waiting to restore it
> until returning to the guest is too late.
>
> At least that's the reasoning on the VMX side as I understand it

The APM doesn't come right out and say it, but I assume/hope that S_CET is saved
on VMRUN and loaded on #VMEXIT, i.e. is the same as VMX for all intents and
purposes.

The host save state definitely has a field for S_CET, and VMRUN documents that the
guest values are loaded, I just can't find anything in the APM that explicitly states
how host S_CET and friends are handled.  E.g. in theory, they could have been
shoved into VMSAVE+VMLOAD, though I very much doubt that's the case.

John?
  

On 6/23/23 17:18, Sean Christopherson wrote:
> On Fri, Jun 09, 2023, Rick P Edgecombe wrote:
>> On Fri, 2023-06-09 at 10:34 -0500, John Allen wrote:
>>>> Is setting XFEATURE_MASK_CET_KERNEL here ok? The host kernel will not
>>>> support XFEATURE_MASK_CET_KERNEL. I guess after this there is a small
>>>> window of time where host IA32_XSS could have non-host supported
>>>> supervisor state.
>>>>
>>>> Sort of separately, how does SVM work with respect to saving and
>>>> restoring guest supervisor CET state (I mean the CET_S stuff)?
>>>
>>> Apart from a minor exception involving SEV-ES, we are piggybacking on the
>>> state saving/restoring in Yang Weijiang's x86/VMX series. So by inspection,
>>> it looks like guest supervisor support is broken as the supervisor XSAVES
>>> state and MSRs are not included in that series. I currently don't have a
>>> way to test this case, but I think there are operating systems that support
>>> it. I'll work on getting a guest set up that can actually test this and
>>> hopefully have working guest supervisor support in the next version of the
>>> series.
>>
>> Hmm, interesting. VMX has some separate non-xsaves thing to save and
>> restore the guests supervisor CET state, so Weijiang's series doesn't
>> use the xsaves supervisor CET support.
> 
> Heh, that and Weijiang's series is a wee bit incomplete.
> 
>> Also, since the host might have CR4.CET set for its own reasons, if the host
>> handled an exit with the the guests MSR_IA32_S_CET set it could suddenly be
>> subjected to CET enforcement that it doesn't expect. Waiting to restore it
>> until returning to the guest is too late.
>>
>> At least that's the reasoning on the VMX side as I understand it
> 
> The APM doesn't come right out and say it, but I assume/hope that S_CET is saved
> on VMRUN and loaded on #VMEXIT, i.e. is the same as VMX for all intents and
> purposes.
> 
> The host save state definitely has a field for S_CET, and VMRUN documents that the
> guest values are loaded, I just can't find anything in the APM that explicitly states
> how host S_CET and friends are handled.  E.g. in theory, they could have been
> shoved into VMSAVE+VMLOAD, though I very much doubt that's the case.

Yes, the host value is saved/restored on VMRUN/#VMEXIT. Anything that is 
in the VMCB Save Area (the non-SEV-ES save area) is fully virtualized 
(unless noted otherwise) and doesn't require special processing to 
save/restore the host values.

S_CET is list in the SVM/SEV VMCB save area. Similarly, for 
SEV-ES/SEV-SNP, S_CET is swap type A and is saved/restored on VMRUN/#VMEXIT.

Thanks,
Tom

> 
> John?
  

On Mon, Jun 26, 2023, Tom Lendacky wrote:
> On 6/23/23 17:18, Sean Christopherson wrote:
> > On Fri, Jun 09, 2023, Rick P Edgecombe wrote:
> > > Also, since the host might have CR4.CET set for its own reasons, if the host
> > > handled an exit with the the guests MSR_IA32_S_CET set it could suddenly be
> > > subjected to CET enforcement that it doesn't expect. Waiting to restore it
> > > until returning to the guest is too late.
> > > 
> > > At least that's the reasoning on the VMX side as I understand it
> > 
> > The APM doesn't come right out and say it, but I assume/hope that S_CET is saved
> > on VMRUN and loaded on #VMEXIT, i.e. is the same as VMX for all intents and
> > purposes.
> > 
> > The host save state definitely has a field for S_CET, and VMRUN documents that the
> > guest values are loaded, I just can't find anything in the APM that explicitly states
> > how host S_CET and friends are handled.  E.g. in theory, they could have been
> > shoved into VMSAVE+VMLOAD, though I very much doubt that's the case.
> 
> Yes, the host value is saved/restored on VMRUN/#VMEXIT. Anything that is in
> the VMCB Save Area (the non-SEV-ES save area) is fully virtualized (unless
> noted otherwise) and doesn't require special processing to save/restore the
> host values.

Would it makes sense to add a column in "Table B-2. VMCB Layout, State Save Area"
to specify whether a field is handled by VMRUN+#VMEXIT vs. VMLOAD+VMSAVE?  I can't
find anywhere in the APM where it explicitly states that VMRUN+#VMEXIT context
switches everything in the Save Area except the fields listed in "15.5.2 VMSAVE
and VMLOAD Instructions".

"15.5 VMRUN Instruction" kinda sorta covers that behavior, but the information is
either incomplete or stale, e.g. for host state it says "at least the following"

  Saving Host State. To ensure that the host can resume operation after #VMEXIT,
  VMRUN saves at least the following host state information:

but for guest state it says "the following"

  Loading Guest State. After saving host state, VMRUN loads the following guest
  state from the VMCB:

and then both provide incomplete lists of state.  A pedantic reading of the guest
case suggests that there's a large pile of state that *isn't* loaded, and the host
case isn't all that helpful because it's way too handwavy.
  

On 6/26/23 11:28, Sean Christopherson wrote:
> On Mon, Jun 26, 2023, Tom Lendacky wrote:
>> On 6/23/23 17:18, Sean Christopherson wrote:
>>> On Fri, Jun 09, 2023, Rick P Edgecombe wrote:
>>>> Also, since the host might have CR4.CET set for its own reasons, if the host
>>>> handled an exit with the the guests MSR_IA32_S_CET set it could suddenly be
>>>> subjected to CET enforcement that it doesn't expect. Waiting to restore it
>>>> until returning to the guest is too late.
>>>>
>>>> At least that's the reasoning on the VMX side as I understand it
>>>
>>> The APM doesn't come right out and say it, but I assume/hope that S_CET is saved
>>> on VMRUN and loaded on #VMEXIT, i.e. is the same as VMX for all intents and
>>> purposes.
>>>
>>> The host save state definitely has a field for S_CET, and VMRUN documents that the
>>> guest values are loaded, I just can't find anything in the APM that explicitly states
>>> how host S_CET and friends are handled.  E.g. in theory, they could have been
>>> shoved into VMSAVE+VMLOAD, though I very much doubt that's the case.
>>
>> Yes, the host value is saved/restored on VMRUN/#VMEXIT. Anything that is in
>> the VMCB Save Area (the non-SEV-ES save area) is fully virtualized (unless
>> noted otherwise) and doesn't require special processing to save/restore the
>> host values.
> 
> Would it makes sense to add a column in "Table B-2. VMCB Layout, State Save Area"
> to specify whether a field is handled by VMRUN+#VMEXIT vs. VMLOAD+VMSAVE?  I can't
> find anywhere in the APM where it explicitly states that VMRUN+#VMEXIT context
> switches everything in the Save Area except the fields listed in "15.5.2 VMSAVE
> and VMLOAD Instructions".
> 
> "15.5 VMRUN Instruction" kinda sorta covers that behavior, but the information is
> either incomplete or stale, e.g. for host state it says "at least the following"
> 
>    Saving Host State. To ensure that the host can resume operation after #VMEXIT,
>    VMRUN saves at least the following host state information:
> 
> but for guest state it says "the following"
> 
>    Loading Guest State. After saving host state, VMRUN loads the following guest
>    state from the VMCB:
> 
> and then both provide incomplete lists of state.  A pedantic reading of the guest
> case suggests that there's a large pile of state that *isn't* loaded, and the host
> case isn't all that helpful because it's way too handwavy.

I'll communicate this feedback to the folks that update the APM volumes 
and see what can be done.

Thanks,
Tom
  

On Mon, Jun 26, 2023, Tom Lendacky wrote:
> On 6/26/23 11:28, Sean Christopherson wrote:
> > On Mon, Jun 26, 2023, Tom Lendacky wrote:
> > > On 6/23/23 17:18, Sean Christopherson wrote:
> > > > On Fri, Jun 09, 2023, Rick P Edgecombe wrote:
> > > > > Also, since the host might have CR4.CET set for its own reasons, if the host
> > > > > handled an exit with the the guests MSR_IA32_S_CET set it could suddenly be
> > > > > subjected to CET enforcement that it doesn't expect. Waiting to restore it
> > > > > until returning to the guest is too late.
> > > > > 
> > > > > At least that's the reasoning on the VMX side as I understand it
> > > > 
> > > > The APM doesn't come right out and say it, but I assume/hope that S_CET is saved
> > > > on VMRUN and loaded on #VMEXIT, i.e. is the same as VMX for all intents and
> > > > purposes.
> > > > 
> > > > The host save state definitely has a field for S_CET, and VMRUN documents that the
> > > > guest values are loaded, I just can't find anything in the APM that explicitly states
> > > > how host S_CET and friends are handled.  E.g. in theory, they could have been
> > > > shoved into VMSAVE+VMLOAD, though I very much doubt that's the case.
> > > 
> > > Yes, the host value is saved/restored on VMRUN/#VMEXIT. Anything that is in
> > > the VMCB Save Area (the non-SEV-ES save area) is fully virtualized (unless
> > > noted otherwise) and doesn't require special processing to save/restore the
> > > host values.
> > 
> > Would it makes sense to add a column in "Table B-2. VMCB Layout, State Save Area"
> > to specify whether a field is handled by VMRUN+#VMEXIT vs. VMLOAD+VMSAVE?  I can't
> > find anywhere in the APM where it explicitly states that VMRUN+#VMEXIT context
> > switches everything in the Save Area except the fields listed in "15.5.2 VMSAVE
> > and VMLOAD Instructions".
> > 
> > "15.5 VMRUN Instruction" kinda sorta covers that behavior, but the information is
> > either incomplete or stale, e.g. for host state it says "at least the following"
> > 
> >    Saving Host State. To ensure that the host can resume operation after #VMEXIT,
> >    VMRUN saves at least the following host state information:
> > 
> > but for guest state it says "the following"
> > 
> >    Loading Guest State. After saving host state, VMRUN loads the following guest
> >    state from the VMCB:
> > 
> > and then both provide incomplete lists of state.  A pedantic reading of the guest
> > case suggests that there's a large pile of state that *isn't* loaded, and the host
> > case isn't all that helpful because it's way too handwavy.
> 
> I'll communicate this feedback to the folks that update the APM volumes and
> see what can be done.

Thanks, much appreciated!