* [PATCH] KVM: x86: dump_vmcs should not assume GUEST_IA32_EFER is valid
@ 2021-02-18 10:04 David Edmondson
2021-02-18 11:54 ` Paolo Bonzini
0 siblings, 1 reply; 8+ messages in thread
From: David Edmondson @ 2021-02-18 10:04 UTC (permalink / raw)
To: linux-kernel
Cc: Borislav Petkov, Wanpeng Li, Thomas Gleixner, Ingo Molnar,
Paolo Bonzini, Vitaly Kuznetsov, x86, H. Peter Anvin,
Sean Christopherson, kvm, Jim Mattson, Joerg Roedel,
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
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] KVM: x86: dump_vmcs should not assume GUEST_IA32_EFER is valid 2021-02-18 10:04 [PATCH] KVM: x86: dump_vmcs should not assume GUEST_IA32_EFER is valid David Edmondson @ 2021-02-18 11:54 ` Paolo Bonzini 2021-02-18 12:56 ` David Edmondson 0 siblings, 1 reply; 8+ messages in thread From: Paolo Bonzini @ 2021-02-18 11:54 UTC (permalink / raw) To: David Edmondson, linux-kernel Cc: Borislav Petkov, Wanpeng Li, Thomas Gleixner, Ingo Molnar, Vitaly Kuznetsov, x86, H. Peter Anvin, Sean Christopherson, kvm, Jim Mattson, Joerg Roedel 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 */ > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] KVM: x86: dump_vmcs should not assume GUEST_IA32_EFER is valid 2021-02-18 11:54 ` Paolo Bonzini @ 2021-02-18 12:56 ` David Edmondson 2021-02-18 13:01 ` Paolo Bonzini 0 siblings, 1 reply; 8+ messages in thread From: David Edmondson @ 2021-02-18 12:56 UTC (permalink / raw) To: Paolo Bonzini, linux-kernel Cc: Borislav Petkov, Wanpeng Li, Thomas Gleixner, Ingo Molnar, Vitaly Kuznetsov, x86, H. Peter Anvin, Sean Christopherson, kvm, Jim Mattson, Joerg Roedel 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? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] KVM: x86: dump_vmcs should not assume GUEST_IA32_EFER is valid 2021-02-18 12:56 ` David Edmondson @ 2021-02-18 13:01 ` Paolo Bonzini 2021-02-18 13:17 ` David Edmondson 2021-02-18 16:35 ` Sean Christopherson 0 siblings, 2 replies; 8+ messages in thread From: Paolo Bonzini @ 2021-02-18 13:01 UTC (permalink / raw) To: David Edmondson, linux-kernel Cc: Borislav Petkov, Wanpeng Li, Thomas Gleixner, Ingo Molnar, Vitaly Kuznetsov, x86, H. Peter Anvin, Sean Christopherson, kvm, Jim Mattson, Joerg Roedel 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. > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] KVM: x86: dump_vmcs should not assume GUEST_IA32_EFER is valid 2021-02-18 13:01 ` Paolo Bonzini @ 2021-02-18 13:17 ` David Edmondson 2021-02-18 16:35 ` Sean Christopherson 1 sibling, 0 replies; 8+ messages in thread From: David Edmondson @ 2021-02-18 13:17 UTC (permalink / raw) To: Paolo Bonzini, linux-kernel Cc: Borislav Petkov, Wanpeng Li, Thomas Gleixner, Ingo Molnar, Vitaly Kuznetsov, x86, H. Peter Anvin, Sean Christopherson, kvm, Jim Mattson, Joerg Roedel 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] KVM: x86: dump_vmcs should not assume GUEST_IA32_EFER is valid 2021-02-18 13:01 ` Paolo Bonzini 2021-02-18 13:17 ` David Edmondson @ 2021-02-18 16:35 ` Sean Christopherson 2021-02-18 17:55 ` Jim Mattson 1 sibling, 1 reply; 8+ messages in thread From: Sean Christopherson @ 2021-02-18 16:35 UTC (permalink / raw) To: Paolo Bonzini Cc: David Edmondson, linux-kernel, Borislav Petkov, Wanpeng Li, Thomas Gleixner, Ingo Molnar, Vitaly Kuznetsov, x86, H. Peter Anvin, kvm, Jim Mattson, Joerg Roedel 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] KVM: x86: dump_vmcs should not assume GUEST_IA32_EFER is valid 2021-02-18 16:35 ` Sean Christopherson @ 2021-02-18 17:55 ` Jim Mattson 2021-02-18 18:04 ` Paolo Bonzini 0 siblings, 1 reply; 8+ messages in thread From: Jim Mattson @ 2021-02-18 17:55 UTC (permalink / raw) To: Sean Christopherson Cc: Paolo Bonzini, David Edmondson, LKML, Borislav Petkov, Wanpeng Li, Thomas Gleixner, Ingo Molnar, Vitaly Kuznetsov, the arch/x86 maintainers, H. Peter Anvin, kvm list, Joerg Roedel 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? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] KVM: x86: dump_vmcs should not assume GUEST_IA32_EFER is valid 2021-02-18 17:55 ` Jim Mattson @ 2021-02-18 18:04 ` Paolo Bonzini 0 siblings, 0 replies; 8+ messages in thread From: Paolo Bonzini @ 2021-02-18 18:04 UTC (permalink / raw) To: Jim Mattson, Sean Christopherson Cc: David Edmondson, LKML, Borislav Petkov, Wanpeng Li, Thomas Gleixner, Ingo Molnar, Vitaly Kuznetsov, the arch/x86 maintainers, H. Peter Anvin, kvm list, Joerg Roedel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-02-18 19:20 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-02-18 10:04 [PATCH] KVM: x86: dump_vmcs should not assume GUEST_IA32_EFER is valid David Edmondson 2021-02-18 11:54 ` Paolo Bonzini 2021-02-18 12:56 ` David Edmondson 2021-02-18 13:01 ` Paolo Bonzini 2021-02-18 13:17 ` David Edmondson 2021-02-18 16:35 ` Sean Christopherson 2021-02-18 17:55 ` Jim Mattson 2021-02-18 18:04 ` Paolo Bonzini
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.