[PATCH] KVM: x86: dump_vmcs should not assume GUEST_IA32_EFER is valid

[PATCH] KVM: x86: dump_vmcs should not assume GUEST_IA32_EFER is valid

[David Edmondson]

When dumping the VMCS, retrieve the current guest value of EFER from
the kvm_vcpu structure if neither VM_EXIT_SAVE_IA32_EFER or
VM_ENTRY_LOAD_IA32_EFER is set, which can occur if the processor does
not support the relevant VM-exit/entry controls.

Fixes: 4eb64dce8d0a ("KVM: x86: dump VMCS on invalid entry")
Signed-off-by: David Edmondson <david.edmondson@oracle.com>
---
 arch/x86/kvm/vmx/vmx.c | 14 +++++++++-----
 arch/x86/kvm/vmx/vmx.h |  2 +-
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index eb69fef57485..74ea4fe6f35e 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -5754,7 +5754,7 @@ static void vmx_dump_dtsel(char *name, uint32_t limit)
 	       vmcs_readl(limit + GUEST_GDTR_BASE - GUEST_GDTR_LIMIT));
 }
 
-void dump_vmcs(void)
+void dump_vmcs(struct kvm_vcpu *vcpu)
 {
 	u32 vmentry_ctl, vmexit_ctl;
 	u32 cpu_based_exec_ctrl, pin_based_exec_ctrl, secondary_exec_control;
@@ -5771,7 +5771,11 @@ void dump_vmcs(void)
 	cpu_based_exec_ctrl = vmcs_read32(CPU_BASED_VM_EXEC_CONTROL);
 	pin_based_exec_ctrl = vmcs_read32(PIN_BASED_VM_EXEC_CONTROL);
 	cr4 = vmcs_readl(GUEST_CR4);
-	efer = vmcs_read64(GUEST_IA32_EFER);
+	if ((vmexit_ctl & VM_EXIT_SAVE_IA32_EFER) ||
+	    (vmentry_ctl & VM_ENTRY_LOAD_IA32_EFER))
+		efer = vmcs_read64(GUEST_IA32_EFER);
+	else
+		efer = vcpu->arch.efer;
 	secondary_exec_control = 0;
 	if (cpu_has_secondary_exec_ctrls())
 		secondary_exec_control = vmcs_read32(SECONDARY_VM_EXEC_CONTROL);
@@ -5955,7 +5959,7 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
 	}
 
 	if (exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) {
-		dump_vmcs();
+		dump_vmcs(vcpu);
 		vcpu->run->exit_reason = KVM_EXIT_FAIL_ENTRY;
 		vcpu->run->fail_entry.hardware_entry_failure_reason
 			= exit_reason;
@@ -5964,7 +5968,7 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
 	}
 
 	if (unlikely(vmx->fail)) {
-		dump_vmcs();
+		dump_vmcs(vcpu);
 		vcpu->run->exit_reason = KVM_EXIT_FAIL_ENTRY;
 		vcpu->run->fail_entry.hardware_entry_failure_reason
 			= vmcs_read32(VM_INSTRUCTION_ERROR);
@@ -6049,7 +6053,7 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
 
 unexpected_vmexit:
 	vcpu_unimpl(vcpu, "vmx: unexpected exit reason 0x%x\n", exit_reason);
-	dump_vmcs();
+	dump_vmcs(vcpu);
 	vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
 	vcpu->run->internal.suberror =
 			KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON;
diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
index 9d3a557949ac..f8a0ce74798e 100644
--- a/arch/x86/kvm/vmx/vmx.h
+++ b/arch/x86/kvm/vmx/vmx.h
@@ -489,6 +489,6 @@ static inline bool vmx_guest_state_valid(struct kvm_vcpu *vcpu)
 	return is_unrestricted_guest(vcpu) || __vmx_guest_state_valid(vcpu);
 }
 
-void dump_vmcs(void);
+void dump_vmcs(struct kvm_vcpu *vcpu);
 
 #endif /* __KVM_X86_VMX_H */
-- 
2.30.0

Re: [PATCH] KVM: x86: dump_vmcs should not assume GUEST_IA32_EFER is valid

[Paolo Bonzini]

