| Message ID | 20240207172646.3981-8-xin3.li@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable FRED with KVM VMX | expand |
On Wed, Feb 07, 2024 at 09:26:27AM -0800, Xin Li wrote: >Add FRED MSRs to the valid passthrough MSR list and set FRED MSRs intercept >based on FRED enumeration. > >Signed-off-by: Xin Li <xin3.li@intel.com> >Tested-by: Shan Kang <shan.kang@intel.com> Reviewed-by: Chao Gao <chao.gao@intel.com> two nits below. >--- > >Change since v1: >* Enable FRED MSRs intercept if FRED is no longer enumerated in CPUID > (Chao Gao). >--- > arch/x86/kvm/vmx/vmx.c | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) > >diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c >index 34b6676f60d8..d58ed2d3d379 100644 >--- a/arch/x86/kvm/vmx/vmx.c >+++ b/arch/x86/kvm/vmx/vmx.c >@@ -693,6 +693,9 @@ static bool is_valid_passthrough_msr(u32 msr) > case MSR_LBR_CORE_TO ... MSR_LBR_CORE_TO + 8: > /* LBR MSRs. These are handled in vmx_update_intercept_for_lbr_msrs() */ > return true; >+ case MSR_IA32_FRED_RSP0 ... MSR_IA32_FRED_CONFIG: >+ /* FRED MSRs should be passthrough to FRED guests only */ This comment sounds weird. It sounds like the code will be something like: if guest supports FRED return true else return false how about "FRED MSRs are pass-thru'd to guests which enumerate FRED"? Or to align with above comment for LBR MSRs, just say /* FRED MSRs. These are handled in vmx_vcpu_config_fred_after_set_cpuid() */ >+ return true; > } > > r = possible_passthrough_msr_slot(msr) != -ENOENT; >@@ -7774,10 +7777,12 @@ static void update_intel_pt_cfg(struct kvm_vcpu *vcpu) > static void vmx_vcpu_config_fred_after_set_cpuid(struct kvm_vcpu *vcpu) > { > struct vcpu_vmx *vmx = to_vmx(vcpu); >+ bool fred_enumerated; > > kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_FRED); >+ fred_enumerated = guest_can_use(vcpu, X86_FEATURE_FRED); > >- if (guest_can_use(vcpu, X86_FEATURE_FRED)) { >+ if (fred_enumerated) { > vm_entry_controls_setbit(vmx, VM_ENTRY_LOAD_IA32_FRED); > secondary_vm_exit_controls_setbit(vmx, > SECONDARY_VM_EXIT_SAVE_IA32_FRED | >@@ -7788,6 +7793,16 @@ static void vmx_vcpu_config_fred_after_set_cpuid(struct kvm_vcpu *vcpu) > SECONDARY_VM_EXIT_SAVE_IA32_FRED | > SECONDARY_VM_EXIT_LOAD_IA32_FRED); > } >+ >+ vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_RSP0, MSR_TYPE_RW, !fred_enumerated); >+ vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_RSP1, MSR_TYPE_RW, !fred_enumerated); >+ vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_RSP2, MSR_TYPE_RW, !fred_enumerated); >+ vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_RSP3, MSR_TYPE_RW, !fred_enumerated); >+ vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_STKLVLS, MSR_TYPE_RW, !fred_enumerated); >+ vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_SSP1, MSR_TYPE_RW, !fred_enumerated); >+ vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_SSP2, MSR_TYPE_RW, !fred_enumerated); >+ vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_SSP3, MSR_TYPE_RW, !fred_enumerated); >+ vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_CONFIG, MSR_TYPE_RW, !fred_enumerated); Use a for-loop here? e.g., for (i = MSR_IA32_FRED_RSP0; i <= MSR_IA32_FRED_CONFIG; i++) > } > > static void vmx_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu) >-- >2.43.0 > >
> >+ case MSR_IA32_FRED_RSP0 ... MSR_IA32_FRED_CONFIG: > >+ /* FRED MSRs should be passthrough to FRED guests only */ > > This comment sounds weird. It sounds like the code will be something like: > if guest supports FRED > return true > else > return false > > how about "FRED MSRs are pass-thru'd to guests which enumerate FRED"? > > Or to align with above comment for LBR MSRs, just say > > /* FRED MSRs. These are handled in vmx_vcpu_config_fred_after_set_cpuid() */ > Maybe both to not confuse people at all
On Wed, Feb 07, 2024, Xin Li wrote: > @@ -7774,10 +7777,12 @@ static void update_intel_pt_cfg(struct kvm_vcpu *vcpu) > static void vmx_vcpu_config_fred_after_set_cpuid(struct kvm_vcpu *vcpu) > { > struct vcpu_vmx *vmx = to_vmx(vcpu); > + bool fred_enumerated; > > kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_FRED); > + fred_enumerated = guest_can_use(vcpu, X86_FEATURE_FRED); "enumerated" isn't correct. Userspace can enumerate FRED to the guest even if FRED is unsupported in KVM. Planning for a future where this becomes guest_cpu_cap_has(), maybe "has_fred"? > - if (guest_can_use(vcpu, X86_FEATURE_FRED)) { > + if (fred_enumerated) { > vm_entry_controls_setbit(vmx, VM_ENTRY_LOAD_IA32_FRED); > secondary_vm_exit_controls_setbit(vmx, > SECONDARY_VM_EXIT_SAVE_IA32_FRED | > @@ -7788,6 +7793,16 @@ static void vmx_vcpu_config_fred_after_set_cpuid(struct kvm_vcpu *vcpu) > SECONDARY_VM_EXIT_SAVE_IA32_FRED | > SECONDARY_VM_EXIT_LOAD_IA32_FRED); > } > + > + vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_RSP0, MSR_TYPE_RW, !fred_enumerated); > + vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_RSP1, MSR_TYPE_RW, !fred_enumerated); > + vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_RSP2, MSR_TYPE_RW, !fred_enumerated); > + vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_RSP3, MSR_TYPE_RW, !fred_enumerated); > + vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_STKLVLS, MSR_TYPE_RW, !fred_enumerated); > + vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_SSP1, MSR_TYPE_RW, !fred_enumerated); > + vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_SSP2, MSR_TYPE_RW, !fred_enumerated); > + vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_SSP3, MSR_TYPE_RW, !fred_enumerated); > + vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_CONFIG, MSR_TYPE_RW, !fred_enumerated); > } > > static void vmx_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu) > -- > 2.43.0 >
On Fri, Apr 19, 2024, Chao Gao wrote: > On Wed, Feb 07, 2024 at 09:26:27AM -0800, Xin Li wrote: > >Add FRED MSRs to the valid passthrough MSR list and set FRED MSRs intercept > >based on FRED enumeration. This needs a *much* more verbose explanation. It's pretty darn obvious _what_ KVM is doing, but it's not at all clear _why_ KVM is passing through FRED MSRs. E.g. why is FRED_SSP0 not included in the set of passthrough MSRs? > > static void vmx_vcpu_config_fred_after_set_cpuid(struct kvm_vcpu *vcpu) > > { > > struct vcpu_vmx *vmx = to_vmx(vcpu); > >+ bool fred_enumerated; > > > > kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_FRED); > >+ fred_enumerated = guest_can_use(vcpu, X86_FEATURE_FRED); > > > >- if (guest_can_use(vcpu, X86_FEATURE_FRED)) { > >+ if (fred_enumerated) { > > vm_entry_controls_setbit(vmx, VM_ENTRY_LOAD_IA32_FRED); > > secondary_vm_exit_controls_setbit(vmx, > > SECONDARY_VM_EXIT_SAVE_IA32_FRED | > >@@ -7788,6 +7793,16 @@ static void vmx_vcpu_config_fred_after_set_cpuid(struct kvm_vcpu *vcpu) > > SECONDARY_VM_EXIT_SAVE_IA32_FRED | > > SECONDARY_VM_EXIT_LOAD_IA32_FRED); > > } > >+ > >+ vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_RSP0, MSR_TYPE_RW, !fred_enumerated); > >+ vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_RSP1, MSR_TYPE_RW, !fred_enumerated); > >+ vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_RSP2, MSR_TYPE_RW, !fred_enumerated); > >+ vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_RSP3, MSR_TYPE_RW, !fred_enumerated); > >+ vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_STKLVLS, MSR_TYPE_RW, !fred_enumerated); > >+ vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_SSP1, MSR_TYPE_RW, !fred_enumerated); > >+ vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_SSP2, MSR_TYPE_RW, !fred_enumerated); > >+ vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_SSP3, MSR_TYPE_RW, !fred_enumerated); > >+ vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_CONFIG, MSR_TYPE_RW, !fred_enumerated); > > Use a for-loop here? e.g., > for (i = MSR_IA32_FRED_RSP0; i <= MSR_IA32_FRED_CONFIG; i++) Hmm, I'd prefer to keep the open coded version. It's not pretty, but I don't expect this to have much, if any, maintenance cost. And using a loop makes it harder to both understand _exactly_ what's happening, and to search for relevant code. E.g. it's quite difficult to see that FRED_SSP0 is still intercepted (see my comment regarding the changelog).
On 6/12/2024 2:32 PM, Sean Christopherson wrote: > On Fri, Apr 19, 2024, Chao Gao wrote: >> On Wed, Feb 07, 2024 at 09:26:27AM -0800, Xin Li wrote: >>> Add FRED MSRs to the valid passthrough MSR list and set FRED MSRs intercept >>> based on FRED enumeration. > > This needs a *much* more verbose explanation. It's pretty darn obvious _what_ > KVM is doing, but it's not at all clear _why_ KVM is passing through FRED MSRs. > E.g. why is FRED_SSP0 not included in the set of passthrough MSRs? > >>> static void vmx_vcpu_config_fred_after_set_cpuid(struct kvm_vcpu *vcpu) >>> { >>> struct vcpu_vmx *vmx = to_vmx(vcpu); >>> + bool fred_enumerated; >>> >>> kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_FRED); >>> + fred_enumerated = guest_can_use(vcpu, X86_FEATURE_FRED); >>> >>> - if (guest_can_use(vcpu, X86_FEATURE_FRED)) { >>> + if (fred_enumerated) { >>> vm_entry_controls_setbit(vmx, VM_ENTRY_LOAD_IA32_FRED); >>> secondary_vm_exit_controls_setbit(vmx, >>> SECONDARY_VM_EXIT_SAVE_IA32_FRED | >>> @@ -7788,6 +7793,16 @@ static void vmx_vcpu_config_fred_after_set_cpuid(struct kvm_vcpu *vcpu) >>> SECONDARY_VM_EXIT_SAVE_IA32_FRED | >>> SECONDARY_VM_EXIT_LOAD_IA32_FRED); >>> } >>> + >>> + vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_RSP0, MSR_TYPE_RW, !fred_enumerated); >>> + vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_RSP1, MSR_TYPE_RW, !fred_enumerated); >>> + vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_RSP2, MSR_TYPE_RW, !fred_enumerated); >>> + vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_RSP3, MSR_TYPE_RW, !fred_enumerated); >>> + vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_STKLVLS, MSR_TYPE_RW, !fred_enumerated); >>> + vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_SSP1, MSR_TYPE_RW, !fred_enumerated); >>> + vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_SSP2, MSR_TYPE_RW, !fred_enumerated); >>> + vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_SSP3, MSR_TYPE_RW, !fred_enumerated); >>> + vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_CONFIG, MSR_TYPE_RW, !fred_enumerated); >> >> Use a for-loop here? e.g., >> for (i = MSR_IA32_FRED_RSP0; i <= MSR_IA32_FRED_CONFIG; i++) > > Hmm, I'd prefer to keep the open coded version. It's not pretty, but I don't > expect this to have much, if any, maintenance cost. And using a loop makes it > harder to both understand _exactly_ what's happening, and to search for relevant > code. E.g. it's quite difficult to see that FRED_SSP0 is still intercepted (see > my comment regarding the changelog). I owe you an explanation; I have been thinking about figuring out a way to include FRED SSP0 in the FRED KVM patch set... MSR_IA32_FRED_SSP0 is an alias of the CET MSR_IA32_PL0_SSP and likely to be used in the same way as FRED RSP0, i.e., host FRED SSP0 _should_ be restored in arch_exit_to_user_mode_prepare(). However as of today Linux has no plan to utilize kernel shadow stack thus no one cares host FRED SSP0 (no?). But lets say anyway it is host's responsibility to manage host FRED SSP0, then KVM only needs to take care of guest FRED SSP0 (just like how KVM should handle guest FRED RSP0) even before the supervisor shadow stack feature is advertised to guest. Another question is should KVM handle userspace request to set/get FRED SSP0? IMO, it should be part of CET state management. Your suggestion? Thanks! Xin
On Thu, Sep 05, 2024, Xin Li wrote: > On 6/12/2024 2:32 PM, Sean Christopherson wrote: > > On Fri, Apr 19, 2024, Chao Gao wrote: > > > On Wed, Feb 07, 2024 at 09:26:27AM -0800, Xin Li wrote: > > > > Add FRED MSRs to the valid passthrough MSR list and set FRED MSRs intercept > > > > based on FRED enumeration. > > > > This needs a *much* more verbose explanation. It's pretty darn obvious _what_ > > KVM is doing, but it's not at all clear _why_ KVM is passing through FRED MSRs. > > E.g. why is FRED_SSP0 not included in the set of passthrough MSRs? > > > > > > static void vmx_vcpu_config_fred_after_set_cpuid(struct kvm_vcpu *vcpu) > > > > { > > > > struct vcpu_vmx *vmx = to_vmx(vcpu); > > > > + bool fred_enumerated; > > > > > > > > kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_FRED); > > > > + fred_enumerated = guest_can_use(vcpu, X86_FEATURE_FRED); > > > > > > > > - if (guest_can_use(vcpu, X86_FEATURE_FRED)) { > > > > + if (fred_enumerated) { > > > > vm_entry_controls_setbit(vmx, VM_ENTRY_LOAD_IA32_FRED); > > > > secondary_vm_exit_controls_setbit(vmx, > > > > SECONDARY_VM_EXIT_SAVE_IA32_FRED | > > > > @@ -7788,6 +7793,16 @@ static void vmx_vcpu_config_fred_after_set_cpuid(struct kvm_vcpu *vcpu) > > > > SECONDARY_VM_EXIT_SAVE_IA32_FRED | > > > > SECONDARY_VM_EXIT_LOAD_IA32_FRED); > > > > } > > > > + > > > > + vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_RSP0, MSR_TYPE_RW, !fred_enumerated); > > > > + vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_RSP1, MSR_TYPE_RW, !fred_enumerated); > > > > + vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_RSP2, MSR_TYPE_RW, !fred_enumerated); > > > > + vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_RSP3, MSR_TYPE_RW, !fred_enumerated); > > > > + vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_STKLVLS, MSR_TYPE_RW, !fred_enumerated); > > > > + vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_SSP1, MSR_TYPE_RW, !fred_enumerated); > > > > + vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_SSP2, MSR_TYPE_RW, !fred_enumerated); > > > > + vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_SSP3, MSR_TYPE_RW, !fred_enumerated); > > > > + vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_CONFIG, MSR_TYPE_RW, !fred_enumerated); > > > > > > Use a for-loop here? e.g., > > > for (i = MSR_IA32_FRED_RSP0; i <= MSR_IA32_FRED_CONFIG; i++) > > > > Hmm, I'd prefer to keep the open coded version. It's not pretty, but I don't > > expect this to have much, if any, maintenance cost. And using a loop makes it > > harder to both understand _exactly_ what's happening, and to search for relevant > > code. E.g. it's quite difficult to see that FRED_SSP0 is still intercepted (see > > my comment regarding the changelog). > > > I owe you an explanation; I have been thinking about figuring out a way > to include FRED SSP0 in the FRED KVM patch set... > > MSR_IA32_FRED_SSP0 is an alias of the CET MSR_IA32_PL0_SSP and likely to > be used in the same way as FRED RSP0, i.e., host FRED SSP0 _should_ be > restored in arch_exit_to_user_mode_prepare(). However as of today Linux > has no plan to utilize kernel shadow stack thus no one cares host FRED > SSP0 (no?). But lets say anyway it is host's responsibility to manage > host FRED SSP0, then KVM only needs to take care of guest FRED SSP0 > (just like how KVM should handle guest FRED RSP0) even before the > supervisor shadow stack feature is advertised to guest. Heh, I'm not sure what your question is, or if there even is a question. KVM needs to context switch FRED SSP0 if FRED is exposed to the guest, but presumably that will be done through XSAVE state? If that's the long term plan, I would prefer to focus on merging CET virtualization first, and then land FRED virtualization on top so that KVM doesn't have to carry intermediate code to deal with the aliased MSR. Ugh, but what happens if a CPU (or the host kernel) supports FRED but not CET SS? Or is that effectively an illegal combination? > Another question is should KVM handle userspace request to set/get FRED > SSP0? IMO, it should be part of CET state management. Yes, KVM needs to allow userspace to get/set FRED SSP0. In general, KVM needs to allow reads/writes to MSRs even if they can be saved/restored through some other means. In most cases, including this one, it's a moot point, because KVM needs to have the necessary code anyways, e.g. if KVM encounters a RDMSR/WRMSR while emulating.
>> MSR_IA32_FRED_SSP0 is an alias of the CET MSR_IA32_PL0_SSP and likely to >> be used in the same way as FRED RSP0, i.e., host FRED SSP0 _should_ be >> restored in arch_exit_to_user_mode_prepare(). However as of today Linux >> has no plan to utilize kernel shadow stack thus no one cares host FRED >> SSP0 (no?). But lets say anyway it is host's responsibility to manage >> host FRED SSP0, then KVM only needs to take care of guest FRED SSP0 >> (just like how KVM should handle guest FRED RSP0) even before the >> supervisor shadow stack feature is advertised to guest. > > Heh, I'm not sure what your question is, or if there even is a question. KVM > needs to context switch FRED SSP0 if FRED is exposed to the guest, but presumably > that will be done through XSAVE state? If that's the long term plan, I would > prefer to focus on merging CET virtualization first, and then land FRED virtualization > on top so that KVM doesn't have to carry intermediate code to deal with the aliased > MSR. You mean the following patch set, right? https://lore.kernel.org/kvm/20240531090331.13713-1-weijiang.yang@intel.com/ I think it's probably better to do so. Otherwise a patch to explicitly save/restore FRED SSP0 would be needed before the CET patch set lands and then reverted immediately after: diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index f828f03d48ab..c790cb7a7d6b 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -1343,8 +1343,10 @@ void vmx_prepare_switch_to_guest(struct kvm_vcpu *vcpu) wrmsrl(MSR_KERNEL_GS_BASE, vmx->msr_guest_kernel_gs_base); - if (cpu_feature_enabled(X86_FEATURE_FRED) && guest_can_use(vcpu, X86_FEATURE_FRED)) + if (cpu_feature_enabled(X86_FEATURE_FRED) && guest_can_use(vcpu, X86_FEATURE_FRED)) { wrmsrns(MSR_IA32_FRED_RSP0, vmx->msr_guest_fred_rsp0); + wrmsrns(MSR_IA32_FRED_SSP0, vmx->msr_guest_fred_ssp0); + } #else savesegment(fs, fs_sel); savesegment(gs, gs_sel); @@ -1393,6 +1395,8 @@ static void vmx_prepare_switch_to_host(struct vcpu_vmx *vmx) if (cpu_feature_enabled(X86_FEATURE_FRED) && guest_can_use(&vmx->vcpu, X86_FEATURE_FRED)) { vmx->msr_guest_fred_rsp0 = read_msr(MSR_IA32_FRED_RSP0); fred_sync_rsp0(vmx->msr_guest_fred_rsp0); + + vmx->msr_guest_fred_ssp0 = read_msr(MSR_IA32_FRED_SSP0); } #endif load_fixmap_gdt(raw_smp_processor_id()); diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h index 9012428984de..d1b9352f56c7 100644 --- a/arch/x86/kvm/vmx/vmx.h +++ b/arch/x86/kvm/vmx/vmx.h @@ -282,6 +282,7 @@ struct vcpu_vmx { u64 msr_host_kernel_gs_base; u64 msr_guest_kernel_gs_base; u64 msr_guest_fred_rsp0; + u64 msr_guest_fred_ssp0; #endif u64 spec_ctrl; > > Ugh, but what happens if a CPU (or the host kernel) supports FRED but not CET SS? > Or is that effectively an illegal combination? The FRED Spec says: IA32_FRED_SSP1, IA32_FRED_SSP2, and IA32_FRED_SSP3 (MSR indices 1D1H– 1D3H). Together with the existing MSR IA32_PL0_SSP (MSR index 6A4H), these are the FRED SSP MSRs. The FRED SSP MSRs are supported by any processor that enumerates CPUID.(EAX=7,ECX=1):EAX.FRED[bit 17] as 1. If such a processor does not support CET, FRED transitions will not use the MSRs (because shadow stacks are not enabled), but the MSRs would still be accessible using RDMSR and WRMSR. So they are independent, just that FRED SSP MSRs are NOT used if supervisor shadow stacks are not enabled (obviously Qemu can be configured to not advertise CET but FRED). When FRED is advertised to a guest, KVM should allow FRED SSP MSRs accesses through disabling FRED SSP MSRs interception no matter whether supervisor shadow stacks are enabled or not. Thanks! Xin
On Wed, Sep 18, 2024, Xin Li wrote: > > > MSR_IA32_FRED_SSP0 is an alias of the CET MSR_IA32_PL0_SSP and likely to > > > be used in the same way as FRED RSP0, i.e., host FRED SSP0 _should_ be > > > restored in arch_exit_to_user_mode_prepare(). However as of today Linux > > > has no plan to utilize kernel shadow stack thus no one cares host FRED > > > SSP0 (no?). But lets say anyway it is host's responsibility to manage > > > host FRED SSP0, then KVM only needs to take care of guest FRED SSP0 > > > (just like how KVM should handle guest FRED RSP0) even before the > > > supervisor shadow stack feature is advertised to guest. > > > > Heh, I'm not sure what your question is, or if there even is a question. KVM > > needs to context switch FRED SSP0 if FRED is exposed to the guest, but presumably > > that will be done through XSAVE state? If that's the long term plan, I would > > prefer to focus on merging CET virtualization first, and then land FRED virtualization > > on top so that KVM doesn't have to carry intermediate code to deal with the aliased > > MSR. > > You mean the following patch set, right? Yep, and presumably the KVM support as well: https://lore.kernel.org/all/20240219074733.122080-1-weijiang.yang@intel.com > https://lore.kernel.org/kvm/20240531090331.13713-1-weijiang.yang@intel.com/ ... > > Ugh, but what happens if a CPU (or the host kernel) supports FRED but not CET SS? > > Or is that effectively an illegal combination? > > The FRED Spec says: > > IA32_FRED_SSP1, IA32_FRED_SSP2, and IA32_FRED_SSP3 (MSR indices 1D1H– > 1D3H). Together with the existing MSR IA32_PL0_SSP (MSR index 6A4H), these > are the FRED SSP MSRs. > > The FRED SSP MSRs are supported by any processor that enumerates > CPUID.(EAX=7,ECX=1):EAX.FRED[bit 17] as 1. If such a processor does not > support CET, FRED transitions will not use the MSRs (because shadow stacks > are not enabled), but the MSRs would still be accessible using RDMSR and > WRMSR. > > > So they are independent, just that FRED SSP MSRs are NOT used if > supervisor shadow stacks are not enabled (obviously Qemu can be > configured to not advertise CET but FRED). > > When FRED is advertised to a guest, KVM should allow FRED SSP MSRs > accesses through disabling FRED SSP MSRs interception no matter whether > supervisor shadow stacks are enabled or not. KVM doesn't necessarily need to disabling MSR interception, e.g. if the expectation is that the guest will rarely/never access the MSRs when CET is unsupported, then we're likely better off going with a trap-and-emulate model. KVM needs to emulate RDMSR and WRMSR no matter what, e.g. in case the guest triggers a WRMSR when KVM is emulating, and so that userspace can get/set MSR values. And this means that yes, FRED virtualization needs to land after CET virtualization, otherwise managing the conflicts/dependencies will be a nightmare.
On 9/25/2024 7:12 AM, Sean Christopherson wrote: > On Wed, Sep 18, 2024, Xin Li wrote: >> You mean the following patch set, right? > > Yep, and presumably the KVM support as well: I assume it's close to KVM upstreaming criteria :) > > https://lore.kernel.org/all/20240219074733.122080-1-weijiang.yang@intel.com > >> https://lore.kernel.org/kvm/20240531090331.13713-1-weijiang.yang@intel.com/ > ... >> >> When FRED is advertised to a guest, KVM should allow FRED SSP MSRs >> accesses through disabling FRED SSP MSRs interception no matter whether >> supervisor shadow stacks are enabled or not. > > KVM doesn't necessarily need to disabling MSR interception, e.g. if the expectation > is that the guest will rarely/never access the MSRs when CET is unsupported, then > we're likely better off going with a trap-and-emulate model. KVM needs to emulate > RDMSR and WRMSR no matter what, e.g. in case the guest triggers a WRMSR when KVM > is emulating, and so that userspace can get/set MSR values. > > And this means that yes, FRED virtualization needs to land after CET virtualization, > otherwise managing the conflicts/dependencies will be a nightmare. > No argument. Thanks! Xin
>>> When FRED is advertised to a guest, KVM should allow FRED SSP MSRs >>> accesses through disabling FRED SSP MSRs interception no matter whether >>> supervisor shadow stacks are enabled or not. >> >> KVM doesn't necessarily need to disabling MSR interception, e.g. if >> the expectation >> is that the guest will rarely/never access the MSRs when CET is >> unsupported, then >> we're likely better off going with a trap-and-emulate model. KVM >> needs to emulate >> RDMSR and WRMSR no matter what, e.g. in case the guest triggers a >> WRMSR when KVM >> is emulating, and so that userspace can get/set MSR values. >> >> And this means that yes, FRED virtualization needs to land after CET >> virtualization, >> otherwise managing the conflicts/dependencies will be a nightmare. >> I still plan to send another iteration of the FRED patch set for review, however I haven't seen your x86 KVM changes land into Linus' tree, it will happen soon, right? > > No argument.
On Fri, Sep 27, 2024, Xin Li wrote: > > > > When FRED is advertised to a guest, KVM should allow FRED SSP MSRs > > > > accesses through disabling FRED SSP MSRs interception no matter whether > > > > supervisor shadow stacks are enabled or not. > > > > > > KVM doesn't necessarily need to disabling MSR interception, e.g. if > > > the expectation > > > is that the guest will rarely/never access the MSRs when CET is > > > unsupported, then > > > we're likely better off going with a trap-and-emulate model. KVM > > > needs to emulate > > > RDMSR and WRMSR no matter what, e.g. in case the guest triggers a > > > WRMSR when KVM > > > is emulating, and so that userspace can get/set MSR values. > > > > > > And this means that yes, FRED virtualization needs to land after CET > > > virtualization, > > > otherwise managing the conflicts/dependencies will be a nightmare. > > > > > I still plan to send another iteration of the FRED patch set for review, > however I haven't seen your x86 KVM changes land into Linus' tree, it > will happen soon, right? Yep, we squeaked into rc1, the pull request to Linus was delayed because of travel and conferences.
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index 34b6676f60d8..d58ed2d3d379 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -693,6 +693,9 @@ static bool is_valid_passthrough_msr(u32 msr) case MSR_LBR_CORE_TO ... MSR_LBR_CORE_TO + 8: /* LBR MSRs. These are handled in vmx_update_intercept_for_lbr_msrs() */ return true; + case MSR_IA32_FRED_RSP0 ... MSR_IA32_FRED_CONFIG: + /* FRED MSRs should be passthrough to FRED guests only */ + return true; } r = possible_passthrough_msr_slot(msr) != -ENOENT; @@ -7774,10 +7777,12 @@ static void update_intel_pt_cfg(struct kvm_vcpu *vcpu) static void vmx_vcpu_config_fred_after_set_cpuid(struct kvm_vcpu *vcpu) { struct vcpu_vmx *vmx = to_vmx(vcpu); + bool fred_enumerated; kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_FRED); + fred_enumerated = guest_can_use(vcpu, X86_FEATURE_FRED); - if (guest_can_use(vcpu, X86_FEATURE_FRED)) { + if (fred_enumerated) { vm_entry_controls_setbit(vmx, VM_ENTRY_LOAD_IA32_FRED); secondary_vm_exit_controls_setbit(vmx, SECONDARY_VM_EXIT_SAVE_IA32_FRED | @@ -7788,6 +7793,16 @@ static void vmx_vcpu_config_fred_after_set_cpuid(struct kvm_vcpu *vcpu) SECONDARY_VM_EXIT_SAVE_IA32_FRED | SECONDARY_VM_EXIT_LOAD_IA32_FRED); } + + vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_RSP0, MSR_TYPE_RW, !fred_enumerated); + vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_RSP1, MSR_TYPE_RW, !fred_enumerated); + vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_RSP2, MSR_TYPE_RW, !fred_enumerated); + vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_RSP3, MSR_TYPE_RW, !fred_enumerated); + vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_STKLVLS, MSR_TYPE_RW, !fred_enumerated); + vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_SSP1, MSR_TYPE_RW, !fred_enumerated); + vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_SSP2, MSR_TYPE_RW, !fred_enumerated); + vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_SSP3, MSR_TYPE_RW, !fred_enumerated); + vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_CONFIG, MSR_TYPE_RW, !fred_enumerated); } static void vmx_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)