[PATCH 1/2] KVM: VMX: clean up declaration of VPID/EPT invalidation types

[PATCH 1/2] KVM: VMX: clean up declaration of VPID/EPT invalidation types

[Jan Dakinevich]

 - Remove VMX_EPT_EXTENT_INDIVIDUAL_ADDR, since there is no such type of
   EPT invalidation

 - Rename VMX_VPID_EXTENT_ALL_CONTEXT to VMX_VPID_EXTENT_GLOBAL_CONTEXT
   for consitency: all-context VPID invalidation is referenced  by
   "global" keyword in all other places of code

 - Add missing VPID types names

Signed-off-by: Jan Dakinevich <jan.dakinevich@gmail.com>
---
 arch/x86/include/asm/vmx.h | 7 +++++--
 arch/x86/kvm/vmx.c         | 4 ++--
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
index a002b07..785390f 100644
--- a/arch/x86/include/asm/vmx.h
+++ b/arch/x86/include/asm/vmx.h
@@ -399,10 +399,11 @@ enum vmcs_field {
 #define IDENTITY_PAGETABLE_PRIVATE_MEMSLOT	(KVM_USER_MEM_SLOTS + 2)
 
 #define VMX_NR_VPIDS				(1 << 16)
+#define VMX_VPID_EXTENT_INDIVIDUAL_ADDR		0
 #define VMX_VPID_EXTENT_SINGLE_CONTEXT		1
-#define VMX_VPID_EXTENT_ALL_CONTEXT		2
+#define VMX_VPID_EXTENT_GLOBAL_CONTEXT		2
+#define VMX_VPID_EXTENT_SINGLE_CONTEXT_RETAINING_GLOBALS 3
 
-#define VMX_EPT_EXTENT_INDIVIDUAL_ADDR		0
 #define VMX_EPT_EXTENT_CONTEXT			1
 #define VMX_EPT_EXTENT_GLOBAL			2
 #define VMX_EPT_EXTENT_SHIFT			24
@@ -419,8 +420,10 @@ enum vmcs_field {
 #define VMX_EPT_EXTENT_GLOBAL_BIT		(1ull << 26)
 
 #define VMX_VPID_INVVPID_BIT                    (1ull << 0) /* (32 - 32) */
+#define VMX_VPID_EXTENT_INDIVIDUAL_ADDR_BIT     (1ull << 8) /* (40 - 32) */
 #define VMX_VPID_EXTENT_SINGLE_CONTEXT_BIT      (1ull << 9) /* (41 - 32) */
 #define VMX_VPID_EXTENT_GLOBAL_CONTEXT_BIT      (1ull << 10) /* (42 - 32) */
+#define VMX_VPID_EXTENT_SINGLE_CONTEXT_RETAINING_GLOBALS_BIT (1ull << 11) /* (43 - 32) */
 
 #define VMX_EPT_DEFAULT_GAW			3
 #define VMX_EPT_MAX_GAW				0x4
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index af1168e8..b727028 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -1521,7 +1521,7 @@ static inline void vpid_sync_vcpu_single(int vpid)
 static inline void vpid_sync_vcpu_global(void)
 {
 	if (cpu_has_vmx_invvpid_global())
-		__invvpid(VMX_VPID_EXTENT_ALL_CONTEXT, 0, 0);
+		__invvpid(VMX_VPID_EXTENT_GLOBAL_CONTEXT, 0, 0);
 }
 
 static inline void vpid_sync_context(int vpid)
@@ -7747,7 +7747,7 @@ static int handle_invvpid(struct kvm_vcpu *vcpu)
 		 * Old versions of KVM use the single-context version so we
 		 * have to support it; just treat it the same as all-context.
 		 */
-	case VMX_VPID_EXTENT_ALL_CONTEXT:
+	case VMX_VPID_EXTENT_GLOBAL_CONTEXT:
 		__vmx_flush_tlb(vcpu, to_vmx(vcpu)->nested.vpid02);
 		nested_vmx_succeed(vcpu);
 		break;
-- 
1.9.1

[PATCH 2/2] KVM: nVMX: invvpid handling improvements

[Jan Dakinevich]

 - Expose all invalidation types to the L1

 - Reject invvpid instruction, if L1 passed zero vpid value to single
   context invalidations

Signed-off-by: Jan Dakinevich <jan.dakinevich@gmail.com>
---
 arch/x86/kvm/vmx.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index b727028..e11507d 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -132,6 +132,11 @@
 
 #define VMX_MISC_EMULATED_PREEMPTION_TIMER_RATE 5
 
+#define VMX_VPID_EXTENT_ALL_MASK (VMX_VPID_EXTENT_INDIVIDUAL_ADDR_BIT |	\
+	VMX_VPID_EXTENT_SINGLE_CONTEXT_BIT |				\
+	VMX_VPID_EXTENT_GLOBAL_CONTEXT_BIT |				\
+	VMX_VPID_EXTENT_SINGLE_CONTEXT_RETAINING_GLOBALS_BIT)
+
 /*
  * These 2 parameters are used to config the controls for Pause-Loop Exiting:
  * ple_gap:    upper bound on the amount of time between two successive
@@ -2838,8 +2843,7 @@ static void nested_vmx_setup_ctls_msrs(struct vcpu_vmx *vmx)
 	 */
 	if (enable_vpid)
 		vmx->nested.nested_vmx_vpid_caps = VMX_VPID_INVVPID_BIT |
-				VMX_VPID_EXTENT_SINGLE_CONTEXT_BIT |
-				VMX_VPID_EXTENT_GLOBAL_CONTEXT_BIT;
+			VMX_VPID_EXTENT_ALL_MASK;
 	else
 		vmx->nested.nested_vmx_vpid_caps = 0;
 
@@ -7720,7 +7724,7 @@ static int handle_invvpid(struct kvm_vcpu *vcpu)
 	vmx_instruction_info = vmcs_read32(VMX_INSTRUCTION_INFO);
 	type = kvm_register_readl(vcpu, (vmx_instruction_info >> 28) & 0xf);
 
-	types = (vmx->nested.nested_vmx_vpid_caps >> 8) & 0x7;
+	types = (vmx->nested.nested_vmx_vpid_caps & VMX_VPID_EXTENT_ALL_MASK) >> 8;
 
 	if (!(types & (1UL << type))) {
 		nested_vmx_failValid(vcpu,
@@ -7742,17 +7746,24 @@ static int handle_invvpid(struct kvm_vcpu *vcpu)
 	}
 
 	switch (type) {
+	case VMX_VPID_EXTENT_INDIVIDUAL_ADDR:
 	case VMX_VPID_EXTENT_SINGLE_CONTEXT:
+	case VMX_VPID_EXTENT_SINGLE_CONTEXT_RETAINING_GLOBALS:
+		if (!vpid) {
+			nested_vmx_failValid(vcpu,
+				VMXERR_INVALID_OPERAND_TO_INVEPT_INVVPID);
+			skip_emulated_instruction(vcpu);
+			return 1;
+		}
+		/* fall through */
+	case VMX_VPID_EXTENT_GLOBAL_CONTEXT:
 		/*
-		 * Old versions of KVM use the single-context version so we
-		 * have to support it; just treat it the same as all-context.
+		 * Treat any invvpid type as all-context invalidation
 		 */
-	case VMX_VPID_EXTENT_GLOBAL_CONTEXT:
 		__vmx_flush_tlb(vcpu, to_vmx(vcpu)->nested.vpid02);
 		nested_vmx_succeed(vcpu);
 		break;
 	default:
-		/* Trap individual address invalidation invvpid calls */
 		BUG_ON(1);
 		break;
 	}
-- 
1.9.1

Re: [PATCH 2/2] KVM: nVMX: invvpid handling improvements

[Radim Krčmář]

2016-10-19 01:45+0300, Jan Dakinevich:
>  - Expose all invalidation types to the L1
> 
>  - Reject invvpid instruction, if L1 passed zero vpid value to single
>    context invalidations
> 
> Signed-off-by: Jan Dakinevich <jan.dakinevich@gmail.com>
> ---
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> @@ -132,6 +132,11 @@
>  
>  #define VMX_MISC_EMULATED_PREEMPTION_TIMER_RATE 5
>  
> +#define VMX_VPID_EXTENT_ALL_MASK (VMX_VPID_EXTENT_INDIVIDUAL_ADDR_BIT |	\

SUPPORTED instead of ALL would be a better name.

> +	VMX_VPID_EXTENT_SINGLE_CONTEXT_BIT |				\
> +	VMX_VPID_EXTENT_GLOBAL_CONTEXT_BIT |				\
> +	VMX_VPID_EXTENT_SINGLE_CONTEXT_RETAINING_GLOBALS_BIT)
> +
>  /*
>   * These 2 parameters are used to config the controls for Pause-Loop Exiting:
>   * ple_gap:    upper bound on the amount of time between two successive
> @@ -2838,8 +2843,7 @@ static void nested_vmx_setup_ctls_msrs(struct vcpu_vmx *vmx)
>  	 */
>  	if (enable_vpid)
>  		vmx->nested.nested_vmx_vpid_caps = VMX_VPID_INVVPID_BIT |
> -				VMX_VPID_EXTENT_SINGLE_CONTEXT_BIT |
> -				VMX_VPID_EXTENT_GLOBAL_CONTEXT_BIT;
> +			VMX_VPID_EXTENT_ALL_MASK;

I'd still support only type 2, because it is the only one we implement,
and type 1, because of buggy KVMs.

Are there some OSes that can't use single or all context invalidation,
so supporting more might benefit something?

>  	else
>  		vmx->nested.nested_vmx_vpid_caps = 0;
> @@ -7720,7 +7724,7 @@ static int handle_invvpid(struct kvm_vcpu *vcpu)
>  	vmx_instruction_info = vmcs_read32(VMX_INSTRUCTION_INFO);
>  	type = kvm_register_readl(vcpu, (vmx_instruction_info >> 28) & 0xf);
>  
> -	types = (vmx->nested.nested_vmx_vpid_caps >> 8) & 0x7;
> +	types = (vmx->nested.nested_vmx_vpid_caps & VMX_VPID_EXTENT_ALL_MASK) >> 8;
>  
>  	if (!(types & (1UL << type))) {
>  		nested_vmx_failValid(vcpu,
> @@ -7742,17 +7746,24 @@ static int handle_invvpid(struct kvm_vcpu *vcpu)
>  	}
>  
>  	switch (type) {
> +	case VMX_VPID_EXTENT_INDIVIDUAL_ADDR:
>  	case VMX_VPID_EXTENT_SINGLE_CONTEXT:
> +	case VMX_VPID_EXTENT_SINGLE_CONTEXT_RETAINING_GLOBALS:
> +		if (!vpid) {
> +			nested_vmx_failValid(vcpu,
> +				VMXERR_INVALID_OPERAND_TO_INVEPT_INVVPID);
> +			skip_emulated_instruction(vcpu);
> +			return 1;

(Just break and share the code.)

> +		}
> +		/* fall through */
> +	case VMX_VPID_EXTENT_GLOBAL_CONTEXT:
>  		/*
> -		 * Old versions of KVM use the single-context version so we
> -		 * have to support it; just treat it the same as all-context.
> +		 * Treat any invvpid type as all-context invalidation
>  		 */
> -	case VMX_VPID_EXTENT_GLOBAL_CONTEXT:
>  		__vmx_flush_tlb(vcpu, to_vmx(vcpu)->nested.vpid02);
>  		nested_vmx_succeed(vcpu);
>  		break;
>  	default:
> -		/* Trap individual address invalidation invvpid calls */
>  		BUG_ON(1);

Yeah, this was wrong when this type was not actually supported.
BUG_ON() is not a good thing to do in any case, please change it into
WARN_ON_ONCE(), because we should never get into the default branch.

Thanks.

Re: [PATCH 2/2] KVM: nVMX: invvpid handling improvements

[Ladi Prosek]

On Fri, Oct 21, 2016 at 10:02 PM, Radim Krčmář <rkrcmar@redhat.com> wrote:
> 2016-10-19 01:45+0300, Jan Dakinevich:
>>  - Expose all invalidation types to the L1
>>
>>  - Reject invvpid instruction, if L1 passed zero vpid value to single
>>    context invalidations
>>
>> Signed-off-by: Jan Dakinevich <jan.dakinevich@gmail.com>
>> ---
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> @@ -132,6 +132,11 @@
>>
>>  #define VMX_MISC_EMULATED_PREEMPTION_TIMER_RATE 5
>>
>> +#define VMX_VPID_EXTENT_ALL_MASK (VMX_VPID_EXTENT_INDIVIDUAL_ADDR_BIT |      \
>
> SUPPORTED instead of ALL would be a better name.
>
>> +     VMX_VPID_EXTENT_SINGLE_CONTEXT_BIT |                            \
>> +     VMX_VPID_EXTENT_GLOBAL_CONTEXT_BIT |                            \
>> +     VMX_VPID_EXTENT_SINGLE_CONTEXT_RETAINING_GLOBALS_BIT)
>> +
>>  /*
>>   * These 2 parameters are used to config the controls for Pause-Loop Exiting:
>>   * ple_gap:    upper bound on the amount of time between two successive
>> @@ -2838,8 +2843,7 @@ static void nested_vmx_setup_ctls_msrs(struct vcpu_vmx *vmx)
>>        */
>>       if (enable_vpid)
>>               vmx->nested.nested_vmx_vpid_caps = VMX_VPID_INVVPID_BIT |
>> -                             VMX_VPID_EXTENT_SINGLE_CONTEXT_BIT |
>> -                             VMX_VPID_EXTENT_GLOBAL_CONTEXT_BIT;
>> +                     VMX_VPID_EXTENT_ALL_MASK;
>
> I'd still support only type 2, because it is the only one we implement,
> and type 1, because of buggy KVMs.
>
> Are there some OSes that can't use single or all context invalidation,
> so supporting more might benefit something?

Windows Server 2016 with Hyper-V enabled requires all four
invalidation types. The log message is not super clear, just
s/allowed/required/ and s/required/available/

"
Hypervisor launch failed;
Processor does not support the minimum features required to run the hypervisor
(MSR index 0x48C, allowed bits 0xF0106104040, required bits 0x60106114041).
"

I have verified that adding VMX_VPID_EXTENT_INDIVIDUAL_ADDR and
VMX_VPID_EXTENT_SINGLE_CONTEXT_RETAINING_GLOBALS solves this, although
it's not the only issue preventing Hyper-V from running on KVM at the
moment.

I'll be happy to test the next version of this patch.

Thanks!
Ladi

>>       else
>>               vmx->nested.nested_vmx_vpid_caps = 0;
>> @@ -7720,7 +7724,7 @@ static int handle_invvpid(struct kvm_vcpu *vcpu)
>>       vmx_instruction_info = vmcs_read32(VMX_INSTRUCTION_INFO);
>>       type = kvm_register_readl(vcpu, (vmx_instruction_info >> 28) & 0xf);
>>
>> -     types = (vmx->nested.nested_vmx_vpid_caps >> 8) & 0x7;
>> +     types = (vmx->nested.nested_vmx_vpid_caps & VMX_VPID_EXTENT_ALL_MASK) >> 8;
>>
>>       if (!(types & (1UL << type))) {
>>               nested_vmx_failValid(vcpu,
>> @@ -7742,17 +7746,24 @@ static int handle_invvpid(struct kvm_vcpu *vcpu)
>>       }
>>
>>       switch (type) {
>> +     case VMX_VPID_EXTENT_INDIVIDUAL_ADDR:
>>       case VMX_VPID_EXTENT_SINGLE_CONTEXT:
>> +     case VMX_VPID_EXTENT_SINGLE_CONTEXT_RETAINING_GLOBALS:
>> +             if (!vpid) {
>> +                     nested_vmx_failValid(vcpu,
>> +                             VMXERR_INVALID_OPERAND_TO_INVEPT_INVVPID);
>> +                     skip_emulated_instruction(vcpu);
>> +                     return 1;
>
> (Just break and share the code.)
>
>> +             }
>> +             /* fall through */
>> +     case VMX_VPID_EXTENT_GLOBAL_CONTEXT:
>>               /*
>> -              * Old versions of KVM use the single-context version so we
>> -              * have to support it; just treat it the same as all-context.
>> +              * Treat any invvpid type as all-context invalidation
>>                */
>> -     case VMX_VPID_EXTENT_GLOBAL_CONTEXT:
>>               __vmx_flush_tlb(vcpu, to_vmx(vcpu)->nested.vpid02);
>>               nested_vmx_succeed(vcpu);
>>               break;
>>       default:
>> -             /* Trap individual address invalidation invvpid calls */
>>               BUG_ON(1);
>
> Yeah, this was wrong when this type was not actually supported.
> BUG_ON() is not a good thing to do in any case, please change it into
> WARN_ON_ONCE(), because we should never get into the default branch.
>
> Thanks.
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] KVM: nVMX: invvpid handling improvements

[Radim Krčmář]

2016-10-25 10:08+0200, Ladi Prosek:
> On Fri, Oct 21, 2016 at 10:02 PM, Radim Krčmář <rkrcmar@redhat.com> wrote:
>> 2016-10-19 01:45+0300, Jan Dakinevich:
>>>  - Expose all invalidation types to the L1
>>>
>>>  - Reject invvpid instruction, if L1 passed zero vpid value to single
>>>    context invalidations
>>>
>>> Signed-off-by: Jan Dakinevich <jan.dakinevich@gmail.com>
>>> ---
>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>> @@ -132,6 +132,11 @@
>>>
>>>  #define VMX_MISC_EMULATED_PREEMPTION_TIMER_RATE 5
>>>
>>> +#define VMX_VPID_EXTENT_ALL_MASK (VMX_VPID_EXTENT_INDIVIDUAL_ADDR_BIT |      \
>>
>> SUPPORTED instead of ALL would be a better name.
>>
>>> +     VMX_VPID_EXTENT_SINGLE_CONTEXT_BIT |                            \
>>> +     VMX_VPID_EXTENT_GLOBAL_CONTEXT_BIT |                            \
>>> +     VMX_VPID_EXTENT_SINGLE_CONTEXT_RETAINING_GLOBALS_BIT)
>>> +
>>>  /*
>>>   * These 2 parameters are used to config the controls for Pause-Loop Exiting:
>>>   * ple_gap:    upper bound on the amount of time between two successive
>>> @@ -2838,8 +2843,7 @@ static void nested_vmx_setup_ctls_msrs(struct vcpu_vmx *vmx)
>>>        */
>>>       if (enable_vpid)
>>>               vmx->nested.nested_vmx_vpid_caps = VMX_VPID_INVVPID_BIT |
>>> -                             VMX_VPID_EXTENT_SINGLE_CONTEXT_BIT |
>>> -                             VMX_VPID_EXTENT_GLOBAL_CONTEXT_BIT;
>>> +                     VMX_VPID_EXTENT_ALL_MASK;
>>
>> I'd still support only type 2, because it is the only one we implement,
>> and type 1, because of buggy KVMs.
>>
>> Are there some OSes that can't use single or all context invalidation,
>> so supporting more might benefit something?
> 
> Windows Server 2016 with Hyper-V enabled requires all four
> invalidation types. The log message is not super clear, just
> s/allowed/required/ and s/required/available/

Yep, I would have understood it the other way around ...

> "
> Hypervisor launch failed;
> Processor does not support the minimum features required to run the hypervisor
> (MSR index 0x48C, allowed bits 0xF0106104040, required bits 0x60106114041).
> "
> 
> I have verified that adding VMX_VPID_EXTENT_INDIVIDUAL_ADDR and
> VMX_VPID_EXTENT_SINGLE_CONTEXT_RETAINING_GLOBALS solves this,

Great info.  Jan, I take back the conservativeness in review -- we'll
want to support the currently existing modes.  (Invalidating unnecessary
entries is sad, but not that bad ... nested is already slow.)

>                                                               although
> it's not the only issue preventing Hyper-V from running on KVM at the
> moment.

You mentioned another "allowed" bit that KVM lacks, which one was it?

Thanks.

Re: [PATCH 2/2] KVM: nVMX: invvpid handling improvements

[Ladi Prosek]

On Tue, Oct 25, 2016 at 3:40 PM, Radim Krčmář <rkrcmar@redhat.com> wrote:
> 2016-10-25 10:08+0200, Ladi Prosek:
>> On Fri, Oct 21, 2016 at 10:02 PM, Radim Krčmář <rkrcmar@redhat.com> wrote:
>>> 2016-10-19 01:45+0300, Jan Dakinevich:
>>>>  - Expose all invalidation types to the L1
>>>>
>>>>  - Reject invvpid instruction, if L1 passed zero vpid value to single
>>>>    context invalidations
>>>>
>>>> Signed-off-by: Jan Dakinevich <jan.dakinevich@gmail.com>
>>>> ---
>>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>>> @@ -132,6 +132,11 @@
>>>>
>>>>  #define VMX_MISC_EMULATED_PREEMPTION_TIMER_RATE 5
>>>>
>>>> +#define VMX_VPID_EXTENT_ALL_MASK (VMX_VPID_EXTENT_INDIVIDUAL_ADDR_BIT |      \
>>>
>>> SUPPORTED instead of ALL would be a better name.
>>>
>>>> +     VMX_VPID_EXTENT_SINGLE_CONTEXT_BIT |                            \
>>>> +     VMX_VPID_EXTENT_GLOBAL_CONTEXT_BIT |                            \
>>>> +     VMX_VPID_EXTENT_SINGLE_CONTEXT_RETAINING_GLOBALS_BIT)
>>>> +
>>>>  /*
>>>>   * These 2 parameters are used to config the controls for Pause-Loop Exiting:
>>>>   * ple_gap:    upper bound on the amount of time between two successive
>>>> @@ -2838,8 +2843,7 @@ static void nested_vmx_setup_ctls_msrs(struct vcpu_vmx *vmx)
>>>>        */
>>>>       if (enable_vpid)
>>>>               vmx->nested.nested_vmx_vpid_caps = VMX_VPID_INVVPID_BIT |
>>>> -                             VMX_VPID_EXTENT_SINGLE_CONTEXT_BIT |
>>>> -                             VMX_VPID_EXTENT_GLOBAL_CONTEXT_BIT;
>>>> +                     VMX_VPID_EXTENT_ALL_MASK;
>>>
>>> I'd still support only type 2, because it is the only one we implement,
>>> and type 1, because of buggy KVMs.
>>>
>>> Are there some OSes that can't use single or all context invalidation,
>>> so supporting more might benefit something?
>>
>> Windows Server 2016 with Hyper-V enabled requires all four
>> invalidation types. The log message is not super clear, just
>> s/allowed/required/ and s/required/available/
>
> Yep, I would have understood it the other way around ...
>
>> "
>> Hypervisor launch failed;
>> Processor does not support the minimum features required to run the hypervisor
>> (MSR index 0x48C, allowed bits 0xF0106104040, required bits 0x60106114041).
>> "
>>
>> I have verified that adding VMX_VPID_EXTENT_INDIVIDUAL_ADDR and
>> VMX_VPID_EXTENT_SINGLE_CONTEXT_RETAINING_GLOBALS solves this,
>
> Great info.  Jan, I take back the conservativeness in review -- we'll
> want to support the currently existing modes.  (Invalidating unnecessary
> entries is sad, but not that bad ... nested is already slow.)
>
>>                                                               although
>> it's not the only issue preventing Hyper-V from running on KVM at the
>> moment.
>
> You mentioned another "allowed" bit that KVM lacks, which one was it?

MSR 48B 'Secondary Processor-Based VM-Execution Controls'
bit 2 'Descriptor-table exiting'

> Thanks.

Re: [PATCH 2/2] KVM: nVMX: invvpid handling improvements

[Paolo Bonzini]

On 25/10/2016 15:57, Ladi Prosek wrote:
> On Tue, Oct 25, 2016 at 3:40 PM, Radim Krčmář <rkrcmar@redhat.com> wrote:
>> 2016-10-25 10:08+0200, Ladi Prosek:
>>> On Fri, Oct 21, 2016 at 10:02 PM, Radim Krčmář <rkrcmar@redhat.com> wrote:
>>>> 2016-10-19 01:45+0300, Jan Dakinevich:
>>>>>  - Expose all invalidation types to the L1
>>>>>
>>>>>  - Reject invvpid instruction, if L1 passed zero vpid value to single
>>>>>    context invalidations
>>>>>
>>>>> Signed-off-by: Jan Dakinevich <jan.dakinevich@gmail.com>
>>>>> ---
>>>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>>>> @@ -132,6 +132,11 @@
>>>>>
>>>>>  #define VMX_MISC_EMULATED_PREEMPTION_TIMER_RATE 5
>>>>>
>>>>> +#define VMX_VPID_EXTENT_ALL_MASK (VMX_VPID_EXTENT_INDIVIDUAL_ADDR_BIT |      \
>>>>
>>>> SUPPORTED instead of ALL would be a better name.
>>>>
>>>>> +     VMX_VPID_EXTENT_SINGLE_CONTEXT_BIT |                            \
>>>>> +     VMX_VPID_EXTENT_GLOBAL_CONTEXT_BIT |                            \
>>>>> +     VMX_VPID_EXTENT_SINGLE_CONTEXT_RETAINING_GLOBALS_BIT)
>>>>> +
>>>>>  /*
>>>>>   * These 2 parameters are used to config the controls for Pause-Loop Exiting:
>>>>>   * ple_gap:    upper bound on the amount of time between two successive
>>>>> @@ -2838,8 +2843,7 @@ static void nested_vmx_setup_ctls_msrs(struct vcpu_vmx *vmx)
>>>>>        */
>>>>>       if (enable_vpid)
>>>>>               vmx->nested.nested_vmx_vpid_caps = VMX_VPID_INVVPID_BIT |
>>>>> -                             VMX_VPID_EXTENT_SINGLE_CONTEXT_BIT |
>>>>> -                             VMX_VPID_EXTENT_GLOBAL_CONTEXT_BIT;
>>>>> +                     VMX_VPID_EXTENT_ALL_MASK;
>>>>
>>>> I'd still support only type 2, because it is the only one we implement,
>>>> and type 1, because of buggy KVMs.
>>>>
>>>> Are there some OSes that can't use single or all context invalidation,
>>>> so supporting more might benefit something?
>>>
>>> Windows Server 2016 with Hyper-V enabled requires all four
>>> invalidation types. The log message is not super clear, just
>>> s/allowed/required/ and s/required/available/
>>
>> Yep, I would have understood it the other way around ...
>>
>>> "
>>> Hypervisor launch failed;
>>> Processor does not support the minimum features required to run the hypervisor
>>> (MSR index 0x48C, allowed bits 0xF0106104040, required bits 0x60106114041).
>>> "
>>>
>>> I have verified that adding VMX_VPID_EXTENT_INDIVIDUAL_ADDR and
>>> VMX_VPID_EXTENT_SINGLE_CONTEXT_RETAINING_GLOBALS solves this,
>>
>> Great info.  Jan, I take back the conservativeness in review -- we'll
>> want to support the currently existing modes.  (Invalidating unnecessary
>> entries is sad, but not that bad ... nested is already slow.)
>>
>>>                                                               although
>>> it's not the only issue preventing Hyper-V from running on KVM at the
>>> moment.
>>
>> You mentioned another "allowed" bit that KVM lacks, which one was it?
> 
> MSR 48B 'Secondary Processor-Based VM-Execution Controls'
> bit 2 'Descriptor-table exiting'

