KVM: SVM: make svm_flush_tlb_gva do a full asid flush if NPT enabled

Red Hat is seeing multiple reports of Windows memory corruptions
(and consequent BSODs) with hv-tlbflush=on, on AMD processors only.
The crashes, while extremely rare, happen even with a stock configuration,
but with Driver Verifier enabled they can be detected after approximately
200 VM hours.  In particular, Alexander Lougovski measured the following:

- on AMD Turin, 15 crashes in 3300 VM hours

- on AMD Milan, 2 crashes in 500 VM hours (there are fewer hours
  here due to the host being smaller)

- on Intel Sapphire Rapids, 0 crashes in 8000 VM hours

- on AMD Turin with full TLB flush (not exactly this patch but
  similar), no crashes in ~2 weeks of run time which should also
  be ~7000 VM hours

For Turin, the microcode version was 0x0b002162, which (assuming
this is the same issue) should not be affected by the problem listed in
https://knowledge.broadcom.com/external/article/419026/bsod-on-virtual-machines-running-on-amd.html;
on the other hand that problem should not apply to earlier processors.
AMD has not provided any information or analysis yet, and when we asked
we didn't know yet that it reproduced on Milan as well.

As to the workload, Alexander threw more or less everything at the same
time at the VM:

- a full Windows Defender scan every 30 minutes

- a disk I/O job

- a loop doing repeated mmap of system files (mostly to hope that
  it triggers some consistency check in the Windows memory manager)

- SQL Express 2022 + StressDB (1.6M rows), with the host doing queries
  (75% write/25% read) via sqlcmd

Driver Verifier is able to detect BSODs more or less at the same time as
the pages are freed.  They mostly happen in the Windows Defender filter
driver, but occasionally also in the networking stack (e.g., afd.sys)
or elsewhere in the filesystem stack (e.g., fltmgr.sys).

The flush is issued from kvm_hv_vcpu_flush_tlb(), which receives the
cross-CPU requests from the Hyper-V TLB flush hypercalls via a kfifo
and is invoked by the KVM_REQ_HV_TLB_FLUSH request.  The mechanism is
the same for both Intel and AMD, and the handler for both vendors is
a simple INVVPID(ADDR)/INVLPGA instruction.

Because the request is handled on the destination CPU, there is a question
of what happens if the VM is migrated across physical CPUs.  In that case,
the INVLPGA instruction would use a stale svm->vmcb->control.asid; but
if anything that might do an *unnecessary* flush (on an asid that's being
used for another VM) and then pre_svm_run() would force a full TLB rebuild.

So, for lack of better ideas, this patch forces a full ASID bump in
svm_flush_tlb_gva().  To avoid paying the price on Intel and also to
avoid unnecessary loops on AMD, the flush_tlb_gva op now returns whether
it did a full flush or not; kvm_hv_vcpu_flush_tlb() takes note and exits
its loops immediately.  While there is an obvious performance impact,
about half of the benefit from Hyper-V tlbflush is preserved (10% vs. 20%
on the SQL Server workload).

kvm_mmu_invalidate_addr() is the only other caller of the flush_tlb_gva op.
The change would have a performance impact on every intercepted INVLPG and,
for nested SVM, on every L1 INVLPGA.  For INVLPGA specifically, this covers
the same suspected issue but for nested hypervisors, so it is correct to
apply the workaround; for INVLPG on shadow paging, instead, the impact
would be stronger and, due to lack of data, for now the use of INVLPGA is
left in place in svm_flush_tlb_gva().