On 18/02/21 11:04, David Edmondson wrote:
> When dumping the VMCS, retrieve the current guest value of EFER from
> the kvm_vcpu structure if neither VM_EXIT_SAVE_IA32_EFER or
> VM_ENTRY_LOAD_IA32_EFER is set, which can occur if the processor does
> not support the relevant VM-exit/entry controls.

Printing vcpu->arch.efer is not the best choice however.  Could we dump 
the whole MSR load/store area instead?

Paolo

> Fixes: 4eb64dce8d0a ("KVM: x86: dump VMCS on invalid entry")
> Signed-off-by: David Edmondson <david.edmondson@oracle.com>
> ---
>   arch/x86/kvm/vmx/vmx.c | 14 +++++++++-----
>   arch/x86/kvm/vmx/vmx.h |  2 +-
>   2 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index eb69fef57485..74ea4fe6f35e 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -5754,7 +5754,7 @@ static void vmx_dump_dtsel(char *name, uint32_t limit)
>   	       vmcs_readl(limit + GUEST_GDTR_BASE - GUEST_GDTR_LIMIT));
>   }
>   
> -void dump_vmcs(void)
> +void dump_vmcs(struct kvm_vcpu *vcpu)
>   {
>   	u32 vmentry_ctl, vmexit_ctl;
>   	u32 cpu_based_exec_ctrl, pin_based_exec_ctrl, secondary_exec_control;
> @@ -5771,7 +5771,11 @@ void dump_vmcs(void)
>   	cpu_based_exec_ctrl = vmcs_read32(CPU_BASED_VM_EXEC_CONTROL);
>   	pin_based_exec_ctrl = vmcs_read32(PIN_BASED_VM_EXEC_CONTROL);
>   	cr4 = vmcs_readl(GUEST_CR4);
> -	efer = vmcs_read64(GUEST_IA32_EFER);
> +	if ((vmexit_ctl & VM_EXIT_SAVE_IA32_EFER) ||
> +	    (vmentry_ctl & VM_ENTRY_LOAD_IA32_EFER))
> +		efer = vmcs_read64(GUEST_IA32_EFER);
> +	else
> +		efer = vcpu->arch.efer;
>   	secondary_exec_control = 0;
>   	if (cpu_has_secondary_exec_ctrls())
>   		secondary_exec_control = vmcs_read32(SECONDARY_VM_EXEC_CONTROL);
> @@ -5955,7 +5959,7 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
>   	}
>   
>   	if (exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) {
> -		dump_vmcs();
> +		dump_vmcs(vcpu);
>   		vcpu->run->exit_reason = KVM_EXIT_FAIL_ENTRY;
>   		vcpu->run->fail_entry.hardware_entry_failure_reason
>   			= exit_reason;
> @@ -5964,7 +5968,7 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
>   	}
>   
>   	if (unlikely(vmx->fail)) {
> -		dump_vmcs();
> +		dump_vmcs(vcpu);
>   		vcpu->run->exit_reason = KVM_EXIT_FAIL_ENTRY;
>   		vcpu->run->fail_entry.hardware_entry_failure_reason
>   			= vmcs_read32(VM_INSTRUCTION_ERROR);
> @@ -6049,7 +6053,7 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
>   
>   unexpected_vmexit:
>   	vcpu_unimpl(vcpu, "vmx: unexpected exit reason 0x%x\n", exit_reason);
> -	dump_vmcs();
> +	dump_vmcs(vcpu);
>   	vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
>   	vcpu->run->internal.suberror =
>   			KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON;
> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> index 9d3a557949ac..f8a0ce74798e 100644
> --- a/arch/x86/kvm/vmx/vmx.h
> +++ b/arch/x86/kvm/vmx/vmx.h
> @@ -489,6 +489,6 @@ static inline bool vmx_guest_state_valid(struct kvm_vcpu *vcpu)
>   	return is_unrestricted_guest(vcpu) || __vmx_guest_state_valid(vcpu);
>   }
>   
> -void dump_vmcs(void);
> +void dump_vmcs(struct kvm_vcpu *vcpu);
>   
>   #endif /* __KVM_X86_VMX_H */
> 

Re: [PATCH] KVM: x86: dump_vmcs should not assume GUEST_IA32_EFER is valid

[David Edmondson]

On Thursday, 2021-02-18 at 12:54:52 +01, Paolo Bonzini wrote:

