[PATCH 0/2] KVM: VMX: Use basic exit reason for cheking and indexing

[PATCH 0/2] KVM: VMX: Use basic exit reason for cheking and indexing

[Xiaoyao Li]

Current KVM directly uses the whole 32-bit EXIT REASON when
1) checking: if (vmx->exit_reason == EXIT_REASON_*)
2) indexing: kvm_vmx_exit_handlers[exit_reason]

However, only the low 16-bit of EXIT REASON serves as basic Exit Reason.
Fix it by using the 16-bit basic exit reason.

BTW, I'm not sure if it's necessary to split nested case into a seperate
patch.

Xiaoyao Li (2):
  kvm: vmx: Use basic exit reason to check if it's the specific VM EXIT
  kvm: nvmx: Use basic(exit_reason) when checking specific EXIT_REASON

 arch/x86/kvm/vmx/nested.c |  6 +++---
 arch/x86/kvm/vmx/nested.h |  2 +-
 arch/x86/kvm/vmx/vmx.c    | 44 ++++++++++++++++++++-------------------
 arch/x86/kvm/vmx/vmx.h    |  2 ++
 4 files changed, 29 insertions(+), 25 deletions(-)

-- 
2.23.0

[PATCH 1/2] kvm: vmx: Use basic exit reason to check if it's the specific VM EXIT

[Xiaoyao Li]

Current kvm uses the 32-bit exit reason to check if it's any specific VM
EXIT, however only the low 16-bit of VM EXIT REASON acts as the basic
exit reason.

