KVM TSS cleanups and speedups
cover KVM TSS cleanups and speedups
The first four patches here are intended to be straightforward cleanups and to make a better base for Thomas' GDT series. They may be a slight speedup, too, because they remove an STR instruction from the VMX entry path.
The last two patches are a reasonably large speedup but need careful review.
FWIW, I can see lots of additional easy-ish speedups here. For example:
- The GDT reload on VM exit isn't really needed at all. Instead let's just change the kernel limit to 0xFFFF. Doing that naively would waste memory, but doing it carefully on top of Thomas' series would be straightforward and almost free.
- RDMSR from MSR_GS_BASE is totally pointless.
- Once I or someone finishes the FSGSBASE series, we get a big speedup there.
- The LDT reload code should be split up and optimized better, I think.
Andy Lutomirski (6):
x86/asm: Define the kernel TSS limit in a macro x86/kvm/vmx: Don't fetch the TSS base from the GDT x86/kvm/vmx: Get rid of segment_base() on 64-bit kernels x86/kvm/vmx: Simplify segment_base()
x86/asm/64: Drop __cacheline_aligned from struct x86_hw_tss x86/kvm/vmx: Defer TR reload after VM exit
arch/x86/include/asm/desc.h | 58 ++++++++++++++++++++++++++++++------ arch/x86/include/asm/processor.h | 12 +++++++- arch/x86/kernel/ioport.c | 5 ++++ arch/x86/kernel/process.c | 10 +++++++ arch/x86/kvm/vmx.c | 63 +++++++++++++++++----------------------- 5 files changed, 102 insertions(+), 46 deletions(-)
Signature · 1 line
2.9.3
PATCH 1/6 x86/asm: Define the kernel TSS limit in a macro
Rather than open-coding the kernel TSS limit in set_tss_desc(), make it a real macro near the TSS layout definition.
This is purely a cleanup.
--- arch/x86/include/asm/desc.h | 10 +--------- arch/x86/include/asm/processor.h | 10 ++++++++++ 2 files changed, 11 insertions(+), 9 deletions(-)
arch/x86/include/asm/desc.h +1 -9
@@ -177,16 +177,8 @@ static inline void __set_tss_desc(unsigned cpu, unsigned int entry, void *addr)struct desc_struct *d =get_cpu_gdt_table (cpu);tss_desc tss; -/* -* sizeof(unsigned long) coming from an extra "long" at the end -* of the iobitmap. See tss_struct definition in processor.h -* -* -1? seg base+limit should be pointing to the address of the -* last valid byte -*/ set_tssldt_descriptor (&tss, (unsigned long )addr,DESC_TSS , -IO_BITMAP_OFFSET +IO_BITMAP_BYTES + -sizeof (unsigned long ) -1 ); + __KERNEL_TSS_LIMIT);write_gdt_entry (d, entry, &tss,DESC_TSS ); }
arch/x86/include/asm/processor.h +10 -0
@@ -342,6 +342,16 @@ struct tss_struct { DECLARE_PER_CPU_SHARED_ALIGNED(struct tss_struct, cpu_tss); +/* +* sizeof(unsigned long) coming from an extra "long" at the end +* of the iobitmap. +* +* -1? seg base+limit should be pointing to the address of the +* last valid byte +*/ +#define __KERNEL_TSS_LIMIT \ + (IO_BITMAP_OFFSET + IO_BITMAP_BYTES + sizeof(unsigned long) - 1) +#ifdef CONFIG_X86_32 DECLARE_PER_CPU(unsigned long, cpu_current_top_of_stack);#endif
Signature · 1 line
2.9.3
PATCH 2/6 x86/kvm/vmx: Don't fetch the TSS base from the GDT
In reply to Andy LutomirskiThe current CPU's TSS base is a foregone conclusion, so there's no need to parse it out of the segment tables. This should save a couple cycles (as STR is surely microcoded and poorly optimized) but, more importantly, it's a cleanup and it means that segment_base() will never be called on 64-bit kernels.
--- arch/x86/kvm/vmx.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-)
arch/x86/kvm/vmx.c +4 -10
@@ -2088,13 +2088,6 @@ static unsigned long segment_base(u16 selector)return v; } -static inline unsigned long kvm_read_tr_base (void ) -{ -u16 tr; - asm("str %0" :"=g" (tr)); -return segment_base (tr); -} -static void vmx_save_host_state (struct kvm_vcpu *vcpu) {struct vcpu_vmx *vmx =to_vmx (vcpu);
@@ -2294,10 +2287,11 @@ static void vmx_vcpu_load(struct kvm_vcpu *vcpu, int cpu)/* * Linux uses per-cpu TSS and GDT, so set these when switching -* processors. +* processors. See 22.2.4. */ -vmcs_writel (HOST_TR_BASE ,kvm_read_tr_base ());/* 22.2.4 */ -vmcs_writel (HOST_GDTR_BASE , gdt->address);/* 22.2.4 */ +vmcs_writel (HOST_TR_BASE , + (unsigned long )this_cpu_ptr (&cpu_tss)); +vmcs_writel (HOST_GDTR_BASE , gdt->address);rdmsrl (MSR_IA32_SYSENTER_ESP , sysenter_esp);vmcs_writel (HOST_IA32_SYSENTER_ESP , sysenter_esp);/* 22.2.3 */
Signature · 1 line
2.9.3
PATCH 3/6 x86/kvm/vmx: Get rid of segment_base() on 64-bit kernels
In reply to Andy LutomirskiIt was a bit buggy (it didn't list all segment types that needed 64-bit fixups), but the bug was irrelevant because it wasn't called in any interesting context on 64-bit kernels and was only used for data segents on 32-bit kernels.
To avoid confusion, make it explicitly 32-bit only.
--- arch/x86/kvm/vmx.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
arch/x86/kvm/vmx.c +7 -4
@@ -2059,6 +2059,12 @@ static bool update_transition_efer(struct vcpu_vmx *vmx, int efer_offset) } } +#ifdef CONFIG_X86_32 +/* +* On 32-bit kernels, VM exits still load the FS and GS bases from the +* VMCS rather than the segment table. KVM uses this helper to figure +* out the current bases to poke them into the VMCS before entry. +*/ static unsigned long segment_base (u16 selector) {struct desc_ptr *gdt =this_cpu_ptr (&host_gdt);
@@ -2081,12 +2087,9 @@ static unsigned long segment_base(u16 selector) } d = (struct desc_struct *)(table_base + (selector & ~7 )); v =get_desc_base (d); -#ifdef CONFIG_X86_64 -if (d->s ==0 && (d->type ==2 || d->type ==9 || d->type ==11 )) - v |= ((unsigned long )((struct ldttss_desc64 *)d)->base3) <<32 ; -#endif return v; } +#endif static void vmx_save_host_state (struct kvm_vcpu *vcpu) {
Signature · 1 line
2.9.3
PATCH 4/6 x86/kvm/vmx: Simplify segment_base()
In reply to Andy LutomirskiUse actual pointer types for pointers (instead of unsigned long) and replace hardcoded constants with the appropriate self-documenting macros.
The function is still a bit messy, but this seems a lot better than before to me.
This is mostly borrowed from a patch by Thomas Garnier.
--- arch/x86/kvm/vmx.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)
arch/x86/kvm/vmx.c +7 -8
@@ -2069,24 +2069,23 @@ static unsigned long segment_base(u16 selector) {struct desc_ptr *gdt =this_cpu_ptr (&host_gdt);struct desc_struct *d; -unsigned long table_base; +struct desc_struct *table;unsigned long v; -if (!(selector & ~3 )) +if (!(selector & ~SEGMENT_RPL_MASK ))return 0 ; - table_base = gdt->address; + table = (struct desc_struct *)gdt->address; -if (selector &4 ) {/* from ldt */ +if ((selector &SEGMENT_TI_MASK ) ==SEGMENT_LDT ) {u16 ldt_selector =kvm_read_ldt (); -if (!(ldt_selector & ~3 )) +if (!(ldt_selector & ~SEGMENT_RPL_MASK ))return 0 ; - table_base =segment_base (ldt_selector); + table = (struct desc_struct *)segment_base (ldt_selector); } - d = (struct desc_struct *)(table_base + (selector & ~7 )); - v =get_desc_base (d); + v =get_desc_base (&table[selector >>3 ]);return v; }#endif
Signature · 1 line
2.9.3
Re: [PATCH 4/6] x86/kvm/vmx: Simplify segment_base()
Quoted 9 lines · Andy Lutomirski
Use actual pointer types for pointers (instead of unsigned long) and replace hardcoded constants with the appropriate self-documenting macros.
The function is still a bit messy, but this seems a lot better than before to me.
This is mostly borrowed from a patch by Thomas Garnier.
Looks good, I like that the 64-bit usage could be removed. Thanks!
Quoted 48 lines
Cc Thomas Garnier <[email protected]>Cc Jim Mattson <[email protected]>Cc Radim Krčmář <[email protected]>Cc Paolo Bonzini <[email protected]>Signed-off-by Andy Lutomirski <[email protected]>--- arch/x86/kvm/vmx.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)arch/x86/kvm/vmx.c +7
-9@@ -2069,24 +2069,23 @@ static unsigned long segment_base(u16 selector) {struct desc_ptr *gdt =this_cpu_ptr (&host_gdt);struct desc_struct *d; -unsigned long table_base; +struct desc_struct *table;unsigned long v; -if (!(selector & ~3 )) +if (!(selector & ~SEGMENT_RPL_MASK ))return 0 ; - table_base = gdt->address; + table = (struct desc_struct *)gdt->address; -if (selector &4 ) {/* from ldt */ +if ((selector &SEGMENT_TI_MASK ) ==SEGMENT_LDT ) {u16 ldt_selector =kvm_read_ldt (); -if (!(ldt_selector & ~3 )) +if (!(ldt_selector & ~SEGMENT_RPL_MASK ))return 0 ; - table_base =segment_base (ldt_selector); + table = (struct desc_struct *)segment_base (ldt_selector); } - d = (struct desc_struct *)(table_base + (selector & ~7 )); - v =get_desc_base (d); + v =get_desc_base (&table[selector >>3 ]);return v; }#endif --2.9.3
Signature · 1 line
Thomas
PATCH 5/6 x86/asm/64: Drop __cacheline_aligned from struct x86_hw_tss
In reply to Andy LutomirskiHistorically, the entire TSS + io bitmap structure was cacheline aligned, but commit ca241c75037b ("x86: unify tss_struct") changed it (presumably inadvertently) so that the fixed-layout hardware part is cacheline-aligned and the io bitmap is after the padding. This wastes 24 bytes (the hardware part should be 104 bytes, but this pads it to 128 bytes) and, serves no purpose, and causes sizeof(struct x86_hw_tss) to have a confusing value.
Drop the pointless alignment.
--- arch/x86/include/asm/processor.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
arch/x86/include/asm/processor.h +1 -1
@@ -304,7 +304,7 @@ struct x86_hw_tss {u16 reserved5;u16 io_bitmap_base; -} __attribute__((packed)) ____cacheline_aligned; +} __attribute__((packed));#endif /*
Signature · 1 line
2.9.3
PATCH 6/6 x86/kvm/vmx: Defer TR reload after VM exit
In reply to Andy LutomirskiIntel's VMX is daft and resets the hidden TSS limit register to 0x67 on VMX reload, and the 0x67 is not configurable. KVM currently reloads TR using the LTR instruction on every exit, but this is quite slow because LTR is serializing.
The 0x67 limit is entirely harmless unless ioperm() is in use, so defer the reload until a task using ioperm() is actually running.
Here's some poorly done benchmarking using kvm-unit-tests:
Before:
cpuid 1313
vmcall 1195mov_from_cr8 11mov_to_cr8 17inl_from_pmtimer 6770inl_from_qemu 6856inl_from_kernel 2435outl_to_kernel 1402
After:
cpuid 1291
vmcall 1181mov_from_cr8 11mov_to_cr8 16inl_from_pmtimer 6457inl_from_qemu 6209inl_from_kernel 2339outl_to_kernel 1391
--- arch/x86/include/asm/desc.h | 48 +++++++++++++++++++++++++++++++++++++++++++++ arch/x86/kernel/ioport.c | 5 +++++ arch/x86/kernel/process.c | 10 ++++++++++ arch/x86/kvm/vmx.c | 23 +++++++++------------- 4 files changed, 72 insertions(+), 14 deletions(-)
arch/x86/include/asm/desc.h +48 -0
@@ -205,6 +205,54 @@ static inline void native_load_tr_desc(void) asmvolatile ("ltr %w0" ::"q" (GDT_ENTRY_TSS *8 )); } +static inline void force_reload_TR (void ) +{ +struct desc_struct *d =get_cpu_gdt_table (smp_processor_id ()); +tss_desc tss; + +memcpy (&tss, &d[GDT_ENTRY_TSS ],sizeof (tss_desc)); + +/* +* LTR requires an available TSS, and the TSS is currently +* busy. Make it be available so that LTR will work. +*/ + tss.type =DESC_TSS ; +write_gdt_entry (d,GDT_ENTRY_TSS , &tss,DESC_TSS ); + +load_TR_desc (); +} + +DECLARE_PER_CPU (bool, need_tr_refresh); + +static inline void refresh_TR (void ) +{ +DEBUG_LOCKS_WARN_ON (preemptible ()); + +if (unlikely (this_cpu_read (need_tr_refresh))) { +force_reload_TR (); +this_cpu_write (need_tr_refresh, false); + } +} + +/* +* If you do something evil that corrupts the cached TSS limit (I'm looking +* at you, VMX exits), call this function. +* +* The optimization here is that the TSS limit only matters for Linux if the +* IO bitmap is in use. If the TSS limit gets forced to its minimum value, +* everything works except that IO bitmap will be ignored and all CPL 3 IO +* instructions will #GP, which is exactly what we want for normal tasks. +*/ +static inline void invalidate_tss_limit (void ) +{ +DEBUG_LOCKS_WARN_ON (preemptible ()); + +if (unlikely (test_thread_flag (TIF_IO_BITMAP ))) +refresh_TR (); +else +this_cpu_write (need_tr_refresh, true); +} +static inline void native_load_gdt (const struct desc_ptr *dtr) { asmvolatile ("lgdt %0" ::"m" (*dtr));
arch/x86/kernel/ioport.c +5 -0
@@ -16,6 +16,7 @@#include <linux/syscalls.h> #include <linux/bitmap.h> #include <asm/syscalls.h> +#include <asm/desc.h> /* * this changes the io permissions bitmap in the current task.
@@ -45,6 +46,10 @@ asmlinkage long sys_ioperm(unsigned long from, unsigned long num, int turn_on)memset (bitmap,0xff ,IO_BITMAP_BYTES ); t->io_bitmap_ptr = bitmap;set_thread_flag (TIF_IO_BITMAP ); + +preempt_disable (); +refresh_TR (); +preempt_enable (); }/*
arch/x86/kernel/process.c +10 -0
@@ -32,6 +32,7 @@#include <asm/mce.h> #include <asm/vm86.h> #include <asm/switch_to.h> +#include <asm/desc.h> /* * per-CPU TSS segments. Threads are completely 'soft' on Linux,
@@ -64,6 +65,9 @@ __visible DEFINE_PER_CPU_SHARED_ALIGNED(struct tss_struct, cpu_tss) = { };EXPORT_PER_CPU_SYMBOL (cpu_tss); +DEFINE_PER_CPU (bool, need_tr_refresh); +EXPORT_PER_CPU_SYMBOL_GPL (need_tr_refresh); +/* * this gets called so that we can store lazy state into memory and copy the * current task into the new thread.
@@ -209,6 +213,12 @@ void __switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p,*/ memcpy (tss->io_bitmap, next->io_bitmap_ptr,max (prev->io_bitmap_max, next->io_bitmap_max)); + +/* +* Make sure that the TSS limit is correct for the CPU +* to notice the IO bitmap. +*/ +refresh_TR (); }else if (test_tsk_thread_flag (prev_p,TIF_IO_BITMAP )) {/* * Clear any possible leftover bits:
arch/x86/kvm/vmx.c +9 -14
@@ -1992,19 +1992,6 @@ static void add_atomic_switch_msr(struct vcpu_vmx *vmx, unsigned msr, m->host[i].value = host_val; } -static void reload_tss (void ) -{ -/* -* VT restores TR but not its size. Useless. -*/ -struct desc_ptr *gdt =this_cpu_ptr (&host_gdt); -struct desc_struct *descs; - - descs = (void *)gdt->address; - descs[GDT_ENTRY_TSS ].type =9 ;/* available TSS */ -load_TR_desc (); -} -static bool update_transition_efer (struct vcpu_vmx *vmx,int efer_offset) {u64 guest_efer = vmx->vcpu.arch.efer;
@@ -2174,7 +2161,7 @@ static void __vmx_load_host_state(struct vcpu_vmx *vmx) loadsegment(es, vmx->host_state.es_sel); }#endif -reload_tss (); +invalidate_tss_limit ();#ifdef CONFIG_X86_64 wrmsrl (MSR_KERNEL_GS_BASE , vmx->msr_host_kernel_gs_base);#endif
@@ -2295,6 +2282,14 @@ static void vmx_vcpu_load(struct kvm_vcpu *vcpu, int cpu) (unsigned long)this_cpu_ptr(&cpu_tss));vmcs_writel (HOST_GDTR_BASE , gdt->address); +/* +* VM exits change the host TR limit to 0x67 after a VM +* exit. This is okay, since 0x67 covers everything except +* the IO bitmap and have have code to handle the IO bitmap +* being lost after a VM exit. +*/ +BUILD_BUG_ON (IO_BITMAP_OFFSET -1 !=0x67 ); +rdmsrl (MSR_IA32_SYSENTER_ESP , sysenter_esp);vmcs_writel (HOST_IA32_SYSENTER_ESP , sysenter_esp);/* 22.2.3 */
Signature · 1 line
2.9.3
Paolo Bonzini
maintainerRe: [PATCH 6/6] x86/kvm/vmx: Defer TR reload after VM exit
Quoted 14 lines · Andy Lutomirski
arch/x86/include/asm/desc.h +14
-0+{ +DEBUG_LOCKS_WARN_ON (preemptible ()); + +if (unlikely (this_cpu_read (need_tr_refresh))) { +force_reload_TR (); +this_cpu_write (need_tr_refresh, false); + } +} + +/* +* If you do something evil that corrupts the cached TSS limit (I'm looking +* at you, VMX exits), call this function. +* +* The optimization here is that the TSS limit only matters for Linux if the
arch/x86/include/asm/desc.h static inline void native_load_tr_desc(void) · line 251
Quoted 10 lines
arch/x86/include/asm/desc.h +10
-0+* IO bitmap is in use. If the TSS limit gets forced to its minimum value, +* everything works except that IO bitmap will be ignored and all CPL 3 IO +* instructions will #GP, which is exactly what we want for normal tasks. +*/ +static inline void invalidate_tss_limit (void ) +{ +DEBUG_LOCKS_WARN_ON (preemptible ()); + +if (unlikely (test_thread_flag (TIF_IO_BITMAP ))) +refresh_TR ();
This must be force_reload_TR, not refresh_TR.
Paolo
Quoted 2 lines
arch/x86/include/asm/desc.h +2
-0+else +this_cpu_write (need_tr_refresh, true);
Paolo Bonzini
maintainerQuoted 7 lines · Andy Lutomirski
The first four patches here are intended to be straightforward cleanups and to make a better base for Thomas' GDT series. They may be a slight speedup, too, because they remove an STR instruction from the VMX entry path.
The last two patches are a reasonably large speedup but need careful review.
I spotted (I think) a bug in patch 6, but apart from that it looks nice.
Paolo
Quoted 29 lines
FWIW, I can see lots of additional easy-ish speedups here. For example:
- The GDT reload on VM exit isn't really needed at all. Instead let's just change the kernel limit to
0xFFFF. Doing that naively would waste memory, but doing it carefully on top of Thomas' series would be straightforward and almost free.- RDMSR from
MSR_GS_BASEis totally pointless.- Once I or someone finishes the FSGSBASE series, we get a big speedup there.
- The LDT reload code should be split up and optimized better, I think.
Andy Lutomirski (6):
x86/asm: Define the kernel TSS limit in a macro x86/kvm/vmx: Don't fetch the TSS base from the GDT x86/kvm/vmx: Get rid ofsegment_base()on 64-bit kernels x86/kvm/vmx: Simplifysegment_base()
x86/asm/64: Drop__cacheline_alignedfrom structx86_hw_tssx86/kvm/vmx: Defer TR reload after VM exitarch/x86/include/asm/desc.h | 58 ++++++++++++++++++++++++++++++------ arch/x86/include/asm/processor.h | 12 +++++++- arch/x86/kernel/ioport.c | 5 ++++ arch/x86/kernel/process.c | 10 +++++++ arch/x86/kvm/vmx.c | 63 +++++++++++++++++----------------------- 5 files changed, 102 insertions(+), 46 deletions(-)