> On 18/02/21 11:04, David Edmondson wrote:
>> When dumping the VMCS, retrieve the current guest value of EFER from
>> the kvm_vcpu structure if neither VM_EXIT_SAVE_IA32_EFER or
>> VM_ENTRY_LOAD_IA32_EFER is set, which can occur if the processor does
>> not support the relevant VM-exit/entry controls.
>
> Printing vcpu->arch.efer is not the best choice however.  Could we dump 
> the whole MSR load/store area instead?

I'm happy to do that, and think that it would be useful, but it won't
help with the original problem (which I should have explained more).

If the guest has EFER_LMA set but we aren't using the entry/exit
controls, vm_read64(GUEST_IA32_EFER) returns 0, causing dump_vmcs() to
erroneously dump the PDPTRs.

> Paolo
>
>> Fixes: 4eb64dce8d0a ("KVM: x86: dump VMCS on invalid entry")
>> Signed-off-by: David Edmondson <david.edmondson@oracle.com>
>> ---
>>   arch/x86/kvm/vmx/vmx.c | 14 +++++++++-----
>>   arch/x86/kvm/vmx/vmx.h |  2 +-
>>   2 files changed, 10 insertions(+), 6 deletions(-)
>> 
>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>> index eb69fef57485..74ea4fe6f35e 100644
>> --- a/arch/x86/kvm/vmx/vmx.c
>> +++ b/arch/x86/kvm/vmx/vmx.c
>> @@ -5754,7 +5754,7 @@ static void vmx_dump_dtsel(char *name, uint32_t limit)
>>   	       vmcs_readl(limit + GUEST_GDTR_BASE - GUEST_GDTR_LIMIT));
>>   }
>>   
>> -void dump_vmcs(void)
>> +void dump_vmcs(struct kvm_vcpu *vcpu)
>>   {
>>   	u32 vmentry_ctl, vmexit_ctl;
>>   	u32 cpu_based_exec_ctrl, pin_based_exec_ctrl, secondary_exec_control;
>> @@ -5771,7 +5771,11 @@ void dump_vmcs(void)
>>   	cpu_based_exec_ctrl = vmcs_read32(CPU_BASED_VM_EXEC_CONTROL);
>>   	pin_based_exec_ctrl = vmcs_read32(PIN_BASED_VM_EXEC_CONTROL);
>>   	cr4 = vmcs_readl(GUEST_CR4);
>> -	efer = vmcs_read64(GUEST_IA32_EFER);
>> +	if ((vmexit_ctl & VM_EXIT_SAVE_IA32_EFER) ||
>> +	    (vmentry_ctl & VM_ENTRY_LOAD_IA32_EFER))
>> +		efer = vmcs_read64(GUEST_IA32_EFER);
>> +	else
>> +		efer = vcpu->arch.efer;
>>   	secondary_exec_control = 0;
>>   	if (cpu_has_secondary_exec_ctrls())
>>   		secondary_exec_control = vmcs_read32(SECONDARY_VM_EXEC_CONTROL);
>> @@ -5955,7 +5959,7 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
>>   	}
>>   
>>   	if (exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) {
>> -		dump_vmcs();
>> +		dump_vmcs(vcpu);
>>   		vcpu->run->exit_reason = KVM_EXIT_FAIL_ENTRY;
>>   		vcpu->run->fail_entry.hardware_entry_failure_reason
>>   			= exit_reason;
>> @@ -5964,7 +5968,7 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
>>   	}
>>   
>>   	if (unlikely(vmx->fail)) {
>> -		dump_vmcs();
>> +		dump_vmcs(vcpu);
>>   		vcpu->run->exit_reason = KVM_EXIT_FAIL_ENTRY;
>>   		vcpu->run->fail_entry.hardware_entry_failure_reason
>>   			= vmcs_read32(VM_INSTRUCTION_ERROR);
>> @@ -6049,7 +6053,7 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
>>   
>>   unexpected_vmexit:
>>   	vcpu_unimpl(vcpu, "vmx: unexpected exit reason 0x%x\n", exit_reason);
>> -	dump_vmcs();
>> +	dump_vmcs(vcpu);
>>   	vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
>>   	vcpu->run->internal.suberror =
>>   			KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON;
>> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
>> index 9d3a557949ac..f8a0ce74798e 100644
>> --- a/arch/x86/kvm/vmx/vmx.h
>> +++ b/arch/x86/kvm/vmx/vmx.h
>> @@ -489,6 +489,6 @@ static inline bool vmx_guest_state_valid(struct kvm_vcpu *vcpu)
>>   	return is_unrestricted_guest(vcpu) || __vmx_guest_state_valid(vcpu);
>>   }
>>   
>> -void dump_vmcs(void);
>> +void dump_vmcs(struct kvm_vcpu *vcpu);
>>   
>>   #endif /* __KVM_X86_VMX_H */
>> 

