DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 1ABA97E424
Subject: Re: [PATCH 1/2] KVM: x86: simplify handling of PKRU
To: Yang Zhang <yang.zhang.wz@gmail.com>, 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>
 <59e6c0e2-6422-7803-5a0f-b3c2b00edb26@gmail.com>
From: Paolo Bonzini <pbonzini@redhat.com>
Message-ID: <fcd2e75e-c291-f524-92ca-c0589df420a8@redhat.com>
Date: Thu, 24 Aug 2017 12:14:57 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.2.1
MIME-Version: 1.0
In-Reply-To: <59e6c0e2-6422-7803-5a0f-b3c2b00edb26@gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3345
Lines: 86

On 24/08/2017 12:05, Yang Zhang wrote:
> 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:

Even better: reading guest's CR4.PKE is actually _not_ slow because
X86_CR4_PKE is not part of KVM_POSSIBLE_CR4_GUEST_BITS.  So
kvm_read_cr4_bits(vcpu, X86_CR4_PKE) is compiled to just "vcpu->arch.cr4
& X86_CR4_PKE".

We need to be careful though to disable guest PKU if host OSPKE is off,
because otherwise __read_pkru and __write_pkru cause a #GP.

I've sent v2 of the series now, incorporating your suggestion.  Thanks!

Paolo

> @@ -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;
>         }
> 
>         /*
>