diff mbox series

[2/7] KVM: nVMX: Move the checks for VM-Execution Control Fields to a separate helper function

Message ID 20181130190438.13591-3-krish.sadhukhan@oracle.com (mailing list archive)
State New, archived
Headers show
Series [1/7] KVM: nVMX: Prepend "nested_" to check_vmentry_{pre,post}reqs() | expand

Commit Message

Krish Sadhukhan Nov. 30, 2018, 7:04 p.m. UTC 
.. to improve readability and maintainability, and to align the code as per
the layout of the checks in chapter "VM Entries" in Intel SDM vol 3C.

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Reviewed-by: Mihai Carabas <mihai.carabas@oracle.com>
Reviewed-by: Mark Kanda <mark.kanda@oracle.com>
---
 arch/x86/kvm/vmx.c | 85 +++++++++++++++++++++++++-----------------------------
 1 file changed, 40 insertions(+), 45 deletions(-)

Comments

Jim Mattson Dec. 3, 2018, 7:40 p.m. UTC | #1 
On Fri, Nov 30, 2018 at 11:28 AM Krish Sadhukhan
<krish.sadhukhan@oracle.com> wrote:
>
> .. to improve readability and maintainability, and to align the code as per
> the layout of the checks in chapter "VM Entries" in Intel SDM vol 3C.
>
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Reviewed-by: Mihai Carabas <mihai.carabas@oracle.com>
> Reviewed-by: Mark Kanda <mark.kanda@oracle.com>

I'm not sure that you've accomplished your goal. For instance, the
very first bullet point under section 26.2.1.1: VM-Execution Control
Fields is:
o  Reserved bits in the pin-based VM-execution controls must be set properly.

Yet, you didn't move that check into nested_check_vm_execution_controls().

I also think that it would be extremely helpful if this reorganization
resulted with all of the tests in the same order in which they are
enumerated in the SDM.
Krish Sadhukhan Dec. 3, 2018, 8:24 p.m. UTC | #2 
On 12/03/2018 11:40 AM, Jim Mattson wrote:
> On Fri, Nov 30, 2018 at 11:28 AM Krish Sadhukhan
> <krish.sadhukhan@oracle.com> wrote:
>> .. to improve readability and maintainability, and to align the code as per
>> the layout of the checks in chapter "VM Entries" in Intel SDM vol 3C.
>>
>> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
>> Reviewed-by: Mihai Carabas <mihai.carabas@oracle.com>
>> Reviewed-by: Mark Kanda <mark.kanda@oracle.com>
> I'm not sure that you've accomplished your goal. For instance, the
> very first bullet point under section 26.2.1.1: VM-Execution Control
> Fields is:
> o  Reserved bits in the pin-based VM-execution controls must be set properly.
>
> Yet, you didn't move that check into nested_check_vm_execution_controls().

Are you referring to the following check ?

!vmx_control_verify(vmcs12->pin_based_vm_exec_control,
vmx->nested.msrs.pinbased_ctls_low,
vmx->nested.msrs.pinbased_ctls_high) ||

>
> I also think that it would be extremely helpful if this reorganization
> resulted with all of the tests in the same order in which they are
> enumerated in the SDM.

Will check the order in the SDM and re-order the code accordingly.
Jim Mattson Dec. 3, 2018, 9:06 p.m. UTC | #3 
On Mon, Dec 3, 2018 at 12:24 PM Krish Sadhukhan
<krish.sadhukhan@oracle.com> wrote:
>
>
>
> On 12/03/2018 11:40 AM, Jim Mattson wrote:
> > On Fri, Nov 30, 2018 at 11:28 AM Krish Sadhukhan
> > <krish.sadhukhan@oracle.com> wrote:
> >> .. to improve readability and maintainability, and to align the code as per
> >> the layout of the checks in chapter "VM Entries" in Intel SDM vol 3C.
> >>
> >> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> >> Reviewed-by: Mihai Carabas <mihai.carabas@oracle.com>
> >> Reviewed-by: Mark Kanda <mark.kanda@oracle.com>
> > I'm not sure that you've accomplished your goal. For instance, the
> > very first bullet point under section 26.2.1.1: VM-Execution Control
> > Fields is:
> > o  Reserved bits in the pin-based VM-execution controls must be set properly.
> >
> > Yet, you didn't move that check into nested_check_vm_execution_controls().
>
> Are you referring to the following check ?
>
> !vmx_control_verify(vmcs12->pin_based_vm_exec_control,
> vmx->nested.msrs.pinbased_ctls_low,
> vmx->nested.msrs.pinbased_ctls_high) ||

