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@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;
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
> >
