diff mbox series

[v1,09/23] KVM: VMX: Switch FRED RSP0 between host and guest

Message ID 20231108183003.5981-10-xin3.li@intel.com
State New
Headers
Series Enable FRED with KVM VMX |

Commit Message

Li, Xin3 Nov. 8, 2023, 6:29 p.m. UTC 
  Switch MSR_IA32_FRED_RSP0 between host and guest in
vmx_prepare_switch_to_{host,guest}().

MSR_IA32_FRED_RSP0 is used during ring 3 event delivery only, thus
KVM, running on ring 0, can run safely with guest FRED RSP0, i.e.,
no need to switch between host/guest FRED RSP0 during VM entry and
exit.

KVM should switch to host FRED RSP0 before returning to user level,
and switch to guest FRED RSP0 before entering guest mode.

Tested-by: Shan Kang <shan.kang@intel.com>
Signed-off-by: Xin Li <xin3.li@intel.com>
---
 arch/x86/kvm/vmx/vmx.c | 17 +++++++++++++++++
 arch/x86/kvm/vmx/vmx.h |  2 ++
 2 files changed, 19 insertions(+)

Comments

Chao Gao Nov. 13, 2023, 3:47 a.m. UTC | #1 
On Wed, Nov 08, 2023 at 10:29:49AM -0800, Xin Li wrote:
>Switch MSR_IA32_FRED_RSP0 between host and guest in
>vmx_prepare_switch_to_{host,guest}().
>
>MSR_IA32_FRED_RSP0 is used during ring 3 event delivery only, thus
>KVM, running on ring 0, can run safely with guest FRED RSP0, i.e.,
>no need to switch between host/guest FRED RSP0 during VM entry and
>exit.
>
>KVM should switch to host FRED RSP0 before returning to user level,
>and switch to guest FRED RSP0 before entering guest mode.
>
>Tested-by: Shan Kang <shan.kang@intel.com>
>Signed-off-by: Xin Li <xin3.li@intel.com>
>---
> arch/x86/kvm/vmx/vmx.c | 17 +++++++++++++++++
> arch/x86/kvm/vmx/vmx.h |  2 ++
> 2 files changed, 19 insertions(+)
>
>diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>index 41772ecdd368..d00ab9d4c93e 100644
>--- a/arch/x86/kvm/vmx/vmx.c
>+++ b/arch/x86/kvm/vmx/vmx.c
>@@ -1344,6 +1344,17 @@ 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_cpuid_has(vcpu, X86_FEATURE_FRED)) {
>+		/*
>+		 * MSR_IA32_FRED_RSP0 is top of task stack, which never changes.
>+		 * Thus it should be initialized only once.
>+		 */
>+		if (unlikely(vmx->msr_host_fred_rsp0 == 0))
>+			vmx->msr_host_fred_rsp0 = read_msr(MSR_IA32_FRED_RSP0);
>+		wrmsrl(MSR_IA32_FRED_RSP0, vmx->msr_guest_fred_rsp0);
>+	}
> #else
> 	savesegment(fs, fs_sel);
> 	savesegment(gs, gs_sel);
>@@ -1388,6 +1399,12 @@ static void vmx_prepare_switch_to_host(struct vcpu_vmx *vmx)
> 	invalidate_tss_limit();
> #ifdef CONFIG_X86_64
> 	wrmsrl(MSR_KERNEL_GS_BASE, vmx->msr_host_kernel_gs_base);
>+
>+	if (cpu_feature_enabled(X86_FEATURE_FRED) &&
>+	    guest_cpuid_has(&vmx->vcpu, X86_FEATURE_FRED)) {

IIUC, vmx_prepare_switch_to_host() is called from IRQ-disabled context. using
guest_cpuid_has() in this context is not desired, see lockdep_assert_irqs_enabled()
in cpuid_entry2_find().

>+		vmx->msr_guest_fred_rsp0 = read_msr(MSR_IA32_FRED_RSP0);
>+		wrmsrl(MSR_IA32_FRED_RSP0, vmx->msr_host_fred_rsp0);
>+	}
> #endif
> 	load_fixmap_gdt(raw_smp_processor_id());
> 	vmx->guest_state_loaded = false;
>diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
>index f8c02bd37069..328a3447f064 100644
>--- a/arch/x86/kvm/vmx/vmx.h
>+++ b/arch/x86/kvm/vmx/vmx.h
>@@ -276,6 +276,8 @@ struct vcpu_vmx {
> #ifdef CONFIG_X86_64
> 	u64		      msr_host_kernel_gs_base;
> 	u64		      msr_guest_kernel_gs_base;
>+	u64		      msr_host_fred_rsp0;
>+	u64		      msr_guest_fred_rsp0;

resetting guest fred rsp0 to 0 during vcpu reset is missing.

> #endif
> 
> 	u64		      spec_ctrl;
>-- 
>2.42.0
>
>
Li, Xin3 Nov. 14, 2023, 5:17 a.m. UTC | #2 
> >+	if (cpu_feature_enabled(X86_FEATURE_FRED) &&
> >+	    guest_cpuid_has(&vmx->vcpu, X86_FEATURE_FRED)) {
> 
> IIUC, vmx_prepare_switch_to_host() is called from IRQ-disabled context. using
> guest_cpuid_has() in this context is not desired, see
> lockdep_assert_irqs_enabled() in cpuid_entry2_find().

Nice catch!

Anyway it's a bad idea to do a search call here, let me find a better way
for all FRED CPUID checks.

> >diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h index
> >f8c02bd37069..328a3447f064 100644
> >--- a/arch/x86/kvm/vmx/vmx.h
> >+++ b/arch/x86/kvm/vmx/vmx.h
> >@@ -276,6 +276,8 @@ struct vcpu_vmx {
> > #ifdef CONFIG_X86_64
> > 	u64		      msr_host_kernel_gs_base;
> > 	u64		      msr_guest_kernel_gs_base;
> >+	u64		      msr_host_fred_rsp0;
> >+	u64		      msr_guest_fred_rsp0;
> 
> resetting guest fred rsp0 to 0 during vcpu reset is missing.

hmm, I assume it gets the same treatment as guest_kernel_gs_base.

It seems we don't reset guest_kernel_gs_base.  No?
Chao Gao Nov. 14, 2023, 7:47 a.m. UTC | #3 
>> >diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h index
>> >f8c02bd37069..328a3447f064 100644
>> >--- a/arch/x86/kvm/vmx/vmx.h
>> >+++ b/arch/x86/kvm/vmx/vmx.h
>> >@@ -276,6 +276,8 @@ struct vcpu_vmx {
>> > #ifdef CONFIG_X86_64
>> > 	u64		      msr_host_kernel_gs_base;
>> > 	u64		      msr_guest_kernel_gs_base;
>> >+	u64		      msr_host_fred_rsp0;
>> >+	u64		      msr_guest_fred_rsp0;
>> 
>> resetting guest fred rsp0 to 0 during vcpu reset is missing.
>
>hmm, I assume it gets the same treatment as guest_kernel_gs_base.
>
>It seems we don't reset guest_kernel_gs_base.  No?

Yes. But for fred MSRs, FRED spec clearly says their RESET values
are 0s. for kernel_gs_base MSR, looks there is no such description
in SDM.
Li, Xin3 Nov. 15, 2023, 3:04 a.m. UTC | #4 
> >> >diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h index
> >> >f8c02bd37069..328a3447f064 100644
> >> >--- a/arch/x86/kvm/vmx/vmx.h
> >> >+++ b/arch/x86/kvm/vmx/vmx.h
> >> >@@ -276,6 +276,8 @@ struct vcpu_vmx {
> >> > #ifdef CONFIG_X86_64
> >> > 	u64		      msr_host_kernel_gs_base;
> >> > 	u64		      msr_guest_kernel_gs_base;
> >> >+	u64		      msr_host_fred_rsp0;
> >> >+	u64		      msr_guest_fred_rsp0;
> >>
> >> resetting guest fred rsp0 to 0 during vcpu reset is missing.
> >
> >hmm, I assume it gets the same treatment as guest_kernel_gs_base.
> >
> >It seems we don't reset guest_kernel_gs_base.  No?
> 
> Yes. But for fred MSRs, FRED spec clearly says their RESET values
> are 0s. for kernel_gs_base MSR, looks there is no such description
> in SDM.

Right, maybe better to set both to 0s.
diff mbox series

Patch

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 41772ecdd368..d00ab9d4c93e 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -1344,6 +1344,17 @@  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_cpuid_has(vcpu, X86_FEATURE_FRED)) {
+		/*
+		 * MSR_IA32_FRED_RSP0 is top of task stack, which never changes.
+		 * Thus it should be initialized only once.
+		 */
+		if (unlikely(vmx->msr_host_fred_rsp0 == 0))
+			vmx->msr_host_fred_rsp0 = read_msr(MSR_IA32_FRED_RSP0);
+		wrmsrl(MSR_IA32_FRED_RSP0, vmx->msr_guest_fred_rsp0);
+	}
 #else
 	savesegment(fs, fs_sel);
 	savesegment(gs, gs_sel);
@@ -1388,6 +1399,12 @@  static void vmx_prepare_switch_to_host(struct vcpu_vmx *vmx)
 	invalidate_tss_limit();
 #ifdef CONFIG_X86_64
 	wrmsrl(MSR_KERNEL_GS_BASE, vmx->msr_host_kernel_gs_base);
+
+	if (cpu_feature_enabled(X86_FEATURE_FRED) &&
+	    guest_cpuid_has(&vmx->vcpu, X86_FEATURE_FRED)) {
+		vmx->msr_guest_fred_rsp0 = read_msr(MSR_IA32_FRED_RSP0);
+		wrmsrl(MSR_IA32_FRED_RSP0, vmx->msr_host_fred_rsp0);
+	}
 #endif
 	load_fixmap_gdt(raw_smp_processor_id());
 	vmx->guest_state_loaded = false;
diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
index f8c02bd37069..328a3447f064 100644
--- a/arch/x86/kvm/vmx/vmx.h
+++ b/arch/x86/kvm/vmx/vmx.h
@@ -276,6 +276,8 @@  struct vcpu_vmx {
 #ifdef CONFIG_X86_64
 	u64		      msr_host_kernel_gs_base;
 	u64		      msr_guest_kernel_gs_base;
+	u64		      msr_host_fred_rsp0;
+	u64		      msr_guest_fred_rsp0;
 #endif
 
 	u64		      spec_ctrl;