That's the one.
Krish Sadhukhan Dec. 3, 2018, 10:24 p.m. UTC | #4 
On 12/03/2018 01:06 PM, Jim Mattson wrote:
> On Mon, Dec 3, 2018 at 12:24 PM Krish Sadhukhan
> <krish.sadhukhan@oracle.com> wrote:
>>
>>
>> On 12/03/2018 11:40 AM, Jim Mattson wrote:
>>> On Fri, Nov 30, 2018 at 11:28 AM Krish Sadhukhan
>>> <krish.sadhukhan@oracle.com> wrote:
>>>> .. to improve readability and maintainability, and to align the code as per
>>>> the layout of the checks in chapter "VM Entries" in Intel SDM vol 3C.
>>>>
>>>> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
>>>> Reviewed-by: Mihai Carabas <mihai.carabas@oracle.com>
>>>> Reviewed-by: Mark Kanda <mark.kanda@oracle.com>
>>> I'm not sure that you've accomplished your goal. For instance, the
>>> very first bullet point under section 26.2.1.1: VM-Execution Control
>>> Fields is:
>>> o  Reserved bits in the pin-based VM-execution controls must be set properly.
>>>
>>> Yet, you didn't move that check into nested_check_vm_execution_controls().
>> Are you referring to the following check ?
>>
>> !vmx_control_verify(vmcs12->pin_based_vm_exec_control,
>> vmx->nested.msrs.pinbased_ctls_low,
>> vmx->nested.msrs.pinbased_ctls_high) ||
> That's the one.

It is in nested_check_vm_execution_controls().  Am I missing something ?
Jim Mattson Dec. 3, 2018, 10:29 p.m. UTC | #5 
On Mon, Dec 3, 2018 at 2:24 PM Krish Sadhukhan
<krish.sadhukhan@oracle.com> wrote:
>
>
>
> On 12/03/2018 01:06 PM, Jim Mattson wrote:
> > On Mon, Dec 3, 2018 at 12:24 PM Krish Sadhukhan
> > <krish.sadhukhan@oracle.com> wrote:
> >>
> >>
> >> On 12/03/2018 11:40 AM, Jim Mattson wrote:
> >>> On Fri, Nov 30, 2018 at 11:28 AM Krish Sadhukhan
> >>> <krish.sadhukhan@oracle.com> wrote:
> >>>> .. to improve readability and maintainability, and to align the code as per
> >>>> the layout of the checks in chapter "VM Entries" in Intel SDM vol 3C.
> >>>>
> >>>> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> >>>> Reviewed-by: Mihai Carabas <mihai.carabas@oracle.com>
> >>>> Reviewed-by: Mark Kanda <mark.kanda@oracle.com>
> >>> I'm not sure that you've accomplished your goal. For instance, the
> >>> very first bullet point under section 26.2.1.1: VM-Execution Control
> >>> Fields is:
> >>> o  Reserved bits in the pin-based VM-execution controls must be set properly.
> >>>
> >>> Yet, you didn't move that check into nested_check_vm_execution_controls().
> >> Are you referring to the following check ?
> >>
> >> !vmx_control_verify(vmcs12->pin_based_vm_exec_control,
> >> vmx->nested.msrs.pinbased_ctls_low,
> >> vmx->nested.msrs.pinbased_ctls_high) ||
> > That's the one.
>
> It is in nested_check_vm_execution_controls().  Am I missing something ?

