Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752215AbdHXKGI (ORCPT ); Thu, 24 Aug 2017 06:06:08 -0400 Received: from mail-oi0-f66.google.com ([209.85.218.66]:33390 "EHLO mail-oi0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751262AbdHXKGG (ORCPT ); Thu, 24 Aug 2017 06:06:06 -0400 Subject: Re: [PATCH 1/2] KVM: x86: simplify handling of PKRU To: Paolo Bonzini , linux-kernel@vger.kernel.org, kvm@vger.kernel.org Cc: junkang.fjk@alibaba-inc.com References: <1503523566-25624-1-git-send-email-pbonzini@redhat.com> <1503523566-25624-2-git-send-email-pbonzini@redhat.com> <8b7cd59e-05b9-e8c4-b686-8a3fda88c191@gmail.com> <40adf946-79ad-87cd-8bfd-6db4dfdbefc3@redhat.com> From: Yang Zhang Message-ID: <59e6c0e2-6422-7803-5a0f-b3c2b00edb26@gmail.com> Date: Thu, 24 Aug 2017 18:05:59 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <40adf946-79ad-87cd-8bfd-6db4dfdbefc3@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2801 Lines: 76 On 2017/8/24 17:19, Paolo Bonzini wrote: > On 24/08/2017 11:09, Yang Zhang wrote: >>> + if (static_cpu_has(X86_FEATURE_OSPKE) && >> >> We expose protection key to VM without check whether OSPKE is enabled or >> not. Why not check guest's cpuid here which also can avoid unnecessary >> access to pkru? > > Checking guest CPUID is pretty slow. We could check CR4.PKE though. > > Also, using static_cpu_has with OSPKE is probably wrong. But if we do > check CR4.PKE, we can check X86_FEATURE_PKU instead, so something like > > if (static_cpu_has(X86_FEATURE_PKU) && > kvm_read_cr4_bits(vcpu, X86_CR4_PKE) && > vcpu->arch.pkru != vmx->host_pkru) > > ... but then, kvm_read_cr4_bits is also pretty slow---and we don't > really need it, since all CR4 writes cause a vmexit. So for now I'd > stay with this patch, only s/static_cpu_has/boot_cpu_has/g. > > Of course you can send improvements on top! ok, since most OS distributions don't support protection key so far which means vcpu->arch.pkru always 0 in it and i remember host_pkru will be set to 55555554 be default. To avoid the unnecessary access to pkru, how about the following change: @@ -4338,6 +4331,9 @@ static int vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4) return 1; } + if (cr4 & X86_CR4_PKE) + to_vmx(vcpu)->guest_pkru_valid = true; + if (to_vmx(vcpu)->nested.vmxon && !nested_cr4_valid(vcpu, cr4)) return 1; @@ -9020,8 +9016,10 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu) if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) vmx_set_interrupt_shadow(vcpu, 0); - if (vmx->guest_pkru_valid) - __write_pkru(vmx->guest_pkru); + if (static_cpu_has(X86_FEATURE_OSPKE) && + vmx->guest_pkru_valid && + vcpu->arch.pkru != vmx->host_pkru) + __write_pkru(vcpu->arch.pkru); atomic_switch_perf_msrs(vmx); debugctlmsr = get_debugctlmsr(); @@ -9169,13 +9167,11 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu) * back on host, so it is safe to read guest PKRU from current * XSAVE. */ - if (boot_cpu_has(X86_FEATURE_OSPKE)) { - vmx->guest_pkru = __read_pkru(); - if (vmx->guest_pkru != vmx->host_pkru) { - vmx->guest_pkru_valid = true; + if (static_cpu_has(X86_FEATURE_OSPKE) && + vmx->guest_pkru_valid) { + vcpu->arch.pkru = __read_pkru(); + if (vcpu->arch.pkru != vmx->host_pkru) __write_pkru(vmx->host_pkru); - } else - vmx->guest_pkru_valid = false; } /* -- Yang Alibaba Cloud Computing