linux-kernel - Re: [PATCH 03/19] Revert "KVM: SVM: Introduce hybrid-AVIC mode"

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YxDK2GIlhr+mQFWd@google.com>
Date:   Thu, 1 Sep 2022 15:08:08 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Maxim Levitsky <mlevitsk@...hat.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Suravee Suthikulpanit <suravee.suthikulpanit@....com>,
        Li RongQing <lirongqing@...du.com>
Subject: Re: [PATCH 03/19] Revert "KVM: SVM: Introduce hybrid-AVIC mode"

Sorry for forking a bunch of different threads, wanted to respond to the other
stuff before you signed off for the day.

On Thu, Sep 01, 2022, Maxim Levitsky wrote:
> However when APIC mode changes to x2apic, I don't think that we zap that
> SPTE.

Hmm, yes. 

> A side efect of this will be that AVIC in hybrid mode will just work and be
> 100% to the spec. 

And I believe we can solve the avic_set_virtual_apic_mode() issue as well.  If
x2APIC is treated as an inhibit for xAPIC but not APICv at large, then setting the
inhibit will zap the SPTEs, and toggling the inhibit will force AVIC to re-assess
its VMCB controls without needing to hook ->set_virtual_apic_mode().

Roughly what I'm thinking.  I'll try to give this a shot today.

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 164b002777cf..0b69acef2bee 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -2381,7 +2381,11 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value)
        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)) {
+       if ((old_value ^ value) & X2APIC_ENABLE) {
+               kvm_set_or_clear_apicv_inhibit(vcpu->kvm,
+                                              APICV_INHIBIT_REASON_X2APIC,
+                                              value & X2APIC_ENABLE;
+       } else if ((old_value ^ value) & MSR_IA32_APICBASE_ENABLE) {
                kvm_vcpu_update_apicv(vcpu);
                static_call_cond(kvm_x86_set_virtual_apic_mode)(vcpu);
        }
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 270403610185..76d93f87071f 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -4156,7 +4156,7 @@ static int kvm_faultin_pfn(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault)
                 * when the AVIC is re-enabled.
                 */
                if (slot && slot->id == APIC_ACCESS_PAGE_PRIVATE_MEMSLOT &&
-                   !kvm_apicv_activated(vcpu->kvm))
+                   !kvm_xapic_apicv_activated(vcpu->kvm))
                        return RET_PF_EMULATE;
        }
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 1328326acfae..ee381d155adc 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -9297,11 +9297,24 @@ static void kvm_pv_kick_cpu_op(struct kvm *kvm, int apicid)
        kvm_irq_delivery_to_apic(kvm, NULL, &lapic_irq, NULL);
 }
 
-bool kvm_apicv_activated(struct kvm *kvm)
+bool kvm_xapic_apicv_activated(struct kvm *kvm)
 {
        return (READ_ONCE(kvm->arch.apicv_inhibit_reasons) == 0);
 }
 
+bool kvm_apicv_activated(struct kvm *kvm)
+{
+       unsigned long inhibits = READ_ONCE(kvm->arch.apicv_inhibit_reasons);
+
+       /*
+        * x2APIC only needs to "inhibit" the MMIO region, all other aspects of
+        * APICv can continue to be utilized.
+        */
+       inhibits &= ~APICV_INHIBIT_REASON_X2APIC;
+
+       return !inhibits;
+}
+
 bool kvm_vcpu_apicv_activated(struct kvm_vcpu *vcpu)
 {
        ulong vm_reasons = READ_ONCE(vcpu->kvm->arch.apicv_inhibit_reasons);