dme.
-- 
Why stay in college? Why go to night school?

Re: [PATCH] KVM: x86: dump_vmcs should not assume GUEST_IA32_EFER is valid

[Paolo Bonzini]

On 18/02/21 13:56, David Edmondson wrote:
> On Thursday, 2021-02-18 at 12:54:52 +01, Paolo Bonzini wrote:
> 
>> On 18/02/21 11:04, David Edmondson wrote:
>>> When dumping the VMCS, retrieve the current guest value of EFER from
>>> the kvm_vcpu structure if neither VM_EXIT_SAVE_IA32_EFER or
>>> VM_ENTRY_LOAD_IA32_EFER is set, which can occur if the processor does
>>> not support the relevant VM-exit/entry controls.
>>
>> Printing vcpu->arch.efer is not the best choice however.  Could we dump
>> the whole MSR load/store area instead?
> 
> I'm happy to do that, and think that it would be useful, but it won't
> help with the original problem (which I should have explained more).
> 
> If the guest has EFER_LMA set but we aren't using the entry/exit
> controls, vm_read64(GUEST_IA32_EFER) returns 0, causing dump_vmcs() to
> erroneously dump the PDPTRs.

Got it now.  It would sort of help, because while dumping the MSR 
load/store area you could get hold of the real EFER, and use it to 
decide whether to dump the PDPTRs.

Thanks,

Paolo


>> Paolo
>>
>>> Fixes: 4eb64dce8d0a ("KVM: x86: dump VMCS on invalid entry")
>>> Signed-off-by: David Edmondson <david.edmondson@oracle.com>
>>> ---
>>>    arch/x86/kvm/vmx/vmx.c | 14 +++++++++-----
>>>    arch/x86/kvm/vmx/vmx.h |  2 +-
>>>    2 files changed, 10 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>>> index eb69fef57485..74ea4fe6f35e 100644
>>> --- a/arch/x86/kvm/vmx/vmx.c
>>> +++ b/arch/x86/kvm/vmx/vmx.c
>>> @@ -5754,7 +5754,7 @@ static void vmx_dump_dtsel(char *name, uint32_t limit)
>>>    	       vmcs_readl(limit + GUEST_GDTR_BASE - GUEST_GDTR_LIMIT));
>>>    }
>>>    
>>> -void dump_vmcs(void)
>>> +void dump_vmcs(struct kvm_vcpu *vcpu)
>>>    {
>>>    	u32 vmentry_ctl, vmexit_ctl;
>>>    	u32 cpu_based_exec_ctrl, pin_based_exec_ctrl, secondary_exec_control;
>>> @@ -5771,7 +5771,11 @@ void dump_vmcs(void)
>>>    	cpu_based_exec_ctrl = vmcs_read32(CPU_BASED_VM_EXEC_CONTROL);
>>>    	pin_based_exec_ctrl = vmcs_read32(PIN_BASED_VM_EXEC_CONTROL);
>>>    	cr4 = vmcs_readl(GUEST_CR4);
>>> -	efer = vmcs_read64(GUEST_IA32_EFER);
>>> +	if ((vmexit_ctl & VM_EXIT_SAVE_IA32_EFER) ||
>>> +	    (vmentry_ctl & VM_ENTRY_LOAD_IA32_EFER))
>>> +		efer = vmcs_read64(GUEST_IA32_EFER);
>>> +	else
>>> +		efer = vcpu->arch.efer;
>>>    	secondary_exec_control = 0;
>>>    	if (cpu_has_secondary_exec_ctrls())
>>>    		secondary_exec_control = vmcs_read32(SECONDARY_VM_EXEC_CONTROL);
>>> @@ -5955,7 +5959,7 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
>>>    	}
>>>    
>>>    	if (exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) {
>>> -		dump_vmcs();
>>> +		dump_vmcs(vcpu);
>>>    		vcpu->run->exit_reason = KVM_EXIT_FAIL_ENTRY;
>>>    		vcpu->run->fail_entry.hardware_entry_failure_reason
>>>    			= exit_reason;
>>> @@ -5964,7 +5968,7 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
>>>    	}
>>>    
>>>    	if (unlikely(vmx->fail)) {
>>> -		dump_vmcs();
>>> +		dump_vmcs(vcpu);
>>>    		vcpu->run->exit_reason = KVM_EXIT_FAIL_ENTRY;
>>>    		vcpu->run->fail_entry.hardware_entry_failure_reason
>>>    			= vmcs_read32(VM_INSTRUCTION_ERROR);
>>> @@ -6049,7 +6053,7 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
>>>    
>>>    unexpected_vmexit:
>>>    	vcpu_unimpl(vcpu, "vmx: unexpected exit reason 0x%x\n", exit_reason);
>>> -	dump_vmcs();
>>> +	dump_vmcs(vcpu);
>>>    	vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
>>>    	vcpu->run->internal.suberror =
>>>    			KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON;
>>> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
>>> index 9d3a557949ac..f8a0ce74798e 100644
>>> --- a/arch/x86/kvm/vmx/vmx.h
>>> +++ b/arch/x86/kvm/vmx/vmx.h
>>> @@ -489,6 +489,6 @@ static inline bool vmx_guest_state_valid(struct kvm_vcpu *vcpu)
>>>    	return is_unrestricted_guest(vcpu) || __vmx_guest_state_valid(vcpu);
>>>    }
>>>    
>>> -void dump_vmcs(void);
>>> +void dump_vmcs(struct kvm_vcpu *vcpu);
>>>    
>>>    #endif /* __KVM_X86_VMX_H */
>>>
> 
> dme.
> 