Maybe I just don't read diffs very well. :-)
Sean Christopherson Dec. 4, 2018, 5:30 p.m. UTC | #6 
On Fri, Nov 30, 2018 at 02:04:33PM -0500, Krish Sadhukhan wrote:
> .. to improve readability and maintainability, and to align the code as per
> the layout of the checks in chapter "VM Entries" in Intel SDM vol 3C.
> 
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Reviewed-by: Mihai Carabas <mihai.carabas@oracle.com>
> Reviewed-by: Mark Kanda <mark.kanda@oracle.com>
> ---
>  arch/x86/kvm/vmx.c | 85 +++++++++++++++++++++++++-----------------------------
>  1 file changed, 40 insertions(+), 45 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 3a74a4c..27892e8 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -13003,44 +13003,23 @@ static int nested_vmx_check_nmi_controls(struct vmcs12 *vmcs12)
>  	return 0;
>  }
>  
> -static int nested_check_vmentry_prereqs(struct kvm_vcpu *vcpu,
> -					struct vmcs12 *vmcs12)
> +/*
> + * Checks related to VM-Execution Control Fields
> + */
> +static int nested_check_vm_execution_controls(struct kvm_vcpu *vcpu,
> +                                              struct vmcs12 *vmcs12)
>  {
>  	struct vcpu_vmx *vmx = to_vmx(vcpu);
> -	bool ia32e;
> -
> -	if (vmcs12->guest_activity_state != GUEST_ACTIVITY_ACTIVE &&
> -	    vmcs12->guest_activity_state != GUEST_ACTIVITY_HLT)
> -		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
> -
> -	if (nested_cpu_has_vpid(vmcs12) && !vmcs12->virtual_processor_id)
> -		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
> -
> -	if (nested_vmx_check_io_bitmap_controls(vcpu, vmcs12))
> -		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
> -
> -	if (nested_vmx_check_msr_bitmap_controls(vcpu, vmcs12))
> -		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
>  
> -	if (nested_vmx_check_apic_access_controls(vcpu, vmcs12))
> -		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
> -
> -	if (nested_vmx_check_tpr_shadow_controls(vcpu, vmcs12))
> -		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
> -
> -	if (nested_vmx_check_apicv_controls(vcpu, vmcs12))
> -		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
> -
> -	if (nested_vmx_check_msr_switch_controls(vcpu, vmcs12))
> -		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
> -
> -	if (nested_vmx_check_pml_controls(vcpu, vmcs12))
> -		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
> -
> -	if (nested_vmx_check_shadow_vmcs_controls(vcpu, vmcs12))
> -		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
> -
> -	if (!vmx_control_verify(vmcs12->cpu_based_vm_exec_control,
> +	if ((nested_cpu_has_vpid(vmcs12) && !vmcs12->virtual_processor_id) ||
> +	    nested_vmx_check_io_bitmap_controls(vcpu, vmcs12) ||
> +	    nested_vmx_check_msr_bitmap_controls(vcpu, vmcs12) ||
> +	    nested_vmx_check_apic_access_controls(vcpu, vmcs12) ||
> +	    nested_vmx_check_tpr_shadow_controls(vcpu, vmcs12) ||
> +	    nested_vmx_check_apicv_controls(vcpu, vmcs12) ||
> +	    nested_vmx_check_pml_controls(vcpu, vmcs12) ||
> +	    nested_vmx_check_shadow_vmcs_controls(vcpu, vmcs12) ||
> +	    !vmx_control_verify(vmcs12->cpu_based_vm_exec_control,
>  				vmx->nested.msrs.procbased_ctls_low,
>  				vmx->nested.msrs.procbased_ctls_high) ||
>  	    (nested_cpu_has(vmcs12, CPU_BASED_ACTIVATE_SECONDARY_CONTROLS) &&

Since you're already going through the effort of condensing the code,
I think it makes sense to do a bit of reordering.  Specifically, move
all calls to vmx_control_verify() to the top of the function.  Every
time I look at this code I can't help but have a knee jerk reaction
wondering why we're consuming e.g. the VPID control bit without first
checking that the secondary execution control field is valid.  IMO
this is even more readable and also aligns better with the SDM.

E.g.:

        ...

        if (!vmx_control_verify(vmcs12->cpu_based_vm_exec_control,
                                vmx->nested.msrs.procbased_ctls_low,
                                vmx->nested.msrs.procbased_ctls_high) ||
            (nested_cpu_has(vmcs12, CPU_BASED_ACTIVATE_SECONDARY_CONTROLS) &&
             !vmx_control_verify(vmcs12->secondary_vm_exec_control,
                                 vmx->nested.msrs.secondary_ctls_low,
                                 vmx->nested.msrs.secondary_ctls_high)) ||
            !vmx_control_verify(vmcs12->pin_based_vm_exec_control,
                                vmx->nested.msrs.pinbased_ctls_low,
                                vmx->nested.msrs.pinbased_ctls_high) ||
            !vmx_control_verify(vmcs12->vm_exit_controls,
                                vmx->nested.msrs.exit_ctls_low,
                                vmx->nested.msrs.exit_ctls_high) ||
            !vmx_control_verify(vmcs12->vm_entry_controls,
                                vmx->nested.msrs.entry_ctls_low,
                                vmx->nested.msrs.entry_ctls_high))
                return -EINVAL;

        if ((nested_cpu_has_vpid(vmcs12) && !vmcs12->virtual_processor_id) ||
            nested_vmx_check_io_bitmap_controls(vcpu, vmcs12) ||
            nested_vmx_check_msr_bitmap_controls(vcpu, vmcs12) ||
            nested_vmx_check_apic_access_controls(vcpu, vmcs12) ||
            nested_vmx_check_tpr_shadow_controls(vcpu, vmcs12) ||
            nested_vmx_check_apicv_controls(vcpu, vmcs12) ||
            nested_vmx_check_pml_controls(vcpu, vmcs12) ||
            nested_vmx_check_shadow_vmcs_controls(vcpu, vmcs12) ||
            nested_vmx_check_nmi_controls(vmcs12))
                return -EINVAL;

> @@ -13055,11 +13034,9 @@ static int nested_check_vmentry_prereqs(struct kvm_vcpu *vcpu,
>  				vmx->nested.msrs.exit_ctls_high) ||
>  	    !vmx_control_verify(vmcs12->vm_entry_controls,
>  				vmx->nested.msrs.entry_ctls_low,
> -				vmx->nested.msrs.entry_ctls_high))
> -		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
> -
> -	if (nested_vmx_check_nmi_controls(vmcs12))
> -		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
> +				vmx->nested.msrs.entry_ctls_high) ||
> +	    nested_vmx_check_nmi_controls(vmcs12))
> +		return -EINVAL;
>  
>  	if (nested_cpu_has_vmfunc(vmcs12)) {
>  		if (vmcs12->vm_function_control &
> @@ -13069,11 +13046,33 @@ static int nested_check_vmentry_prereqs(struct kvm_vcpu *vcpu,
>  		if (nested_cpu_has_eptp_switching(vmcs12)) {
>  			if (!nested_cpu_has_ept(vmcs12) ||
>  			    !page_address_valid(vcpu, vmcs12->eptp_list_address))
> -				return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
> +				return -EINVAL;
>  		}

Doesn't have to be in this series, but this is a good candidate for a
nested_vmx_check_vmfunc_controls() helper.  But if you do add that as an
earlier patch then we'd end up with two (big) if statement, e.g. one for
the control fields and one for the in-depth checks.

>  	}
>  
>  	if (vmcs12->cr3_target_count > nested_cpu_vmx_misc_cr3_count(vcpu))
> +		return -EINVAL;
> +
> +	if (nested_cpu_has_ept(vmcs12) &&
> +	    !valid_ept_address(vcpu, vmcs12->ept_pointer))
> +		return -EINVAL;a

These can be smushed into the condensed check.

> +
> +	return 0;
> +}
> +
> +static int nested_check_vmentry_prereqs(struct kvm_vcpu *vcpu,
> +					struct vmcs12 *vmcs12)
> +{
> +	bool ia32e;
> +
> +	if (vmcs12->guest_activity_state != GUEST_ACTIVITY_ACTIVE &&
> +	    vmcs12->guest_activity_state != GUEST_ACTIVITY_HLT)
> +		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
> +
> +	if (nested_check_vm_execution_controls(vcpu, vmcs12))
> +		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
> +
> +	if (nested_vmx_check_msr_switch_controls(vcpu, vmcs12))
>  		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
>  
>  	if (!nested_host_cr0_valid(vcpu, vmcs12->host_cr0) ||
> @@ -13152,10 +13151,6 @@ static int nested_check_vmentry_prereqs(struct kvm_vcpu *vcpu,
>  		}
>  	}
>  
> -	if (nested_cpu_has_ept(vmcs12) &&
> -	    !valid_ept_address(vcpu, vmcs12->ept_pointer))
> -		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
> -
>  	return 0;
>  }
>  
> -- 
> 2.9.5
>
Sean Christopherson Dec. 4, 2018, 5:34 p.m. UTC | #7 
On Tue, Dec 04, 2018 at 09:30:40AM -0800, Sean Christopherson wrote:
> On Fri, Nov 30, 2018 at 02:04:33PM -0500, Krish Sadhukhan wrote:
> > .. to improve readability and maintainability, and to align the code as per
> > the layout of the checks in chapter "VM Entries" in Intel SDM vol 3C.
> > 
> > Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> > Reviewed-by: Mihai Carabas <mihai.carabas@oracle.com>
> > Reviewed-by: Mark Kanda <mark.kanda@oracle.com>
> > ---
> >  arch/x86/kvm/vmx.c | 85 +++++++++++++++++++++++++-----------------------------
> >  1 file changed, 40 insertions(+), 45 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > index 3a74a4c..27892e8 100644
> > --- a/arch/x86/kvm/vmx.c
> > +++ b/arch/x86/kvm/vmx.c
> > @@ -13003,44 +13003,23 @@ static int nested_vmx_check_nmi_controls(struct vmcs12 *vmcs12)
> >  	return 0;
> >  }
> >  
> > -static int nested_check_vmentry_prereqs(struct kvm_vcpu *vcpu,
> > -					struct vmcs12 *vmcs12)
> > +/*
> > + * Checks related to VM-Execution Control Fields
> > + */
> > +static int nested_check_vm_execution_controls(struct kvm_vcpu *vcpu,
> > +                                              struct vmcs12 *vmcs12)
> >  {
> >  	struct vcpu_vmx *vmx = to_vmx(vcpu);
> > -	bool ia32e;
> > -
> > -	if (vmcs12->guest_activity_state != GUEST_ACTIVITY_ACTIVE &&
> > -	    vmcs12->guest_activity_state != GUEST_ACTIVITY_HLT)
> > -		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
> > -
> > -	if (nested_cpu_has_vpid(vmcs12) && !vmcs12->virtual_processor_id)
> > -		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
> > -
> > -	if (nested_vmx_check_io_bitmap_controls(vcpu, vmcs12))
> > -		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
> > -
> > -	if (nested_vmx_check_msr_bitmap_controls(vcpu, vmcs12))
> > -		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
> >  
> > -	if (nested_vmx_check_apic_access_controls(vcpu, vmcs12))
> > -		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
> > -
> > -	if (nested_vmx_check_tpr_shadow_controls(vcpu, vmcs12))
> > -		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
> > -
> > -	if (nested_vmx_check_apicv_controls(vcpu, vmcs12))
> > -		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
> > -
> > -	if (nested_vmx_check_msr_switch_controls(vcpu, vmcs12))
> > -		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
> > -
> > -	if (nested_vmx_check_pml_controls(vcpu, vmcs12))
> > -		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
> > -
> > -	if (nested_vmx_check_shadow_vmcs_controls(vcpu, vmcs12))
> > -		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
> > -
> > -	if (!vmx_control_verify(vmcs12->cpu_based_vm_exec_control,
> > +	if ((nested_cpu_has_vpid(vmcs12) && !vmcs12->virtual_processor_id) ||
> > +	    nested_vmx_check_io_bitmap_controls(vcpu, vmcs12) ||
> > +	    nested_vmx_check_msr_bitmap_controls(vcpu, vmcs12) ||
> > +	    nested_vmx_check_apic_access_controls(vcpu, vmcs12) ||
> > +	    nested_vmx_check_tpr_shadow_controls(vcpu, vmcs12) ||
> > +	    nested_vmx_check_apicv_controls(vcpu, vmcs12) ||
> > +	    nested_vmx_check_pml_controls(vcpu, vmcs12) ||
> > +	    nested_vmx_check_shadow_vmcs_controls(vcpu, vmcs12) ||
> > +	    !vmx_control_verify(vmcs12->cpu_based_vm_exec_control,
> >  				vmx->nested.msrs.procbased_ctls_low,
> >  				vmx->nested.msrs.procbased_ctls_high) ||
> >  	    (nested_cpu_has(vmcs12, CPU_BASED_ACTIVATE_SECONDARY_CONTROLS) &&
> 
> Since you're already going through the effort of condensing the code,
> I think it makes sense to do a bit of reordering.  Specifically, move
> all calls to vmx_control_verify() to the top of the function.  Every
> time I look at this code I can't help but have a knee jerk reaction
> wondering why we're consuming e.g. the VPID control bit without first
> checking that the secondary execution control field is valid.  IMO
> this is even more readable and also aligns better with the SDM.
>
> E.g.:
> 
>         ...
> 
>         if (!vmx_control_verify(vmcs12->cpu_based_vm_exec_control,
>                                 vmx->nested.msrs.procbased_ctls_low,
>                                 vmx->nested.msrs.procbased_ctls_high) ||
>             (nested_cpu_has(vmcs12, CPU_BASED_ACTIVATE_SECONDARY_CONTROLS) &&
>              !vmx_control_verify(vmcs12->secondary_vm_exec_control,
>                                  vmx->nested.msrs.secondary_ctls_low,
>                                  vmx->nested.msrs.secondary_ctls_high)) ||
>             !vmx_control_verify(vmcs12->pin_based_vm_exec_control,
>                                 vmx->nested.msrs.pinbased_ctls_low,
>                                 vmx->nested.msrs.pinbased_ctls_high) ||
>             !vmx_control_verify(vmcs12->vm_exit_controls,
>                                 vmx->nested.msrs.exit_ctls_low,
>                                 vmx->nested.msrs.exit_ctls_high) ||
>             !vmx_control_verify(vmcs12->vm_entry_controls,
>                                 vmx->nested.msrs.entry_ctls_low,
>                                 vmx->nested.msrs.entry_ctls_high))
>                 return -EINVAL;

I just realized that following patches break out vm_exit_controls() and
vm_entry_controls().  The associated vmx_control_verify() checks on the
fields themselves belong in their respective function, not here.

> 
>         if ((nested_cpu_has_vpid(vmcs12) && !vmcs12->virtual_processor_id) ||
>             nested_vmx_check_io_bitmap_controls(vcpu, vmcs12) ||
>             nested_vmx_check_msr_bitmap_controls(vcpu, vmcs12) ||
>             nested_vmx_check_apic_access_controls(vcpu, vmcs12) ||
>             nested_vmx_check_tpr_shadow_controls(vcpu, vmcs12) ||
>             nested_vmx_check_apicv_controls(vcpu, vmcs12) ||
>             nested_vmx_check_pml_controls(vcpu, vmcs12) ||
>             nested_vmx_check_shadow_vmcs_controls(vcpu, vmcs12) ||
>             nested_vmx_check_nmi_controls(vmcs12))
>                 return -EINVAL;
> 
> > @@ -13055,11 +13034,9 @@ static int nested_check_vmentry_prereqs(struct kvm_vcpu *vcpu,
> >  				vmx->nested.msrs.exit_ctls_high) ||
> >  	    !vmx_control_verify(vmcs12->vm_entry_controls,
> >  				vmx->nested.msrs.entry_ctls_low,
> > -				vmx->nested.msrs.entry_ctls_high))
> > -		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
> > -
> > -	if (nested_vmx_check_nmi_controls(vmcs12))
> > -		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
> > +				vmx->nested.msrs.entry_ctls_high) ||
> > +	    nested_vmx_check_nmi_controls(vmcs12))
> > +		return -EINVAL;
> >  
> >  	if (nested_cpu_has_vmfunc(vmcs12)) {
> >  		if (vmcs12->vm_function_control &
> > @@ -13069,11 +13046,33 @@ static int nested_check_vmentry_prereqs(struct kvm_vcpu *vcpu,
> >  		if (nested_cpu_has_eptp_switching(vmcs12)) {
> >  			if (!nested_cpu_has_ept(vmcs12) ||
> >  			    !page_address_valid(vcpu, vmcs12->eptp_list_address))
> > -				return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
> > +				return -EINVAL;
> >  		}
> 
> Doesn't have to be in this series, but this is a good candidate for a
> nested_vmx_check_vmfunc_controls() helper.  But if you do add that as an
> earlier patch then we'd end up with two (big) if statement, e.g. one for
> the control fields and one for the in-depth checks.
> 
> >  	}
> >  
> >  	if (vmcs12->cr3_target_count > nested_cpu_vmx_misc_cr3_count(vcpu))
> > +		return -EINVAL;
> > +
> > +	if (nested_cpu_has_ept(vmcs12) &&
> > +	    !valid_ept_address(vcpu, vmcs12->ept_pointer))
> > +		return -EINVAL;a
> 
> These can be smushed into the condensed check.
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static int nested_check_vmentry_prereqs(struct kvm_vcpu *vcpu,
> > +					struct vmcs12 *vmcs12)
> > +{
> > +	bool ia32e;
> > +
> > +	if (vmcs12->guest_activity_state != GUEST_ACTIVITY_ACTIVE &&
> > +	    vmcs12->guest_activity_state != GUEST_ACTIVITY_HLT)
> > +		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
> > +
> > +	if (nested_check_vm_execution_controls(vcpu, vmcs12))
> > +		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
> > +
> > +	if (nested_vmx_check_msr_switch_controls(vcpu, vmcs12))
> >  		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
> >  
> >  	if (!nested_host_cr0_valid(vcpu, vmcs12->host_cr0) ||
> > @@ -13152,10 +13151,6 @@ static int nested_check_vmentry_prereqs(struct kvm_vcpu *vcpu,
> >  		}
> >  	}
> >  
> > -	if (nested_cpu_has_ept(vmcs12) &&
> > -	    !valid_ept_address(vcpu, vmcs12->ept_pointer))
> > -		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
> > -
> >  	return 0;
> >  }
> >  
> > -- 
> > 2.9.5
> >
diff mbox series

Patch

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 3a74a4c..27892e8 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -13003,44 +13003,23 @@  static int nested_vmx_check_nmi_controls(struct vmcs12 *vmcs12)
 	return 0;
 }
 
-static int nested_check_vmentry_prereqs(struct kvm_vcpu *vcpu,
-					struct vmcs12 *vmcs12)
+/*
+ * Checks related to VM-Execution Control Fields
+ */
+static int nested_check_vm_execution_controls(struct kvm_vcpu *vcpu,
+                                              struct vmcs12 *vmcs12)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
-	bool ia32e;
-
-	if (vmcs12->guest_activity_state != GUEST_ACTIVITY_ACTIVE &&
-	    vmcs12->guest_activity_state != GUEST_ACTIVITY_HLT)
-		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
-
-	if (nested_cpu_has_vpid(vmcs12) && !vmcs12->virtual_processor_id)
-		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
-
-	if (nested_vmx_check_io_bitmap_controls(vcpu, vmcs12))
-		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
-
-	if (nested_vmx_check_msr_bitmap_controls(vcpu, vmcs12))
-		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
 
-	if (nested_vmx_check_apic_access_controls(vcpu, vmcs12))
-		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
-
-	if (nested_vmx_check_tpr_shadow_controls(vcpu, vmcs12))
-		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
-
-	if (nested_vmx_check_apicv_controls(vcpu, vmcs12))
-		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
-
-	if (nested_vmx_check_msr_switch_controls(vcpu, vmcs12))
-		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
-
-	if (nested_vmx_check_pml_controls(vcpu, vmcs12))
-		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
-
-	if (nested_vmx_check_shadow_vmcs_controls(vcpu, vmcs12))
-		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
-
-	if (!vmx_control_verify(vmcs12->cpu_based_vm_exec_control,
+	if ((nested_cpu_has_vpid(vmcs12) && !vmcs12->virtual_processor_id) ||
+	    nested_vmx_check_io_bitmap_controls(vcpu, vmcs12) ||
+	    nested_vmx_check_msr_bitmap_controls(vcpu, vmcs12) ||
+	    nested_vmx_check_apic_access_controls(vcpu, vmcs12) ||
+	    nested_vmx_check_tpr_shadow_controls(vcpu, vmcs12) ||
+	    nested_vmx_check_apicv_controls(vcpu, vmcs12) ||
+	    nested_vmx_check_pml_controls(vcpu, vmcs12) ||
+	    nested_vmx_check_shadow_vmcs_controls(vcpu, vmcs12) ||
+	    !vmx_control_verify(vmcs12->cpu_based_vm_exec_control,
 				vmx->nested.msrs.procbased_ctls_low,
 				vmx->nested.msrs.procbased_ctls_high) ||
 	    (nested_cpu_has(vmcs12, CPU_BASED_ACTIVATE_SECONDARY_CONTROLS) &&
@@ -13055,11 +13034,9 @@  static int nested_check_vmentry_prereqs(struct kvm_vcpu *vcpu,
 				vmx->nested.msrs.exit_ctls_high) ||
 	    !vmx_control_verify(vmcs12->vm_entry_controls,
 				vmx->nested.msrs.entry_ctls_low,
-				vmx->nested.msrs.entry_ctls_high))
-		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
-
-	if (nested_vmx_check_nmi_controls(vmcs12))
-		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
+				vmx->nested.msrs.entry_ctls_high) ||
+	    nested_vmx_check_nmi_controls(vmcs12))
+		return -EINVAL;
 
 	if (nested_cpu_has_vmfunc(vmcs12)) {
 		if (vmcs12->vm_function_control &
@@ -13069,11 +13046,33 @@  static int nested_check_vmentry_prereqs(struct kvm_vcpu *vcpu,
 		if (nested_cpu_has_eptp_switching(vmcs12)) {
 			if (!nested_cpu_has_ept(vmcs12) ||
 			    !page_address_valid(vcpu, vmcs12->eptp_list_address))
-				return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
+				return -EINVAL;
 		}
 	}
 
 	if (vmcs12->cr3_target_count > nested_cpu_vmx_misc_cr3_count(vcpu))
+		return -EINVAL;
+
+	if (nested_cpu_has_ept(vmcs12) &&
+	    !valid_ept_address(vcpu, vmcs12->ept_pointer))
+		return -EINVAL;
+
+	return 0;
+}
+
+static int nested_check_vmentry_prereqs(struct kvm_vcpu *vcpu,
+					struct vmcs12 *vmcs12)
+{
+	bool ia32e;
+
+	if (vmcs12->guest_activity_state != GUEST_ACTIVITY_ACTIVE &&
+	    vmcs12->guest_activity_state != GUEST_ACTIVITY_HLT)
+		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
+
+	if (nested_check_vm_execution_controls(vcpu, vmcs12))
+		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
+
+	if (nested_vmx_check_msr_switch_controls(vcpu, vmcs12))
 		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
 
 	if (!nested_host_cr0_valid(vcpu, vmcs12->host_cr0) ||
@@ -13152,10 +13151,6 @@  static int nested_check_vmentry_prereqs(struct kvm_vcpu *vcpu,
 		}
 	}
 
-	if (nested_cpu_has_ept(vmcs12) &&
-	    !valid_ept_address(vcpu, vmcs12->ept_pointer))
-		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
-
 	return 0;
 }