On 09/05/2018 22:56, Krish Sadhukhan wrote:
> From: Jim Mattson <jmattson@xxxxxxxxxx>
>
> Previously, we toggled between SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE
> and SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES, depending on whether or
> not the EXTD bit was set in MSR_IA32_APICBASE. However, if the local
> APIC is disabled, we should not set either of these APIC
> virtualization control bits.
>
> Signed-off-by: Jim Mattson <jmattson@xxxxxxxxxx>
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@xxxxxxxxxx>
What is the buggy behavior that happens due to this? Can it be turned
into a testcase?
Paolo
> ---
> arch/x86/include/asm/kvm_host.h | 2 +-
> arch/x86/kvm/lapic.c | 12 +++++------
> arch/x86/kvm/svm.c | 4 ++--
> arch/x86/kvm/vmx.c | 48 +++++++++++++++++++++++++----------------
> 4 files changed, 38 insertions(+), 28 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index c25775f..52e685b 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -995,7 +995,7 @@ struct kvm_x86_ops {
> void (*hwapic_irr_update)(struct kvm_vcpu *vcpu, int max_irr);
> void (*hwapic_isr_update)(struct kvm_vcpu *vcpu, int isr);
> void (*load_eoi_exitmap)(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap);
> - void (*set_virtual_x2apic_mode)(struct kvm_vcpu *vcpu, bool set);
> + void (*set_virtual_apic_mode)(struct kvm_vcpu *vcpu);
> void (*set_apic_access_page_addr)(struct kvm_vcpu *vcpu, hpa_t hpa);
> void (*deliver_posted_interrupt)(struct kvm_vcpu *vcpu, int vector);
> int (*sync_pir_to_irr)(struct kvm_vcpu *vcpu);
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index b74c9c1..776391c 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -1990,13 +1990,11 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value)
> }
> }
>
> - if ((old_value ^ value) & X2APIC_ENABLE) {
> - if (value & X2APIC_ENABLE) {
> - kvm_apic_set_x2apic_id(apic, vcpu->vcpu_id);
> - kvm_x86_ops->set_virtual_x2apic_mode(vcpu, true);
> - } else
> - kvm_x86_ops->set_virtual_x2apic_mode(vcpu, false);
> - }
> + if (((old_value ^ value) & X2APIC_ENABLE) && (value & X2APIC_ENABLE))
> + kvm_apic_set_x2apic_id(apic, vcpu->vcpu_id);
> +
> + if ((old_value ^ value) & (MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE))
> + kvm_x86_ops->set_virtual_apic_mode(vcpu);
>
> apic->base_address = apic->vcpu->arch.apic_base &
> MSR_IA32_APICBASE_BASE;
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 1fc05e4..220e5a89 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -5036,7 +5036,7 @@ static void update_cr8_intercept(struct kvm_vcpu *vcpu, int tpr, int irr)
> set_cr_intercept(svm, INTERCEPT_CR8_WRITE);
> }
>
> -static void svm_set_virtual_x2apic_mode(struct kvm_vcpu *vcpu, bool set)
> +static void svm_set_virtual_apic_mode(struct kvm_vcpu *vcpu)
> {
> return;
> }
> @@ -7076,7 +7076,7 @@ static struct kvm_x86_ops svm_x86_ops __ro_after_init = {
> .enable_nmi_window = enable_nmi_window,
> .enable_irq_window = enable_irq_window,
> .update_cr8_intercept = update_cr8_intercept,
> - .set_virtual_x2apic_mode = svm_set_virtual_x2apic_mode,
> + .set_virtual_apic_mode = svm_set_virtual_apic_mode,
> .get_enable_apicv = svm_get_enable_apicv,
> .refresh_apicv_exec_ctrl = svm_refresh_apicv_exec_ctrl,
> .load_eoi_exitmap = svm_load_eoi_exitmap,
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index c766880..c7a3dea 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -481,7 +481,8 @@ struct nested_vmx {
> bool sync_shadow_vmcs;
> bool dirty_vmcs12;
>
> - bool change_vmcs01_virtual_x2apic_mode;
> + bool change_vmcs01_virtual_apic_mode;
> +
> /* L2 must run next, and mustn't decide to exit to L1. */
> bool nested_run_pending;
>
> @@ -9248,31 +9249,43 @@ static void update_cr8_intercept(struct kvm_vcpu *vcpu, int tpr, int irr)
> vmcs_write32(TPR_THRESHOLD, irr);
> }
>
> -static void vmx_set_virtual_x2apic_mode(struct kvm_vcpu *vcpu, bool set)
> +static void vmx_set_virtual_apic_mode(struct kvm_vcpu *vcpu)
> {
> u32 sec_exec_control;
>
> + if (!lapic_in_kernel(vcpu))
> + return;
> +
> /* Postpone execution until vmcs01 is the current VMCS. */
> if (is_guest_mode(vcpu)) {
> - to_vmx(vcpu)->nested.change_vmcs01_virtual_x2apic_mode = true;
> + to_vmx(vcpu)->nested.change_vmcs01_virtual_apic_mode = true;
> return;
> }
>
> - if (!cpu_has_vmx_virtualize_x2apic_mode())
> - return;
> -
> if (!cpu_need_tpr_shadow(vcpu))
> return;
>
> sec_exec_control = vmcs_read32(SECONDARY_VM_EXEC_CONTROL);
> + sec_exec_control &= ~(SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES |
> + SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE);
>
> - if (set) {
> - sec_exec_control &= ~SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES;
> - sec_exec_control |= SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE;
> - } else {
> - sec_exec_control &= ~SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE;
> - sec_exec_control |= SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES;
> - vmx_flush_tlb(vcpu, true);
> + switch (kvm_get_apic_mode(vcpu)) {
> + case LAPIC_MODE_INVALID:
> + WARN_ONCE(true, "Invalid local APIC state");
> + case LAPIC_MODE_DISABLED:
> + break;
> + case LAPIC_MODE_XAPIC:
> + if (flexpriority_enabled) {
> + sec_exec_control |=
> + SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES;
> + vmx_flush_tlb(vcpu, true);
> + }
> + break;
> + case LAPIC_MODE_X2APIC:
> + if (cpu_has_vmx_virtualize_x2apic_mode())
> + sec_exec_control |=
> + SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE;
> + break;
> }
> vmcs_write32(SECONDARY_VM_EXEC_CONTROL, sec_exec_control);
>
> @@ -12060,10 +12073,9 @@ static void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 exit_reason,
> if (kvm_has_tsc_control)
> decache_tsc_multiplier(vmx);
>
> - if (vmx->nested.change_vmcs01_virtual_x2apic_mode) {
> - vmx->nested.change_vmcs01_virtual_x2apic_mode = false;
> - vmx_set_virtual_x2apic_mode(vcpu,
> - vcpu->arch.apic_base & X2APIC_ENABLE);
> + if (vmx->nested.change_vmcs01_virtual_apic_mode) {
> + vmx->nested.change_vmcs01_virtual_apic_mode = false;
> + vmx_set_virtual_apic_mode(vcpu);
> } else if (!nested_cpu_has_ept(vmcs12) &&
> nested_cpu_has2(vmcs12,
> SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES)) {
> @@ -12691,7 +12703,7 @@ static struct kvm_x86_ops vmx_x86_ops __ro_after_init = {
> .enable_nmi_window = enable_nmi_window,
> .enable_irq_window = enable_irq_window,
> .update_cr8_intercept = update_cr8_intercept,
> - .set_virtual_x2apic_mode = vmx_set_virtual_x2apic_mode,
> + .set_virtual_apic_mode = vmx_set_virtual_apic_mode,
> .set_apic_access_page_addr = vmx_set_apic_access_page_addr,
> .get_enable_apicv = vmx_get_enable_apicv,
> .refresh_apicv_exec_ctrl = vmx_refresh_apicv_exec_ctrl,
>
