[PATCH RFC 0/3] x86/kvm: avoid expensive rdmsrs for FS/GS base MSRs

[PATCH RFC 0/3] x86/kvm: avoid expensive rdmsrs for FS/GS base MSRs

[Vitaly Kuznetsov]

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

[PATCH RFC 1/3] x86/kvm/vmx: read MSR_FS_BASE from current->thread

[Vitaly Kuznetsov]

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.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 arch/x86/kvm/vmx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index cab6ea1f8be5..ddefb7aeb96d 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -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

[PATCH RFC 2/3] x86/kvm/vmx: read MSR_KERNEL_GS_BASE from current->thread

[Vitaly Kuznetsov]

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.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 arch/x86/kvm/vmx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index ddefb7aeb96d..5be34d13d88f 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -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

[PATCH RFC 3/3] x86/kvm/vmx: avoid expensive rdmsr for MSR_GS_BASE

[Vitaly Kuznetsov]

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.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 arch/x86/kernel/cpu/common.c | 1 +
 arch/x86/kvm/vmx.c           | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 348cf4821240..057393711093 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -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
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 5be34d13d88f..bf856b7ece3f 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -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

Re: [PATCH RFC 1/3] x86/kvm/vmx: read MSR_FS_BASE from current->thread

[Andy Lutomirski]

On Fri, Mar 2, 2018 at 10:55 AM, Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
> vmx_save_host_state() is only called from kvm_arch_vcpu_ioctl_run() so
> the context is pretty well defined
>

True.

> and MSR_FS_BASE should always be
> equal to current->thread.fsbase.

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().

Re: [PATCH RFC 3/3] x86/kvm/vmx: avoid expensive rdmsr for MSR_GS_BASE

[Andy Lutomirski]

On Fri, Mar 2, 2018 at 10:55 AM, Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
> 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.
>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  arch/x86/kernel/cpu/common.c | 1 +
>  arch/x86/kvm/vmx.c           | 3 ++-
>  2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 348cf4821240..057393711093 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -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);

GPL.

Also, can you add a static inline unsigned long
this_cpu_kernelmode_gs_base() that returns the actual value and then
use it here and in arch/x86/cpu/common.c?  I really don't like the way
that KVM code hardcodes all kinds of assumptions about how the rest of
the x86 code works rather than improving the x86 code to have the
right hooks for KVM's use.

Re: [PATCH RFC 1/3] x86/kvm/vmx: read MSR_FS_BASE from current->thread

[Vitaly Kuznetsov]

Andy Lutomirski <luto@kernel.org> writes:

> On Fri, Mar 2, 2018 at 10:55 AM, Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>> vmx_save_host_state() is only called from kvm_arch_vcpu_ioctl_run() so
>> the context is pretty well defined
>>
>
> True.
>
>> and MSR_FS_BASE should always be
>> equal to current->thread.fsbase.
>
> 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

Re: [PATCH RFC 3/3] x86/kvm/vmx: avoid expensive rdmsr for MSR_GS_BASE

[Vitaly Kuznetsov]

Andy Lutomirski <luto@kernel.org> writes:

> On Fri, Mar 2, 2018 at 10:55 AM, Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>> 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.
>>
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>>  arch/x86/kernel/cpu/common.c | 1 +
>>  arch/x86/kvm/vmx.c           | 3 ++-
>>  2 files changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
>> index 348cf4821240..057393711093 100644
>> --- a/arch/x86/kernel/cpu/common.c
>> +++ b/arch/x86/kernel/cpu/common.c
>> @@ -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);
>
> GPL.
>
> Also, can you add a static inline unsigned long
> this_cpu_kernelmode_gs_base() that returns the actual value and then
> use it here and in arch/x86/cpu/common.c?  I really don't like the way
> that KVM code hardcodes all kinds of assumptions about how the rest of
> the x86 code works rather than improving the x86 code to have the
> right hooks for KVM's use.

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

Re: [PATCH RFC 0/3] x86/kvm: avoid expensive rdmsrs for FS/GS base MSRs

[Paolo Bonzini]

On 02/03/2018 11:55, Vitaly Kuznetsov wrote:
> 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.

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

> 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(-)
> 

Re: [PATCH RFC 1/3] x86/kvm/vmx: read MSR_FS_BASE from current->thread

[Vitaly Kuznetsov]

Andy Lutomirski <luto@kernel.org> writes:

> On Fri, Mar 2, 2018 at 10:55 AM, Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>> vmx_save_host_state() is only called from kvm_arch_vcpu_ioctl_run() so
>> the context is pretty well defined
>>
>
> True.
>
>> and MSR_FS_BASE should always be
>> equal to current->thread.fsbase.
>
> 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().

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