| Message ID | 20210219144632.2288189-2-david.edmondson@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: dump_vmcs: don't assume GUEST_IA32_EFER, show MSR autoloads/autosaves | expand |
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 >
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?
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).
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.
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.
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));
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(-)