Introduce Macro basic(exit_reaso) to help retrieve the basic exit reason
from VM EXIT REASON, and use the basic exit reason for checking and
indexing the exit hanlder.

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
 arch/x86/kvm/vmx/vmx.c | 44 ++++++++++++++++++++++--------------------
 arch/x86/kvm/vmx/vmx.h |  2 ++
 2 files changed, 25 insertions(+), 21 deletions(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 9a6664886f2e..85da72d4dc92 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -1584,7 +1584,7 @@ static int skip_emulated_instruction(struct kvm_vcpu *vcpu)
 	 * i.e. we end up advancing IP with some random value.
 	 */
 	if (!static_cpu_has(X86_FEATURE_HYPERVISOR) ||
-	    to_vmx(vcpu)->exit_reason != EXIT_REASON_EPT_MISCONFIG) {
+	    basic(to_vmx(vcpu)->exit_reason) != EXIT_REASON_EPT_MISCONFIG) {
 		rip = kvm_rip_read(vcpu);
 		rip += vmcs_read32(VM_EXIT_INSTRUCTION_LEN);
 		kvm_rip_write(vcpu, rip);
@@ -5797,6 +5797,7 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu,
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 	u32 exit_reason = vmx->exit_reason;
+	u16 basic_exit_reason = basic(exit_reason);
 	u32 vectoring_info = vmx->idt_vectoring_info;
 
 	trace_kvm_exit(exit_reason, vcpu, KVM_ISA_VMX);
@@ -5842,17 +5843,17 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu,
 	 * will cause infinite loop.
 	 */
 	if ((vectoring_info & VECTORING_INFO_VALID_MASK) &&
-			(exit_reason != EXIT_REASON_EXCEPTION_NMI &&
-			exit_reason != EXIT_REASON_EPT_VIOLATION &&
-			exit_reason != EXIT_REASON_PML_FULL &&
-			exit_reason != EXIT_REASON_TASK_SWITCH)) {
+			(basic_exit_reason != EXIT_REASON_EXCEPTION_NMI &&
+			 basic_exit_reason != EXIT_REASON_EPT_VIOLATION &&
+			 basic_exit_reason != EXIT_REASON_PML_FULL &&
+			 basic_exit_reason != EXIT_REASON_TASK_SWITCH)) {
 		vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
 		vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_DELIVERY_EV;
 		vcpu->run->internal.ndata = 3;
 		vcpu->run->internal.data[0] = vectoring_info;
 		vcpu->run->internal.data[1] = exit_reason;
 		vcpu->run->internal.data[2] = vcpu->arch.exit_qualification;
-		if (exit_reason == EXIT_REASON_EPT_MISCONFIG) {
+		if (basic_exit_reason == EXIT_REASON_EPT_MISCONFIG) {
 			vcpu->run->internal.ndata++;
 			vcpu->run->internal.data[3] =
 				vmcs_read64(GUEST_PHYSICAL_ADDRESS);
@@ -5884,32 +5885,32 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu,
 		return 1;
 	}
 
-	if (exit_reason >= kvm_vmx_max_exit_handlers)
+	if (basic_exit_reason >= kvm_vmx_max_exit_handlers)
 		goto unexpected_vmexit;
 #ifdef CONFIG_RETPOLINE
-	if (exit_reason == EXIT_REASON_MSR_WRITE)
+	if (basic_exit_reason == EXIT_REASON_MSR_WRITE)
 		return kvm_emulate_wrmsr(vcpu);
-	else if (exit_reason == EXIT_REASON_PREEMPTION_TIMER)
+	else if (basic_exit_reason == EXIT_REASON_PREEMPTION_TIMER)
 		return handle_preemption_timer(vcpu);
-	else if (exit_reason == EXIT_REASON_INTERRUPT_WINDOW)
+	else if (basic_exit_reason == EXIT_REASON_INTERRUPT_WINDOW)
 		return handle_interrupt_window(vcpu);
-	else if (exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT)
+	else if (basic_exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT)
 		return handle_external_interrupt(vcpu);
-	else if (exit_reason == EXIT_REASON_HLT)
+	else if (basic_exit_reason == EXIT_REASON_HLT)
 		return kvm_emulate_halt(vcpu);
-	else if (exit_reason == EXIT_REASON_EPT_MISCONFIG)
+	else if (basic_exit_reason == EXIT_REASON_EPT_MISCONFIG)
 		return handle_ept_misconfig(vcpu);
 #endif
 
-	exit_reason = array_index_nospec(exit_reason,
+	basic_exit_reason = array_index_nospec(basic_exit_reason,
 					 kvm_vmx_max_exit_handlers);
-	if (!kvm_vmx_exit_handlers[exit_reason])
+	if (!kvm_vmx_exit_handlers[basic_exit_reason])
 		goto unexpected_vmexit;
 
-	return kvm_vmx_exit_handlers[exit_reason](vcpu);
+	return kvm_vmx_exit_handlers[basic_exit_reason](vcpu);
 
 unexpected_vmexit:
-	vcpu_unimpl(vcpu, "vmx: unexpected exit reason 0x%x\n", exit_reason);
+	vcpu_unimpl(vcpu, "vmx: unexpected exit reason 0x%x\n", basic_exit_reason);
 	dump_vmcs();
 	vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
 	vcpu->run->internal.suberror =
@@ -6241,13 +6242,14 @@ static void vmx_handle_exit_irqoff(struct kvm_vcpu *vcpu,
 	enum exit_fastpath_completion *exit_fastpath)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
+	u16 basic_exit_reason = basic(vmx->exit_reason);
 
-	if (vmx->exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT)
+	if (basic_exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT)
 		handle_external_interrupt_irqoff(vcpu);
-	else if (vmx->exit_reason == EXIT_REASON_EXCEPTION_NMI)
+	else if (basic_exit_reason == EXIT_REASON_EXCEPTION_NMI)
 		handle_exception_nmi_irqoff(vmx);
 	else if (!is_guest_mode(vcpu) &&
-		vmx->exit_reason == EXIT_REASON_MSR_WRITE)
+		 basic_exit_reason == EXIT_REASON_MSR_WRITE)
 		*exit_fastpath = handle_fastpath_set_msr_irqoff(vcpu);
 }
 
@@ -6621,7 +6623,7 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
 	vmx->idt_vectoring_info = 0;
 
 	vmx->exit_reason = vmx->fail ? 0xdead : vmcs_read32(VM_EXIT_REASON);
-	if ((u16)vmx->exit_reason == EXIT_REASON_MCE_DURING_VMENTRY)
+	if (basic(vmx->exit_reason) == EXIT_REASON_MCE_DURING_VMENTRY)
 		kvm_machine_check();
 
 	if (vmx->fail || (vmx->exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY))
diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
index 7f42cf3dcd70..c6ba33eedb59 100644
--- a/arch/x86/kvm/vmx/vmx.h
+++ b/arch/x86/kvm/vmx/vmx.h
@@ -22,6 +22,8 @@ extern u32 get_umwait_control_msr(void);
 
 #define X2APIC_MSR(r) (APIC_BASE_MSR + ((r) >> 4))
 
+#define basic(exit_reason) ((u16)(exit_reason))
+
 #ifdef CONFIG_X86_64
 #define NR_SHARED_MSRS	7
 #else
-- 
2.23.0

[PATCH 2/2] kvm: nvmx: Use basic(exit_reason) when checking specific EXIT_REASON

[Xiaoyao Li]

Use the basic exit reason when checking if it equals the specific EXIT
REASON for nested.

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
 arch/x86/kvm/vmx/nested.c | 6 +++---
 arch/x86/kvm/vmx/nested.h | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 657c2eda357c..2746687fd186 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -4281,7 +4281,7 @@ void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 exit_reason,
 		 * message for details.
 		 */
 		if (nested_exit_intr_ack_set(vcpu) &&
-		    exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT &&
+		    basic(exit_reason) == EXIT_REASON_EXTERNAL_INTERRUPT &&
 		    kvm_cpu_has_interrupt(vcpu)) {
 			int irq = kvm_cpu_get_interrupt(vcpu);
 			WARN_ON(irq < 0);
@@ -5321,7 +5321,7 @@ static bool nested_vmx_exit_handled_msr(struct kvm_vcpu *vcpu,
 	 * First we need to figure out which of the four to use:
 	 */
 	bitmap = vmcs12->msr_bitmap;
-	if (exit_reason == EXIT_REASON_MSR_WRITE)
+	if (basic(exit_reason) == EXIT_REASON_MSR_WRITE)
 		bitmap += 2048;
 	if (msr_index >= 0xc0000000) {
 		msr_index -= 0xc0000000;
@@ -5487,7 +5487,7 @@ bool nested_vmx_exit_reflected(struct kvm_vcpu *vcpu, u32 exit_reason)
 				vmcs_read32(VM_EXIT_INTR_ERROR_CODE),
 				KVM_ISA_VMX);
 
-	switch (exit_reason) {
+	switch (basic(exit_reason)) {
 	case EXIT_REASON_EXCEPTION_NMI:
 		if (is_nmi(intr_info))
 			return false;
diff --git a/arch/x86/kvm/vmx/nested.h b/arch/x86/kvm/vmx/nested.h
index fc874d4ead0f..6af2e0dabe94 100644
--- a/arch/x86/kvm/vmx/nested.h
+++ b/arch/x86/kvm/vmx/nested.h
@@ -83,7 +83,7 @@ static inline int nested_vmx_reflect_vmexit(struct kvm_vcpu *vcpu,
 	 * is only valid for EXCEPTION_NMI exits.  For EXTERNAL_INTERRUPT
 	 * we need to query the in-kernel LAPIC.
 	 */
-	WARN_ON(exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT);
+	WARN_ON(basic(exit_reason) == EXIT_REASON_EXTERNAL_INTERRUPT);
 	if ((exit_intr_info &
 	     (INTR_INFO_VALID_MASK | INTR_INFO_DELIVER_CODE_MASK)) ==
 	    (INTR_INFO_VALID_MASK | INTR_INFO_DELIVER_CODE_MASK)) {
-- 
2.23.0

Re: [PATCH 1/2] kvm: vmx: Use basic exit reason to check if it's the specific VM EXIT

[Vitaly Kuznetsov]

Xiaoyao Li <xiaoyao.li@intel.com> writes:

> Current kvm uses the 32-bit exit reason to check if it's any specific VM
> EXIT, however only the low 16-bit of VM EXIT REASON acts as the basic
> exit reason.
>
> Introduce Macro basic(exit_reaso)

"exit_reason"

>  to help retrieve the basic exit reason
> from VM EXIT REASON, and use the basic exit reason for checking and
> indexing the exit hanlder.
>
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
>  arch/x86/kvm/vmx/vmx.c | 44 ++++++++++++++++++++++--------------------
>  arch/x86/kvm/vmx/vmx.h |  2 ++
>  2 files changed, 25 insertions(+), 21 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 9a6664886f2e..85da72d4dc92 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -1584,7 +1584,7 @@ static int skip_emulated_instruction(struct kvm_vcpu *vcpu)
>  	 * i.e. we end up advancing IP with some random value.
>  	 */
>  	if (!static_cpu_has(X86_FEATURE_HYPERVISOR) ||
> -	    to_vmx(vcpu)->exit_reason != EXIT_REASON_EPT_MISCONFIG) {
> +	    basic(to_vmx(vcpu)->exit_reason) != EXIT_REASON_EPT_MISCONFIG) {

"basic" word is probably 'too basic' to be used for this purpose. Even
if we need a macro for it (I'm not really convinced it improves the
readability), I'd suggest we name it 'basic_exit_reason()' instead.

>  		rip = kvm_rip_read(vcpu);
>  		rip += vmcs_read32(VM_EXIT_INSTRUCTION_LEN);
>  		kvm_rip_write(vcpu, rip);
> @@ -5797,6 +5797,7 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu,
>  {
>  	struct vcpu_vmx *vmx = to_vmx(vcpu);
>  	u32 exit_reason = vmx->exit_reason;
> +	u16 basic_exit_reason = basic(exit_reason);

I don't think renaming local variable is needed, let's just do

'u16 exit_reason = basic_exit_reason(vmx->exit_reason)' and keep the
rest of the code as-is.

>  	u32 vectoring_info = vmx->idt_vectoring_info;
>  
>  	trace_kvm_exit(exit_reason, vcpu, KVM_ISA_VMX);
> @@ -5842,17 +5843,17 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu,
>  	 * will cause infinite loop.
>  	 */
>  	if ((vectoring_info & VECTORING_INFO_VALID_MASK) &&
> -			(exit_reason != EXIT_REASON_EXCEPTION_NMI &&
> -			exit_reason != EXIT_REASON_EPT_VIOLATION &&
> -			exit_reason != EXIT_REASON_PML_FULL &&
> -			exit_reason != EXIT_REASON_TASK_SWITCH)) {
> +			(basic_exit_reason != EXIT_REASON_EXCEPTION_NMI &&
> +			 basic_exit_reason != EXIT_REASON_EPT_VIOLATION &&
> +			 basic_exit_reason != EXIT_REASON_PML_FULL &&
> +			 basic_exit_reason != EXIT_REASON_TASK_SWITCH)) {
>  		vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
>  		vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_DELIVERY_EV;
>  		vcpu->run->internal.ndata = 3;
>  		vcpu->run->internal.data[0] = vectoring_info;
>  		vcpu->run->internal.data[1] = exit_reason;
>  		vcpu->run->internal.data[2] = vcpu->arch.exit_qualification;
> -		if (exit_reason == EXIT_REASON_EPT_MISCONFIG) {
> +		if (basic_exit_reason == EXIT_REASON_EPT_MISCONFIG) {
>  			vcpu->run->internal.ndata++;
>  			vcpu->run->internal.data[3] =
>  				vmcs_read64(GUEST_PHYSICAL_ADDRESS);
> @@ -5884,32 +5885,32 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu,
>  		return 1;
>  	}
>  
> -	if (exit_reason >= kvm_vmx_max_exit_handlers)
> +	if (basic_exit_reason >= kvm_vmx_max_exit_handlers)
>  		goto unexpected_vmexit;
>  #ifdef CONFIG_RETPOLINE
> -	if (exit_reason == EXIT_REASON_MSR_WRITE)
> +	if (basic_exit_reason == EXIT_REASON_MSR_WRITE)
>  		return kvm_emulate_wrmsr(vcpu);
> -	else if (exit_reason == EXIT_REASON_PREEMPTION_TIMER)
> +	else if (basic_exit_reason == EXIT_REASON_PREEMPTION_TIMER)
>  		return handle_preemption_timer(vcpu);
> -	else if (exit_reason == EXIT_REASON_INTERRUPT_WINDOW)
> +	else if (basic_exit_reason == EXIT_REASON_INTERRUPT_WINDOW)
>  		return handle_interrupt_window(vcpu);
> -	else if (exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT)
> +	else if (basic_exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT)
>  		return handle_external_interrupt(vcpu);
> -	else if (exit_reason == EXIT_REASON_HLT)
> +	else if (basic_exit_reason == EXIT_REASON_HLT)
>  		return kvm_emulate_halt(vcpu);
> -	else if (exit_reason == EXIT_REASON_EPT_MISCONFIG)
> +	else if (basic_exit_reason == EXIT_REASON_EPT_MISCONFIG)
>  		return handle_ept_misconfig(vcpu);
>  #endif
>  
> -	exit_reason = array_index_nospec(exit_reason,
> +	basic_exit_reason = array_index_nospec(basic_exit_reason,
>  					 kvm_vmx_max_exit_handlers);
> -	if (!kvm_vmx_exit_handlers[exit_reason])
> +	if (!kvm_vmx_exit_handlers[basic_exit_reason])
>  		goto unexpected_vmexit;
>  
> -	return kvm_vmx_exit_handlers[exit_reason](vcpu);
> +	return kvm_vmx_exit_handlers[basic_exit_reason](vcpu);
>  
>  unexpected_vmexit:
> -	vcpu_unimpl(vcpu, "vmx: unexpected exit reason 0x%x\n", exit_reason);
> +	vcpu_unimpl(vcpu, "vmx: unexpected exit reason 0x%x\n", basic_exit_reason);
>  	dump_vmcs();
>  	vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
>  	vcpu->run->internal.suberror =
> @@ -6241,13 +6242,14 @@ static void vmx_handle_exit_irqoff(struct kvm_vcpu *vcpu,
>  	enum exit_fastpath_completion *exit_fastpath)
>  {
>  	struct vcpu_vmx *vmx = to_vmx(vcpu);
> +	u16 basic_exit_reason = basic(vmx->exit_reason);

Here I'd suggest we also use the same 

'u16 exit_reason = basic_exit_reason(vmx->exit_reason)'

as above.

>  
> -	if (vmx->exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT)
> +	if (basic_exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT)
>  		handle_external_interrupt_irqoff(vcpu);
> -	else if (vmx->exit_reason == EXIT_REASON_EXCEPTION_NMI)
> +	else if (basic_exit_reason == EXIT_REASON_EXCEPTION_NMI)
>  		handle_exception_nmi_irqoff(vmx);
>  	else if (!is_guest_mode(vcpu) &&
> -		vmx->exit_reason == EXIT_REASON_MSR_WRITE)
> +		 basic_exit_reason == EXIT_REASON_MSR_WRITE)
>  		*exit_fastpath = handle_fastpath_set_msr_irqoff(vcpu);
>  }
>  
> @@ -6621,7 +6623,7 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
>  	vmx->idt_vectoring_info = 0;
>  
>  	vmx->exit_reason = vmx->fail ? 0xdead : vmcs_read32(VM_EXIT_REASON);
> -	if ((u16)vmx->exit_reason == EXIT_REASON_MCE_DURING_VMENTRY)
> +	if (basic(vmx->exit_reason) == EXIT_REASON_MCE_DURING_VMENTRY)
>  		kvm_machine_check();
>  
>  	if (vmx->fail || (vmx->exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY))
> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> index 7f42cf3dcd70..c6ba33eedb59 100644
> --- a/arch/x86/kvm/vmx/vmx.h
> +++ b/arch/x86/kvm/vmx/vmx.h
> @@ -22,6 +22,8 @@ extern u32 get_umwait_control_msr(void);
>  
>  #define X2APIC_MSR(r) (APIC_BASE_MSR + ((r) >> 4))
>  
> +#define basic(exit_reason) ((u16)(exit_reason))
> +
>  #ifdef CONFIG_X86_64
>  #define NR_SHARED_MSRS	7
>  #else

-- 
Vitaly

Re: [PATCH 1/2] kvm: vmx: Use basic exit reason to check if it's the specific VM EXIT

[Xiaoyao Li]

On 2/24/2020 6:16 PM, Vitaly Kuznetsov wrote:
> Xiaoyao Li <xiaoyao.li@intel.com> writes:
> 
>> Current kvm uses the 32-bit exit reason to check if it's any specific VM
>> EXIT, however only the low 16-bit of VM EXIT REASON acts as the basic
>> exit reason.
>>
>> Introduce Macro basic(exit_reaso)
> 
> "exit_reason"

Ah, will correct it in v2.

>>   to help retrieve the basic exit reason
>> from VM EXIT REASON, and use the basic exit reason for checking and
>> indexing the exit hanlder.
>>
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> ---
>>   arch/x86/kvm/vmx/vmx.c | 44 ++++++++++++++++++++++--------------------
>>   arch/x86/kvm/vmx/vmx.h |  2 ++
>>   2 files changed, 25 insertions(+), 21 deletions(-)
>>
>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>> index 9a6664886f2e..85da72d4dc92 100644
>> --- a/arch/x86/kvm/vmx/vmx.c
>> +++ b/arch/x86/kvm/vmx/vmx.c
>> @@ -1584,7 +1584,7 @@ static int skip_emulated_instruction(struct kvm_vcpu *vcpu)
>>   	 * i.e. we end up advancing IP with some random value.
>>   	 */
>>   	if (!static_cpu_has(X86_FEATURE_HYPERVISOR) ||
>> -	    to_vmx(vcpu)->exit_reason != EXIT_REASON_EPT_MISCONFIG) {
>> +	    basic(to_vmx(vcpu)->exit_reason) != EXIT_REASON_EPT_MISCONFIG) {
> 
> "basic" word is probably 'too basic' to be used for this purpose. Even
> if we need a macro for it (I'm not really convinced it improves the
> readability), I'd suggest we name it 'basic_exit_reason()' instead.

Agreed.

>>   		rip = kvm_rip_read(vcpu);
>>   		rip += vmcs_read32(VM_EXIT_INSTRUCTION_LEN);
>>   		kvm_rip_write(vcpu, rip);
>> @@ -5797,6 +5797,7 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu,
>>   {
>>   	struct vcpu_vmx *vmx = to_vmx(vcpu);
>>   	u32 exit_reason = vmx->exit_reason;
>> +	u16 basic_exit_reason = basic(exit_reason);
> 
> I don't think renaming local variable is needed, let's just do
> 
> 'u16 exit_reason = basic_exit_reason(vmx->exit_reason)' and keep the
> rest of the code as-is.

No, we can't do this.

It's not just renaming local variable, the full 32-bit exit reason is 
used elsewhere in this function that needs the upper 16-bit.

Here variable basic_exit_reason is added for the cases where only basic 
exit reason number is needed.

>>   	u32 vectoring_info = vmx->idt_vectoring_info;
>>   
>>   	trace_kvm_exit(exit_reason, vcpu, KVM_ISA_VMX);
>> @@ -5842,17 +5843,17 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu,
>>   	 * will cause infinite loop.
>>   	 */
>>   	if ((vectoring_info & VECTORING_INFO_VALID_MASK) &&
>> -			(exit_reason != EXIT_REASON_EXCEPTION_NMI &&
>> -			exit_reason != EXIT_REASON_EPT_VIOLATION &&
>> -			exit_reason != EXIT_REASON_PML_FULL &&
>> -			exit_reason != EXIT_REASON_TASK_SWITCH)) {
>> +			(basic_exit_reason != EXIT_REASON_EXCEPTION_NMI &&
>> +			 basic_exit_reason != EXIT_REASON_EPT_VIOLATION &&
>> +			 basic_exit_reason != EXIT_REASON_PML_FULL &&
>> +			 basic_exit_reason != EXIT_REASON_TASK_SWITCH)) {
>>   		vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
>>   		vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_DELIVERY_EV;
>>   		vcpu->run->internal.ndata = 3;
>>   		vcpu->run->internal.data[0] = vectoring_info;
>>   		vcpu->run->internal.data[1] = exit_reason;
>>   		vcpu->run->internal.data[2] = vcpu->arch.exit_qualification;
>> -		if (exit_reason == EXIT_REASON_EPT_MISCONFIG) {
>> +		if (basic_exit_reason == EXIT_REASON_EPT_MISCONFIG) {
>>   			vcpu->run->internal.ndata++;
>>   			vcpu->run->internal.data[3] =
>>   				vmcs_read64(GUEST_PHYSICAL_ADDRESS);
>> @@ -5884,32 +5885,32 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu,
>>   		return 1;
>>   	}
>>   
>> -	if (exit_reason >= kvm_vmx_max_exit_handlers)
>> +	if (basic_exit_reason >= kvm_vmx_max_exit_handlers)
>>   		goto unexpected_vmexit;
>>   #ifdef CONFIG_RETPOLINE
>> -	if (exit_reason == EXIT_REASON_MSR_WRITE)
>> +	if (basic_exit_reason == EXIT_REASON_MSR_WRITE)
>>   		return kvm_emulate_wrmsr(vcpu);
>> -	else if (exit_reason == EXIT_REASON_PREEMPTION_TIMER)
>> +	else if (basic_exit_reason == EXIT_REASON_PREEMPTION_TIMER)
>>   		return handle_preemption_timer(vcpu);
>> -	else if (exit_reason == EXIT_REASON_INTERRUPT_WINDOW)
>> +	else if (basic_exit_reason == EXIT_REASON_INTERRUPT_WINDOW)
>>   		return handle_interrupt_window(vcpu);
>> -	else if (exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT)
>> +	else if (basic_exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT)
>>   		return handle_external_interrupt(vcpu);
>> -	else if (exit_reason == EXIT_REASON_HLT)
>> +	else if (basic_exit_reason == EXIT_REASON_HLT)
>>   		return kvm_emulate_halt(vcpu);
>> -	else if (exit_reason == EXIT_REASON_EPT_MISCONFIG)
>> +	else if (basic_exit_reason == EXIT_REASON_EPT_MISCONFIG)
>>   		return handle_ept_misconfig(vcpu);
>>   #endif
>>   
>> -	exit_reason = array_index_nospec(exit_reason,
>> +	basic_exit_reason = array_index_nospec(basic_exit_reason,
>>   					 kvm_vmx_max_exit_handlers);
>> -	if (!kvm_vmx_exit_handlers[exit_reason])
>> +	if (!kvm_vmx_exit_handlers[basic_exit_reason])
>>   		goto unexpected_vmexit;
>>   
>> -	return kvm_vmx_exit_handlers[exit_reason](vcpu);
>> +	return kvm_vmx_exit_handlers[basic_exit_reason](vcpu);
>>   
>>   unexpected_vmexit:
>> -	vcpu_unimpl(vcpu, "vmx: unexpected exit reason 0x%x\n", exit_reason);
>> +	vcpu_unimpl(vcpu, "vmx: unexpected exit reason 0x%x\n", basic_exit_reason);
>>   	dump_vmcs();
>>   	vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
>>   	vcpu->run->internal.suberror =
>> @@ -6241,13 +6242,14 @@ static void vmx_handle_exit_irqoff(struct kvm_vcpu *vcpu,
>>   	enum exit_fastpath_completion *exit_fastpath)
>>   {
>>   	struct vcpu_vmx *vmx = to_vmx(vcpu);
>> +	u16 basic_exit_reason = basic(vmx->exit_reason);
> 
> Here I'd suggest we also use the same
> 
> 'u16 exit_reason = basic_exit_reason(vmx->exit_reason)'
> 
> as above.
> 
>>   
>> -	if (vmx->exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT)
>> +	if (basic_exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT)
>>   		handle_external_interrupt_irqoff(vcpu);
>> -	else if (vmx->exit_reason == EXIT_REASON_EXCEPTION_NMI)
>> +	else if (basic_exit_reason == EXIT_REASON_EXCEPTION_NMI)
>>   		handle_exception_nmi_irqoff(vmx);
>>   	else if (!is_guest_mode(vcpu) &&
>> -		vmx->exit_reason == EXIT_REASON_MSR_WRITE)
>> +		 basic_exit_reason == EXIT_REASON_MSR_WRITE)
>>   		*exit_fastpath = handle_fastpath_set_msr_irqoff(vcpu);
>>   }
>>   
>> @@ -6621,7 +6623,7 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
>>   	vmx->idt_vectoring_info = 0;
>>   
>>   	vmx->exit_reason = vmx->fail ? 0xdead : vmcs_read32(VM_EXIT_REASON);
>> -	if ((u16)vmx->exit_reason == EXIT_REASON_MCE_DURING_VMENTRY)
>> +	if (basic(vmx->exit_reason) == EXIT_REASON_MCE_DURING_VMENTRY)
>>   		kvm_machine_check();
>>   
>>   	if (vmx->fail || (vmx->exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY))
>> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
>> index 7f42cf3dcd70..c6ba33eedb59 100644
>> --- a/arch/x86/kvm/vmx/vmx.h
>> +++ b/arch/x86/kvm/vmx/vmx.h
>> @@ -22,6 +22,8 @@ extern u32 get_umwait_control_msr(void);
>>   
>>   #define X2APIC_MSR(r) (APIC_BASE_MSR + ((r) >> 4))
>>   
>> +#define basic(exit_reason) ((u16)(exit_reason))
>> +
>>   #ifdef CONFIG_X86_64
>>   #define NR_SHARED_MSRS	7
>>   #else
> 

Re: [PATCH 1/2] kvm: vmx: Use basic exit reason to check if it's the specific VM EXIT

[Vitaly Kuznetsov]

Xiaoyao Li <xiaoyao.li@intel.com> writes:

> On 2/24/2020 6:16 PM, Vitaly Kuznetsov wrote:
>> Xiaoyao Li <xiaoyao.li@intel.com> writes:
>> 

...

>>>   		rip = kvm_rip_read(vcpu);
>>>   		rip += vmcs_read32(VM_EXIT_INSTRUCTION_LEN);
>>>   		kvm_rip_write(vcpu, rip);
>>> @@ -5797,6 +5797,7 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu,
>>>   {
>>>   	struct vcpu_vmx *vmx = to_vmx(vcpu);
>>>   	u32 exit_reason = vmx->exit_reason;
>>> +	u16 basic_exit_reason = basic(exit_reason);
>> 
>> I don't think renaming local variable is needed, let's just do
>> 
>> 'u16 exit_reason = basic_exit_reason(vmx->exit_reason)' and keep the
>> rest of the code as-is.
>
> No, we can't do this.
>
> It's not just renaming local variable, the full 32-bit exit reason is 
> used elsewhere in this function that needs the upper 16-bit.
>
> Here variable basic_exit_reason is added for the cases where only basic 
> exit reason number is needed.
>

Can we do the other way around, i.e. introduce 'extended_exit_reason'
and use it where all 32 bits are needed? I'm fine with the change, just
trying to minimize the (unneeded) code churn.

-- 
Vitaly

Re: [PATCH 1/2] kvm: vmx: Use basic exit reason to check if it's the specific VM EXIT

[Sean Christopherson]

On Mon, Feb 24, 2020 at 02:04:46PM +0100, Vitaly Kuznetsov wrote:
> Xiaoyao Li <xiaoyao.li@intel.com> writes:
> 
> > On 2/24/2020 6:16 PM, Vitaly Kuznetsov wrote:
> >> Xiaoyao Li <xiaoyao.li@intel.com> writes:
> >> 
> 
> ...
> 
> >>>   		rip = kvm_rip_read(vcpu);
> >>>   		rip += vmcs_read32(VM_EXIT_INSTRUCTION_LEN);
> >>>   		kvm_rip_write(vcpu, rip);
> >>> @@ -5797,6 +5797,7 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu,
> >>>   {
> >>>   	struct vcpu_vmx *vmx = to_vmx(vcpu);
> >>>   	u32 exit_reason = vmx->exit_reason;
> >>> +	u16 basic_exit_reason = basic(exit_reason);
> >> 
> >> I don't think renaming local variable is needed, let's just do
> >> 
> >> 'u16 exit_reason = basic_exit_reason(vmx->exit_reason)' and keep the
> >> rest of the code as-is.
> >
> > No, we can't do this.
> >
> > It's not just renaming local variable, the full 32-bit exit reason is 
> > used elsewhere in this function that needs the upper 16-bit.
> >
> > Here variable basic_exit_reason is added for the cases where only basic 
> > exit reason number is needed.
> >
> 
> Can we do the other way around, i.e. introduce 'extended_exit_reason'
> and use it where all 32 bits are needed? I'm fine with the change, just
> trying to minimize the (unneeded) code churn.

100% agree.  Even better than adding a second field to vcpu_vmx would be
to make it a union, though we'd probably want to call it something like
full_exit_reason in that case.  That should give us compile-time checks on
exit_reason, e.g. if we try to query one of the upper bits using a u16, e.g.

--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -5818,7 +5818,7 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu,
        if (is_guest_mode(vcpu) && nested_vmx_exit_reflected(vcpu, exit_reason))
                return nested_vmx_reflect_vmexit(vcpu, exit_reason);

-       if (exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) {
+       if (vmx->full_exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) {
                dump_vmcs();
                vcpu->run->exit_reason = KVM_EXIT_FAIL_ENTRY;
                vcpu->run->fail_entry.hardware_entry_failure_reason
@@ -6620,11 +6620,12 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
        vmx->nested.nested_run_pending = 0;
        vmx->idt_vectoring_info = 0;

-       vmx->exit_reason = vmx->fail ? 0xdead : vmcs_read32(VM_EXIT_REASON);
-       if ((u16)vmx->exit_reason == EXIT_REASON_MCE_DURING_VMENTRY)
+       vmx->full_exit_reason = vmx->fail ? 0xdead : vmcs_read32(VM_EXIT_REASON);
+       if (vmx->exit_reason == EXIT_REASON_MCE_DURING_VMENTRY)
                kvm_machine_check();

-       if (vmx->fail || (vmx->exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY))
+       if (vmx->fail ||
+           (vmx->full_exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY))
                return;

        vmx->loaded_vmcs->launched = 1;
diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
index 7f42cf3dcd70..60c09640ea59 100644
--- a/arch/x86/kvm/vmx/vmx.h
+++ b/arch/x86/kvm/vmx/vmx.h
@@ -260,7 +260,10 @@ struct vcpu_vmx {
        int vpid;
        bool emulation_required;

-       u32 exit_reason;
+       union {
+               u16 exit_reason;
+               u32 full_exit_reason;
+       }

        /* Posted interrupt descriptor */
        struct pi_desc pi_desc;





> -- 
> Vitaly
> 

Re: [PATCH 1/2] kvm: vmx: Use basic exit reason to check if it's the specific VM EXIT

[Xiaoyao Li]

On 2/25/2020 12:17 AM, Sean Christopherson wrote:
> On Mon, Feb 24, 2020 at 02:04:46PM +0100, Vitaly Kuznetsov wrote:
>> Xiaoyao Li <xiaoyao.li@intel.com> writes:
>>
>>> On 2/24/2020 6:16 PM, Vitaly Kuznetsov wrote:
>>>> Xiaoyao Li <xiaoyao.li@intel.com> writes:
>>>>
>>
>> ...
>>
>>>>>    		rip = kvm_rip_read(vcpu);
>>>>>    		rip += vmcs_read32(VM_EXIT_INSTRUCTION_LEN);
>>>>>    		kvm_rip_write(vcpu, rip);
>>>>> @@ -5797,6 +5797,7 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu,
>>>>>    {
>>>>>    	struct vcpu_vmx *vmx = to_vmx(vcpu);
>>>>>    	u32 exit_reason = vmx->exit_reason;
>>>>> +	u16 basic_exit_reason = basic(exit_reason);
>>>>
>>>> I don't think renaming local variable is needed, let's just do
>>>>
>>>> 'u16 exit_reason = basic_exit_reason(vmx->exit_reason)' and keep the
>>>> rest of the code as-is.
>>>
>>> No, we can't do this.
>>>
>>> It's not just renaming local variable, the full 32-bit exit reason is
>>> used elsewhere in this function that needs the upper 16-bit.
>>>
>>> Here variable basic_exit_reason is added for the cases where only basic
>>> exit reason number is needed.
>>>
>>
>> Can we do the other way around, i.e. introduce 'extended_exit_reason'
>> and use it where all 32 bits are needed? I'm fine with the change, just
>> trying to minimize the (unneeded) code churn.
> 
> 100% agree.  Even better than adding a second field to vcpu_vmx would be
> to make it a union, though we'd probably want to call it something like
> full_exit_reason in that case.  That should give us compile-time checks on
> exit_reason, e.g. if we try to query one of the upper bits using a u16, e.g.

I have thought about union, but it seems

union {
	u16 exit_reason;
	u32 full_exit_reason;
}

is not a good name. Since there are many codes in vmx.c and nested.c 
assume that exit_reason stands for 32-bit EXIT REASON vmcs field as well 
as evmcs->vm_exit_reason and vmcs12->vm_exit_reason. Do we really want 
to also rename them to full_exit_reason?

Maybe we name it

union {
	u16 basic_exit_reason;
	u32 exit_reason;
}

as what SDM defines?

> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -5818,7 +5818,7 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu,
>          if (is_guest_mode(vcpu) && nested_vmx_exit_reflected(vcpu, exit_reason))
>                  return nested_vmx_reflect_vmexit(vcpu, exit_reason);
> 
> -       if (exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) {
> +       if (vmx->full_exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) {
>                  dump_vmcs();
>                  vcpu->run->exit_reason = KVM_EXIT_FAIL_ENTRY;
>                  vcpu->run->fail_entry.hardware_entry_failure_reason
> @@ -6620,11 +6620,12 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
>          vmx->nested.nested_run_pending = 0;
>          vmx->idt_vectoring_info = 0;
> 
> -       vmx->exit_reason = vmx->fail ? 0xdead : vmcs_read32(VM_EXIT_REASON);
> -       if ((u16)vmx->exit_reason == EXIT_REASON_MCE_DURING_VMENTRY)
> +       vmx->full_exit_reason = vmx->fail ? 0xdead : vmcs_read32(VM_EXIT_REASON);
> +       if (vmx->exit_reason == EXIT_REASON_MCE_DURING_VMENTRY)
>                  kvm_machine_check();
> 
> -       if (vmx->fail || (vmx->exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY))
> +       if (vmx->fail ||
> +           (vmx->full_exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY))
>                  return;
> 
>          vmx->loaded_vmcs->launched = 1;
> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> index 7f42cf3dcd70..60c09640ea59 100644
> --- a/arch/x86/kvm/vmx/vmx.h
> +++ b/arch/x86/kvm/vmx/vmx.h
> @@ -260,7 +260,10 @@ struct vcpu_vmx {
>          int vpid;
>          bool emulation_required;
> 
> -       u32 exit_reason;
> +       union {
> +               u16 exit_reason;
> +               u32 full_exit_reason;
> +       }
> 
>          /* Posted interrupt descriptor */
>          struct pi_desc pi_desc;
> 
> 
> 
> 
> 
>> -- 
>> Vitaly
>>

Re: [PATCH 1/2] kvm: vmx: Use basic exit reason to check if it's the specific VM EXIT

[Krish Sadhukhan]

On 02/24/2020 04:01 AM, Xiaoyao Li wrote:
> On 2/24/2020 6:16 PM, Vitaly Kuznetsov wrote:
>> Xiaoyao Li <xiaoyao.li@intel.com> writes:
>>
>>> Current kvm uses the 32-bit exit reason to check if it's any 
>>> specific VM
>>> EXIT, however only the low 16-bit of VM EXIT REASON acts as the basic
>>> exit reason.
>>>
>>> Introduce Macro basic(exit_reaso)
>>
>> "exit_reason"
>
> Ah, will correct it in v2.
>
>>>   to help retrieve the basic exit reason
>>> from VM EXIT REASON, and use the basic exit reason for checking and
>>> indexing the exit hanlder.
>>>
>>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>>> ---
>>>   arch/x86/kvm/vmx/vmx.c | 44 
>>> ++++++++++++++++++++++--------------------
>>>   arch/x86/kvm/vmx/vmx.h |  2 ++
>>>   2 files changed, 25 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>>> index 9a6664886f2e..85da72d4dc92 100644
>>> --- a/arch/x86/kvm/vmx/vmx.c
>>> +++ b/arch/x86/kvm/vmx/vmx.c
>>> @@ -1584,7 +1584,7 @@ static int skip_emulated_instruction(struct 
>>> kvm_vcpu *vcpu)
>>>        * i.e. we end up advancing IP with some random value.
>>>        */
>>>       if (!static_cpu_has(X86_FEATURE_HYPERVISOR) ||
>>> -        to_vmx(vcpu)->exit_reason != EXIT_REASON_EPT_MISCONFIG) {
>>> +        basic(to_vmx(vcpu)->exit_reason) != 
>>> EXIT_REASON_EPT_MISCONFIG) {
>>
>> "basic" word is probably 'too basic' to be used for this purpose. Even
>> if we need a macro for it (I'm not really convinced it improves the
>> readability), I'd suggest we name it 'basic_exit_reason()' instead.
>
> Agreed.
>
>>>           rip = kvm_rip_read(vcpu);
>>>           rip += vmcs_read32(VM_EXIT_INSTRUCTION_LEN);
>>>           kvm_rip_write(vcpu, rip);
>>> @@ -5797,6 +5797,7 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu,
>>>   {
>>>       struct vcpu_vmx *vmx = to_vmx(vcpu);
>>>       u32 exit_reason = vmx->exit_reason;
>>> +    u16 basic_exit_reason = basic(exit_reason);
>>
>> I don't think renaming local variable is needed, let's just do
>>
>> 'u16 exit_reason = basic_exit_reason(vmx->exit_reason)' and keep the
>> rest of the code as-is.
>
> No, we can't do this.
>
> It's not just renaming local variable, the full 32-bit exit reason is 
> used elsewhere in this function that needs the upper 16-bit.
>
> Here variable basic_exit_reason is added for the cases where only 
> basic exit reason number is needed.
>
>>>       u32 vectoring_info = vmx->idt_vectoring_info;
>>>         trace_kvm_exit(exit_reason, vcpu, KVM_ISA_VMX);
>>> @@ -5842,17 +5843,17 @@ static int vmx_handle_exit(struct kvm_vcpu 
>>> *vcpu,
>>>        * will cause infinite loop.
>>>        */
>>>       if ((vectoring_info & VECTORING_INFO_VALID_MASK) &&
>>> -            (exit_reason != EXIT_REASON_EXCEPTION_NMI &&
>>> -            exit_reason != EXIT_REASON_EPT_VIOLATION &&
>>> -            exit_reason != EXIT_REASON_PML_FULL &&
>>> -            exit_reason != EXIT_REASON_TASK_SWITCH)) {
>>> +            (basic_exit_reason != EXIT_REASON_EXCEPTION_NMI &&
>>> +             basic_exit_reason != EXIT_REASON_EPT_VIOLATION &&
>>> +             basic_exit_reason != EXIT_REASON_PML_FULL &&
>>> +             basic_exit_reason != EXIT_REASON_TASK_SWITCH)) {
>>>           vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
>>>           vcpu->run->internal.suberror = 
>>> KVM_INTERNAL_ERROR_DELIVERY_EV;
>>>           vcpu->run->internal.ndata = 3;
>>>           vcpu->run->internal.data[0] = vectoring_info;
>>>           vcpu->run->internal.data[1] = exit_reason;
>>>           vcpu->run->internal.data[2] = vcpu->arch.exit_qualification;
>>> -        if (exit_reason == EXIT_REASON_EPT_MISCONFIG) {
>>> +        if (basic_exit_reason == EXIT_REASON_EPT_MISCONFIG) {
>>>               vcpu->run->internal.ndata++;
>>>               vcpu->run->internal.data[3] =
>>>                   vmcs_read64(GUEST_PHYSICAL_ADDRESS);
>>> @@ -5884,32 +5885,32 @@ static int vmx_handle_exit(struct kvm_vcpu 
>>> *vcpu,
>>>           return 1;
>>>       }
>>>   -    if (exit_reason >= kvm_vmx_max_exit_handlers)
>>> +    if (basic_exit_reason >= kvm_vmx_max_exit_handlers)
>>>           goto unexpected_vmexit;
>>>   #ifdef CONFIG_RETPOLINE
>>> -    if (exit_reason == EXIT_REASON_MSR_WRITE)
>>> +    if (basic_exit_reason == EXIT_REASON_MSR_WRITE)
>>>           return kvm_emulate_wrmsr(vcpu);
>>> -    else if (exit_reason == EXIT_REASON_PREEMPTION_TIMER)
>>> +    else if (basic_exit_reason == EXIT_REASON_PREEMPTION_TIMER)
>>>           return handle_preemption_timer(vcpu);
>>> -    else if (exit_reason == EXIT_REASON_INTERRUPT_WINDOW)
>>> +    else if (basic_exit_reason == EXIT_REASON_INTERRUPT_WINDOW)
>>>           return handle_interrupt_window(vcpu);
>>> -    else if (exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT)
>>> +    else if (basic_exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT)
>>>           return handle_external_interrupt(vcpu);
>>> -    else if (exit_reason == EXIT_REASON_HLT)
>>> +    else if (basic_exit_reason == EXIT_REASON_HLT)
>>>           return kvm_emulate_halt(vcpu);
>>> -    else if (exit_reason == EXIT_REASON_EPT_MISCONFIG)
>>> +    else if (basic_exit_reason == EXIT_REASON_EPT_MISCONFIG)
>>>           return handle_ept_misconfig(vcpu);
>>>   #endif
>>>   -    exit_reason = array_index_nospec(exit_reason,
>>> +    basic_exit_reason = array_index_nospec(basic_exit_reason,
>>>                        kvm_vmx_max_exit_handlers);
>>> -    if (!kvm_vmx_exit_handlers[exit_reason])
>>> +    if (!kvm_vmx_exit_handlers[basic_exit_reason])
>>>           goto unexpected_vmexit;
>>>   -    return kvm_vmx_exit_handlers[exit_reason](vcpu);
>>> +    return kvm_vmx_exit_handlers[basic_exit_reason](vcpu);
>>>     unexpected_vmexit:
>>> -    vcpu_unimpl(vcpu, "vmx: unexpected exit reason 0x%x\n", 
>>> exit_reason);
>>> +    vcpu_unimpl(vcpu, "vmx: unexpected exit reason 0x%x\n", 
>>> basic_exit_reason);
>>>       dump_vmcs();
>>>       vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
>>>       vcpu->run->internal.suberror =
>>> @@ -6241,13 +6242,14 @@ static void vmx_handle_exit_irqoff(struct 
>>> kvm_vcpu *vcpu,
>>>       enum exit_fastpath_completion *exit_fastpath)
>>>   {
>>>       struct vcpu_vmx *vmx = to_vmx(vcpu);
>>> +    u16 basic_exit_reason = basic(vmx->exit_reason);
>>
>> Here I'd suggest we also use the same
>>
>> 'u16 exit_reason = basic_exit_reason(vmx->exit_reason)'
>>
>> as above.
>>
>>>   -    if (vmx->exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT)
>>> +    if (basic_exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT)
>>>           handle_external_interrupt_irqoff(vcpu);
>>> -    else if (vmx->exit_reason == EXIT_REASON_EXCEPTION_NMI)
>>> +    else if (basic_exit_reason == EXIT_REASON_EXCEPTION_NMI)
>>>           handle_exception_nmi_irqoff(vmx);
>>>       else if (!is_guest_mode(vcpu) &&
>>> -        vmx->exit_reason == EXIT_REASON_MSR_WRITE)
>>> +         basic_exit_reason == EXIT_REASON_MSR_WRITE)
>>>           *exit_fastpath = handle_fastpath_set_msr_irqoff(vcpu);
>>>   }
>>>   @@ -6621,7 +6623,7 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
>>>       vmx->idt_vectoring_info = 0;
>>>         vmx->exit_reason = vmx->fail ? 0xdead : 
>>> vmcs_read32(VM_EXIT_REASON);
>>> -    if ((u16)vmx->exit_reason == EXIT_REASON_MCE_DURING_VMENTRY)
>>> +    if (basic(vmx->exit_reason) == EXIT_REASON_MCE_DURING_VMENTRY)
>>>           kvm_machine_check();
>>>         if (vmx->fail || (vmx->exit_reason & 
>>> VMX_EXIT_REASONS_FAILED_VMENTRY))
>>> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
>>> index 7f42cf3dcd70..c6ba33eedb59 100644
>>> --- a/arch/x86/kvm/vmx/vmx.h
>>> +++ b/arch/x86/kvm/vmx/vmx.h
>>> @@ -22,6 +22,8 @@ extern u32 get_umwait_control_msr(void);
>>>     #define X2APIC_MSR(r) (APIC_BASE_MSR + ((r) >> 4))
>>>   +#define basic(exit_reason) ((u16)(exit_reason))

We have a macro for bit 31,

     VMX_EXIT_REASONS_FAILED_VMENTRY                0x80000000


Does it make sense to define a macro like that instead ? Say,

     VMX_BASIC_EXIT_REASON        0x0000ffff

and then we do,

     u32 exit_reason = vmx->exit_reason;
     u16 basic_exit_reason = exit_reason & VMX_BASIC_EXIT_REASON;


>>> +
>>>   #ifdef CONFIG_X86_64
>>>   #define NR_SHARED_MSRS    7
>>>   #else
>>
>

Re: [PATCH 1/2] kvm: vmx: Use basic exit reason to check if it's the specific VM EXIT

[Sean Christopherson]

On Tue, Feb 25, 2020 at 08:13:15AM +0800, Xiaoyao Li wrote:
> On 2/25/2020 12:17 AM, Sean Christopherson wrote:
> >On Mon, Feb 24, 2020 at 02:04:46PM +0100, Vitaly Kuznetsov wrote:
> >>Xiaoyao Li <xiaoyao.li@intel.com> writes:
> >>
> >>>On 2/24/2020 6:16 PM, Vitaly Kuznetsov wrote:
> >>>>Xiaoyao Li <xiaoyao.li@intel.com> writes:
> >>>>
> >>>Here variable basic_exit_reason is added for the cases where only basic
> >>>exit reason number is needed.
> >>>
> >>
> >>Can we do the other way around, i.e. introduce 'extended_exit_reason'
> >>and use it where all 32 bits are needed? I'm fine with the change, just
> >>trying to minimize the (unneeded) code churn.
> >
> >100% agree.  Even better than adding a second field to vcpu_vmx would be
> >to make it a union, though we'd probably want to call it something like
> >full_exit_reason in that case.  That should give us compile-time checks on
> >exit_reason, e.g. if we try to query one of the upper bits using a u16, e.g.
> 
> I have thought about union, but it seems
> 
> union {
> 	u16 exit_reason;
> 	u32 full_exit_reason;
> }
> 
> is not a good name. Since there are many codes in vmx.c and nested.c assume
> that exit_reason stands for 32-bit EXIT REASON vmcs field as well as
> evmcs->vm_exit_reason and vmcs12->vm_exit_reason. Do we really want to also
> rename them to full_exit_reason?

It's actually the opposite, almost all of the VMX code assumes exit_reason
holds only the basic exit reason, i.e. a 16-bit value.  For example, SGX
adds a modifier flag to denote a VM-Exit was from enclave mode, and that
bit needs to be stripped from exit_reason, otherwise all the checks like
"if (exit_reason == blah_blah_blah)" fail.  

Making exit_reason a 16-bit alias of the full/extended exit_reason neatly
sidesteps that issue.  And it is an issue that has caused actual problems
in the past, e.g. see commit beb8d93b3e42 ("KVM: VMX: Fix handling of #MC
that occurs during VM-Entry").  Coincidentally, that commit also removes a
local "u16 basic_exit_reason" :-).

Except for one mistake, the pseudo-patch below is the entirety of required
changes.  Most (all?) of the functions that take "u32 exit_reason" can (and
should) continue to take a u32.

As for the name, I strongly prefer keeping the exit_reason name for the
basic exit reason.  The vast majority of VM-Exits do not have modifiers
set, i.e. "basic exit reason" == vmcs.EXIT_REASON for nearly all normal
usage.  This holds true in every form of communication, e.g. when discussing
VM-Exit reasons, it's never qualified with "basic", it's simply the exit
reason.  IMO the code is better off following the colloquial usage of "exit
reason".  A simple comment above the union would suffice to clear up any
confusion with respect to the SDM.

> Maybe we name it
> 
> union {
> 	u16 basic_exit_reason;
> 	u32 exit_reason;
> }
> 
> as what SDM defines?
> 
> >--- a/arch/x86/kvm/vmx/vmx.c
> >+++ b/arch/x86/kvm/vmx/vmx.c
> >@@ -5818,7 +5818,7 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu,
> >         if (is_guest_mode(vcpu) && nested_vmx_exit_reflected(vcpu, exit_reason))
> >                 return nested_vmx_reflect_vmexit(vcpu, exit_reason);
> >
> >-       if (exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) {
> >+       if (vmx->full_exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) {

If we do go the union route, this snippet of code is insufficient, the
full/extended exit reason needs to be snapshotted early for use in the
tracepoint and in fail_entry.hardware_entry_failure_reason.

> >                 dump_vmcs();
> >                 vcpu->run->exit_reason = KVM_EXIT_FAIL_ENTRY;
> >                 vcpu->run->fail_entry.hardware_entry_failure_reason
> >@@ -6620,11 +6620,12 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
> >         vmx->nested.nested_run_pending = 0;
> >         vmx->idt_vectoring_info = 0;
> >
> >-       vmx->exit_reason = vmx->fail ? 0xdead : vmcs_read32(VM_EXIT_REASON);
> >-       if ((u16)vmx->exit_reason == EXIT_REASON_MCE_DURING_VMENTRY)
> >+       vmx->full_exit_reason = vmx->fail ? 0xdead : vmcs_read32(VM_EXIT_REASON);
> >+       if (vmx->exit_reason == EXIT_REASON_MCE_DURING_VMENTRY)
> >                 kvm_machine_check();
> >
> >-       if (vmx->fail || (vmx->exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY))
> >+       if (vmx->fail ||
> >+           (vmx->full_exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY))
> >                 return;
> >
> >         vmx->loaded_vmcs->launched = 1;
> >diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> >index 7f42cf3dcd70..60c09640ea59 100644
> >--- a/arch/x86/kvm/vmx/vmx.h
> >+++ b/arch/x86/kvm/vmx/vmx.h
> >@@ -260,7 +260,10 @@ struct vcpu_vmx {
> >         int vpid;
> >         bool emulation_required;
> >
> >-       u32 exit_reason;
> >+       union {
> >+               u16 exit_reason;
> >+               u32 full_exit_reason;
> >+       }
> >
> >         /* Posted interrupt descriptor */
> >         struct pi_desc pi_desc;
> >
> >
> >
> >
> >
> >>-- 
> >>Vitaly
> >>
> 

Re: [PATCH 1/2] kvm: vmx: Use basic exit reason to check if it's the specific VM EXIT

[Xiaoyao Li]

On 2/25/2020 2:13 PM, Sean Christopherson wrote:
> On Tue, Feb 25, 2020 at 08:13:15AM +0800, Xiaoyao Li wrote:
>> On 2/25/2020 12:17 AM, Sean Christopherson wrote:
>>> On Mon, Feb 24, 2020 at 02:04:46PM +0100, Vitaly Kuznetsov wrote:
>>>> Xiaoyao Li <xiaoyao.li@intel.com> writes:
>>>>
>>>>> On 2/24/2020 6:16 PM, Vitaly Kuznetsov wrote:
>>>>>> Xiaoyao Li <xiaoyao.li@intel.com> writes:
>>>>>>
>>>>> Here variable basic_exit_reason is added for the cases where only basic
>>>>> exit reason number is needed.
>>>>>
>>>>
>>>> Can we do the other way around, i.e. introduce 'extended_exit_reason'
>>>> and use it where all 32 bits are needed? I'm fine with the change, just
>>>> trying to minimize the (unneeded) code churn.
>>>
>>> 100% agree.  Even better than adding a second field to vcpu_vmx would be
>>> to make it a union, though we'd probably want to call it something like
>>> full_exit_reason in that case.  That should give us compile-time checks on
>>> exit_reason, e.g. if we try to query one of the upper bits using a u16, e.g.
>>
>> I have thought about union, but it seems
>>
>> union {
>> 	u16 exit_reason;
>> 	u32 full_exit_reason;
>> }
>>
>> is not a good name. Since there are many codes in vmx.c and nested.c assume
>> that exit_reason stands for 32-bit EXIT REASON vmcs field as well as
>> evmcs->vm_exit_reason and vmcs12->vm_exit_reason. Do we really want to also
>> rename them to full_exit_reason?
> 
> It's actually the opposite, almost all of the VMX code assumes exit_reason
> holds only the basic exit reason, i.e. a 16-bit value.  For example, SGX
> adds a modifier flag to denote a VM-Exit was from enclave mode, and that
> bit needs to be stripped from exit_reason, otherwise all the checks like
> "if (exit_reason == blah_blah_blah)" fail.
> 
> Making exit_reason a 16-bit alias of the full/extended exit_reason neatly
> sidesteps that issue.  And it is an issue that has caused actual problems
> in the past, e.g. see commit beb8d93b3e42 ("KVM: VMX: Fix handling of #MC
> that occurs during VM-Entry").  Coincidentally, that commit also removes a
> local "u16 basic_exit_reason" :-).
> 
> Except for one mistake, the pseudo-patch below is the entirety of required
> changes.  Most (all?) of the functions that take "u32 exit_reason" can (and
> should) continue to take a u32.
> 
> As for the name, I strongly prefer keeping the exit_reason name for the
> basic exit reason.  The vast majority of VM-Exits do not have modifiers
> set, i.e. "basic exit reason" == vmcs.EXIT_REASON for nearly all normal
> usage.  This holds true in every form of communication, e.g. when discussing
> VM-Exit reasons, it's never qualified with "basic", it's simply the exit
> reason.  IMO the code is better off following the colloquial usage of "exit
> reason".  A simple comment above the union would suffice to clear up any
> confusion with respect to the SDM.

Well, for this reason we can keep exit_reason for 16-bit usage, and 
define full/extended_exit_reason for 32-bit cases. This makes less code 
churn.

But after we choose to use exit_reason and full/extended_exit_reason, 
what if someday new modifier flags are added and we want to enable some 
modifier flags for nested case?
I guess we need to change existing exit_reason to 
full/extended_exit_reason in nested.c/nested.h to keep the naming rule 
consistent.

>> Maybe we name it
>>
>> union {
>> 	u16 basic_exit_reason;
>> 	u32 exit_reason;
>> }
>>
>> as what SDM defines?
>>
>>> --- a/arch/x86/kvm/vmx/vmx.c
>>> +++ b/arch/x86/kvm/vmx/vmx.c
>>> @@ -5818,7 +5818,7 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu,
>>>          if (is_guest_mode(vcpu) && nested_vmx_exit_reflected(vcpu, exit_reason))
>>>                  return nested_vmx_reflect_vmexit(vcpu, exit_reason);
>>>
>>> -       if (exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) {
>>> +       if (vmx->full_exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) {
> 
> If we do go the union route, this snippet of code is insufficient, the
> full/extended exit reason needs to be snapshotted early for use in the
> tracepoint and in fail_entry.hardware_entry_failure_reason.
> 
>>>                  dump_vmcs();
>>>                  vcpu->run->exit_reason = KVM_EXIT_FAIL_ENTRY;
>>>                  vcpu->run->fail_entry.hardware_entry_failure_reason
>>> @@ -6620,11 +6620,12 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
>>>          vmx->nested.nested_run_pending = 0;
>>>          vmx->idt_vectoring_info = 0;
>>>
>>> -       vmx->exit_reason = vmx->fail ? 0xdead : vmcs_read32(VM_EXIT_REASON);
>>> -       if ((u16)vmx->exit_reason == EXIT_REASON_MCE_DURING_VMENTRY)
>>> +       vmx->full_exit_reason = vmx->fail ? 0xdead : vmcs_read32(VM_EXIT_REASON);
>>> +       if (vmx->exit_reason == EXIT_REASON_MCE_DURING_VMENTRY)
>>>                  kvm_machine_check();
>>>
>>> -       if (vmx->fail || (vmx->exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY))
>>> +       if (vmx->fail ||
>>> +           (vmx->full_exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY))
>>>                  return;
>>>
>>>          vmx->loaded_vmcs->launched = 1;
>>> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
>>> index 7f42cf3dcd70..60c09640ea59 100644
>>> --- a/arch/x86/kvm/vmx/vmx.h
>>> +++ b/arch/x86/kvm/vmx/vmx.h
>>> @@ -260,7 +260,10 @@ struct vcpu_vmx {
>>>          int vpid;
>>>          bool emulation_required;
>>>
>>> -       u32 exit_reason;
>>> +       union {
>>> +               u16 exit_reason;
>>> +               u32 full_exit_reason;
>>> +       }
>>>
>>>          /* Posted interrupt descriptor */
>>>          struct pi_desc pi_desc;
>>>
>>>
>>>
>>>
>>>
>>>> -- 
>>>> Vitaly
>>>>
>>

Re: [PATCH 1/2] kvm: vmx: Use basic exit reason to check if it's the specific VM EXIT

[Vitaly Kuznetsov]

Krish Sadhukhan <krish.sadhukhan@oracle.com> writes:

>
> We have a macro for bit 31,
>
>      VMX_EXIT_REASONS_FAILED_VMENTRY                0x80000000
>
>
> Does it make sense to define a macro like that instead ? Say,
>
>      VMX_BASIC_EXIT_REASON        0x0000ffff
>

0xffffU ?

> and then we do,
>
>      u32 exit_reason = vmx->exit_reason;
>      u16 basic_exit_reason = exit_reason & VMX_BASIC_EXIT_REASON;
>

Just a naming suggestion: if we decide to go down this road, let's name
it e.g. VMX_BASIC_EXIT_REASON_MASK to make it clear this is *not* an
exit reason.  

-- 
Vitaly

Re: [PATCH 1/2] kvm: vmx: Use basic exit reason to check if it's the specific VM EXIT

[Krish Sadhukhan]

On 2/25/20 5:11 AM, Vitaly Kuznetsov wrote:
> Krish Sadhukhan <krish.sadhukhan@oracle.com> writes:
>
>> We have a macro for bit 31,
>>
>>       VMX_EXIT_REASONS_FAILED_VMENTRY                0x80000000
>>
>>
>> Does it make sense to define a macro like that instead ? Say,
>>
>>       VMX_BASIC_EXIT_REASON        0x0000ffff
>>
> 0xffffU ?
>
>> and then we do,
>>
>>       u32 exit_reason = vmx->exit_reason;
>>       u16 basic_exit_reason = exit_reason & VMX_BASIC_EXIT_REASON;
>>
> Just a naming suggestion: if we decide to go down this road, let's name
> it e.g. VMX_BASIC_EXIT_REASON_MASK to make it clear this is *not* an
> exit reason.
>

VMX_BASIC_EXIT_REASON_MASK  works.

Re: [PATCH 1/2] kvm: vmx: Use basic exit reason to check if it's the specific VM EXIT

[Sean Christopherson]

On Tue, Feb 25, 2020 at 02:41:20PM +0800, Xiaoyao Li wrote:
> On 2/25/2020 2:13 PM, Sean Christopherson wrote:
> >On Tue, Feb 25, 2020 at 08:13:15AM +0800, Xiaoyao Li wrote:
> >>On 2/25/2020 12:17 AM, Sean Christopherson wrote:
> >>I have thought about union, but it seems
> >>
> >>union {
> >>	u16 exit_reason;
> >>	u32 full_exit_reason;
> >>}
> >>
> >>is not a good name. Since there are many codes in vmx.c and nested.c assume
> >>that exit_reason stands for 32-bit EXIT REASON vmcs field as well as
> >>evmcs->vm_exit_reason and vmcs12->vm_exit_reason. Do we really want to also
> >>rename them to full_exit_reason?
> >
> >It's actually the opposite, almost all of the VMX code assumes exit_reason
> >holds only the basic exit reason, i.e. a 16-bit value.  For example, SGX
> >adds a modifier flag to denote a VM-Exit was from enclave mode, and that
> >bit needs to be stripped from exit_reason, otherwise all the checks like
> >"if (exit_reason == blah_blah_blah)" fail.
> >
> >Making exit_reason a 16-bit alias of the full/extended exit_reason neatly
> >sidesteps that issue.  And it is an issue that has caused actual problems
> >in the past, e.g. see commit beb8d93b3e42 ("KVM: VMX: Fix handling of #MC
> >that occurs during VM-Entry").  Coincidentally, that commit also removes a
> >local "u16 basic_exit_reason" :-).
> >
> >Except for one mistake, the pseudo-patch below is the entirety of required
> >changes.  Most (all?) of the functions that take "u32 exit_reason" can (and
> >should) continue to take a u32.
> >
> >As for the name, I strongly prefer keeping the exit_reason name for the
> >basic exit reason.  The vast majority of VM-Exits do not have modifiers
> >set, i.e. "basic exit reason" == vmcs.EXIT_REASON for nearly all normal
> >usage.  This holds true in every form of communication, e.g. when discussing
> >VM-Exit reasons, it's never qualified with "basic", it's simply the exit
> >reason.  IMO the code is better off following the colloquial usage of "exit
> >reason".  A simple comment above the union would suffice to clear up any
> >confusion with respect to the SDM.
> 
> Well, for this reason we can keep exit_reason for 16-bit usage, and define
> full/extended_exit_reason for 32-bit cases. This makes less code churn.
> 
> But after we choose to use exit_reason and full/extended_exit_reason, what
> if someday new modifier flags are added and we want to enable some modifier
> flags for nested case?
> I guess we need to change existing exit_reason to full/extended_exit_reason
> in nested.c/nested.h to keep the naming rule consistent.

Ah, good point.  But, that's just another bug in my psuedo patch :-)
It's literally one call site that needs to be updated.  E.g.

	if (is_guest_mode(vcpu) && nested_vmx_exit_reflected(vcpu, exit_reason))
		return nested_vmx_reflect_vmexit(vcpu, full_exit_reason);

Everywhere else KVM calls nested_vmx_reflect_vmexit() is (currently) done
with a hardcoded value (except handle_vmfunc(), but I actually want to
change that one).

Re: [PATCH 1/2] kvm: vmx: Use basic exit reason to check if it's the specific VM EXIT

[Xiaoyao Li]

On 2/27/2020 7:59 AM, Sean Christopherson wrote:
> On Tue, Feb 25, 2020 at 02:41:20PM +0800, Xiaoyao Li wrote:
>> On 2/25/2020 2:13 PM, Sean Christopherson wrote:
>>> On Tue, Feb 25, 2020 at 08:13:15AM +0800, Xiaoyao Li wrote:
>>>> On 2/25/2020 12:17 AM, Sean Christopherson wrote:
>>>> I have thought about union, but it seems
>>>>
>>>> union {
>>>> 	u16 exit_reason;
>>>> 	u32 full_exit_reason;
>>>> }
>>>>
>>>> is not a good name. Since there are many codes in vmx.c and nested.c assume
>>>> that exit_reason stands for 32-bit EXIT REASON vmcs field as well as
>>>> evmcs->vm_exit_reason and vmcs12->vm_exit_reason. Do we really want to also
>>>> rename them to full_exit_reason?
>>>
>>> It's actually the opposite, almost all of the VMX code assumes exit_reason
>>> holds only the basic exit reason, i.e. a 16-bit value.  For example, SGX
>>> adds a modifier flag to denote a VM-Exit was from enclave mode, and that
>>> bit needs to be stripped from exit_reason, otherwise all the checks like
>>> "if (exit_reason == blah_blah_blah)" fail.
>>>
>>> Making exit_reason a 16-bit alias of the full/extended exit_reason neatly
>>> sidesteps that issue.  And it is an issue that has caused actual problems
>>> in the past, e.g. see commit beb8d93b3e42 ("KVM: VMX: Fix handling of #MC
>>> that occurs during VM-Entry").  Coincidentally, that commit also removes a
>>> local "u16 basic_exit_reason" :-).
>>>
>>> Except for one mistake, the pseudo-patch below is the entirety of required
>>> changes.  Most (all?) of the functions that take "u32 exit_reason" can (and
>>> should) continue to take a u32.
>>>
>>> As for the name, I strongly prefer keeping the exit_reason name for the
>>> basic exit reason.  The vast majority of VM-Exits do not have modifiers
>>> set, i.e. "basic exit reason" == vmcs.EXIT_REASON for nearly all normal
>>> usage.  This holds true in every form of communication, e.g. when discussing
>>> VM-Exit reasons, it's never qualified with "basic", it's simply the exit
>>> reason.  IMO the code is better off following the colloquial usage of "exit
>>> reason".  A simple comment above the union would suffice to clear up any
>>> confusion with respect to the SDM.
>>
>> Well, for this reason we can keep exit_reason for 16-bit usage, and define
>> full/extended_exit_reason for 32-bit cases. This makes less code churn.
>>
>> But after we choose to use exit_reason and full/extended_exit_reason, what
>> if someday new modifier flags are added and we want to enable some modifier
>> flags for nested case?
>> I guess we need to change existing exit_reason to full/extended_exit_reason
>> in nested.c/nested.h to keep the naming rule consistent.
> 
> Ah, good point.  But, that's just another bug in my psuedo patch :-)
> It's literally one call site that needs to be updated.  E.g.
> 
> 	if (is_guest_mode(vcpu) && nested_vmx_exit_reflected(vcpu, exit_reason))
> 		return nested_vmx_reflect_vmexit(vcpu, full_exit_reason);
> 

shouldn't we also pass full_exit_reason to nested_vmx_exit_reflected()?

> Everywhere else KVM calls nested_vmx_reflect_vmexit() is (currently) done

I guess you wanted to say nested_vmx_vmexit() not 
nested_vmx_reflect_vmexit() here.

> with a hardcoded value (except handle_vmfunc(), but I actually want to
> change that one).
> 

Re: [PATCH 1/2] kvm: vmx: Use basic exit reason to check if it's the specific VM EXIT

[Sean Christopherson]

On Thu, Feb 27, 2020 at 04:35:20PM +0800, Xiaoyao Li wrote:
> On 2/27/2020 7:59 AM, Sean Christopherson wrote:
> >Ah, good point.  But, that's just another bug in my psuedo patch :-)
> >It's literally one call site that needs to be updated.  E.g.
> >
> >	if (is_guest_mode(vcpu) && nested_vmx_exit_reflected(vcpu, exit_reason))
> >		return nested_vmx_reflect_vmexit(vcpu, full_exit_reason);
> >
> 
> shouldn't we also pass full_exit_reason to nested_vmx_exit_reflected()?

Yep, see the patch I sent.  Alternatively, and perhaps a better approach
once we have the union, would be to not pass exit_reason at all and instead
have nested_vmx_exit_reflected() grab it directly from vmx->...

> 
> >Everywhere else KVM calls nested_vmx_reflect_vmexit() is (currently) done
> 
> I guess you wanted to say nested_vmx_vmexit() not
> nested_vmx_reflect_vmexit() here.

Ya.
 
> >with a hardcoded value (except handle_vmfunc(), but I actually want to
> >change that one).
> >
>