That's trivial to add it.  Let me take a look.

Thanks,

Paolo

Re: [PATCH 2/2] KVM: nVMX: invvpid handling improvements

[Paolo Bonzini]

On 21/10/2016 22:02, Radim Krčmář wrote:
>>
>> +	case VMX_VPID_EXTENT_INDIVIDUAL_ADDR:
>>  	case VMX_VPID_EXTENT_SINGLE_CONTEXT:
>> +	case VMX_VPID_EXTENT_SINGLE_CONTEXT_RETAINING_GLOBALS:

The comment below can be moved here, saying that Hyper-V and old
versions of KVM require these cases to be supported:

	/*
	 * Hyper-V and old versions of KVM require that we support these
	 * cases, even though we cannot really do anything special about
	 * them and treat them as all-context invalidation.
	 */

Paolo

>> +		if (!vpid) {
>> +			nested_vmx_failValid(vcpu,
>> +				VMXERR_INVALID_OPERAND_TO_INVEPT_INVVPID);
>> +			skip_emulated_instruction(vcpu);
>> +			return 1;
> 
> (Just break and share the code.)
> 
>> +		}
>> +		/* fall through */
>> +	case VMX_VPID_EXTENT_GLOBAL_CONTEXT:
>>  		/*
>> -		 * Old versions of KVM use the single-context version so we
>> -		 * have to support it; just treat it the same as all-context.
>> +		 * Treat any invvpid type as all-context invalidation
>>  		 */
>> -	case VMX_VPID_EXTENT_GLOBAL_CONTEXT:
>>  		__vmx_flush_tlb(vcpu, to_vmx(vcpu)->nested.vpid02);
>>  		nested_vmx_succeed(vcpu);
>>  		break;

Re: [PATCH 1/2] KVM: VMX: clean up declaration of VPID/EPT invalidation types

[Radim Krčmář]

2016-10-19 01:45+0300, Jan Dakinevich:
>  - Remove VMX_EPT_EXTENT_INDIVIDUAL_ADDR, since there is no such type of
>    EPT invalidation
> 
>  - Rename VMX_VPID_EXTENT_ALL_CONTEXT to VMX_VPID_EXTENT_GLOBAL_CONTEXT
>    for consitency: all-context VPID invalidation is referenced  by
>    "global" keyword in all other places of code

SDM names this invalidation as "all contexts" and global translations
have a different meaning, so I would keep it, for clarity.

We already use the other meaning in SINGLE_CONTEXT_RETAINING_GLOBALS and
for example INVPCID has type for "all contexts, including globals" and
"all contexts, excluding globals", so we will have to really distinguish
global and global at some point ...

>  - Add missing VPID types names
> 
> Signed-off-by: Jan Dakinevich <jan.dakinevich@gmail.com>
> ---