I actually have a kvm-unit-test for this. I'll try to get it crossported today.
On Thu, May 10, 2018 at 6:30 AM, Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote:
> 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,
>>
>