Re: [PATCH] KVM: x86: dump_vmcs should not assume GUEST_IA32_EFER is valid

[David Edmondson]

On Thursday, 2021-02-18 at 14:01:40 +01, Paolo Bonzini wrote:

> On 18/02/21 13:56, David Edmondson wrote:
>> On Thursday, 2021-02-18 at 12:54:52 +01, Paolo Bonzini wrote:
>> 
>>> On 18/02/21 11:04, David Edmondson wrote:
>>>> When dumping the VMCS, retrieve the current guest value of EFER from
>>>> the kvm_vcpu structure if neither VM_EXIT_SAVE_IA32_EFER or
>>>> VM_ENTRY_LOAD_IA32_EFER is set, which can occur if the processor does
>>>> not support the relevant VM-exit/entry controls.
>>>
>>> Printing vcpu->arch.efer is not the best choice however.  Could we dump
>>> the whole MSR load/store area instead?
>> 
>> I'm happy to do that, and think that it would be useful, but it won't
>> help with the original problem (which I should have explained more).
>> 
>> If the guest has EFER_LMA set but we aren't using the entry/exit
>> controls, vm_read64(GUEST_IA32_EFER) returns 0, causing dump_vmcs() to
>> erroneously dump the PDPTRs.
>
> Got it now.  It would sort of help, because while dumping the MSR 
> load/store area you could get hold of the real EFER, and use it to 
> decide whether to dump the PDPTRs.

Okay, I'll do that and come back. Thanks!

