* [PATCH v2 0/3] KVM: x86: dump_vmcs: don't assume GUEST_IA32_EFER, show MSR autoloads/autosaves
@ 2021-02-19 14:46 David Edmondson
2021-02-19 14:46 ` [PATCH v2 1/3] KVM: x86: dump_vmcs should not assume GUEST_IA32_EFER is valid David Edmondson
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: David Edmondson @ 2021-02-19 14:46 UTC (permalink / raw)
To: linux-kernel
Cc: H. Peter Anvin, Joerg Roedel, x86, Thomas Gleixner, kvm,
Paolo Bonzini, Wanpeng Li, Ingo Molnar, Sean Christopherson,
Jim Mattson, Borislav Petkov, Vitaly Kuznetsov, David Edmondson
v2:
- Don't use vcpu->arch.efer when GUEST_IA32_EFER is not available (Paolo).
- Show EFER and PAT seperately, as appropriate.
- Dump the MSR autoload/autosave lists (Paolo).
David Edmondson (3):
KVM: x86: dump_vmcs should not assume GUEST_IA32_EFER is valid
KVM: x86: dump_vmcs should not conflate EFER and PAT presence in VMCS
KVM: x86: dump_vmcs should include the autoload/autostore MSR lists
arch/x86/kvm/vmx/vmx.c | 48 ++++++++++++++++++++++++++++--------------
arch/x86/kvm/vmx/vmx.h | 2 +-
2 files changed, 33 insertions(+), 17 deletions(-)
--
2.30.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 1/3] KVM: x86: dump_vmcs should not assume GUEST_IA32_EFER is valid
2021-02-19 14:46 [PATCH v2 0/3] KVM: x86: dump_vmcs: don't assume GUEST_IA32_EFER, show MSR autoloads/autosaves David Edmondson
@ 2021-02-19 14:46 ` David Edmondson
2021-02-23 22:51 ` Sean Christopherson
2021-02-23 23:18 ` Jim Mattson
2021-02-19 14:46 ` [PATCH v2 2/3] KVM: x86: dump_vmcs should not conflate EFER and PAT presence in VMCS David Edmondson
2021-02-19 14:46 ` [PATCH v2 3/3] KVM: x86: dump_vmcs should include the autoload/autostore MSR lists David Edmondson
2 siblings, 2 replies; 11+ messages in thread
From: David Edmondson @ 2021-02-19 14:46 UTC (permalink / raw)
To: linux-kernel
Cc: H. Peter Anvin, Joerg Roedel, x86, Thomas Gleixner, kvm,
Paolo Bonzini, Wanpeng Li, Ingo Molnar, Sean Christopherson,
Jim Mattson, Borislav Petkov, Vitaly Kuznetsov, David Edmondson
If the VM entry/exit controls for loading/saving MSR_EFER are either
not available (an older processor or explicitly disabled) or not
used (host and guest values are the same), reading GUEST_IA32_EFER
from the VMCS returns an inaccurate value.
Because of this, in dump_vmcs() don't use GUEST_IA32_EFER to decide
whether to print the PDPTRs - do so if the EPT is in use and CR4.PAE
is set.
Fixes: 4eb64dce8d0a ("KVM: x86: dump VMCS on invalid entry")
Signed-off-by: David Edmondson <david.edmondson@oracle.com>
---
arch/x86/kvm/vmx/vmx.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index eb69fef57485..818051c9fa10 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -5759,7 +5759,6 @@ void dump_vmcs(void)
u32 vmentry_ctl, vmexit_ctl;
u32 cpu_based_exec_ctrl, pin_based_exec_ctrl, secondary_exec_control;
unsigned long cr4;
- u64 efer;
if (!dump_invalid_vmcs) {
pr_warn_ratelimited("set kvm_intel.dump_invalid_vmcs=1 to dump internal KVM state.\n");
@@ -5771,7 +5770,6 @@ 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);
secondary_exec_control = 0;
if (cpu_has_secondary_exec_ctrls())
secondary_exec_control = vmcs_read32(SECONDARY_VM_EXEC_CONTROL);
@@ -5784,8 +5782,7 @@ void dump_vmcs(void)
cr4, vmcs_readl(CR4_READ_SHADOW), vmcs_readl(CR4_GUEST_HOST_MASK));
pr_err("CR3 = 0x%016lx\n", vmcs_readl(GUEST_CR3));
if ((secondary_exec_control & SECONDARY_EXEC_ENABLE_EPT) &&
- (cr4 & X86_CR4_PAE) && !(efer & EFER_LMA))
- {
+ (cr4 & X86_CR4_PAE)) {
pr_err("PDPTR0 = 0x%016llx PDPTR1 = 0x%016llx\n",
vmcs_read64(GUEST_PDPTR0), vmcs_read64(GUEST_PDPTR1));
pr_err("PDPTR2 = 0x%016llx PDPTR3 = 0x%016llx\n",
@@ -5811,7 +5808,8 @@ void dump_vmcs(void)
if ((vmexit_ctl & (VM_EXIT_SAVE_IA32_PAT | VM_EXIT_SAVE_IA32_EFER)) ||
(vmentry_ctl & (VM_ENTRY_LOAD_IA32_PAT | VM_ENTRY_LOAD_IA32_EFER)))
pr_err("EFER = 0x%016llx PAT = 0x%016llx\n",
- efer, vmcs_read64(GUEST_IA32_PAT));
+ vmcs_read64(GUEST_IA32_EFER),
+ vmcs_read64(GUEST_IA32_PAT));
pr_err("DebugCtl = 0x%016llx DebugExceptions = 0x%016lx\n",
vmcs_read64(GUEST_IA32_DEBUGCTL),
vmcs_readl(GUEST_PENDING_DBG_EXCEPTIONS));
--
2.30.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 2/3] KVM: x86: dump_vmcs should not conflate EFER and PAT presence in VMCS
2021-02-19 14:46 [PATCH v2 0/3] KVM: x86: dump_vmcs: don't assume GUEST_IA32_EFER, show MSR autoloads/autosaves David Edmondson
2021-02-19 14:46 ` [PATCH v2 1/3] KVM: x86: dump_vmcs should not assume GUEST_IA32_EFER is valid David Edmondson
@ 2021-02-19 14:46 ` David Edmondson
2021-02-23 22:58 ` Sean Christopherson
2021-02-19 14:46 ` [PATCH v2 3/3] KVM: x86: dump_vmcs should include the autoload/autostore MSR lists David Edmondson
2 siblings, 1 reply; 11+ messages in thread
From: David Edmondson @ 2021-02-19 14:46 UTC (permalink / raw)
To: linux-kernel
Cc: H. Peter Anvin, Joerg Roedel, x86, Thomas Gleixner, kvm,
Paolo Bonzini, Wanpeng Li, Ingo Molnar, Sean Christopherson,
Jim Mattson, Borislav Petkov, Vitaly Kuznetsov, David Edmondson
Show EFER and PAT based on their individual entry/exit controls.
Signed-off-by: David Edmondson <david.edmondson@oracle.com>
---
arch/x86/kvm/vmx/vmx.c | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 818051c9fa10..25090e3683ca 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -5805,11 +5805,12 @@ void dump_vmcs(void)
vmx_dump_sel("LDTR:", GUEST_LDTR_SELECTOR);
vmx_dump_dtsel("IDTR:", GUEST_IDTR_LIMIT);
vmx_dump_sel("TR: ", GUEST_TR_SELECTOR);
- if ((vmexit_ctl & (VM_EXIT_SAVE_IA32_PAT | VM_EXIT_SAVE_IA32_EFER)) ||
- (vmentry_ctl & (VM_ENTRY_LOAD_IA32_PAT | VM_ENTRY_LOAD_IA32_EFER)))
- pr_err("EFER = 0x%016llx PAT = 0x%016llx\n",
- vmcs_read64(GUEST_IA32_EFER),
- vmcs_read64(GUEST_IA32_PAT));
+ if ((vmexit_ctl & VM_EXIT_SAVE_IA32_EFER) ||
+ (vmentry_ctl & VM_ENTRY_LOAD_IA32_EFER))
+ pr_err("EFER= 0x%016llx\n", vmcs_read64(GUEST_IA32_EFER));
+ if ((vmexit_ctl & VM_EXIT_SAVE_IA32_PAT) ||
+ (vmentry_ctl & VM_ENTRY_LOAD_IA32_PAT))
+ pr_err("PAT = 0x%016llx\n", vmcs_read64(GUEST_IA32_PAT));
pr_err("DebugCtl = 0x%016llx DebugExceptions = 0x%016lx\n",
vmcs_read64(GUEST_IA32_DEBUGCTL),
vmcs_readl(GUEST_PENDING_DBG_EXCEPTIONS));
@@ -5846,10 +5847,10 @@ void dump_vmcs(void)
vmcs_readl(HOST_IA32_SYSENTER_ESP),
vmcs_read32(HOST_IA32_SYSENTER_CS),
vmcs_readl(HOST_IA32_SYSENTER_EIP));
- if (vmexit_ctl & (VM_EXIT_LOAD_IA32_PAT | VM_EXIT_LOAD_IA32_EFER))
- pr_err("EFER = 0x%016llx PAT = 0x%016llx\n",
- vmcs_read64(HOST_IA32_EFER),
- vmcs_read64(HOST_IA32_PAT));
+ if (vmexit_ctl & VM_EXIT_LOAD_IA32_EFER)
+ pr_err("EFER= 0x%016llx\n", vmcs_read64(HOST_IA32_EFER));
+ if (vmexit_ctl & VM_EXIT_LOAD_IA32_PAT)
+ pr_err("PAT = 0x%016llx\n", vmcs_read64(HOST_IA32_PAT));
if (cpu_has_load_perf_global_ctrl() &&
vmexit_ctl & VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL)
pr_err("PerfGlobCtl = 0x%016llx\n",
--
2.30.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 3/3] KVM: x86: dump_vmcs should include the autoload/autostore MSR lists
2021-02-19 14:46 [PATCH v2 0/3] KVM: x86: dump_vmcs: don't assume GUEST_IA32_EFER, show MSR autoloads/autosaves David Edmondson
2021-02-19 14:46 ` [PATCH v2 1/3] KVM: x86: dump_vmcs should not assume GUEST_IA32_EFER is valid David Edmondson
2021-02-19 14:46 ` [PATCH v2 2/3] KVM: x86: dump_vmcs should not conflate EFER and PAT presence in VMCS David Edmondson
@ 2021-02-19 14:46 ` David Edmondson
2 siblings, 0 replies; 11+ messages in thread
From: David Edmondson @ 2021-02-19 14:46 UTC (permalink / raw)
To: linux-kernel
Cc: H. Peter Anvin, Joerg Roedel, x86, Thomas Gleixner, kvm,
Paolo Bonzini, Wanpeng Li, Ingo Molnar, Sean Christopherson,
Jim Mattson, Borislav Petkov, Vitaly Kuznetsov, David Edmondson
When dumping the current VMCS state, include the MSRs that are being
automatically loaded/stored during VM entry/exit.
Signed-off-by: David Edmondson <david.edmondson@oracle.com>
---
arch/x86/kvm/vmx/vmx.c | 25 +++++++++++++++++++++----
arch/x86/kvm/vmx/vmx.h | 2 +-
2 files changed, 22 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 25090e3683ca..5dbaab73e576 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -5754,8 +5754,19 @@ static void vmx_dump_dtsel(char *name, uint32_t limit)
vmcs_readl(limit + GUEST_GDTR_BASE - GUEST_GDTR_LIMIT));
}
-void dump_vmcs(void)
+static void vmx_dump_msrs(char *name, struct vmx_msrs *m)
{
+ unsigned int i;
+ struct vmx_msr_entry *e;
+
+ pr_err("MSR %s:\n", name);
+ for (i = 0, e = m->val; i < m->nr; ++i, ++e)
+ pr_err(" %2d: msr=0x%08x value=0x%016llx\n", i, e->index, e->value);
+}
+
+void dump_vmcs(struct kvm_vcpu *vcpu)
+{
+ struct vcpu_vmx *vmx = to_vmx(vcpu);
u32 vmentry_ctl, vmexit_ctl;
u32 cpu_based_exec_ctrl, pin_based_exec_ctrl, secondary_exec_control;
unsigned long cr4;
@@ -5826,6 +5837,10 @@ void dump_vmcs(void)
if (secondary_exec_control & SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY)
pr_err("InterruptStatus = %04x\n",
vmcs_read16(GUEST_INTR_STATUS));
+ if (vmcs_read32(VM_ENTRY_MSR_LOAD_COUNT) > 0)
+ vmx_dump_msrs("guest autoload", &vmx->msr_autoload.guest);
+ if (vmcs_read32(VM_EXIT_MSR_STORE_COUNT) > 0)
+ vmx_dump_msrs("guest autostore", &vmx->msr_autostore.guest);
pr_err("*** Host State ***\n");
pr_err("RIP = 0x%016lx RSP = 0x%016lx\n",
@@ -5855,6 +5870,8 @@ void dump_vmcs(void)
vmexit_ctl & VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL)
pr_err("PerfGlobCtl = 0x%016llx\n",
vmcs_read64(HOST_IA32_PERF_GLOBAL_CTRL));
+ if (vmcs_read32(VM_EXIT_MSR_LOAD_COUNT) > 0)
+ vmx_dump_msrs("host autoload", &vmx->msr_autoload.host);
pr_err("*** Control State ***\n");
pr_err("PinBased=%08x CPUBased=%08x SecondaryExec=%08x\n",
@@ -5954,7 +5971,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;
@@ -5963,7 +5980,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);
@@ -6048,7 +6065,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] 11+ messages in thread
* Re: [PATCH v2 1/3] KVM: x86: dump_vmcs should not assume GUEST_IA32_EFER is valid
2021-02-19 14:46 ` [PATCH v2 1/3] KVM: x86: dump_vmcs should not assume GUEST_IA32_EFER is valid David Edmondson
@ 2021-02-23 22:51 ` Sean Christopherson
2021-02-23 23:13 ` Jim Mattson
2021-02-23 23:18 ` Jim Mattson
1 sibling, 1 reply; 11+ messages in thread
From: Sean Christopherson @ 2021-02-23 22:51 UTC (permalink / raw)
To: David Edmondson
Cc: linux-kernel, H. Peter Anvin, Joerg Roedel, x86, Thomas Gleixner,
kvm, Paolo Bonzini, Wanpeng Li, Ingo Molnar, Jim Mattson,
Borislav Petkov, Vitaly Kuznetsov
On Fri, Feb 19, 2021, David Edmondson wrote:
> If the VM entry/exit controls for loading/saving MSR_EFER are either
> not available (an older processor or explicitly disabled) or not
> used (host and guest values are the same), reading GUEST_IA32_EFER
> from the VMCS returns an inaccurate value.
>
> Because of this, in dump_vmcs() don't use GUEST_IA32_EFER to decide
> whether to print the PDPTRs - do so if the EPT is in use and CR4.PAE
> is set.
This isn't necessarily correct either. In a way, it's less correct as PDPTRs
are more likely to be printed when they shouldn't, assuming most guests are
64-bit guests. It's annoying to calculate the effective guest EFER, but so
awful that it's worth risking confusion over PDTPRs.
> Fixes: 4eb64dce8d0a ("KVM: x86: dump VMCS on invalid entry")
> Signed-off-by: David Edmondson <david.edmondson@oracle.com>
> ---
> arch/x86/kvm/vmx/vmx.c | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index eb69fef57485..818051c9fa10 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -5759,7 +5759,6 @@ void dump_vmcs(void)
> u32 vmentry_ctl, vmexit_ctl;
> u32 cpu_based_exec_ctrl, pin_based_exec_ctrl, secondary_exec_control;
> unsigned long cr4;
> - u64 efer;
>
> if (!dump_invalid_vmcs) {
> pr_warn_ratelimited("set kvm_intel.dump_invalid_vmcs=1 to dump internal KVM state.\n");
> @@ -5771,7 +5770,6 @@ 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);
> secondary_exec_control = 0;
> if (cpu_has_secondary_exec_ctrls())
> secondary_exec_control = vmcs_read32(SECONDARY_VM_EXEC_CONTROL);
> @@ -5784,8 +5782,7 @@ void dump_vmcs(void)
> cr4, vmcs_readl(CR4_READ_SHADOW), vmcs_readl(CR4_GUEST_HOST_MASK));
> pr_err("CR3 = 0x%016lx\n", vmcs_readl(GUEST_CR3));
> if ((secondary_exec_control & SECONDARY_EXEC_ENABLE_EPT) &&
> - (cr4 & X86_CR4_PAE) && !(efer & EFER_LMA))
> - {
> + (cr4 & X86_CR4_PAE)) {
> pr_err("PDPTR0 = 0x%016llx PDPTR1 = 0x%016llx\n",
> vmcs_read64(GUEST_PDPTR0), vmcs_read64(GUEST_PDPTR1));
> pr_err("PDPTR2 = 0x%016llx PDPTR3 = 0x%016llx\n",
> @@ -5811,7 +5808,8 @@ void dump_vmcs(void)
> if ((vmexit_ctl & (VM_EXIT_SAVE_IA32_PAT | VM_EXIT_SAVE_IA32_EFER)) ||
> (vmentry_ctl & (VM_ENTRY_LOAD_IA32_PAT | VM_ENTRY_LOAD_IA32_EFER)))
> pr_err("EFER = 0x%016llx PAT = 0x%016llx\n",
> - efer, vmcs_read64(GUEST_IA32_PAT));
> + vmcs_read64(GUEST_IA32_EFER),
> + vmcs_read64(GUEST_IA32_PAT));
> pr_err("DebugCtl = 0x%016llx DebugExceptions = 0x%016lx\n",
> vmcs_read64(GUEST_IA32_DEBUGCTL),
> vmcs_readl(GUEST_PENDING_DBG_EXCEPTIONS));
> --
> 2.30.0
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/3] KVM: x86: dump_vmcs should not conflate EFER and PAT presence in VMCS
2021-02-19 14:46 ` [PATCH v2 2/3] KVM: x86: dump_vmcs should not conflate EFER and PAT presence in VMCS David Edmondson
@ 2021-02-23 22:58 ` Sean Christopherson
2021-02-24 13:31 ` David Edmondson
0 siblings, 1 reply; 11+ messages in thread
From: Sean Christopherson @ 2021-02-23 22:58 UTC (permalink / raw)
To: David Edmondson
Cc: linux-kernel, H. Peter Anvin, Joerg Roedel, x86, Thomas Gleixner,
kvm, Paolo Bonzini, Wanpeng Li, Ingo Molnar, Jim Mattson,
Borislav Petkov, Vitaly Kuznetsov
On Fri, Feb 19, 2021, David Edmondson wrote:
> Show EFER and PAT based on their individual entry/exit controls.
>
> Signed-off-by: David Edmondson <david.edmondson@oracle.com>
> ---
> arch/x86/kvm/vmx/vmx.c | 19 ++++++++++---------
> 1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 818051c9fa10..25090e3683ca 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -5805,11 +5805,12 @@ void dump_vmcs(void)
> vmx_dump_sel("LDTR:", GUEST_LDTR_SELECTOR);
> vmx_dump_dtsel("IDTR:", GUEST_IDTR_LIMIT);
> vmx_dump_sel("TR: ", GUEST_TR_SELECTOR);
> - if ((vmexit_ctl & (VM_EXIT_SAVE_IA32_PAT | VM_EXIT_SAVE_IA32_EFER)) ||
> - (vmentry_ctl & (VM_ENTRY_LOAD_IA32_PAT | VM_ENTRY_LOAD_IA32_EFER)))
> - pr_err("EFER = 0x%016llx PAT = 0x%016llx\n",
> - vmcs_read64(GUEST_IA32_EFER),
> - vmcs_read64(GUEST_IA32_PAT));
> + if ((vmexit_ctl & VM_EXIT_SAVE_IA32_EFER) ||
Not your code, and completely benign since VM_EXIT_SAVE is never set, but I
don't like checking the VM_EXIT_SAVE_* flag as saving a field on VM-Exit has
zero impact on whether VM-Entry succeeds or fails. Same complaint on the PAT
field.
> + (vmentry_ctl & VM_ENTRY_LOAD_IA32_EFER))
> + pr_err("EFER= 0x%016llx\n", vmcs_read64(GUEST_IA32_EFER));
Tying into the previous patch, I think we should print both the effective EFER
and vmcs.EFER. The effective EFER is relevant for several consistency checks.
Maybe something like this?
pr_err("EFER= 0x%016llx ", effective_efer);
if (vmentry_ctl & VM_ENTRY_LOAD_IA32_EFER)
pr_cont("vmcs.EFER= 0x%016llx\n", vmcs_read64(GUEST_IA32_EFER));
else
pr_cont("vmcs.EFER not loaded\n")
> + if ((vmexit_ctl & VM_EXIT_SAVE_IA32_PAT) ||
> + (vmentry_ctl & VM_ENTRY_LOAD_IA32_PAT))
> + pr_err("PAT = 0x%016llx\n", vmcs_read64(GUEST_IA32_PAT));
> pr_err("DebugCtl = 0x%016llx DebugExceptions = 0x%016lx\n",
> vmcs_read64(GUEST_IA32_DEBUGCTL),
> vmcs_readl(GUEST_PENDING_DBG_EXCEPTIONS));
> @@ -5846,10 +5847,10 @@ void dump_vmcs(void)
> vmcs_readl(HOST_IA32_SYSENTER_ESP),
> vmcs_read32(HOST_IA32_SYSENTER_CS),
> vmcs_readl(HOST_IA32_SYSENTER_EIP));
> - if (vmexit_ctl & (VM_EXIT_LOAD_IA32_PAT | VM_EXIT_LOAD_IA32_EFER))
> - pr_err("EFER = 0x%016llx PAT = 0x%016llx\n",
> - vmcs_read64(HOST_IA32_EFER),
> - vmcs_read64(HOST_IA32_PAT));
> + if (vmexit_ctl & VM_EXIT_LOAD_IA32_EFER)
> + pr_err("EFER= 0x%016llx\n", vmcs_read64(HOST_IA32_EFER));
> + if (vmexit_ctl & VM_EXIT_LOAD_IA32_PAT)
> + pr_err("PAT = 0x%016llx\n", vmcs_read64(HOST_IA32_PAT));
> if (cpu_has_load_perf_global_ctrl() &&
> vmexit_ctl & VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL)
> pr_err("PerfGlobCtl = 0x%016llx\n",
> --
> 2.30.0
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] KVM: x86: dump_vmcs should not assume GUEST_IA32_EFER is valid
2021-02-23 22:51 ` Sean Christopherson
@ 2021-02-23 23:13 ` Jim Mattson
2021-02-23 23:41 ` Sean Christopherson
2021-02-24 13:30 ` David Edmondson
0 siblings, 2 replies; 11+ messages in thread
From: Jim Mattson @ 2021-02-23 23:13 UTC (permalink / raw)
To: Sean Christopherson
Cc: David Edmondson, LKML, H. Peter Anvin, Joerg Roedel,
the arch/x86 maintainers, Thomas Gleixner, kvm list,
Paolo Bonzini, Wanpeng Li, Ingo Molnar, Borislav Petkov,
Vitaly Kuznetsov
On Tue, Feb 23, 2021 at 2:51 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Fri, Feb 19, 2021, David Edmondson wrote:
> > If the VM entry/exit controls for loading/saving MSR_EFER are either
> > not available (an older processor or explicitly disabled) or not
> > used (host and guest values are the same), reading GUEST_IA32_EFER
> > from the VMCS returns an inaccurate value.
> >
> > Because of this, in dump_vmcs() don't use GUEST_IA32_EFER to decide
> > whether to print the PDPTRs - do so if the EPT is in use and CR4.PAE
> > is set.
>
> This isn't necessarily correct either. In a way, it's less correct as PDPTRs
> are more likely to be printed when they shouldn't, assuming most guests are
> 64-bit guests. It's annoying to calculate the effective guest EFER, but so
> awful that it's worth risking confusion over PDTPRs.
I still prefer a dump_vmcs that always dumps every VMCS field. But if
you really want to skip printing the PDPTEs when they're irrelevant,
can't you just use the "IA-32e mode guest" VM-entry control as a proxy
for EFER.LMA?
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] KVM: x86: dump_vmcs should not assume GUEST_IA32_EFER is valid
2021-02-19 14:46 ` [PATCH v2 1/3] KVM: x86: dump_vmcs should not assume GUEST_IA32_EFER is valid David Edmondson
2021-02-23 22:51 ` Sean Christopherson
@ 2021-02-23 23:18 ` Jim Mattson
1 sibling, 0 replies; 11+ messages in thread
From: Jim Mattson @ 2021-02-23 23:18 UTC (permalink / raw)
To: David Edmondson
Cc: LKML, H. Peter Anvin, Joerg Roedel, the arch/x86 maintainers,
Thomas Gleixner, kvm list, Paolo Bonzini, Wanpeng Li,
Ingo Molnar, Sean Christopherson, Borislav Petkov,
Vitaly Kuznetsov
On Fri, Feb 19, 2021 at 6:46 AM David Edmondson
<david.edmondson@oracle.com> wrote:
>
> If the VM entry/exit controls for loading/saving MSR_EFER are either
> not available (an older processor or explicitly disabled) or not
> used (host and guest values are the same), reading GUEST_IA32_EFER
> from the VMCS returns an inaccurate value.
>
> Because of this, in dump_vmcs() don't use GUEST_IA32_EFER to decide
> whether to print the PDPTRs - do so if the EPT is in use and CR4.PAE
> is set.
>
> Fixes: 4eb64dce8d0a ("KVM: x86: dump VMCS on invalid entry")
> Signed-off-by: David Edmondson <david.edmondson@oracle.com>
> ---
> arch/x86/kvm/vmx/vmx.c | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index eb69fef57485..818051c9fa10 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -5759,7 +5759,6 @@ void dump_vmcs(void)
> u32 vmentry_ctl, vmexit_ctl;
> u32 cpu_based_exec_ctrl, pin_based_exec_ctrl, secondary_exec_control;
> unsigned long cr4;
> - u64 efer;
>
> if (!dump_invalid_vmcs) {
> pr_warn_ratelimited("set kvm_intel.dump_invalid_vmcs=1 to dump internal KVM state.\n");
> @@ -5771,7 +5770,6 @@ 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);
> secondary_exec_control = 0;
> if (cpu_has_secondary_exec_ctrls())
> secondary_exec_control = vmcs_read32(SECONDARY_VM_EXEC_CONTROL);
> @@ -5784,8 +5782,7 @@ void dump_vmcs(void)
> cr4, vmcs_readl(CR4_READ_SHADOW), vmcs_readl(CR4_GUEST_HOST_MASK));
> pr_err("CR3 = 0x%016lx\n", vmcs_readl(GUEST_CR3));
> if ((secondary_exec_control & SECONDARY_EXEC_ENABLE_EPT) &&
> - (cr4 & X86_CR4_PAE) && !(efer & EFER_LMA))
> - {
> + (cr4 & X86_CR4_PAE)) {
Assuming that we really want to restrict the printing of the PDPTEs, I
think you also need to test "cr0 & CR0.PG" (cf. section 26.3.1.6 of
the SDM, volume 3).
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] KVM: x86: dump_vmcs should not assume GUEST_IA32_EFER is valid
2021-02-23 23:13 ` Jim Mattson
@ 2021-02-23 23:41 ` Sean Christopherson
2021-02-24 13:30 ` David Edmondson
1 sibling, 0 replies; 11+ messages in thread
From: Sean Christopherson @ 2021-02-23 23:41 UTC (permalink / raw)
To: Jim Mattson
Cc: David Edmondson, LKML, H. Peter Anvin, Joerg Roedel,
the arch/x86 maintainers, Thomas Gleixner, kvm list,
Paolo Bonzini, Wanpeng Li, Ingo Molnar, Borislav Petkov,
Vitaly Kuznetsov
On Tue, Feb 23, 2021, Jim Mattson wrote:
> On Tue, Feb 23, 2021 at 2:51 PM Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Fri, Feb 19, 2021, David Edmondson wrote:
> > > If the VM entry/exit controls for loading/saving MSR_EFER are either
> > > not available (an older processor or explicitly disabled) or not
> > > used (host and guest values are the same), reading GUEST_IA32_EFER
> > > from the VMCS returns an inaccurate value.
> > >
> > > Because of this, in dump_vmcs() don't use GUEST_IA32_EFER to decide
> > > whether to print the PDPTRs - do so if the EPT is in use and CR4.PAE
> > > is set.
> >
> > This isn't necessarily correct either. In a way, it's less correct as PDPTRs
> > are more likely to be printed when they shouldn't, assuming most guests are
> > 64-bit guests. It's annoying to calculate the effective guest EFER, but so
> > awful that it's worth risking confusion over PDTPRs.
>
> I still prefer a dump_vmcs that always dumps every VMCS field.
I'm a-ok with that approach.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] KVM: x86: dump_vmcs should not assume GUEST_IA32_EFER is valid
2021-02-23 23:13 ` Jim Mattson
2021-02-23 23:41 ` Sean Christopherson
@ 2021-02-24 13:30 ` David Edmondson
1 sibling, 0 replies; 11+ messages in thread
From: David Edmondson @ 2021-02-24 13:30 UTC (permalink / raw)
To: Jim Mattson, Sean Christopherson
Cc: LKML, H. Peter Anvin, Joerg Roedel, the arch/x86 maintainers,
Thomas Gleixner, kvm list, Paolo Bonzini, Wanpeng Li,
Ingo Molnar, Borislav Petkov, Vitaly Kuznetsov
On Tuesday, 2021-02-23 at 15:13:54 -08, Jim Mattson wrote:
> On Tue, Feb 23, 2021 at 2:51 PM Sean Christopherson <seanjc@google.com> wrote:
>>
>> On Fri, Feb 19, 2021, David Edmondson wrote:
>> > If the VM entry/exit controls for loading/saving MSR_EFER are either
>> > not available (an older processor or explicitly disabled) or not
>> > used (host and guest values are the same), reading GUEST_IA32_EFER
>> > from the VMCS returns an inaccurate value.
>> >
>> > Because of this, in dump_vmcs() don't use GUEST_IA32_EFER to decide
>> > whether to print the PDPTRs - do so if the EPT is in use and CR4.PAE
>> > is set.
>>
>> This isn't necessarily correct either. In a way, it's less correct as PDPTRs
>> are more likely to be printed when they shouldn't, assuming most guests are
>> 64-bit guests. It's annoying to calculate the effective guest EFER, but so
>> awful that it's worth risking confusion over PDTPRs.
>
> I still prefer a dump_vmcs that always dumps every VMCS field.
v3 now always shows the PDPTRs if they exist.
dme.
--
I've got a little black book with my poems in.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/3] KVM: x86: dump_vmcs should not conflate EFER and PAT presence in VMCS
2021-02-23 22:58 ` Sean Christopherson
@ 2021-02-24 13:31 ` David Edmondson
0 siblings, 0 replies; 11+ messages in thread
From: David Edmondson @ 2021-02-24 13:31 UTC (permalink / raw)
To: Sean Christopherson
Cc: linux-kernel, H. Peter Anvin, Joerg Roedel, x86, Thomas Gleixner,
kvm, Paolo Bonzini, Wanpeng Li, Ingo Molnar, Jim Mattson,
Borislav Petkov, Vitaly Kuznetsov
On Tuesday, 2021-02-23 at 14:58:29 -08, Sean Christopherson wrote:
> On Fri, Feb 19, 2021, David Edmondson wrote:
>> Show EFER and PAT based on their individual entry/exit controls.
>>
>> Signed-off-by: David Edmondson <david.edmondson@oracle.com>
>> ---
>> arch/x86/kvm/vmx/vmx.c | 19 ++++++++++---------
>> 1 file changed, 10 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>> index 818051c9fa10..25090e3683ca 100644
>> --- a/arch/x86/kvm/vmx/vmx.c
>> +++ b/arch/x86/kvm/vmx/vmx.c
>> @@ -5805,11 +5805,12 @@ void dump_vmcs(void)
>> vmx_dump_sel("LDTR:", GUEST_LDTR_SELECTOR);
>> vmx_dump_dtsel("IDTR:", GUEST_IDTR_LIMIT);
>> vmx_dump_sel("TR: ", GUEST_TR_SELECTOR);
>> - if ((vmexit_ctl & (VM_EXIT_SAVE_IA32_PAT | VM_EXIT_SAVE_IA32_EFER)) ||
>> - (vmentry_ctl & (VM_ENTRY_LOAD_IA32_PAT | VM_ENTRY_LOAD_IA32_EFER)))
>> - pr_err("EFER = 0x%016llx PAT = 0x%016llx\n",
>> - vmcs_read64(GUEST_IA32_EFER),
>> - vmcs_read64(GUEST_IA32_PAT));
>> + if ((vmexit_ctl & VM_EXIT_SAVE_IA32_EFER) ||
>
> Not your code, and completely benign since VM_EXIT_SAVE is never set, but I
> don't like checking the VM_EXIT_SAVE_* flag as saving a field on VM-Exit has
> zero impact on whether VM-Entry succeeds or fails. Same complaint on the PAT
> field.
Added to v3.
>> + (vmentry_ctl & VM_ENTRY_LOAD_IA32_EFER))
>> + pr_err("EFER= 0x%016llx\n", vmcs_read64(GUEST_IA32_EFER));
>
> Tying into the previous patch, I think we should print both the effective EFER
> and vmcs.EFER. The effective EFER is relevant for several consistency checks.
> Maybe something like this?
>
> pr_err("EFER= 0x%016llx ", effective_efer);
> if (vmentry_ctl & VM_ENTRY_LOAD_IA32_EFER)
> pr_cont("vmcs.EFER= 0x%016llx\n", vmcs_read64(GUEST_IA32_EFER));
> else
> pr_cont("vmcs.EFER not loaded\n")
Added something similar, that makes it clear where the value came from,
in v3.
>> + if ((vmexit_ctl & VM_EXIT_SAVE_IA32_PAT) ||
>> + (vmentry_ctl & VM_ENTRY_LOAD_IA32_PAT))
>> + pr_err("PAT = 0x%016llx\n", vmcs_read64(GUEST_IA32_PAT));
>> pr_err("DebugCtl = 0x%016llx DebugExceptions = 0x%016lx\n",
>> vmcs_read64(GUEST_IA32_DEBUGCTL),
>> vmcs_readl(GUEST_PENDING_DBG_EXCEPTIONS));
>> @@ -5846,10 +5847,10 @@ void dump_vmcs(void)
>> vmcs_readl(HOST_IA32_SYSENTER_ESP),
>> vmcs_read32(HOST_IA32_SYSENTER_CS),
>> vmcs_readl(HOST_IA32_SYSENTER_EIP));
>> - if (vmexit_ctl & (VM_EXIT_LOAD_IA32_PAT | VM_EXIT_LOAD_IA32_EFER))
>> - pr_err("EFER = 0x%016llx PAT = 0x%016llx\n",
>> - vmcs_read64(HOST_IA32_EFER),
>> - vmcs_read64(HOST_IA32_PAT));
>> + if (vmexit_ctl & VM_EXIT_LOAD_IA32_EFER)
>> + pr_err("EFER= 0x%016llx\n", vmcs_read64(HOST_IA32_EFER));
>> + if (vmexit_ctl & VM_EXIT_LOAD_IA32_PAT)
>> + pr_err("PAT = 0x%016llx\n", vmcs_read64(HOST_IA32_PAT));
>> if (cpu_has_load_perf_global_ctrl() &&
>> vmexit_ctl & VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL)
>> pr_err("PerfGlobCtl = 0x%016llx\n",
>> --
>> 2.30.0
>>
dme.
--
And the sign said: long haired freaky people need not apply.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-02-24 14:51 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-19 14:46 [PATCH v2 0/3] KVM: x86: dump_vmcs: don't assume GUEST_IA32_EFER, show MSR autoloads/autosaves David Edmondson
2021-02-19 14:46 ` [PATCH v2 1/3] KVM: x86: dump_vmcs should not assume GUEST_IA32_EFER is valid David Edmondson
2021-02-23 22:51 ` Sean Christopherson
2021-02-23 23:13 ` Jim Mattson
2021-02-23 23:41 ` Sean Christopherson
2021-02-24 13:30 ` David Edmondson
2021-02-23 23:18 ` Jim Mattson
2021-02-19 14:46 ` [PATCH v2 2/3] KVM: x86: dump_vmcs should not conflate EFER and PAT presence in VMCS David Edmondson
2021-02-23 22:58 ` Sean Christopherson
2021-02-24 13:31 ` David Edmondson
2021-02-19 14:46 ` [PATCH v2 3/3] KVM: x86: dump_vmcs should include the autoload/autostore MSR lists David Edmondson
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.