From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753403AbdKOGxs (ORCPT ); Wed, 15 Nov 2017 01:53:48 -0500 Received: from mail-oi0-f68.google.com ([209.85.218.68]:55759 "EHLO mail-oi0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751072AbdKOGxn (ORCPT ); Wed, 15 Nov 2017 01:53:43 -0500 X-Google-Smtp-Source: AGs4zMbuYqufNR2ligoQzqCWPc9TbB0wsM0dvlhqvVnFk3H1hCekAMnuCK2yBwuAld6k6twcI29vOA== Subject: Re: [PATCH 1/2] x86,kvm: move qemu/guest FPU switching out to vcpu_run To: riel@redhat.com, pbonzini@redhat.com Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org, david@redhat.com, borntraeger@de.ibm.com, tglx@linutronix.de, rkrcmar@redhat.com, Quan Xu References: <20171114215424.32214-1-riel@redhat.com> <20171114215424.32214-2-riel@redhat.com> From: "quan.xu04@gmail.com" Message-ID: <2e83dd3f-8f03-03ac-45e0-f2fae1d17013@gmail.com> Date: Wed, 15 Nov 2017 14:53:34 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <20171114215424.32214-2-riel@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2017/11/15 05:54, riel@redhat.com wrote: > From: Rik van Riel > > Currently, every time a VCPU is scheduled out, the host kernel will > first save the guest FPU/xstate context, then load the qemu userspace > FPU context, only to then immediately save the qemu userspace FPU > context back to memory. When scheduling in a VCPU, the same extraneous > FPU loads and saves are done. Rik, be careful with VM migration. with you patch, I don't think you could load fpu/xstate   context accurately after VM migration. Quan Alibaba Cloud > This could be avoided by moving from a model where the guest FPU is > loaded and stored with preemption disabled, to a model where the > qemu userspace FPU is swapped out for the guest FPU context for > the duration of the KVM_RUN ioctl. > > This is done under the VCPU mutex, which is also taken when other > tasks inspect the VCPU FPU context, so the code should already be > safe for this change. That should come as no surprise, given that > s390 already has this optimization. > > No performance changes were detected in quick ping-pong tests on > my 4 socket system, which is expected since an FPU+xstate load is > on the order of 0.1us, while ping-ponging between CPUs is on the > order of 20us, and somewhat noisy. > > There may be other tests where performance changes are noticeable. > > Signed-off-by: Rik van Riel > Suggested-by: Christian Borntraeger > --- > arch/x86/include/asm/kvm_host.h | 13 +++++++++++++ > arch/x86/kvm/x86.c | 34 +++++++++++++--------------------- > include/linux/kvm_host.h | 2 +- > 3 files changed, 27 insertions(+), 22 deletions(-) > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index 9d7d856b2d89..ffe54958491f 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -536,7 +536,20 @@ struct kvm_vcpu_arch { > struct kvm_mmu_memory_cache mmu_page_cache; > struct kvm_mmu_memory_cache mmu_page_header_cache; > > + /* > + * QEMU userspace and the guest each have their own FPU state. > + * In vcpu_run, we switch between the user and guest FPU contexts. > + * While running a VCPU, the VCPU thread will have the guest FPU > + * context. > + * > + * Note that while the PKRU state lives inside the fpu registers, > + * it is switched out separately at VMENTER and VMEXIT time. The > + * "guest_fpu" state here contains the guest FPU context, with the > + * host PRKU bits. > + */ > + struct fpu user_fpu; > struct fpu guest_fpu; > + > u64 xcr0; > u64 guest_supported_xcr0; > u32 guest_xstate_size; > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 03869eb7fcd6..aad5181ed4e9 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -2917,7 +2917,6 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu) > srcu_read_unlock(&vcpu->kvm->srcu, idx); > pagefault_enable(); > kvm_x86_ops->vcpu_put(vcpu); > - kvm_put_guest_fpu(vcpu); > vcpu->arch.last_host_tsc = rdtsc(); > } > > @@ -5228,13 +5227,10 @@ static void emulator_halt(struct x86_emulate_ctxt *ctxt) > > static void emulator_get_fpu(struct x86_emulate_ctxt *ctxt) > { > - preempt_disable(); > - kvm_load_guest_fpu(emul_to_vcpu(ctxt)); > } > > static void emulator_put_fpu(struct x86_emulate_ctxt *ctxt) > { > - preempt_enable(); > } > > static int emulator_intercept(struct x86_emulate_ctxt *ctxt, > @@ -6908,7 +6904,6 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) > preempt_disable(); > > kvm_x86_ops->prepare_guest_switch(vcpu); > - kvm_load_guest_fpu(vcpu); > > /* > * Disable IRQs before setting IN_GUEST_MODE. Posted interrupt > @@ -7255,12 +7250,14 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) > } > } > > + kvm_load_guest_fpu(vcpu); > + > if (unlikely(vcpu->arch.complete_userspace_io)) { > int (*cui)(struct kvm_vcpu *) = vcpu->arch.complete_userspace_io; > vcpu->arch.complete_userspace_io = NULL; > r = cui(vcpu); > if (r <= 0) > - goto out; > + goto out_fpu; > } else > WARN_ON(vcpu->arch.pio.count || vcpu->mmio_needed); > > @@ -7269,6 +7266,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) > else > r = vcpu_run(vcpu); > > +out_fpu: > + kvm_put_guest_fpu(vcpu); > out: > post_kvm_run_save(vcpu); > if (vcpu->sigset_active) > @@ -7663,32 +7662,25 @@ static void fx_init(struct kvm_vcpu *vcpu) > vcpu->arch.cr0 |= X86_CR0_ET; > } > > +/* Swap (qemu) user FPU context for the guest FPU context. */ > void kvm_load_guest_fpu(struct kvm_vcpu *vcpu) > { > - if (vcpu->guest_fpu_loaded) > - return; > - > - /* > - * Restore all possible states in the guest, > - * and assume host would use all available bits. > - * Guest xcr0 would be loaded later. > - */ > - vcpu->guest_fpu_loaded = 1; > - __kernel_fpu_begin(); > + preempt_disable(); > + copy_fpregs_to_fpstate(&vcpu->arch.user_fpu); > /* PKRU is separately restored in kvm_x86_ops->run. */ > __copy_kernel_to_fpregs(&vcpu->arch.guest_fpu.state, > ~XFEATURE_MASK_PKRU); > + preempt_enable(); > trace_kvm_fpu(1); > } > > +/* When vcpu_run ends, restore user space FPU context. */ > void kvm_put_guest_fpu(struct kvm_vcpu *vcpu) > { > - if (!vcpu->guest_fpu_loaded) > - return; > - > - vcpu->guest_fpu_loaded = 0; > + preempt_disable(); > copy_fpregs_to_fpstate(&vcpu->arch.guest_fpu); > - __kernel_fpu_end(); > + copy_kernel_to_fpregs(&vcpu->arch.user_fpu.state); > + preempt_enable(); > ++vcpu->stat.fpu_reload; > trace_kvm_fpu(0); > } > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index 6882538eda32..354608487b8d 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -232,7 +232,7 @@ struct kvm_vcpu { > struct mutex mutex; > struct kvm_run *run; > > - int guest_fpu_loaded, guest_xcr0_loaded; > + int guest_xcr0_loaded; > struct swait_queue_head wq; > struct pid __rcu *pid; > int sigset_active;