> Thanks,
>
> Paolo
>
>
>>> Paolo
>>>
>>>> Fixes: 4eb64dce8d0a ("KVM: x86: dump VMCS on invalid entry")
>>>> Signed-off-by: David Edmondson <david.edmondson@oracle.com>
>>>> ---
>>>>    arch/x86/kvm/vmx/vmx.c | 14 +++++++++-----
>>>>    arch/x86/kvm/vmx/vmx.h |  2 +-
>>>>    2 files changed, 10 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>>>> index eb69fef57485..74ea4fe6f35e 100644
>>>> --- a/arch/x86/kvm/vmx/vmx.c
>>>> +++ b/arch/x86/kvm/vmx/vmx.c
>>>> @@ -5754,7 +5754,7 @@ static void vmx_dump_dtsel(char *name, uint32_t limit)
>>>>    	       vmcs_readl(limit + GUEST_GDTR_BASE - GUEST_GDTR_LIMIT));
>>>>    }
>>>>    
>>>> -void dump_vmcs(void)
>>>> +void dump_vmcs(struct kvm_vcpu *vcpu)
>>>>    {
>>>>    	u32 vmentry_ctl, vmexit_ctl;
>>>>    	u32 cpu_based_exec_ctrl, pin_based_exec_ctrl, secondary_exec_control;
>>>> @@ -5771,7 +5771,11 @@ void dump_vmcs(void)
>>>>    	cpu_based_exec_ctrl = vmcs_read32(CPU_BASED_VM_EXEC_CONTROL);
>>>>    	pin_based_exec_ctrl = vmcs_read32(PIN_BASED_VM_EXEC_CONTROL);
>>>>    	cr4 = vmcs_readl(GUEST_CR4);
>>>> -	efer = vmcs_read64(GUEST_IA32_EFER);
>>>> +	if ((vmexit_ctl & VM_EXIT_SAVE_IA32_EFER) ||
>>>> +	    (vmentry_ctl & VM_ENTRY_LOAD_IA32_EFER))
>>>> +		efer = vmcs_read64(GUEST_IA32_EFER);
>>>> +	else
>>>> +		efer = vcpu->arch.efer;
>>>>    	secondary_exec_control = 0;
>>>>    	if (cpu_has_secondary_exec_ctrls())
>>>>    		secondary_exec_control = vmcs_read32(SECONDARY_VM_EXEC_CONTROL);
>>>> @@ -5955,7 +5959,7 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
>>>>    	}
>>>>    
>>>>    	if (exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) {
>>>> -		dump_vmcs();
>>>> +		dump_vmcs(vcpu);
>>>>    		vcpu->run->exit_reason = KVM_EXIT_FAIL_ENTRY;
>>>>    		vcpu->run->fail_entry.hardware_entry_failure_reason
>>>>    			= exit_reason;
>>>> @@ -5964,7 +5968,7 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
>>>>    	}
>>>>    
>>>>    	if (unlikely(vmx->fail)) {
>>>> -		dump_vmcs();
>>>> +		dump_vmcs(vcpu);
>>>>    		vcpu->run->exit_reason = KVM_EXIT_FAIL_ENTRY;
>>>>    		vcpu->run->fail_entry.hardware_entry_failure_reason
>>>>    			= vmcs_read32(VM_INSTRUCTION_ERROR);
>>>> @@ -6049,7 +6053,7 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
>>>>    
>>>>    unexpected_vmexit:
>>>>    	vcpu_unimpl(vcpu, "vmx: unexpected exit reason 0x%x\n", exit_reason);
>>>> -	dump_vmcs();
>>>> +	dump_vmcs(vcpu);
>>>>    	vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
>>>>    	vcpu->run->internal.suberror =
>>>>    			KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON;
>>>> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
>>>> index 9d3a557949ac..f8a0ce74798e 100644
>>>> --- a/arch/x86/kvm/vmx/vmx.h
>>>> +++ b/arch/x86/kvm/vmx/vmx.h
>>>> @@ -489,6 +489,6 @@ static inline bool vmx_guest_state_valid(struct kvm_vcpu *vcpu)
>>>>    	return is_unrestricted_guest(vcpu) || __vmx_guest_state_valid(vcpu);
>>>>    }
>>>>    
>>>> -void dump_vmcs(void);
>>>> +void dump_vmcs(struct kvm_vcpu *vcpu);
>>>>    
>>>>    #endif /* __KVM_X86_VMX_H */
>>>>
>> 
>> dme.
>> 

dme.
-- 
But he said, leave me alone, I'm a family man.

Re: [PATCH] KVM: x86: dump_vmcs should not assume GUEST_IA32_EFER is valid

[Sean Christopherson]

