On 27/07/2017 20:29, David Hildenbrand wrote:
> On 27.07.2017 15:20, Paolo Bonzini wrote:
>> Expose the "Enable INVPCID" secondary execution control to the guest
>> and properly reflect the exit reason.
>>
>> In addition, before this patch the guest was always running with
>> INVPCID enabled, causing pcid.flat's "Test on INVPCID when disabled"
>> test to fail.
>
> Did you wanted to send "KVM: nVMX: do not fill vm_exit_intr_error_code
> in prepare_vmcs12" I can spot on kvm/queue? (but sent this patch twice
> instead?)
>
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
>> ---
>> arch/x86/kvm/vmx.c | 34 +++++++++++++++++++++++++---------
>> 1 file changed, 25 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index ed43fd824264..9c3c6c524430 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -8189,6 +8189,10 @@ static bool nested_vmx_exit_reflected(struct kvm_vcpu *vcpu, u32 exit_reason)
>> * table is L0's fault.
>> */
>> return false;
>> + case EXIT_REASON_INVPCID:
>> + return
>> + nested_cpu_has2(vmcs12, SECONDARY_EXEC_ENABLE_INVPCID) &&
>> + nested_cpu_has(vmcs12, CPU_BASED_INVLPG_EXITING);
>
> (why the extra line after the return ?)
I'm used to do
return
first_long_condition &&
second_long_condition;
but I admit it looks a bit weird with 8-space indent.
>> case EXIT_REASON_WBINVD:
>> return nested_cpu_has2(vmcs12, SECONDARY_EXEC_WBINVD_EXITING);
>> case EXIT_REASON_XSETBV:
>> @@ -9440,7 +9444,6 @@ static void nested_vmx_cr_fixed1_bits_update(struct kvm_vcpu *vcpu)
>>
>> static void vmx_cpuid_update(struct kvm_vcpu *vcpu)
>> {
>> - struct kvm_cpuid_entry2 *best;
>> struct vcpu_vmx *vmx = to_vmx(vcpu);
>> u32 secondary_exec_ctl = vmx_secondary_exec_control(vmx);
>>
>> @@ -9459,15 +9462,27 @@ static void vmx_cpuid_update(struct kvm_vcpu *vcpu)
>> }
>> }
>>
>> - /* Exposing INVPCID only when PCID is exposed */
>> - best = kvm_find_cpuid_entry(vcpu, 0x7, 0);
>> - if (vmx_invpcid_supported() &&
>> - (!best || !(best->ebx & bit(X86_FEATURE_INVPCID)) ||
>> - !guest_cpuid_has_pcid(vcpu))) {
>> - secondary_exec_ctl &= ~SECONDARY_EXEC_ENABLE_INVPCID;
>> + if (vmx_invpcid_supported()) {
>> + /* Exposing INVPCID only when PCID is exposed */
>> + struct kvm_cpuid_entry2 *best = kvm_find_cpuid_entry(vcpu, 0x7, 0);
>> + bool invpcid_enabled =
>> + best && best->ebx & bit(X86_FEATURE_INVPCID) &&
>
> I thought parentheses are recommended around &, but I am usually wrong
> about these things :)
In general the compiler complains about those things, so I didn't add
the parentheses here. In can see why it doesn't, because & ^ | are
consistently above && ||. The problems in general stem from the
precedence between & ^ | and && ||.
>> + guest_cpuid_has_pcid(vcpu);
>>
>> - if (best)
>> - best->ebx &= ~bit(X86_FEATURE_INVPCID);
>> + if (!invpcid_enabled) {
>> + secondary_exec_ctl &= ~SECONDARY_EXEC_ENABLE_INVPCID;
>> + if (best)
>> + best->ebx &= ~bit(X86_FEATURE_INVPCID);
>> + }
>
> Can't we rewrite that a little bit, avoiding that "best" handling
> (introducing guest_cpuid_disable_invpcid() and guest_cpuid_has_invpcid())
>
> bool invpcid_enabled = guest_cpuid_has_pcid(vcpu) &&
> guest_cpuid_has_invpcid();
>
> if (!invpcid_enabled) {
> secondary_exec_ctl &= ~SECONDARY_EXEC_ENABLE_INVPCID;
> /* make sure there is no no INVPCID without PCID */
> guest_cpuid_disable_invpcid(vcpu);
> }
I don't know... if you need a comment, it means the different structure
of the code doesn't spare many doubts from the reader. And the code
doesn't become much simpler since you have to handle "nested" anyway.
What I tried to do was to mimic as much as possible the rdtscp case, but
it cannot be exactly the same due to the interaction between PCID and
INVPCID.
Paolo
