| Message ID | 9c4547ea234a2ba09ebe05219f180f08ac6fc2e3.1708933498.git.isaku.yamahata@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v19,001/130] x86/virt/tdx: Rename _offset to _member for TD_SYSINFO_MAP() macro | expand |
On 2/26/2024 4:26 PM, isaku.yamahata@intel.com wrote: > From: Isaku Yamahata <isaku.yamahata@intel.com> > > TDX doesn't support system-management mode (SMM) and system-management > interrupt (SMI) in guest TDs. Because guest state (vcpu state, memory > state) is protected, it must go through the TDX module APIs to change guest > state, injecting SMI and changing vcpu mode into SMM. The TDX module > doesn't provide a way for VMM to inject SMI into guest TD and a way for VMM ^ or > to switch guest vcpu mode into SMM. > > We have two options in KVM when handling SMM or SMI in the guest TD or the > device model (e.g. QEMU): 1) silently ignore the request or 2) return a > meaningful error. > > For simplicity, we implemented the option 1). > > Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com> > --- [...] > + > +static void vt_enable_smi_window(struct kvm_vcpu *vcpu) > +{ > + if (is_td_vcpu(vcpu)) { > + tdx_enable_smi_window(vcpu); > + return; Can "return tdx_enable_smi_window(vcpu);" directly. > + } > + > + /* RSM will cause a vmexit anyway. */ > + vmx_enable_smi_window(vcpu); > +} > +#endif > + [...] > > +#if defined(CONFIG_INTEL_TDX_HOST) && defined(CONFIG_KVM_SMM) > +int tdx_smi_allowed(struct kvm_vcpu *vcpu, bool for_injection); > +int tdx_enter_smm(struct kvm_vcpu *vcpu, union kvm_smram *smram); > +int tdx_leave_smm(struct kvm_vcpu *vcpu, const union kvm_smram *smram); > +void tdx_enable_smi_window(struct kvm_vcpu *vcpu); > +#else #elif defined(CONFIG_KVM_SMM) These functions are only needed when CONFIG_KVM_SMM is defined. > +static inline int tdx_smi_allowed(struct kvm_vcpu *vcpu, bool for_injection) { return false; } > +static inline int tdx_enter_smm(struct kvm_vcpu *vcpu, union kvm_smram *smram) { return 0; } > +static inline int tdx_leave_smm(struct kvm_vcpu *vcpu, const union kvm_smram *smram) { return 0; } > +static inline void tdx_enable_smi_window(struct kvm_vcpu *vcpu) {} > +#endif > + > #endif /* __KVM_X86_VMX_X86_OPS_H */
On Mon, Feb 26, 2024, isaku.yamahata@intel.com wrote: > From: Isaku Yamahata <isaku.yamahata@intel.com> > > TDX doesn't support system-management mode (SMM) and system-management > interrupt (SMI) in guest TDs. Because guest state (vcpu state, memory > state) is protected, it must go through the TDX module APIs to change guest > state, injecting SMI and changing vcpu mode into SMM. The TDX module > doesn't provide a way for VMM to inject SMI into guest TD and a way for VMM > to switch guest vcpu mode into SMM. > > We have two options in KVM when handling SMM or SMI in the guest TD or the > device model (e.g. QEMU): 1) silently ignore the request or 2) return a > meaningful error. > > For simplicity, we implemented the option 1). > > Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com> > --- > arch/x86/kvm/smm.h | 7 +++++- > arch/x86/kvm/vmx/main.c | 45 ++++++++++++++++++++++++++++++++++---- > arch/x86/kvm/vmx/tdx.c | 29 ++++++++++++++++++++++++ > arch/x86/kvm/vmx/x86_ops.h | 12 ++++++++++ > 4 files changed, 88 insertions(+), 5 deletions(-) > > diff --git a/arch/x86/kvm/smm.h b/arch/x86/kvm/smm.h > index a1cf2ac5bd78..bc77902f5c18 100644 > --- a/arch/x86/kvm/smm.h > +++ b/arch/x86/kvm/smm.h > @@ -142,7 +142,12 @@ union kvm_smram { > > static inline int kvm_inject_smi(struct kvm_vcpu *vcpu) > { > - kvm_make_request(KVM_REQ_SMI, vcpu); > + /* > + * If SMM isn't supported (e.g. TDX), silently discard SMI request. > + * Assume that SMM supported = MSR_IA32_SMBASE supported. > + */ > + if (static_call(kvm_x86_has_emulated_msr)(vcpu->kvm, MSR_IA32_SMBASE)) > + kvm_make_request(KVM_REQ_SMI, vcpu); > return 0; No, just do what KVM already does for CONFIG_KVM_SMM=n, and return -ENOTTY. The *entire* point of have a return code is to handle setups that don't support SMM. if (!static_call(kvm_x86_has_emulated_msr)(vcpu->kvm, MSR_IA32_SMBASE))) return -ENOTTY; And with that, I would drop the comment, it's pretty darn clear what "assumption" is being made. In quotes because it's not an assumption, it's literally KVM's implementation. And then the changelog can say "do what KVM does for CONFIG_KVM_SMM=n" without having to explain why we decided to do something completely arbitrary for TDX. > } > > diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c > index ed46e7e57c18..4f3b872cd401 100644 > --- a/arch/x86/kvm/vmx/main.c > +++ b/arch/x86/kvm/vmx/main.c > @@ -283,6 +283,43 @@ static void vt_msr_filter_changed(struct kvm_vcpu *vcpu) > vmx_msr_filter_changed(vcpu); > } > > +#ifdef CONFIG_KVM_SMM > +static int vt_smi_allowed(struct kvm_vcpu *vcpu, bool for_injection) > +{ > + if (is_td_vcpu(vcpu)) > + return tdx_smi_allowed(vcpu, for_injection); Adding stubs for something that TDX will never support is silly. Bug the VM and return an error. if (KVM_BUG_ON(is_td_vcpu(vcpu))) return -EIO; And I wouldn't even bother with vt_* wrappers, just put that right in vmx_*(). Same thing for everything below.
On Fri, Apr 19, 2024 at 06:52:42AM -0700, Sean Christopherson <seanjc@google.com> wrote: > On Mon, Feb 26, 2024, isaku.yamahata@intel.com wrote: > > From: Isaku Yamahata <isaku.yamahata@intel.com> > > > > TDX doesn't support system-management mode (SMM) and system-management > > interrupt (SMI) in guest TDs. Because guest state (vcpu state, memory > > state) is protected, it must go through the TDX module APIs to change guest > > state, injecting SMI and changing vcpu mode into SMM. The TDX module > > doesn't provide a way for VMM to inject SMI into guest TD and a way for VMM > > to switch guest vcpu mode into SMM. > > > > We have two options in KVM when handling SMM or SMI in the guest TD or the > > device model (e.g. QEMU): 1) silently ignore the request or 2) return a > > meaningful error. > > > > For simplicity, we implemented the option 1). > > > > Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com> > > --- > > arch/x86/kvm/smm.h | 7 +++++- > > arch/x86/kvm/vmx/main.c | 45 ++++++++++++++++++++++++++++++++++---- > > arch/x86/kvm/vmx/tdx.c | 29 ++++++++++++++++++++++++ > > arch/x86/kvm/vmx/x86_ops.h | 12 ++++++++++ > > 4 files changed, 88 insertions(+), 5 deletions(-) > > > > diff --git a/arch/x86/kvm/smm.h b/arch/x86/kvm/smm.h > > index a1cf2ac5bd78..bc77902f5c18 100644 > > --- a/arch/x86/kvm/smm.h > > +++ b/arch/x86/kvm/smm.h > > @@ -142,7 +142,12 @@ union kvm_smram { > > > > static inline int kvm_inject_smi(struct kvm_vcpu *vcpu) > > { > > - kvm_make_request(KVM_REQ_SMI, vcpu); > > + /* > > + * If SMM isn't supported (e.g. TDX), silently discard SMI request. > > + * Assume that SMM supported = MSR_IA32_SMBASE supported. > > + */ > > + if (static_call(kvm_x86_has_emulated_msr)(vcpu->kvm, MSR_IA32_SMBASE)) > > + kvm_make_request(KVM_REQ_SMI, vcpu); > > return 0; > > No, just do what KVM already does for CONFIG_KVM_SMM=n, and return -ENOTTY. The > *entire* point of have a return code is to handle setups that don't support SMM. > > if (!static_call(kvm_x86_has_emulated_msr)(vcpu->kvm, MSR_IA32_SMBASE))) > return -ENOTTY; > > And with that, I would drop the comment, it's pretty darn clear what "assumption" > is being made. In quotes because it's not an assumption, it's literally KVM's > implementation. > > And then the changelog can say "do what KVM does for CONFIG_KVM_SMM=n" without > having to explain why we decided to do something completely arbitrary for TDX. Ok. > > } > > > > diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c > > index ed46e7e57c18..4f3b872cd401 100644 > > --- a/arch/x86/kvm/vmx/main.c > > +++ b/arch/x86/kvm/vmx/main.c > > @@ -283,6 +283,43 @@ static void vt_msr_filter_changed(struct kvm_vcpu *vcpu) > > vmx_msr_filter_changed(vcpu); > > } > > > > +#ifdef CONFIG_KVM_SMM > > +static int vt_smi_allowed(struct kvm_vcpu *vcpu, bool for_injection) > > +{ > > + if (is_td_vcpu(vcpu)) > > + return tdx_smi_allowed(vcpu, for_injection); > > Adding stubs for something that TDX will never support is silly. Bug the VM and > return an error. > > if (KVM_BUG_ON(is_td_vcpu(vcpu))) > return -EIO; > > And I wouldn't even bother with vt_* wrappers, just put that right in vmx_*(). > Same thing for everything below. Will drop them. Those are traces to support guest debug. It's future topic and we have arch.guest_state_protected check now.
On 4/19/2024 9:52 PM, Sean Christopherson wrote: > On Mon, Feb 26, 2024, isaku.yamahata@intel.com wrote: >> From: Isaku Yamahata <isaku.yamahata@intel.com> >> >> TDX doesn't support system-management mode (SMM) and system-management >> interrupt (SMI) in guest TDs. Because guest state (vcpu state, memory >> state) is protected, it must go through the TDX module APIs to change guest >> state, injecting SMI and changing vcpu mode into SMM. The TDX module >> doesn't provide a way for VMM to inject SMI into guest TD and a way for VMM >> to switch guest vcpu mode into SMM. >> >> We have two options in KVM when handling SMM or SMI in the guest TD or the >> device model (e.g. QEMU): 1) silently ignore the request or 2) return a >> meaningful error. >> >> For simplicity, we implemented the option 1). >> >> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com> >> --- >> arch/x86/kvm/smm.h | 7 +++++- >> arch/x86/kvm/vmx/main.c | 45 ++++++++++++++++++++++++++++++++++---- >> arch/x86/kvm/vmx/tdx.c | 29 ++++++++++++++++++++++++ >> arch/x86/kvm/vmx/x86_ops.h | 12 ++++++++++ >> 4 files changed, 88 insertions(+), 5 deletions(-) >> >> diff --git a/arch/x86/kvm/smm.h b/arch/x86/kvm/smm.h >> index a1cf2ac5bd78..bc77902f5c18 100644 >> --- a/arch/x86/kvm/smm.h >> +++ b/arch/x86/kvm/smm.h >> @@ -142,7 +142,12 @@ union kvm_smram { >> >> static inline int kvm_inject_smi(struct kvm_vcpu *vcpu) >> { >> - kvm_make_request(KVM_REQ_SMI, vcpu); >> + /* >> + * If SMM isn't supported (e.g. TDX), silently discard SMI request. >> + * Assume that SMM supported = MSR_IA32_SMBASE supported. >> + */ >> + if (static_call(kvm_x86_has_emulated_msr)(vcpu->kvm, MSR_IA32_SMBASE)) >> + kvm_make_request(KVM_REQ_SMI, vcpu); >> return 0; > No, just do what KVM already does for CONFIG_KVM_SMM=n, and return -ENOTTY. The > *entire* point of have a return code is to handle setups that don't support SMM. > > if (!static_call(kvm_x86_has_emulated_msr)(vcpu->kvm, MSR_IA32_SMBASE))) > return -ENOTTY; > > And with that, I would drop the comment, it's pretty darn clear what "assumption" > is being made. In quotes because it's not an assumption, it's literally KVM's > implementation. > > And then the changelog can say "do what KVM does for CONFIG_KVM_SMM=n" without > having to explain why we decided to do something completely arbitrary for TDX. > >> } >> >> diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c >> index ed46e7e57c18..4f3b872cd401 100644 >> --- a/arch/x86/kvm/vmx/main.c >> +++ b/arch/x86/kvm/vmx/main.c >> @@ -283,6 +283,43 @@ static void vt_msr_filter_changed(struct kvm_vcpu *vcpu) >> vmx_msr_filter_changed(vcpu); >> } >> >> +#ifdef CONFIG_KVM_SMM >> +static int vt_smi_allowed(struct kvm_vcpu *vcpu, bool for_injection) >> +{ >> + if (is_td_vcpu(vcpu)) >> + return tdx_smi_allowed(vcpu, for_injection); > Adding stubs for something that TDX will never support is silly. Bug the VM and > return an error. > > if (KVM_BUG_ON(is_td_vcpu(vcpu))) > return -EIO; is_td_vcpu() is defined in tdx.h. Do you mind using open code to check whether the VM is TD in vmx.c? "vcpu->kvm->arch.vm_type == KVM_X86_TDX_VM" > > And I wouldn't even bother with vt_* wrappers, just put that right in vmx_*(). > Same thing for everything below. >
On Tue, Jun 11, 2024 at 3:18 PM Binbin Wu <binbin.wu@linux.intel.com> wrote: > >> } > >> > >> diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c > >> index ed46e7e57c18..4f3b872cd401 100644 > >> --- a/arch/x86/kvm/vmx/main.c > >> +++ b/arch/x86/kvm/vmx/main.c > >> @@ -283,6 +283,43 @@ static void vt_msr_filter_changed(struct kvm_vcpu *vcpu) > >> vmx_msr_filter_changed(vcpu); > >> } > >> > >> +#ifdef CONFIG_KVM_SMM > >> +static int vt_smi_allowed(struct kvm_vcpu *vcpu, bool for_injection) > >> +{ > >> + if (is_td_vcpu(vcpu)) > >> + return tdx_smi_allowed(vcpu, for_injection); > > Adding stubs for something that TDX will never support is silly. Bug the VM and > > return an error. > > > > if (KVM_BUG_ON(is_td_vcpu(vcpu))) > > return -EIO; > > is_td_vcpu() is defined in tdx.h. > Do you mind using open code to check whether the VM is TD in vmx.c? > "vcpu->kvm->arch.vm_type == KVM_X86_TDX_VM" I'd move it to some place that main.c can see. Or vmx.c as Sean says below, but I am not sure I like the idea too much. Paolo > > And I wouldn't even bother with vt_* wrappers, just put that right in vmx_*(). > > Same thing for everything below. If it's a KVM_BUG_ON()
On 6/11/2024 10:11 PM, Paolo Bonzini wrote: > On Tue, Jun 11, 2024 at 3:18 PM Binbin Wu <binbin.wu@linux.intel.com> wrote: >>>> } >>>> >>>> diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c >>>> index ed46e7e57c18..4f3b872cd401 100644 >>>> --- a/arch/x86/kvm/vmx/main.c >>>> +++ b/arch/x86/kvm/vmx/main.c >>>> @@ -283,6 +283,43 @@ static void vt_msr_filter_changed(struct kvm_vcpu *vcpu) >>>> vmx_msr_filter_changed(vcpu); >>>> } >>>> >>>> +#ifdef CONFIG_KVM_SMM >>>> +static int vt_smi_allowed(struct kvm_vcpu *vcpu, bool for_injection) >>>> +{ >>>> + if (is_td_vcpu(vcpu)) >>>> + return tdx_smi_allowed(vcpu, for_injection); >>> Adding stubs for something that TDX will never support is silly. Bug the VM and >>> return an error. >>> >>> if (KVM_BUG_ON(is_td_vcpu(vcpu))) >>> return -EIO; >> is_td_vcpu() is defined in tdx.h. >> Do you mind using open code to check whether the VM is TD in vmx.c? >> "vcpu->kvm->arch.vm_type == KVM_X86_TDX_VM" > I'd move it to some place that main.c can see. is_td_vcpu() can be seen in main.c > Or vmx.c as Sean says > below, but I am not sure I like the idea too much. Which you may not like? Remove the vt_* wrapper or use KVM_BUG_ON()? > > Paolo > >>> And I wouldn't even bother with vt_* wrappers, just put that right in vmx_*(). >>> Same thing for everything below. > If it's a KVM_BUG_ON() >
On Wed, Jun 12, 2024 at 3:06 PM Binbin Wu <binbin.wu@linux.intel.com> wrote: > On 6/11/2024 10:11 PM, Paolo Bonzini wrote: > > On Tue, Jun 11, 2024 at 3:18 PM Binbin Wu <binbin.wu@linux.intel.com> wrote: > > > is_td_vcpu() is defined in tdx.h. > > > Do you mind using open code to check whether the VM is TD in vmx.c? > > > "vcpu->kvm->arch.vm_type == KVM_X86_TDX_VM" > > I'd move it to some place that main.c can see. > > is_td_vcpu() can be seen in main.c Ok. > > Or vmx.c as Sean says > > below, but I am not sure I like the idea too much. > > Which you may not like? Remove the vt_* wrapper or use KVM_BUG_ON()? Removing the vt_* wrapper. Paolo
diff --git a/arch/x86/kvm/smm.h b/arch/x86/kvm/smm.h index a1cf2ac5bd78..bc77902f5c18 100644 --- a/arch/x86/kvm/smm.h +++ b/arch/x86/kvm/smm.h @@ -142,7 +142,12 @@ union kvm_smram { static inline int kvm_inject_smi(struct kvm_vcpu *vcpu) { - kvm_make_request(KVM_REQ_SMI, vcpu); + /* + * If SMM isn't supported (e.g. TDX), silently discard SMI request. + * Assume that SMM supported = MSR_IA32_SMBASE supported. + */ + if (static_call(kvm_x86_has_emulated_msr)(vcpu->kvm, MSR_IA32_SMBASE)) + kvm_make_request(KVM_REQ_SMI, vcpu); return 0; } diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c index ed46e7e57c18..4f3b872cd401 100644 --- a/arch/x86/kvm/vmx/main.c +++ b/arch/x86/kvm/vmx/main.c @@ -283,6 +283,43 @@ static void vt_msr_filter_changed(struct kvm_vcpu *vcpu) vmx_msr_filter_changed(vcpu); } +#ifdef CONFIG_KVM_SMM +static int vt_smi_allowed(struct kvm_vcpu *vcpu, bool for_injection) +{ + if (is_td_vcpu(vcpu)) + return tdx_smi_allowed(vcpu, for_injection); + + return vmx_smi_allowed(vcpu, for_injection); +} + +static int vt_enter_smm(struct kvm_vcpu *vcpu, union kvm_smram *smram) +{ + if (unlikely(is_td_vcpu(vcpu))) + return tdx_enter_smm(vcpu, smram); + + return vmx_enter_smm(vcpu, smram); +} + +static int vt_leave_smm(struct kvm_vcpu *vcpu, const union kvm_smram *smram) +{ + if (unlikely(is_td_vcpu(vcpu))) + return tdx_leave_smm(vcpu, smram); + + return vmx_leave_smm(vcpu, smram); +} + +static void vt_enable_smi_window(struct kvm_vcpu *vcpu) +{ + if (is_td_vcpu(vcpu)) { + tdx_enable_smi_window(vcpu); + return; + } + + /* RSM will cause a vmexit anyway. */ + vmx_enable_smi_window(vcpu); +} +#endif + static void vt_apicv_pre_state_restore(struct kvm_vcpu *vcpu) { struct pi_desc *pi = vcpu_to_pi_desc(vcpu); @@ -700,10 +737,10 @@ struct kvm_x86_ops vt_x86_ops __initdata = { .setup_mce = vmx_setup_mce, #ifdef CONFIG_KVM_SMM - .smi_allowed = vmx_smi_allowed, - .enter_smm = vmx_enter_smm, - .leave_smm = vmx_leave_smm, - .enable_smi_window = vmx_enable_smi_window, + .smi_allowed = vt_smi_allowed, + .enter_smm = vt_enter_smm, + .leave_smm = vt_leave_smm, + .enable_smi_window = vt_enable_smi_window, #endif .check_emulate_instruction = vmx_check_emulate_instruction, diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c index 725cb40d0814..d9b36373e7d0 100644 --- a/arch/x86/kvm/vmx/tdx.c +++ b/arch/x86/kvm/vmx/tdx.c @@ -2044,6 +2044,35 @@ int tdx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr) } } +#ifdef CONFIG_KVM_SMM +int tdx_smi_allowed(struct kvm_vcpu *vcpu, bool for_injection) +{ + /* SMI isn't supported for TDX. */ + WARN_ON_ONCE(1); + return false; +} + +int tdx_enter_smm(struct kvm_vcpu *vcpu, union kvm_smram *smram) +{ + /* smi_allowed() is always false for TDX as above. */ + WARN_ON_ONCE(1); + return 0; +} + +int tdx_leave_smm(struct kvm_vcpu *vcpu, const union kvm_smram *smram) +{ + WARN_ON_ONCE(1); + return 0; +} + +void tdx_enable_smi_window(struct kvm_vcpu *vcpu) +{ + /* SMI isn't supported for TDX. Silently discard SMI request. */ + WARN_ON_ONCE(1); + vcpu->arch.smi_pending = false; +} +#endif + static int tdx_get_capabilities(struct kvm_tdx_cmd *cmd) { struct kvm_tdx_capabilities __user *user_caps; diff --git a/arch/x86/kvm/vmx/x86_ops.h b/arch/x86/kvm/vmx/x86_ops.h index 017a73ab34bb..7c63b2b48125 100644 --- a/arch/x86/kvm/vmx/x86_ops.h +++ b/arch/x86/kvm/vmx/x86_ops.h @@ -241,4 +241,16 @@ int tdx_pre_memory_mapping(struct kvm_vcpu *vcpu, void tdx_post_memory_mapping(struct kvm_vcpu *vcpu, struct kvm_memory_mapping *mapping) {} #endif +#if defined(CONFIG_INTEL_TDX_HOST) && defined(CONFIG_KVM_SMM) +int tdx_smi_allowed(struct kvm_vcpu *vcpu, bool for_injection); +int tdx_enter_smm(struct kvm_vcpu *vcpu, union kvm_smram *smram); +int tdx_leave_smm(struct kvm_vcpu *vcpu, const union kvm_smram *smram); +void tdx_enable_smi_window(struct kvm_vcpu *vcpu); +#else +static inline int tdx_smi_allowed(struct kvm_vcpu *vcpu, bool for_injection) { return false; } +static inline int tdx_enter_smm(struct kvm_vcpu *vcpu, union kvm_smram *smram) { return 0; } +static inline int tdx_leave_smm(struct kvm_vcpu *vcpu, const union kvm_smram *smram) { return 0; } +static inline void tdx_enable_smi_window(struct kvm_vcpu *vcpu) {} +#endif + #endif /* __KVM_X86_VMX_X86_OPS_H */