Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1700845 > unrolled thread

Re: [PATCH] KVM: nVMX: INVPCID support

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] KVM: nVMX: INVPCID support Paolo Bonzini <pbonzini@redhat.com> - 2017-08-01 12:50 +0200
    Re: [PATCH] KVM: nVMX: INVPCID support David Hildenbrand <david@redhat.com> - 2017-08-01 13:20 +0200
      Re: [PATCH] KVM: nVMX: INVPCID support Paolo Bonzini <pbonzini@redhat.com> - 2017-08-01 13:40 +0200
        Re: [PATCH] KVM: nVMX: INVPCID support Radim Krčmář <rkrcmar@redhat.com> - 2017-08-01 19:40 +0200
          Re: [PATCH] KVM: nVMX: INVPCID support Paolo Bonzini <pbonzini@redhat.com> - 2017-08-02 09:40 +0200
            Re: [PATCH] KVM: nVMX: INVPCID support David Hildenbrand <david@redhat.com> - 2017-08-02 11:00 +0200

#1700845 — Re: [PATCH] KVM: nVMX: INVPCID support

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

[toc] | [next] | [standalone]

#1700889

>> 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.

It's more about the handling of best here, which can be avoided quite
easily as I showed (by encapsulating how cpuids are looked up/modified).

But you are the maintainer, so feel free to stick to what you have. :)

> 
> Paolo
> 


-- 

Thanks,

David

[toc] | [prev] | [next] | [standalone]

#1700898

On 01/08/2017 13:18, David Hildenbrand wrote:
> 
>>> 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.
> 
> It's more about the handling of best here, which can be avoided quite
> easily as I showed (by encapsulating how cpuids are looked up/modified).

Yeah, I don't like either option. :)  Luckily there is a second maintainer!

Paolo

[toc] | [prev] | [next] | [standalone]

#1701267

2017-08-01 13:35+0200, Paolo Bonzini:
> On 01/08/2017 13:18, David Hildenbrand wrote:
> > 
> >>> 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.
> > 
> > It's more about the handling of best here, which can be avoided quite
> > easily as I showed (by encapsulating how cpuids are looked up/modified).
> 
> Yeah, I don't like either option. :)  Luckily there is a second maintainer!

With three people, we'll just have three options! :)
I'd go with Paolo's version, because it is efficient and also follows
patterns the bit clearing in kvm_update_cpuid().

---
I would like the wrappers if they were more generic, e.g.:

   guest_cpuid_has(vcpu, X86_FEATURE_INVPCID);
   guest_cpuid_clear(vcpu, X86_FEATURE_INVPCID);

To do that, we need a mapping from X86_FEATURE to (leaf, subleaf,
register) triple, which can be done with cpuid_leafs.

I haven't tried to compile the idea below, but if the optimizer is good,
then we should have only slightly worse byte code as we do now thanks to
x86_feature being a compile-time constant.

Do you it's less ugly than the other two options?


static inline bool x86_feature_cpuid(int x86_feature, int *id, int *count, int *register)
{
	static struct {int x86_leaf; int id; int count; int register;} cpuid[] = {
		{CPUID_1_EDX, 1, 0, 3},
		{CPUID_8000_0001_EDX, 0x80000001, 0, 3},
		{CPUID_8086_0001_EDX, 0x80860001, 0, 3},
		{CPUID_1_ECX, 1, 0, 2},
		... // you get the idea, it's tedious :)
		};
	
	for (size_t i = 0; i < ARRAY_SIZE(cpuid); i++)
		if (cpuid[i].x86_leaf == x86_feature / 32) {
			*id = cpuid[i].id;
			*count = cpuid[i].count;
			*register = cpuid[i].register;
			return true;
		}
	
	return false;
}

static inline bool guest_cpuid_has(struct kvm_vcpu *vcpu, int x86_feature)
{
	int id, count, register;
	u32 *reg;
	struct kvm_cpuid_entry2 *best;

	if (!x86_feature_cpuid(x86_feature, &id, &count, &register))
		return false;

	if (!(best = kvm_find_cpuid_entry(id, count)))
		return false;

	reg = &best->eax + register; // rewrite to be safer
	return *reg & bit(x86_feature);
}

Similar for guest_cpuid_clear and we can also factor out the common part
for getting the register (all up to the last line, with NULL as failure)

[toc] | [prev] | [next] | [standalone]

#1701801

On 01/08/2017 19:37, Radim Krčmář wrote:
> 
> Do you it's less ugly than the other two options?

It's awesome, but it's a non-trivial project of its own. :)

Paolo

[toc] | [prev] | [next] | [standalone]

#1701916

On 02.08.2017 09:38, Paolo Bonzini wrote:
> On 01/08/2017 19:37, Radim Krčmář wrote:
>>
>> Do you it's less ugly than the other two options?
> 
> It's awesome, but it's a non-trivial project of its own. :)
> 
> Paolo
> 

That would be a perfect cleanup and I'll be happy to review it ;)

... until then, Paolo's patch in the current form is fine from my side.

-- 

Thanks,

David

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web