On Thu, Feb 18, 2021, Paolo Bonzini wrote:
> On 18/02/21 13:56, David Edmondson wrote:
> > On Thursday, 2021-02-18 at 12:54:52 +01, Paolo Bonzini wrote:
> > 
> > > On 18/02/21 11:04, David Edmondson wrote:
> > > > When dumping the VMCS, retrieve the current guest value of EFER from
> > > > the kvm_vcpu structure if neither VM_EXIT_SAVE_IA32_EFER or
> > > > VM_ENTRY_LOAD_IA32_EFER is set, which can occur if the processor does
> > > > not support the relevant VM-exit/entry controls.
> > > 
> > > Printing vcpu->arch.efer is not the best choice however.  Could we dump
> > > the whole MSR load/store area instead?
> > 
> > I'm happy to do that, and think that it would be useful, but it won't
> > help with the original problem (which I should have explained more).
> > 
> > If the guest has EFER_LMA set but we aren't using the entry/exit
> > controls, vm_read64(GUEST_IA32_EFER) returns 0, causing dump_vmcs() to
> > erroneously dump the PDPTRs.
> 
> Got it now.  It would sort of help, because while dumping the MSR load/store
> area you could get hold of the real EFER, and use it to decide whether to
> dump the PDPTRs.

EFER isn't guaranteed to be in the load list, either, e.g. if guest and host
have the same desired value.

The proper way to retrieve the effective EFER is to reuse the logic in
nested_vmx_calc_efer(), i.e. look at VM_ENTRY_IA32E_MODE if EFER isn't being
loaded via VMCS.

Re: [PATCH] KVM: x86: dump_vmcs should not assume GUEST_IA32_EFER is valid

[Jim Mattson]

On Thu, Feb 18, 2021 at 8:35 AM Sean Christopherson <seanjc@google.com> wrote:
>
> On Thu, Feb 18, 2021, Paolo Bonzini wrote:
> > On 18/02/21 13:56, David Edmondson wrote:
> > > On Thursday, 2021-02-18 at 12:54:52 +01, Paolo Bonzini wrote:
> > >
> > > > On 18/02/21 11:04, David Edmondson wrote:
> > > > > When dumping the VMCS, retrieve the current guest value of EFER from
> > > > > the kvm_vcpu structure if neither VM_EXIT_SAVE_IA32_EFER or
> > > > > VM_ENTRY_LOAD_IA32_EFER is set, which can occur if the processor does
> > > > > not support the relevant VM-exit/entry controls.
> > > >
> > > > Printing vcpu->arch.efer is not the best choice however.  Could we dump
> > > > the whole MSR load/store area instead?
> > >
> > > I'm happy to do that, and think that it would be useful, but it won't
> > > help with the original problem (which I should have explained more).
> > >
> > > If the guest has EFER_LMA set but we aren't using the entry/exit
> > > controls, vm_read64(GUEST_IA32_EFER) returns 0, causing dump_vmcs() to
> > > erroneously dump the PDPTRs.
> >
> > Got it now.  It would sort of help, because while dumping the MSR load/store
> > area you could get hold of the real EFER, and use it to decide whether to
> > dump the PDPTRs.
>
> EFER isn't guaranteed to be in the load list, either, e.g. if guest and host
> have the same desired value.
>
> The proper way to retrieve the effective EFER is to reuse the logic in
> nested_vmx_calc_efer(), i.e. look at VM_ENTRY_IA32E_MODE if EFER isn't being
> loaded via VMCS.

Shouldn't dump_vmcs() simply dump the contents of the VMCS, in its
entirety? What does it matter what the value of EFER is?

Re: [PATCH] KVM: x86: dump_vmcs should not assume GUEST_IA32_EFER is valid

[Paolo Bonzini]

On 18/02/21 18:55, Jim Mattson wrote:
>>> Got it now.  It would sort of help, because while dumping the MSR load/store
>>> area you could get hold of the real EFER, and use it to decide whether to
>>> dump the PDPTRs.
>> EFER isn't guaranteed to be in the load list, either, e.g. if guest and host
>> have the same desired value.
>>
>> The proper way to retrieve the effective EFER is to reuse the logic in
>> nested_vmx_calc_efer(), i.e. look at VM_ENTRY_IA32E_MODE if EFER isn't being
>> loaded via VMCS.
>
> Shouldn't dump_vmcs() simply dump the contents of the VMCS, in its
> entirety? What does it matter what the value of EFER is?

Currently it has some conditionals, but it wouldn't be a problem indeed 
to remove them.

The MSR load list is missing state that dump_vmcs should print though.

Paolo