Does this change play well with the other functions that toggle
secondary control bits without updating vmx->secondary_exec_control?
e.g. vmx_disable_shadow_vmcs, set_current_vmptr,
vmx_set_virtual_x2apic_mode, vmcs_set_secondary_exec_control, and
possibly others?
Perhaps the proposed call to vmcs_write32(SECONDARY_VM_EXEC_CONTROL,
vmx->secondary_exec_control) should instead be a call to
vmcs_set_secondary_exec_control(vmx->secondary_exec_control)?
On Wed, Nov 22, 2017 at 2:23 AM, Arbel Moshe <arbel.moshe@xxxxxxxxxx> wrote:
> Handle apicv secondary exec ctrls correctly, without duplicating
> logic.
> This commit doesn't change semantics.
>
> Signed-off-by: Arbel Moshe <arbel.moshe@xxxxxxxxxx>
> Reviewed-by: Liran Alon <liran.alon@xxxxxxxxxx>
> Reviewed-by: Krish Sadhukhan <krish.sadhukhan@xxxxxxxxxx>
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@xxxxxxxxxx>
> ---
> arch/x86/kvm/vmx.c | 14 ++++++--------
> 1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 7c3522a989d0..84ccd3b2762c 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -5237,20 +5237,18 @@ static u32 vmx_pin_based_exec_ctrl(struct vcpu_vmx *vmx)
> return pin_based_exec_ctrl;
> }
>
> +static void vmx_compute_secondary_exec_control(struct vcpu_vmx *vmx);
> +
> static void vmx_refresh_apicv_exec_ctrl(struct kvm_vcpu *vcpu)
> {
> struct vcpu_vmx *vmx = to_vmx(vcpu);
>
> vmcs_write32(PIN_BASED_VM_EXEC_CONTROL, vmx_pin_based_exec_ctrl(vmx));
> +
> if (cpu_has_secondary_exec_ctrls()) {
> - if (kvm_vcpu_apicv_active(vcpu))
> - vmcs_set_bits(SECONDARY_VM_EXEC_CONTROL,
> - SECONDARY_EXEC_APIC_REGISTER_VIRT |
> - SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY);
> - else
> - vmcs_clear_bits(SECONDARY_VM_EXEC_CONTROL,
> - SECONDARY_EXEC_APIC_REGISTER_VIRT |
> - SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY);
> + vmx_compute_secondary_exec_control(vmx);
> + vmcs_write32(SECONDARY_VM_EXEC_CONTROL,
> + vmx->secondary_exec_control);
> }
>
> if (cpu_has_vmx_msr_bitmap())
> --
> 2.14.1
>
