Re: [PATCH] KVM: nVMX: INVPCID support

[Paolo Bonzini <pbonzini@redhat.com>]

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@redhat.com>
>> ---
>>  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