Re: [Qemu-devel] [PATCH 3/3] Target-ppc: Remove unnecessary variable
On 09/25/2015 02:37 AM, Shraddha Barke wrote: > Compress lines and remove the variable. > > +++ b/target-ppc/kvm.c > @@ -1782,8 +1782,7 @@ uint32_t kvmppc_get_tbfreq(void) > > ns++; > > -retval = atoi(ns); > -return retval; > +return atoi(ns); atoi() is lousy; it cannot properly detect user input errors. This should probably be converted to use the appropriate qemu_strtol variant instead. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
[PATCH v2 0/2] KVM/arm: enhance arvm7 vfp/simd lazy switch support
Current lazy vfp/simd implementation switches hardware context only on guest access and again on exit to host, otherwise hardware context is skipped. This patch set builds on that functionality and executes a hardware context switch only when vCPU is scheduled out or returns to user space. Patches were tested on FVP sw platform. FP crunching applications summing up values, with outcome compared to known result were executed on several guests, and host. Changes since v1->v2: * Fixed vfp/simd trap configuration to enable trace trapping * Removed set_hcptr branch label * Fixed handling of FPEXC to restore guest and host versions on vcpu_put Mario Smarduch (2): add hooks for armv7 fp/simd lazy switch support enable armv7 fp/simd lazy switch arch/arm/include/asm/kvm_asm.h | 1 + arch/arm/include/asm/kvm_host.h | 6 + arch/arm/kernel/asm-offsets.c | 2 ++ arch/arm/kvm/arm.c | 17 arch/arm/kvm/interrupts.S | 60 ++--- arch/arm/kvm/interrupts_head.S | 12 ++--- 6 files changed, 79 insertions(+), 19 deletions(-) -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/2] KVM/arm: add hooks for armv7 fp/simd lazy switch support
This patch adds vcpu fields to track lazy state, save host FPEXC, and offsets to fields. Signed-off-by: Mario Smarduch--- arch/arm/include/asm/kvm_host.h | 6 ++ arch/arm/kernel/asm-offsets.c | 2 ++ 2 files changed, 8 insertions(+) diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h index dcba0fa..194a8ef 100644 --- a/arch/arm/include/asm/kvm_host.h +++ b/arch/arm/include/asm/kvm_host.h @@ -111,6 +111,12 @@ struct kvm_vcpu_arch { /* Interrupt related fields */ u32 irq_lines; /* IRQ and FIQ levels */ + /* Track fp/simd lazy switch state */ + u32 vfp_lazy; + + /* Save host FPEXC register to restore on vcpu put */ + u32 saved_fpexc; + /* Exception Information */ struct kvm_vcpu_fault_info fault; diff --git a/arch/arm/kernel/asm-offsets.c b/arch/arm/kernel/asm-offsets.c index 871b826..e1c3a41 100644 --- a/arch/arm/kernel/asm-offsets.c +++ b/arch/arm/kernel/asm-offsets.c @@ -186,6 +186,8 @@ int main(void) DEFINE(VCPU_CPSR,offsetof(struct kvm_vcpu, arch.regs.usr_regs.ARM_cpsr)); DEFINE(VCPU_HCR, offsetof(struct kvm_vcpu, arch.hcr)); DEFINE(VCPU_IRQ_LINES, offsetof(struct kvm_vcpu, arch.irq_lines)); + DEFINE(VCPU_VFP_LAZY,offsetof(struct kvm_vcpu, arch.vfp_lazy)); + DEFINE(VCPU_VFP_FPEXC, offsetof(struct kvm_vcpu, arch.saved_fpexc)); DEFINE(VCPU_HSR, offsetof(struct kvm_vcpu, arch.fault.hsr)); DEFINE(VCPU_HxFAR, offsetof(struct kvm_vcpu, arch.fault.hxfar)); DEFINE(VCPU_HPFAR, offsetof(struct kvm_vcpu, arch.fault.hpfar)); -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/2] KVM/arm: enable enhanced armv7 fp/simd lazy switch
This patch enhances current lazy vfp/simd hardware switch. In addition to current lazy switch, it tracks vfp/simd hardware state with a vcpu lazy flag. vcpu lazy flag is set on guest access and trap to vfp/simd hardware switch handler. On vm-enter if lazy flag is set skip trap enable and saving host fpexc. On vm-exit if flag is set skip hardware context switch and return to host with guest context. On vcpu_put check if vcpu lazy flag is set, and execute a hardware context switch to restore host. Signed-off-by: Mario Smarduch--- arch/arm/include/asm/kvm_asm.h | 1 + arch/arm/kvm/arm.c | 17 arch/arm/kvm/interrupts.S | 60 +++--- arch/arm/kvm/interrupts_head.S | 12 ++--- 4 files changed, 71 insertions(+), 19 deletions(-) diff --git a/arch/arm/include/asm/kvm_asm.h b/arch/arm/include/asm/kvm_asm.h index 194c91b..4b45d79 100644 --- a/arch/arm/include/asm/kvm_asm.h +++ b/arch/arm/include/asm/kvm_asm.h @@ -97,6 +97,7 @@ extern char __kvm_hyp_code_end[]; extern void __kvm_flush_vm_context(void); extern void __kvm_tlb_flush_vmid_ipa(struct kvm *kvm, phys_addr_t ipa); extern void __kvm_tlb_flush_vmid(struct kvm *kvm); +extern void __kvm_restore_host_vfp_state(struct kvm_vcpu *vcpu); extern int __kvm_vcpu_run(struct kvm_vcpu *vcpu); #endif diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c index ce404a5..79f49c7 100644 --- a/arch/arm/kvm/arm.c +++ b/arch/arm/kvm/arm.c @@ -105,6 +105,20 @@ void kvm_arch_check_processor_compat(void *rtn) *(int *)rtn = 0; } +/** + * kvm_switch_fp_regs() - switch guest/host VFP/SIMD registers + * @vcpu: pointer to vcpu structure. + * + */ +static void kvm_switch_fp_regs(struct kvm_vcpu *vcpu) +{ +#ifdef CONFIG_ARM + if (vcpu->arch.vfp_lazy == 1) { + kvm_call_hyp(__kvm_restore_host_vfp_state, vcpu); + vcpu->arch.vfp_lazy = 0; + } +#endif +} /** * kvm_arch_init_vm - initializes a VM data structure @@ -295,6 +309,9 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu) void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu) { + /* Check if Guest accessed VFP registers */ + kvm_switch_fp_regs(vcpu); + /* * The arch-generic KVM code expects the cpu field of a vcpu to be -1 * if the vcpu is no longer assigned to a cpu. This is used for the diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S index 900ef6d..6d98232 100644 --- a/arch/arm/kvm/interrupts.S +++ b/arch/arm/kvm/interrupts.S @@ -96,6 +96,29 @@ ENTRY(__kvm_flush_vm_context) bx lr ENDPROC(__kvm_flush_vm_context) +/** + * void __kvm_restore_host_vfp_state(struct vcpu *vcpu) - Executes a lazy + * fp/simd switch, saves the guest, restores host. + * + */ +ENTRY(__kvm_restore_host_vfp_state) +#ifdef CONFIG_VFPv3 + push{r3-r7} + + add r7, r0, #VCPU_VFP_GUEST + store_vfp_state r7 + + add r7, r0, #VCPU_VFP_HOST + ldr r7, [r7] + restore_vfp_state r7 + + ldr r3, [vcpu, #VCPU_VFP_FPEXC] + VFPFMXR FPEXC, r3 + + pop {r3-r7} +#endif + bx lr +ENDPROC(__kvm_restore_host_vfp_state) / * Hypervisor world-switch code @@ -119,11 +142,15 @@ ENTRY(__kvm_vcpu_run) @ If the host kernel has not been configured with VFPv3 support, @ then it is safer if we deny guests from using it as well. #ifdef CONFIG_VFPv3 + @ r7 must be preserved until next vfp lazy check + vfp_inlazy_mode r7, skip_save_host_fpexc + @ Set FPEXC_EN so the guest doesn't trap floating point instructions VFPFMRX r2, FPEXC @ VMRS - push{r2} + str r2, [vcpu, #VCPU_VFP_FPEXC] orr r2, r2, #FPEXC_EN VFPFMXR FPEXC, r2 @ VMSR +skip_save_host_fpexc: #endif @ Configure Hyp-role @@ -131,7 +158,14 @@ ENTRY(__kvm_vcpu_run) @ Trap coprocessor CRx accesses set_hstr vmentry - set_hcptr vmentry, (HCPTR_TTA | HCPTR_TCP(10) | HCPTR_TCP(11)) + set_hcptr vmentry, (HCPTR_TTA) + + @ check if vfp_lazy flag set + cmp r7, #1 + beq skip_guest_vfp_trap + set_hcptr vmentry, (HCPTR_TCP(10) | HCPTR_TCP(11)) +skip_guest_vfp_trap: + set_hdcr vmentry @ Write configured ID register into MIDR alias @@ -170,22 +204,14 @@ __kvm_vcpu_return: @ Don't trap coprocessor accesses for host kernel set_hstr vmexit set_hdcr vmexit - set_hcptr vmexit, (HCPTR_TTA | HCPTR_TCP(10) | HCPTR_TCP(11)), after_vfp_restore + set_hcptr vmexit, (HCPTR_TTA | HCPTR_TCP(10) | HCPTR_TCP(11)) #ifdef CONFIG_VFPv3 - @ Switch VFP/NEON hardware state to the host's - add r7, vcpu, #VCPU_VFP_GUEST - store_vfp_state r7 - add r7, vcpu, #VCPU_VFP_HOST - ldr r7, [r7] -
Re: [PATCH] x86: Use entire page for the per-cpu GDT only if paravirt-enabled
On 09/26/2015 09:50 PM, H. Peter Anvin wrote: > NAK. We really should map the GDT read-only on all 64 bit systems, > since we can't hide the address from SLDT. Same with the IDT. Sorry, I don't understand your point. > On September 26, 2015 11:00:40 AM PDT, Denys Vlasenko> wrote: >> We have our GDT in a page-sized per-cpu structure, gdt_page. >> >> On x86_64 kernel, GDT is 128 bytes - only ~3% of that page is used. >> >> It is page-sized because of paravirt. Hypervisors need to know when >> GDT is changed, so they remap it read-only and handle write faults. >> If it's not in its own page, other writes nearby will cause >> those faults too. >> >> In other words, we need GDT to live in a separate page >> only if CONFIG_HYPERVISOR_GUEST=y. >> >> This patch reduces GDT alignment to cacheline-aligned >> if CONFIG_HYPERVISOR_GUEST is not set. >> >> Patch also renames gdt_page to cpu_gdt (mimicking naming of existing >> cpu_tss per-cpu variable), since now it is not always a full page. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] x86: Use entire page for the per-cpu GDT only if paravirt-enabled
We have our GDT in a page-sized per-cpu structure, gdt_page. On x86_64 kernel, GDT is 128 bytes - only ~3% of that page is used. It is page-sized because of paravirt. Hypervisors need to know when GDT is changed, so they remap it read-only and handle write faults. If it's not in its own page, other writes nearby will cause those faults too. In other words, we need GDT to live in a separate page only if CONFIG_HYPERVISOR_GUEST=y. This patch reduces GDT alignment to cacheline-aligned if CONFIG_HYPERVISOR_GUEST is not set. Patch also renames gdt_page to cpu_gdt (mimicking naming of existing cpu_tss per-cpu variable), since now it is not always a full page. $ objdump -x vmlinux | grep .data..percpu | sort Before: (offset)(size) wO .data..percpu 4000 irq_stack_union 4000 wO .data..percpu 5000 exception_stacks 9000 wO .data..percpu 1000 gdt_page <<< HERE a000 wO .data..percpu 0008 espfix_waddr a008 wO .data..percpu 0008 espfix_stack ... 00019398 g .data..percpu __per_cpu_end After: wO .data..percpu 4000 irq_stack_union 4000 wO .data..percpu 5000 exception_stacks 9000 wO .data..percpu 0008 espfix_waddr 9008 wO .data..percpu 0008 espfix_stack ... 00013c80 wO .data..percpu 0040 cyc2ns 00013cc0 wO .data..percpu 22c0 cpu_tss 00015f80 wO .data..percpu 0080 cpu_gdt <<< HERE 00016000 wO .data..percpu 0018 cpu_tlbstate ... 00018418 g .data..percpu __per_cpu_end Run-tested on a 144 CPU machine. Signed-off-by: Denys VlasenkoCC: Ingo Molnar CC: H. Peter Anvin CC: Konrad Rzeszutek Wilk CC: Boris Ostrovsky CC: David Vrabel CC: Joerg Roedel CC: Gleb Natapov CC: Paolo Bonzini CC: kvm@vger.kernel.org CC: x...@kernel.org CC: linux-ker...@vger.kernel.org --- arch/x86/entry/entry_32.S| 2 +- arch/x86/include/asm/desc.h | 16 +++- arch/x86/kernel/cpu/common.c | 10 -- arch/x86/kernel/cpu/perf_event.c | 2 +- arch/x86/kernel/head_32.S| 4 ++-- arch/x86/kernel/head_64.S| 2 +- arch/x86/kernel/vmlinux.lds.S| 2 +- arch/x86/tools/relocs.c | 2 +- arch/x86/xen/enlighten.c | 4 ++-- 9 files changed, 28 insertions(+), 16 deletions(-) diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S index b2909bf..bc6ae1c 100644 --- a/arch/x86/entry/entry_32.S +++ b/arch/x86/entry/entry_32.S @@ -429,7 +429,7 @@ ldt_ss: * compensating for the offset by changing to the ESPFIX segment with * a base address that matches for the difference. */ -#define GDT_ESPFIX_SS PER_CPU_VAR(gdt_page) + (GDT_ENTRY_ESPFIX_SS * 8) +#define GDT_ESPFIX_SS PER_CPU_VAR(cpu_gdt) + (GDT_ENTRY_ESPFIX_SS * 8) mov %esp, %edx /* load kernel esp */ mov PT_OLDESP(%esp), %eax /* load userspace esp */ mov %dx, %ax/* eax: new kernel esp */ diff --git a/arch/x86/include/asm/desc.h b/arch/x86/include/asm/desc.h index 4e10d73..76de300 100644 --- a/arch/x86/include/asm/desc.h +++ b/arch/x86/include/asm/desc.h @@ -39,15 +39,21 @@ extern gate_desc idt_table[]; extern struct desc_ptr debug_idt_descr; extern gate_desc debug_idt_table[]; -struct gdt_page { +struct cpu_gdt { struct desc_struct gdt[GDT_ENTRIES]; -} __attribute__((aligned(PAGE_SIZE))); - -DECLARE_PER_CPU_PAGE_ALIGNED(struct gdt_page, gdt_page); +} +#ifdef CONFIG_HYPERVISOR_GUEST +/* Xen et al want GDT to have its own page. They remap it read-only */ +__attribute__((aligned(PAGE_SIZE))); +DECLARE_PER_CPU_PAGE_ALIGNED(struct cpu_gdt, cpu_gdt); +#else +cacheline_aligned; +DECLARE_PER_CPU_ALIGNED(struct cpu_gdt, cpu_gdt); +#endif static inline struct desc_struct *get_cpu_gdt_table(unsigned int cpu) { - return per_cpu(gdt_page, cpu).gdt; + return per_cpu(cpu_gdt, cpu).gdt; } #ifdef CONFIG_X86_64 diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c index de22ea7..6b90785 100644 --- a/arch/x86/kernel/cpu/common.c +++ b/arch/x86/kernel/cpu/common.c @@ -92,7 +92,13 @@ static const struct cpu_dev default_cpu = { static const struct cpu_dev *this_cpu = _cpu; -DEFINE_PER_CPU_PAGE_ALIGNED(struct gdt_page, gdt_page) = { .gdt = { +#ifdef CONFIG_HYPERVISOR_GUEST +/* Xen et al want GDT to have its own page. They remap it read-only */
Re: [PATCH] x86: Use entire page for the per-cpu GDT only if paravirt-enabled
NAK. We really should map the GDT read-only on all 64 bit systems, since we can't hide the address from SLDT. Same with the IDT. On September 26, 2015 11:00:40 AM PDT, Denys Vlasenkowrote: >We have our GDT in a page-sized per-cpu structure, gdt_page. > >On x86_64 kernel, GDT is 128 bytes - only ~3% of that page is used. > >It is page-sized because of paravirt. Hypervisors need to know when >GDT is changed, so they remap it read-only and handle write faults. >If it's not in its own page, other writes nearby will cause >those faults too. > >In other words, we need GDT to live in a separate page >only if CONFIG_HYPERVISOR_GUEST=y. > >This patch reduces GDT alignment to cacheline-aligned >if CONFIG_HYPERVISOR_GUEST is not set. > >Patch also renames gdt_page to cpu_gdt (mimicking naming of existing >cpu_tss per-cpu variable), since now it is not always a full page. > >$ objdump -x vmlinux | grep .data..percpu | sort >Before: >(offset)(size) > wO .data..percpu 4000 >irq_stack_union >4000 wO .data..percpu 5000 >exception_stacks >9000 wO .data..percpu 1000 gdt_page <<< >HERE > a000 wO .data..percpu 0008 espfix_waddr > a008 wO .data..percpu 0008 espfix_stack >... > 00019398 g .data..percpu __per_cpu_end >After: > wO .data..percpu 4000 >irq_stack_union >4000 wO .data..percpu 5000 >exception_stacks > 9000 wO .data..percpu 0008 espfix_waddr > 9008 wO .data..percpu 0008 espfix_stack >... >00013c80 wO .data..percpu 0040 cyc2ns >00013cc0 wO .data..percpu 22c0 cpu_tss >00015f80 wO .data..percpu 0080 cpu_gdt <<< >HERE > 00016000 wO .data..percpu 0018 cpu_tlbstate >... > 00018418 g .data..percpu __per_cpu_end > >Run-tested on a 144 CPU machine. > >Signed-off-by: Denys Vlasenko >CC: Ingo Molnar >CC: H. Peter Anvin >CC: Konrad Rzeszutek Wilk >CC: Boris Ostrovsky >CC: David Vrabel >CC: Joerg Roedel >CC: Gleb Natapov >CC: Paolo Bonzini >CC: kvm@vger.kernel.org >CC: x...@kernel.org >CC: linux-ker...@vger.kernel.org >--- > arch/x86/entry/entry_32.S| 2 +- > arch/x86/include/asm/desc.h | 16 +++- > arch/x86/kernel/cpu/common.c | 10 -- > arch/x86/kernel/cpu/perf_event.c | 2 +- > arch/x86/kernel/head_32.S| 4 ++-- > arch/x86/kernel/head_64.S| 2 +- > arch/x86/kernel/vmlinux.lds.S| 2 +- > arch/x86/tools/relocs.c | 2 +- > arch/x86/xen/enlighten.c | 4 ++-- > 9 files changed, 28 insertions(+), 16 deletions(-) > >diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S >index b2909bf..bc6ae1c 100644 >--- a/arch/x86/entry/entry_32.S >+++ b/arch/x86/entry/entry_32.S >@@ -429,7 +429,7 @@ ldt_ss: > * compensating for the offset by changing to the ESPFIX segment with > * a base address that matches for the difference. > */ >-#define GDT_ESPFIX_SS PER_CPU_VAR(gdt_page) + (GDT_ENTRY_ESPFIX_SS * >8) >+#define GDT_ESPFIX_SS PER_CPU_VAR(cpu_gdt) + (GDT_ENTRY_ESPFIX_SS * 8) > mov %esp, %edx /* load kernel esp */ > mov PT_OLDESP(%esp), %eax /* load userspace esp */ > mov %dx, %ax/* eax: new kernel esp */ >diff --git a/arch/x86/include/asm/desc.h b/arch/x86/include/asm/desc.h >index 4e10d73..76de300 100644 >--- a/arch/x86/include/asm/desc.h >+++ b/arch/x86/include/asm/desc.h >@@ -39,15 +39,21 @@ extern gate_desc idt_table[]; > extern struct desc_ptr debug_idt_descr; > extern gate_desc debug_idt_table[]; > >-struct gdt_page { >+struct cpu_gdt { > struct desc_struct gdt[GDT_ENTRIES]; >-} __attribute__((aligned(PAGE_SIZE))); >- >-DECLARE_PER_CPU_PAGE_ALIGNED(struct gdt_page, gdt_page); >+} >+#ifdef CONFIG_HYPERVISOR_GUEST >+/* Xen et al want GDT to have its own page. They remap it read-only */ >+__attribute__((aligned(PAGE_SIZE))); >+DECLARE_PER_CPU_PAGE_ALIGNED(struct cpu_gdt, cpu_gdt); >+#else >+cacheline_aligned; >+DECLARE_PER_CPU_ALIGNED(struct cpu_gdt, cpu_gdt); >+#endif > > static inline struct desc_struct *get_cpu_gdt_table(unsigned int cpu) > { >- return per_cpu(gdt_page, cpu).gdt; >+ return per_cpu(cpu_gdt, cpu).gdt; > } > > #ifdef CONFIG_X86_64 >diff --git a/arch/x86/kernel/cpu/common.c >b/arch/x86/kernel/cpu/common.c >index de22ea7..6b90785 100644 >--- a/arch/x86/kernel/cpu/common.c >+++ b/arch/x86/kernel/cpu/common.c >@@