x86/kvm: avoid expensive rdmsrs for FS/GS base MSRs

Some time ago Paolo suggested to take a look at probably unneeded expensive rdmsrs for FS/GS base MSR in vmx_save_host_state(). This is called on every vcpu run when we need to handle vmexit in userspace.

I have to admit I got a bit lost in our kernel FS/GS magic. I managed to convince myself that in the well defined context (ioctl from userspace) we can always get the required values from in-kernel variables and avoid rdmsrs. But I may have missed something really important, thus RFC.

My debug shows we're shaving off 240 cpu cycles (E5-2603 v3).

In case these patches turn out to be worthwile AMD SVM can probably be optimized the ame way.

Vitaly Kuznetsov (3):
x86/kvm/vmx: read MSR_FS_BASE from current->thread x86/kvm/vmx: read MSR_KERNEL_GS_BASE from current->thread x86/kvm/vmx: avoid expensive rdmsr for MSR_GS_BASE

 arch/x86/kernel/cpu/common.c | 1 +
 arch/x86/kvm/vmx.c           | 7 ++++---
 2 files changed, 5 insertions(+), 3 deletions(-)

2.14.3

vmx_save_host_state() is only called from kvm_arch_vcpu_ioctl_run() so the context is pretty well defined and MSR_FS_BASE should always be equal to current->thread.fsbase.

---
 arch/x86/kvm/vmx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index cab6ea1f8be5..ddefb7aeb96d 100644
@@ -2157,7 +2157,7 @@ static void vmx_save_host_state(struct kvm_vcpu *vcpu)
 #endif
 
 #ifdef CONFIG_X86_64
-	vmcs_writel(HOST_FS_BASE, read_msr(MSR_FS_BASE));
+	vmcs_writel(HOST_FS_BASE, current->thread.fsbase);
 	vmcs_writel(HOST_GS_BASE, read_msr(MSR_GS_BASE));
 #else
 	vmcs_writel(HOST_FS_BASE, segment_base(vmx->host_state.fs_sel));

2.14.3

True.

Not true. current->thread.fsbase is almost entirely undefined in this context. What you *could* do is export save_fsgs() and call it first. When FSGSBASE support lands (which will happen eventually!), the code in your patch will be completely wrong.

Admittedly, your patch isn't 100% bogus, but the reason is subtle and you need lots of comments there *and* in save_fsgs().

Thank you Andy,

I'll take a look.

  Vitaly

Just to make sure I understand the reason,

Currently, the only way for processes to change FS/GS base is to call ARCH_SET_FS/GS prctls and these reflect the changes they make in thread.fs/gsbase so *conceptually* reading them is OK now.

Now there's so called X86_BUG_NULL_SEG: on Intel CPUs writing '0' to FS/GS selectors zeroes the base (and on AMDs it doesn't). save_fsgs() checks fs/gs selectors and adjusts thread.fs/gsbase accordingly. (de-facto no-issue for my patch as it only touches Intel's VMX but we don't want to rely on vendor, detect_null_seg_behavior() does real check of the behavior).

Now FSGSBASE support comes to play. Userspace will start changing FS/GS base without kernel's intervention so we really need to do read if we want to figure out what's there. Luckily, reads are now cheaper thanks to new instructions.

So what I think we need to do here is introduce "sync_process_fs_gs()" ('save_fsgs' now) api which we will call from both __switch_to() and KVM's vmx_save_host_state() before reading thread.fs/gsbase.

  Vitaly

vmx_save_host_state() is only called from kvm_arch_vcpu_ioctl_run() so the context is pretty well defined and as we're past 'swapgs' MSR_KERNEL_GS_BASE should be current->thread.gsbase.

---
 arch/x86/kvm/vmx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index ddefb7aeb96d..5be34d13d88f 100644
@@ -2165,7 +2165,7 @@ static void vmx_save_host_state(struct kvm_vcpu *vcpu)
 #endif
 
 #ifdef CONFIG_X86_64
-	rdmsrl(MSR_KERNEL_GS_BASE, vmx->msr_host_kernel_gs_base);
+	vmx->msr_host_kernel_gs_base = current->thread.gsbase;
 	if (is_long_mode(&vmx->vcpu))
 		wrmsrl(MSR_KERNEL_GS_BASE, vmx->msr_guest_kernel_gs_base);
 #endif

2.14.3

vmx_save_host_state() is only called from kvm_arch_vcpu_ioctl_run() so the context is pretty well defined and as we're past 'swapgs' MSR_GS_BASE should contain kernel's GS base which we point to irq_stack_union.

irq_stack_union needs to be exported as KVM can be a module.

---
 arch/x86/kernel/cpu/common.c | 1 +
 arch/x86/kvm/vmx.c           | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

index 348cf4821240..057393711093 100644
@@ -1398,6 +1398,7 @@ __setup("clearcpuid=", setup_clearcpuid);
 #ifdef CONFIG_X86_64
 DEFINE_PER_CPU_FIRST(union irq_stack_union,
 		     irq_stack_union) __aligned(PAGE_SIZE) __visible;
+EXPORT_PER_CPU_SYMBOL(irq_stack_union);
 
 /*
  * The following percpu variables are hot.  Align current_task to

index 5be34d13d88f..bf856b7ece3f 100644
@@ -2158,7 +2158,8 @@ static void vmx_save_host_state(struct kvm_vcpu *vcpu)
 
 #ifdef CONFIG_X86_64
 	vmcs_writel(HOST_FS_BASE, current->thread.fsbase);
-	vmcs_writel(HOST_GS_BASE, read_msr(MSR_GS_BASE));
+	vmcs_writel(HOST_GS_BASE, (unsigned long)
+		    per_cpu(irq_stack_union.gs_base, smp_processor_id()));
 #else
 	vmcs_writel(HOST_FS_BASE, segment_base(vmx->host_state.fs_sel));
 	vmcs_writel(HOST_GS_BASE, segment_base(vmx->host_state.gs_sel));

2.14.3

Sure, will do.

Doing plain rdmsr() in KVM is definitely cleaner and less fragile but assuming 240 cpu cycles I'm cutting with this series justify the increased complexity suggestions to avoid hardcoding x86 internals and make clean API are more than welcome. Thank you!

  Vitaly

SVM is a bit different, because it uses VMLOAD/VMSAVE and so it doesn't have an equivalent of vmx_save_host_state(). Unfortunately, you cannot really eliminate VMLOAD/VMSAVE because it's the only way to load the hidden state of TR and LDTR---so you might as well use it to load FS and GS, even in 64-bit mode.

In order to decrease the cost of vmload/vmsave, we could single out the simplest vmexit handlers and process them without even getting out of svm_vcpu_run, thus skipping all four of stgi/vmload/vmsave/clgi. However, this probably couldn't be done for the really common vmexits such as nested page fault, PIO or most MSR accesses. We _could_ do it for nested virt-related vmexits, but the advantage of that is getting smaller too, since Zen provides hardware support for nested GIF and nested VMLOAD/VMSAVE.

Paolo