Re: [PATCH RFC 3/3] x86/kvm/vmx: avoid expensive rdmsr for MSR_GS_BASE
Andy Lutomirskiwrites: > On Fri, Mar 2, 2018 at 10:55 AM, Vitaly Kuznetsov 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 >> --- >> 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 3/3] x86/kvm/vmx: avoid expensive rdmsr for MSR_GS_BASE
Andy Lutomirski writes: > On Fri, Mar 2, 2018 at 10:55 AM, Vitaly Kuznetsov 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 >> --- >> 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 3/3] x86/kvm/vmx: avoid expensive rdmsr for MSR_GS_BASE
On Fri, Mar 2, 2018 at 10:55 AM, Vitaly Kuznetsovwrote: > 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 > --- > 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 3/3] x86/kvm/vmx: avoid expensive rdmsr for MSR_GS_BASE
On Fri, Mar 2, 2018 at 10:55 AM, Vitaly Kuznetsov 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 > --- > 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.