| Message ID | 20190408213516.17966-6-krish.sadhukhan@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/6,v5,nVMX] : Check "load IA32_PAT" VM-exit control on vmentry | expand |
On Mon, Apr 08, 2019 at 05:35:15PM -0400, Krish Sadhukhan wrote: > ..to reflect the architectural Exit Reason for VM-entry failures due to > invalid guest state. > > Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> > Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com>
On 08/04/19 23:35, Krish Sadhukhan wrote: > ..to reflect the architectural Exit Reason for VM-entry failures due to > invalid guest state. > > Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> > Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > arch/x86/kvm/vmx/nested.c | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c > index 1ec5ddc4ea50..bde17d079a36 100644 > --- a/arch/x86/kvm/vmx/nested.c > +++ b/arch/x86/kvm/vmx/nested.c > @@ -2701,11 +2701,14 @@ static int nested_vmx_check_vmentry_postreqs(struct kvm_vcpu *vcpu, > *exit_qual = ENTRY_FAIL_DEFAULT; > > if (nested_check_guest_cregs_dregs_msrs(vcpu, vmcs12)) > - return 1; > + return VMX_EXIT_REASONS_FAILED_VMENTRY | > + EXIT_REASON_INVALID_STATE; > > if (nested_vmx_check_vmcs_link_ptr(vcpu, vmcs12)) { > *exit_qual = ENTRY_FAIL_VMCS_LINK_PTR; > - return 1; > + > + return VMX_EXIT_REASONS_FAILED_VMENTRY | > + EXIT_REASON_INVALID_STATE; > } > > /* > @@ -2724,13 +2727,17 @@ static int nested_vmx_check_vmentry_postreqs(struct kvm_vcpu *vcpu, > ia32e != !!(vmcs12->guest_ia32_efer & EFER_LMA) || > ((vmcs12->guest_cr0 & X86_CR0_PG) && > ia32e != !!(vmcs12->guest_ia32_efer & EFER_LME))) > - return 1; > + > + return VMX_EXIT_REASONS_FAILED_VMENTRY | > + EXIT_REASON_INVALID_STATE; > } > > if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_BNDCFGS) && > (is_noncanonical_address(vmcs12->guest_bndcfgs & PAGE_MASK, vcpu) || > (vmcs12->guest_bndcfgs & MSR_IA32_BNDCFGS_RSVD))) > - return 1; > + > + return VMX_EXIT_REASONS_FAILED_VMENTRY | > + EXIT_REASON_INVALID_STATE; > > return 0; > } > This gives the reader a false impression that the return value is actually reflected in the exit reason If anything I would change those to -EINVAL, similar to what you did in patch 4 (but without applying patch 3 which, as I understand it, is mostly a "trick" to make this patch less verbose). Paolo
On Wed, Apr 10, 2019 at 11:50:50AM +0200, Paolo Bonzini wrote: > On 08/04/19 23:35, Krish Sadhukhan wrote: > > ..to reflect the architectural Exit Reason for VM-entry failures due to > > invalid guest state. > > > > Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> > > Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com> > > --- > > arch/x86/kvm/vmx/nested.c | 15 +++++++++++---- > > 1 file changed, 11 insertions(+), 4 deletions(-) > > > > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c > > index 1ec5ddc4ea50..bde17d079a36 100644 > > --- a/arch/x86/kvm/vmx/nested.c > > +++ b/arch/x86/kvm/vmx/nested.c > > @@ -2701,11 +2701,14 @@ static int nested_vmx_check_vmentry_postreqs(struct kvm_vcpu *vcpu, > > *exit_qual = ENTRY_FAIL_DEFAULT; > > > > if (nested_check_guest_cregs_dregs_msrs(vcpu, vmcs12)) > > - return 1; > > + return VMX_EXIT_REASONS_FAILED_VMENTRY | > > + EXIT_REASON_INVALID_STATE; > > > > if (nested_vmx_check_vmcs_link_ptr(vcpu, vmcs12)) { > > *exit_qual = ENTRY_FAIL_VMCS_LINK_PTR; > > - return 1; > > + > > + return VMX_EXIT_REASONS_FAILED_VMENTRY | > > + EXIT_REASON_INVALID_STATE; > > } > > > > /* > > @@ -2724,13 +2727,17 @@ static int nested_vmx_check_vmentry_postreqs(struct kvm_vcpu *vcpu, > > ia32e != !!(vmcs12->guest_ia32_efer & EFER_LMA) || > > ((vmcs12->guest_cr0 & X86_CR0_PG) && > > ia32e != !!(vmcs12->guest_ia32_efer & EFER_LME))) > > - return 1; > > + > > + return VMX_EXIT_REASONS_FAILED_VMENTRY | > > + EXIT_REASON_INVALID_STATE; > > } > > > > if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_BNDCFGS) && > > (is_noncanonical_address(vmcs12->guest_bndcfgs & PAGE_MASK, vcpu) || > > (vmcs12->guest_bndcfgs & MSR_IA32_BNDCFGS_RSVD))) > > - return 1; > > + > > + return VMX_EXIT_REASONS_FAILED_VMENTRY | > > + EXIT_REASON_INVALID_STATE; > > > > return 0; > > } > > > > This gives the reader a false impression that the return value is > actually reflected in the exit reason If anything I would change those > to -EINVAL, similar to what you did in patch 4 (but without applying > patch 3 which, as I understand it, is mostly a "trick" to make this > patch less verbose). Good point, though IMO it'd be better to go one step further and actually consume the return value in nested_vmx_enter_non_root_mode(). For me, having the exit reason in nested_vmx_check_vmentry_postreqs() is a nice mental reminder that "postreqs" is referring to checks that happen once the CPU has "committed" to VM-Enter. What about the attached patch as fixup? From 377aa4cc91aa2af140ade76f61c30040a77fd660 Mon Sep 17 00:00:00 2001 From: Sean Christopherson <sean.j.christopherson@intel.com> Date: Wed, 10 Apr 2019 09:03:16 -0700 Subject: [PATCH] KVM: nVMX: Fixup nested_vmx_check_vmentry_postreqs() return val handling Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- arch/x86/kvm/vmx/nested.c | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c index 6bbb28ef313e..94405dcaccec 100644 --- a/arch/x86/kvm/vmx/nested.c +++ b/arch/x86/kvm/vmx/nested.c @@ -2691,14 +2691,11 @@ static int nested_vmx_check_vmentry_postreqs(struct kvm_vcpu *vcpu, *exit_qual = ENTRY_FAIL_DEFAULT; if (nested_check_guest_cregs_dregs_msrs(vcpu, vmcs12)) - return VMX_EXIT_REASONS_FAILED_VMENTRY | - EXIT_REASON_INVALID_STATE; + return EXIT_REASON_INVALID_STATE; if (nested_vmx_check_vmcs_link_ptr(vcpu, vmcs12)) { *exit_qual = ENTRY_FAIL_VMCS_LINK_PTR; - - return VMX_EXIT_REASONS_FAILED_VMENTRY | - EXIT_REASON_INVALID_STATE; + return EXIT_REASON_INVALID_STATE; } /* @@ -2717,17 +2714,13 @@ static int nested_vmx_check_vmentry_postreqs(struct kvm_vcpu *vcpu, ia32e != !!(vmcs12->guest_ia32_efer & EFER_LMA) || ((vmcs12->guest_cr0 & X86_CR0_PG) && ia32e != !!(vmcs12->guest_ia32_efer & EFER_LME))) - - return VMX_EXIT_REASONS_FAILED_VMENTRY | - EXIT_REASON_INVALID_STATE; + return EXIT_REASON_INVALID_STATE; } if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_BNDCFGS) && (is_noncanonical_address(vmcs12->guest_bndcfgs & PAGE_MASK, vcpu) || (vmcs12->guest_bndcfgs & MSR_IA32_BNDCFGS_RSVD))) - - return VMX_EXIT_REASONS_FAILED_VMENTRY | - EXIT_REASON_INVALID_STATE; + return EXIT_REASON_INVALID_STATE; return 0; } @@ -2975,7 +2968,7 @@ int nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu, bool from_vmentry) struct vcpu_vmx *vmx = to_vmx(vcpu); struct vmcs12 *vmcs12 = get_vmcs12(vcpu); bool evaluate_pending_interrupts; - u32 exit_reason = EXIT_REASON_INVALID_STATE; + u32 exit_reason; u32 exit_qual; evaluate_pending_interrupts = vmcs_read32(CPU_BASED_VM_EXEC_CONTROL) & @@ -3001,7 +2994,9 @@ int nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu, bool from_vmentry) return -1; } - if (nested_vmx_check_vmentry_postreqs(vcpu, vmcs12, &exit_qual)) + exit_reason = nested_vmx_check_vmentry_postreqs(vcpu, vmcs12, + &exit_qual); + if (exit_reason) goto vmentry_fail_vmexit; }
On 10/04/19 18:08, Sean Christopherson wrote: > Good point, though IMO it'd be better to go one step further and actually > consume the return value in nested_vmx_enter_non_root_mode(). For me, > having the exit reason in nested_vmx_check_vmentry_postreqs() is a nice > mental reminder that "postreqs" is referring to checks that happen once > the CPU has "committed" to VM-Enter. It's certainly better if you don't have to return VMX_EXIT_REASONS_FAILED_VMENTRY. However, I think it still complicates things a bit, after all the result is always EXIT_REASON_INVALID_STATE. The SDM says "VM-entry failure due to invalid guest state. A VM entry failed one of the checks identified in Section 26.3.1" so the bool (or 0/-EINVAL) return code is a nice reminder that the function covers a subset of 26.3.1. Paolo
On Wed, Apr 10, 2019 at 07:05:30PM +0200, Paolo Bonzini wrote: > On 10/04/19 18:08, Sean Christopherson wrote: > > Good point, though IMO it'd be better to go one step further and actually > > consume the return value in nested_vmx_enter_non_root_mode(). For me, > > having the exit reason in nested_vmx_check_vmentry_postreqs() is a nice > > mental reminder that "postreqs" is referring to checks that happen once > > the CPU has "committed" to VM-Enter. > > It's certainly better if you don't have to return > VMX_EXIT_REASONS_FAILED_VMENTRY. However, I think it still complicates > things a bit, after all the result is always EXIT_REASON_INVALID_STATE. > > The SDM says "VM-entry failure due to invalid guest state. A VM entry > failed one of the checks identified in Section 26.3.1" so the bool (or > 0/-EINVAL) return code is a nice reminder that the function covers a > subset of 26.3.1. Heh, for me, returning EXIT_REASON_INVALID_STATE is the reminder that the function covers 26.3.1. What if we rename the function to nested_vmx_check_vmentry_guest_state()? My desire to return the exit reason mostly stems from the name "postreqs" since I tend to forget what "postreqs" is referring to. And it'd be more appropriate since the MSR load checks are handled elsewhere and really should be considered "postreqs" as well.
On 04/10/2019 10:55 AM, Sean Christopherson wrote: > On Wed, Apr 10, 2019 at 07:05:30PM +0200, Paolo Bonzini wrote: >> On 10/04/19 18:08, Sean Christopherson wrote: >>> Good point, though IMO it'd be better to go one step further and actually >>> consume the return value in nested_vmx_enter_non_root_mode(). For me, >>> having the exit reason in nested_vmx_check_vmentry_postreqs() is a nice >>> mental reminder that "postreqs" is referring to checks that happen once >>> the CPU has "committed" to VM-Enter. >> It's certainly better if you don't have to return >> VMX_EXIT_REASONS_FAILED_VMENTRY. However, I think it still complicates >> things a bit, after all the result is always EXIT_REASON_INVALID_STATE. >> >> The SDM says "VM-entry failure due to invalid guest state. A VM entry >> failed one of the checks identified in Section 26.3.1" so the bool (or >> 0/-EINVAL) return code is a nice reminder that the function covers a >> subset of 26.3.1. > Heh, for me, returning EXIT_REASON_INVALID_STATE is the reminder that > the function covers 26.3.1. I think the cleaner approach would be to return -EINVAL in both *_pre{post}reqs functions and carry the exit code via a new function parameter. That will make the code more readable. > What if we rename the function to nested_vmx_check_vmentry_guest_state()? > My desire to return the exit reason mostly stems from the name "postreqs" > since I tend to forget what "postreqs" is referring to. And it'd be more > appropriate since the MSR load checks are handled elsewhere and really > should be considered "postreqs" as well. May be we should also rename its counterpart (i.e.,nested_vmx_check_vmentry_prereqs) to something like nested_vmx_check_vmentry_pre_guest_state or something similar ? The SDM doesn't explicitly mention whether MSR loading happens right after guest state is loaded, though it seems that's the order. If so, we should probably put these checks in function called nested_vmx_check_vmentry_msr_load or something like that.
On 11/04/19 02:15, Krish Sadhukhan wrote: > May be we should also rename its counterpart > (i.e.,nested_vmx_check_vmentry_prereqs) to something like > nested_vmx_check_vmentry_pre_guest_state or something similar ? I think we should have three functions nested_vmx_vmentry_check_{controls,host_state,guest_state}. Paolo
On Thu, Apr 11, 2019 at 02:14:35PM +0200, Paolo Bonzini wrote: > On 11/04/19 02:15, Krish Sadhukhan wrote: > > May be we should also rename its counterpart > > (i.e.,nested_vmx_check_vmentry_prereqs) to something like > > nested_vmx_check_vmentry_pre_guest_state or something similar ? > > I think we should have three functions > nested_vmx_vmentry_check_{controls,host_state,guest_state}. Four if you count nested_vmx_load_msr(). But anyways, I agree. On a related subject, an invalid guest activity state results in a VM-Exit, not VM-Fail, i.e. nested_check_guest_non_reg_state() belongs in nested_vmx_check_vmentry_postreqs(). Moving that code to its proper location would eliminate the last odditiy for renaming "postreqs" to nested_vmx_vmentry_check_guest_state(). I'll send a series, including patches 1 and 2 (the PAT checks) from Krish's series?
On 04/11/2019 09:29 AM, Sean Christopherson wrote: > On Thu, Apr 11, 2019 at 02:14:35PM +0200, Paolo Bonzini wrote: >> On 11/04/19 02:15, Krish Sadhukhan wrote: >>> May be we should also rename its counterpart >>> (i.e.,nested_vmx_check_vmentry_prereqs) to something like >>> nested_vmx_check_vmentry_pre_guest_state or something similar ? >> I think we should have three functions >> nested_vmx_vmentry_check_{controls,host_state,guest_state}. > Four if you count nested_vmx_load_msr(). But anyways, I agree. > > On a related subject, an invalid guest activity state results in a VM-Exit, > not VM-Fail, i.e. nested_check_guest_non_reg_state() belongs in > nested_vmx_check_vmentry_postreqs(). That's correct ! > Moving that code to its proper > location would eliminate the last odditiy for renaming "postreqs" to > nested_vmx_vmentry_check_guest_state(). > > I'll send a series, including patches 1 and 2 (the PAT checks) from > Krish's series? Sounds good. Thanks.
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c index 1ec5ddc4ea50..bde17d079a36 100644 --- a/arch/x86/kvm/vmx/nested.c +++ b/arch/x86/kvm/vmx/nested.c @@ -2701,11 +2701,14 @@ static int nested_vmx_check_vmentry_postreqs(struct kvm_vcpu *vcpu, *exit_qual = ENTRY_FAIL_DEFAULT; if (nested_check_guest_cregs_dregs_msrs(vcpu, vmcs12)) - return 1; + return VMX_EXIT_REASONS_FAILED_VMENTRY | + EXIT_REASON_INVALID_STATE; if (nested_vmx_check_vmcs_link_ptr(vcpu, vmcs12)) { *exit_qual = ENTRY_FAIL_VMCS_LINK_PTR; - return 1; + + return VMX_EXIT_REASONS_FAILED_VMENTRY | + EXIT_REASON_INVALID_STATE; } /* @@ -2724,13 +2727,17 @@ static int nested_vmx_check_vmentry_postreqs(struct kvm_vcpu *vcpu, ia32e != !!(vmcs12->guest_ia32_efer & EFER_LMA) || ((vmcs12->guest_cr0 & X86_CR0_PG) && ia32e != !!(vmcs12->guest_ia32_efer & EFER_LME))) - return 1; + + return VMX_EXIT_REASONS_FAILED_VMENTRY | + EXIT_REASON_INVALID_STATE; } if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_BNDCFGS) && (is_noncanonical_address(vmcs12->guest_bndcfgs & PAGE_MASK, vcpu) || (vmcs12->guest_bndcfgs & MSR_IA32_BNDCFGS_RSVD))) - return 1; + + return VMX_EXIT_REASONS_FAILED_VMENTRY | + EXIT_REASON_INVALID_STATE; return 0; }
..to reflect the architectural Exit Reason for VM-entry failures due to invalid guest state. Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com> --- arch/x86/kvm/vmx/nested.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)