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@xxxxxxxxxx>
> Reviewed-by: Mihai Carabas <mihai.carabas@xxxxxxxxxx>
> Reviewed-by: Mark Kanda <mark.kanda@xxxxxxxxxx>
> ---
> 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
>