Analyzed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Analyzed-by: Alexander Lougovski <alougovs@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 5f6c1ce9673b7f5185e304ae14a7f58ce42056ca..6db5b5f79df9b85798e84e5373a3a84f1ae668e1 100644 (file)
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1858,7 +1858,7 @@ struct kvm_x86_ops {
         * Can potentially get non-canonical addresses through INVLPGs, which
         * the implementation may choose to ignore if appropriate.
         */
-       void (*flush_tlb_gva)(struct kvm_vcpu *vcpu, gva_t addr);
+       void (*flush_tlb_gva)(struct kvm_vcpu *vcpu, gva_t addr, bool *full);
 
        /*
         * Flush any TLB entries created by the guest.  Like tlb_flush_gva(),

diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index fd4eb1e561f7fb48e247da384af8a0ead447ebe5..d8736b5dfcd3a81b3918425d27b7934958cded1a 100644 (file)
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -1974,6 +1974,7 @@ int kvm_hv_vcpu_flush_tlb(struct kvm_vcpu *vcpu)
        u64 entries[KVM_HV_TLB_FLUSH_FIFO_SIZE];
        int i, j, count;
        gva_t gva;
+       bool full = false;
 
        if (!tdp_enabled || !hv_vcpu)
                return -EINVAL;
@@ -1982,7 +1983,7 @@ int kvm_hv_vcpu_flush_tlb(struct kvm_vcpu *vcpu)
 
        count = kfifo_out(&tlb_flush_fifo->entries, entries, KVM_HV_TLB_FLUSH_FIFO_SIZE);
 
-       for (i = 0; i < count; i++) {
+       for (i = 0; i < count && !full; i++) {
                if (entries[i] == KVM_HV_TLB_FLUSHALL_ENTRY)
                        goto out_flush_all;
 
@@ -1991,11 +1992,11 @@ int kvm_hv_vcpu_flush_tlb(struct kvm_vcpu *vcpu)
                 * pages to flush.
                 */
                gva = entries[i] & PAGE_MASK;
-               for (j = 0; j < (entries[i] & ~PAGE_MASK) + 1; j++) {
+               for (j = 0; j < (entries[i] & ~PAGE_MASK) + 1 && !full; j++) {
                        if (is_noncanonical_invlpg_address(gva + j * PAGE_SIZE, vcpu))
                                continue;
 
-                       kvm_x86_call(flush_tlb_gva)(vcpu, gva + j * PAGE_SIZE);
+                       kvm_x86_call(flush_tlb_gva)(vcpu, gva + j * PAGE_SIZE, &full);
                }
 
                ++vcpu->stat.tlb_flush;

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 66e69d2a41b3c5aef74864c69114c4240b090a18..c9e4739b26d7111e95b93d331e7840fd4e230a66 100644 (file)
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -6652,7 +6652,7 @@ void kvm_mmu_invalidate_addr(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
                if (is_noncanonical_invlpg_address(addr, vcpu))
                        return;
 
-               kvm_x86_call(flush_tlb_gva)(vcpu, addr);
+               kvm_x86_call(flush_tlb_gva)(vcpu, addr, NULL);
        }
 
        if (!mmu->sync_spte)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index d0971685034b92319a4bdfa2ca33657cca5dfb6d..60495447f0ca027487a27617b617410e5dcb8f94 100644 (file)
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -4227,18 +4227,31 @@ static void svm_flush_tlb_all(struct kvm_vcpu *vcpu)
        svm_flush_tlb_asid(vcpu);
 }
 
-static void svm_flush_tlb_gva(struct kvm_vcpu *vcpu, gva_t gva)
+static void svm_flush_tlb_guest(struct kvm_vcpu *vcpu)
 {
-       struct vcpu_svm *svm = to_svm(vcpu);
+       kvm_register_mark_dirty(vcpu, VCPU_REG_ERAPS);
 
-       invlpga(gva, svm->vmcb->control.asid);
+       svm_flush_tlb_asid(vcpu);
 }
 
-static void svm_flush_tlb_guest(struct kvm_vcpu *vcpu)
+static void svm_flush_tlb_gva(struct kvm_vcpu *vcpu, gva_t gva, bool *full)
 {
-       kvm_register_mark_dirty(vcpu, VCPU_REG_ERAPS);
+       struct vcpu_svm *svm = to_svm(vcpu);
 
-       svm_flush_tlb_asid(vcpu);
+       /*
+        * INVLPGA has had errata on Genoa and Turin, and even on older
+        * generations there were reports of Windows BSODs if INVLPGA
+        * was used for Hyper-V tlbflush.  Use it only for shadow paging
+        * where it seems to be okay.
+        */
+       if (!npt_enabled) {
+               invlpga(gva, svm->vmcb->control.asid);
+               return;
+       }
+
+       svm_flush_tlb_guest(vcpu);
+       if (full)
+               *full = true;
 }
 
 static inline void sync_cr8_to_lapic(struct kvm_vcpu *vcpu)

diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
index 83d9921277eac2df42e51466eab79fd7cf3b0c9f..f204a0fc0a5711d427d8bd9450b7222a45bde9d4 100644 (file)
--- a/arch/x86/kvm/vmx/main.c
+++ b/arch/x86/kvm/vmx/main.c
@@ -535,12 +535,12 @@ static void vt_flush_tlb_current(struct kvm_vcpu *vcpu)
        vmx_flush_tlb_current(vcpu);
 }
 
-static void vt_flush_tlb_gva(struct kvm_vcpu *vcpu, gva_t addr)
+static void vt_flush_tlb_gva(struct kvm_vcpu *vcpu, gva_t addr, bool *full)
 {
        if (is_td_vcpu(vcpu))
                return;
 
-       vmx_flush_tlb_gva(vcpu, addr);
+       vmx_flush_tlb_gva(vcpu, addr, full);
 }
 
 static void vt_flush_tlb_guest(struct kvm_vcpu *vcpu)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index cc75feec05daa47969bcadd4926576c458ae3036..b8d745f6fd2219e7b25c5a02624d27386ef7df13 100644 (file)
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -3361,7 +3361,7 @@ void vmx_flush_tlb_current(struct kvm_vcpu *vcpu)
                vpid_sync_context(vmx_get_current_vpid(vcpu));
 }
 
-void vmx_flush_tlb_gva(struct kvm_vcpu *vcpu, gva_t addr)
+void vmx_flush_tlb_gva(struct kvm_vcpu *vcpu, gva_t addr, bool *full)
 {
        /*
         * vpid_sync_vcpu_addr() is a nop if vpid==0, see the comment in

diff --git a/arch/x86/kvm/vmx/x86_ops.h b/arch/x86/kvm/vmx/x86_ops.h
index 4098580742462ce41ce5779fa23ef3aecd3113da..17595d52985cf1ca2ed0e36796855b49ec94450d 100644 (file)
--- a/arch/x86/kvm/vmx/x86_ops.h
+++ b/arch/x86/kvm/vmx/x86_ops.h
@@ -82,7 +82,7 @@ void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags);
 bool vmx_get_if_flag(struct kvm_vcpu *vcpu);
 void vmx_flush_tlb_all(struct kvm_vcpu *vcpu);
 void vmx_flush_tlb_current(struct kvm_vcpu *vcpu);
-void vmx_flush_tlb_gva(struct kvm_vcpu *vcpu, gva_t addr);
+void vmx_flush_tlb_gva(struct kvm_vcpu *vcpu, gva_t addr, bool *full);
 void vmx_flush_tlb_guest(struct kvm_vcpu *vcpu);
 void vmx_set_interrupt_shadow(struct kvm_vcpu *vcpu, int mask);
 u32 vmx_get_interrupt_shadow(struct kvm_vcpu *vcpu);