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
When dumping the current VMCS state, include the MSRs that are being
automatically loaded/stored during VM entry/exit.
Signed-off-by: David Edmondson <[email protected]>
---
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
Show EFER and PAT based on their individual entry/exit controls.
Signed-off-by: David Edmondson <[email protected]>
---
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
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 <[email protected]>
---
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
On Tue, Feb 23, 2021 at 2:51 PM Sean Christopherson <[email protected]> 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?
On Fri, Feb 19, 2021, David Edmondson wrote:
> Show EFER and PAT based on their individual entry/exit controls.
>
> Signed-off-by: David Edmondson <[email protected]>
> ---
> 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
>
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 <[email protected]>
> ---
> 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
>
On Tue, Feb 23, 2021, Jim Mattson wrote:
> On Tue, Feb 23, 2021 at 2:51 PM Sean Christopherson <[email protected]> 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.
On Fri, Feb 19, 2021 at 6:46 AM David Edmondson
<[email protected]> 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 <[email protected]>
> ---
> 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).
On Tuesday, 2021-02-23 at 15:13:54 -08, Jim Mattson wrote:
> On Tue, Feb 23, 2021 at 2:51 PM Sean Christopherson <[email protected]> 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.
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 <[email protected]>
>> ---
>> 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.