KVM: VMX: Drop MTRR virtualization, honor guest PAT

arch/x86/kvm/mtrr.c +0 -1

-#include "mmu.h"
 

arch/x86/kvm/vmx/vmx.c +0 -9

index 7a74388f9ecf..66bf79decdad 100644
@@ -7596,39 +7596,27 @@ static int vmx_vm_init(struct kvm *kvm)
 
 static u8 vmx_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
 {
-	/* We wanted to honor guest CD/MTRR/PAT, but doing so could result in
-	 * memory aliases with conflicting memory types and sometimes MCEs.
-	 * We have to be careful as to what are honored and when.
-	 *
-	 * For MMIO, guest CD/MTRR are ignored.  The EPT memory type is set to
-	 * UC.  The effective memory type is UC or WC depending on guest PAT.
-	 * This was historically the source of MCEs and we want to be
-	 * conservative.
-	 *

arch/x86/kvm/vmx/vmx.c +0 -1

 	 */
-
 	if (is_mmio)
 		return MTRR_TYPE_UNCACHABLE << VMX_EPT_MT_EPTE_SHIFT;
 

Quoted 20 lines · Sean Christopherson

arch/x86/kvm/vmx/vmx.c +0 -12

index 7a74388f9ecf..66bf79decdad 100644
@@ -7596,39 +7596,27 @@ static int vmx_vm_init(struct kvm *kvm)
 
 static u8 vmx_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
 {
-	/* We wanted to honor guest CD/MTRR/PAT, but doing so could result in
-	 * memory aliases with conflicting memory types and sometimes MCEs.
-	 * We have to be careful as to what are honored and when.
-	 *
-	 * For MMIO, guest CD/MTRR are ignored.  The EPT memory type is set to
-	 * UC.  The effective memory type is UC or WC depending on guest PAT.
-	 * This was historically the source of MCEs and we want to be
-	 * conservative.
-	 *
-	 * When there is no need to deal with noncoherent DMA (e.g., no VT-d
-	 * or VT-d has snoop control), guest CD/MTRR/PAT are all ignored.  The
-	 * EPT memory type is set to WB.  The effective memory type is forced

arch/x86/kvm/vmx/vmx.c static int vmx_vm_init(struct kvm *kvm) · line 7601

Quoted 7 lines

arch/x86/kvm/vmx/vmx.c +3 -4

-	 * WB.
-	 *
-	 * Otherwise, we trust guest.  Guest CD/MTRR/PAT are all honored.  The
-	 * EPT memory type is used to emulate guest CD/MTRR.
+	/*
+	 * Force UC for host MMIO regions, as allowing the guest to access MMIO
+	 * with cacheable accesses will result in Machine Checks.

This does not always force UC. If guest PAT is WC, the effecitve one is WC.

Remove KVM's support for virtualizing guest MTRR memtypes, as full MTRR adds no value, negatively impacts guest performance, and is a maintenance burden due to it's complexity and oddities.

KVM's approach to virtualizating MTRRs make no sense, at all. KVM *only* honors guest MTRR memtypes if EPT is enabled *and* the guest has a device that may perform non-coherent DMA access. From a hardware virtualization perspective of guest MTRRs, there is _nothing_ special about EPT. Legacy shadowing paging doesn't magically account for guest MTRRs, nor does NPT.

arch/x86/kvm/mmu/mmu.c +1 -8

-bool __kvm_mmu_honors_guest_mtrrs(bool vm_has_noncoherent_dma)
+bool kvm_mmu_may_ignore_guest_pat(void)
 {
 	/*
-	 * If host MTRRs are ignored (shadow_memtype_mask is non-zero), and the
-	 * VM has non-coherent DMA (DMA doesn't snoop CPU caches), KVM's ABI is
-	 * to honor the memtype from the guest's MTRRs so that guest accesses
-	 * to memory that is DMA'd aren't cached against the guest's wishes.
-	 *
-	 * Note, KVM may still ultimately ignore guest MTRRs for certain PFNs,
-	 * e.g. KVM will force UC memtype for host MMIO.

Quoted 9 lines · Sean Christopherson

Remove KVM's support for virtualizing guest MTRR memtypes, as full MTRR adds no value, negatively impacts guest performance, and is a maintenance burden due to it's complexity and oddities.

KVM's approach to virtualizating MTRRs make no sense, at all. KVM *only* honors guest MTRR memtypes if EPT is enabled *and* the guest has a device that may perform non-coherent DMA access. From a hardware virtualization perspective of guest MTRRs, there is _nothing_ special about EPT. Legacy shadowing paging doesn't magically account for guest MTRRs, nor does NPT.

[snip]

Quoted 18 lines

arch/x86/kvm/mmu/mmu.c +7 -8

-bool __kvm_mmu_honors_guest_mtrrs(bool vm_has_noncoherent_dma)
+bool kvm_mmu_may_ignore_guest_pat(void)
 {
 	/*
-	 * If host MTRRs are ignored (shadow_memtype_mask is non-zero), and the
-	 * VM has non-coherent DMA (DMA doesn't snoop CPU caches), KVM's ABI is
-	 * to honor the memtype from the guest's MTRRs so that guest accesses
-	 * to memory that is DMA'd aren't cached against the guest's wishes.
-	 *
-	 * Note, KVM may still ultimately ignore guest MTRRs for certain PFNs,
-	 * e.g. KVM will force UC memtype for host MMIO.
+	 * When EPT is enabled (shadow_memtype_mask is non-zero), and the VM
+	 * has non-coherent DMA (DMA doesn't snoop CPU caches), KVM's ABI is to
+	 * honor the memtype from the guest's PAT so that guest accesses to
+	 * memory that is DMA'd aren't cached against the guest's wishes.  As a
+	 * result, KVM _may_ ignore guest PAT, whereas without non-coherent DMA,
+	 * KVM _always_ ignores guest PAT (when EPT is enabled).

arch/x86/kvm/mmu/mmu.c static int kvm_tdp_mmu_page_fault(struct kvm_vcpu *vcpu, · line 4633

Quoted 5 lines

arch/x86/kvm/mmu/mmu.c +1 -1

 	 */
-	return vm_has_noncoherent_dma && shadow_memtype_mask;
+	return shadow_memtype_mask;
 }

Any special reason to use the naming 'may_ignore_guest_pat', but not 'may_honor_guest_pat'?

Since it is also controlled by other cases, e.g., kvm_arch_has_noncoherent_dma() at vmx_get_mt_mask(), it can be 'may_honor_guest_pat' too?

Therefore, why not directly use 'shadow_memtype_mask' (without the API), or some naming like "ept_enabled_for_hardware".

Even with the code from PATCH 5/5, we still have high chance that VM has non-coherent DMA?

bool kvm_mmu_may_ignore_guest_pat(void)
{

(quoted diff) +5 -3

 	/*
-	 * When EPT is enabled (shadow_memtype_mask is non-zero), and the VM
+	 * When EPT is enabled (shadow_memtype_mask is non-zero), the CPU does
+	 * not support self-snoop (or is affected by an erratum), and the VM
 	 * has non-coherent DMA (DMA doesn't snoop CPU caches), KVM's ABI is to
 	 * honor the memtype from the guest's PAT so that guest accesses to
 	 * memory that is DMA'd aren't cached against the guest's wishes.  As a
 	 * result, KVM _may_ ignore guest PAT, whereas without non-coherent DMA,
-	 * KVM _always_ ignores guest PAT (when EPT is enabled).
+	 * KVM _always_ ignores or honors guest PAT, i.e. doesn't toggle SPTE
+	 * bits in response to non-coherent device (un)registration.
 	 */
-	return shadow_memtype_mask;
+	return !static_cpu_has(X86_FEATURE_SELFSNOOP) && shadow_memtype_mask;
 }

Thank you very much!

Dongli Zhang

Quoted 33 lines · Dongli Zhang

Quoted 9 lines · Sean Christopherson

Remove KVM's support for virtualizing guest MTRR memtypes, as full MTRR adds no value, negatively impacts guest performance, and is a maintenance burden due to it's complexity and oddities.

KVM's approach to virtualizating MTRRs make no sense, at all. KVM *only* honors guest MTRR memtypes if EPT is enabled *and* the guest has a device that may perform non-coherent DMA access. From a hardware virtualization perspective of guest MTRRs, there is _nothing_ special about EPT. Legacy shadowing paging doesn't magically account for guest MTRRs, nor does NPT.

[snip]

Quoted 18 lines

arch/x86/kvm/mmu/mmu.c +7 -8

-bool __kvm_mmu_honors_guest_mtrrs(bool vm_has_noncoherent_dma)
+bool kvm_mmu_may_ignore_guest_pat(void)
 {
 	/*
-	 * If host MTRRs are ignored (shadow_memtype_mask is non-zero), and the
-	 * VM has non-coherent DMA (DMA doesn't snoop CPU caches), KVM's ABI is
-	 * to honor the memtype from the guest's MTRRs so that guest accesses
-	 * to memory that is DMA'd aren't cached against the guest's wishes.
-	 *
-	 * Note, KVM may still ultimately ignore guest MTRRs for certain PFNs,
-	 * e.g. KVM will force UC memtype for host MMIO.
+	 * When EPT is enabled (shadow_memtype_mask is non-zero), and the VM
+	 * has non-coherent DMA (DMA doesn't snoop CPU caches), KVM's ABI is to
+	 * honor the memtype from the guest's PAT so that guest accesses to
+	 * memory that is DMA'd aren't cached against the guest's wishes.  As a
+	 * result, KVM _may_ ignore guest PAT, whereas without non-coherent DMA,
+	 * KVM _always_ ignores guest PAT (when EPT is enabled).

Quoted 8 lines

arch/x86/kvm/mmu/mmu.c static int kvm_tdp_mmu_page_fault(struct kvm_vcpu *vcpu, · line 4633

Quoted 5 lines

arch/x86/kvm/mmu/mmu.c +1 -1

 	 */
-	return vm_has_noncoherent_dma && shadow_memtype_mask;
+	return shadow_memtype_mask;
 }

Any special reason to use the naming 'may_ignore_guest_pat', but not 'may_honor_guest_pat'?

Because which (after this series) is would either be misleading or outright wrong. If KVM returns true from the helper based solely on shadow_memtype_mask, then it's misleading because KVM will *always* honors guest PAT for such CPUs. I.e. that name would yield this misleading statement.

If the CPU supports self-snoop, KVM may honor guest PAT.

If KVM returns true iff self-snoop is NOT available (as proposed in this series), then it's outright wrong as KVM would return false, i.e. would make this incorrect statement:

If the CPU supports self-snoop, KVM never honors guest PAT.

As saying that KVM may not or cannot do something is saying that KVM will never do that thing.

And because the EPT flag is "ignore guest PAT", not "honor guest PAT", but that's as much coincidence as it is anything else.

Quoted 5 lines

Since it is also controlled by other cases, e.g., kvm_arch_has_noncoherent_dma() at vmx_get_mt_mask(), it can be 'may_honor_guest_pat' too?

Therefore, why not directly use 'shadow_memtype_mask' (without the API), or some naming like "ept_enabled_for_hardware".

Again, after this series, KVM will *always* honor guest PAT for CPUs with self-snoop, i.e. KVM will *never* ignore guest PAT. But for CPUs without self-snoop (or with errata), KVM conditionally honors/ignores guest PAT.

Quoted 2 lines

Even with the code from PATCH 5/5, we still have high chance that VM has non-coherent DMA?

I don't follow. On CPUs with self-snoop, whether or not the VM has non-coherent DMA (from VFIO!) is irrelevant. If the CPU has self-snoop, then KVM can safely honor guest PAT at all times.

Quoted 11 lines · Sean Christopherson

Quoted 5 lines · Dongli Zhang

Since it is also controlled by other cases, e.g., kvm_arch_has_noncoherent_dma() at vmx_get_mt_mask(), it can be 'may_honor_guest_pat' too?

Therefore, why not directly use 'shadow_memtype_mask' (without the API), or some naming like "ept_enabled_for_hardware".

Again, after this series, KVM will *always* honor guest PAT for CPUs with self-snoop, i.e. KVM will *never* ignore guest PAT. But for CPUs without self-snoop (or with errata), KVM conditionally honors/ignores guest PAT.

Quoted 6 lines

Quoted 2 lines

Even with the code from PATCH 5/5, we still have high chance that VM has non-coherent DMA?

I don't follow. On CPUs with self-snoop, whether or not the VM has non-coherent DMA (from VFIO!) is irrelevant. If the CPU has self-snoop, then KVM can safely honor guest PAT at all times.

Thank you very much for the explanation.

According to my understanding of the explanation (after this series):

1. When static_cpu_has(X86_FEATURE_SELFSNOOP) == true, it is 100% to "honor guest PAT".

2. When static_cpu_has(X86_FEATURE_SELFSNOOP) == false (and shadow_memtype_mask), although only 50% chance (depending on where there is non-coherent DMA), at least now it is NOT 100% (to honor guest PAT) any longer.

Due to the fact it is not 100% (to honor guest PAT) any longer, there starts the trend (from 100% to 50%) to "ignore guest PAT", that is:
kvm_mmu_may_ignore_guest_pat().

From: Yan Zhao <[email protected]>

Ensure a full memory barrier is emitted in the VM-Exit path, as a full barrier is required on Intel CPUs to evict WC buffers. This will allow unconditionally honoring guest PAT on Intel CPUs that support self-snoop.

As srcu_read_lock() is always called in the VM-Exit path and it internally has a smp_mb(), call smp_mb__after_srcu_read_lock() to avoid adding a second fence and make sure smp_mb() is called without dependency on implementation details of srcu_read_lock().

arch/x86/kvm/x86.c +5 -0

+	/*
+	 * Call this to ensure WC buffers in guest are evicted after each VM
+	 * Exit, so that the evicted WC writes can be snooped across all cpus
+	 */
+	smp_mb__after_srcu_read_lock();

Quoted 6 lines · Sean Christopherson

From: Yan Zhao <[email protected]>

Ensure a full memory barrier is emitted in the VM-Exit path, as a full barrier is required on Intel CPUs to evict WC buffers. This will allow unconditionally honoring guest PAT on Intel CPUs that support self-snoop.

Quoted 4 lines

As srcu_read_lock() is always called in the VM-Exit path and it internally has a smp_mb(), call smp_mb__after_srcu_read_lock() to avoid adding a second fence and make sure smp_mb() is called without dependency on implementation details of srcu_read_lock().

Do you really need mfence or is a locked operation enough? mfence is mb(), not smp_mb().

Paolo

Quoted 5 lines

arch/x86/kvm/x86.c +5 -0

+	/*
+	 * Call this to ensure WC buffers in guest are evicted after each VM
+	 * Exit, so that the evicted WC writes can be snooped across all cpus
+	 */
+	smp_mb__after_srcu_read_lock();

Quoted 6 lines · Sean Christopherson

From: Yan Zhao <[email protected]>

Ensure a full memory barrier is emitted in the VM-Exit path, as a full barrier is required on Intel CPUs to evict WC buffers. This will allow unconditionally honoring guest PAT on Intel CPUs that support self-snoop.

Quoted 4 lines

As srcu_read_lock() is always called in the VM-Exit path and it internally has a smp_mb(), call smp_mb__after_srcu_read_lock() to avoid adding a second fence and make sure smp_mb() is called without dependency on implementation details of srcu_read_lock().

Do you really need mfence or is a locked operation enough? mfence is mb(), not smp_mb().

Quoted 6 lines

arch/x86/kvm/x86.c +5 -0

+	/*
+	 * Call this to ensure WC buffers in guest are evicted after each VM
+	 * Exit, so that the evicted WC writes can be snooped across all cpus
+	 */
+	smp_mb__after_srcu_read_lock();

Unconditionally honor guest PAT on CPUs that support self-snoop, as Intel has confirmed that CPUs that support self-snoop always snoop caches and store buffers. I.e. CPUs with self-snoop maintain cache coherency even in the presence of aliased memtypes, thus there is no need to trust the guest behaves and only honor PAT as a last resort, as KVM does today.

Honoring guest PAT is desirable for use cases where the guest has access to non-coherent DMA _without_ bouncing through VFIO, e.g. when a virtual (mediated, for all intents and purposes) GPU is exposed to the guest, along with buffers that are consumed directly by the physical GPU, i.e. which can't be proxied by the host to ensure writes from the guest are performed with the correct memory type for the GPU.

Cc Yiwei Zhang <[email protected]>

Suggested-by Yan Zhao <[email protected]>

Suggested-by Kevin Tian <[email protected]>

Signed-off-by Sean Christopherson <[email protected]>

---
 arch/x86/kvm/mmu/mmu.c |  8 +++++---
 arch/x86/kvm/vmx/vmx.c | 10 ++++++----
 2 files changed, 11 insertions(+), 7 deletions(-)

arch/x86/kvm/mmu/mmu.c +5 -3

index 403cd8f914cd..7fa514830628 100644
@@ -4622,14 +4622,16 @@ static int kvm_tdp_mmu_page_fault(struct kvm_vcpu *vcpu,
 bool kvm_mmu_may_ignore_guest_pat(void)
 {
 	/*
-	 * When EPT is enabled (shadow_memtype_mask is non-zero), and the VM
+	 * When EPT is enabled (shadow_memtype_mask is non-zero), the CPU does
+	 * not support self-snoop (or is affected by an erratum), and the VM
 	 * has non-coherent DMA (DMA doesn't snoop CPU caches), KVM's ABI is to
 	 * honor the memtype from the guest's PAT so that guest accesses to
 	 * memory that is DMA'd aren't cached against the guest's wishes.  As a
 	 * result, KVM _may_ ignore guest PAT, whereas without non-coherent DMA,
-	 * KVM _always_ ignores guest PAT (when EPT is enabled).
+	 * KVM _always_ ignores or honors guest PAT, i.e. doesn't toggle SPTE
+	 * bits in response to non-coherent device (un)registration.
 	 */
-	return shadow_memtype_mask;
+	return !static_cpu_has(X86_FEATURE_SELFSNOOP) && shadow_memtype_mask;
 }
 
 int kvm_tdp_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault)

arch/x86/kvm/vmx/vmx.c +0 -2

index 17a8e4fdf9c4..5dc4c24ae203 100644
@@ -7605,11 +7605,13 @@ static u8 vmx_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
 
 	/*
 	 * Force WB and ignore guest PAT if the VM does NOT have a non-coherent
-	 * device attached.  Letting the guest control memory types on Intel
-	 * CPUs may result in unexpected behavior, and so KVM's ABI is to trust

arch/x86/kvm/vmx/vmx.c +0 -0

 	return (MTRR_TYPE_WRBACK << VMX_EPT_MT_EPTE_SHIFT);

Signature · 1 line

2.44.0.278.ge034bb2e1d-goog

Quoted 16 lines · Sean Christopherson

arch/x86/kvm/vmx/vmx.c +4 -3

index 17a8e4fdf9c4..5dc4c24ae203 100644
@@ -7605,11 +7605,13 @@ static u8 vmx_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
 
 	/*
 	 * Force WB and ignore guest PAT if the VM does NOT have a non-coherent
-	 * device attached.  Letting the guest control memory types on Intel
-	 * CPUs may result in unexpected behavior, and so KVM's ABI is to trust
-	 * the guest to behave only as a last resort.
+	 * device attached and the CPU doesn't support self-snoop.  Letting the
+	 * guest control memory types on Intel CPUs without self-snoop may
+	 * result in unexpected behavior, and so KVM's (historical) ABI is to
+	 * trust the guest to behave only as a last resort.
 	 */

arch/x86/kvm/vmx/vmx.c static u8 vmx_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio) · line 7615

Quoted 4 lines

arch/x86/kvm/vmx/vmx.c +2 -1

-	if (!kvm_arch_has_noncoherent_dma(vcpu->kvm))
+	if (!static_cpu_has(X86_FEATURE_SELFSNOOP) &&
+	    !kvm_arch_has_noncoherent_dma(vcpu->kvm))
 		return (MTRR_TYPE_WRBACK << VMX_EPT_MT_EPTE_SHIFT) | VMX_EPT_IPAT_BIT;

For the case of !static_cpu_has(X86_FEATURE_SELFSNOOP) && kvm_arch_has_noncoherent_dma(vcpu->kvm), I think we at least should warn about unsafe before honoring guest memory type.

Though it's a KVM's historical ABI, it's not safe in the security perspective because page aliasing without proper cache flush handling on CPUs without self-snoop may open a door for guest to read uninitialized host data. e.g. when there's a noncoherent DMA device attached, and if there's a memory region that is not pinned in vfio/iommufd side, (e.g. memory region in vfio's skipped section), then though the guest memory from this memory region is not accessible to noncoherent DMAs, vCPUs can still access this part of guest memory. Then if vCPUs use WC as guest type, it may bypass host's initialization data in cache and read stale data in host, causing information leak.

My preference is still to force WB
(i.e. (MTRR_TYPE_WRBACK << VMX_EPT_MT_EPTE_SHIFT) | VMX_EPT_IPAT_BIT) in case of !static_cpu_has(X86_FEATURE_SELFSNOOP) && kvm_arch_has_noncoherent_dma(vcpu->kvm). Firstly, it's because there're few CPUs with features VMX without self-snoop;

Secondly, security takes priority over functionality :)

From: Sean Christopherson <[email protected]>
Sent: Tuesday, March 12, 2024 8:26 AM

Quoted 6 lines · Yan Zhao

arch/x86/kvm/vmx/vmx.c static u8 vmx_get_mt_mask(struct kvm_vcpu

Quoted 5 lines · Sean Christopherson

arch/x86/kvm/vmx/vmx.c +0 -0

index 17a8e4fdf9c4..5dc4c24ae203 100644
@@ -7605,11 +7605,13 @@ static u8 vmx_get_mt_mask(struct kvm_vcpu

*vcpu, gfn_t gfn, bool is_mmio)

Quoted 3 lines

arch/x86/kvm/vmx/vmx.c static u8 vmx_get_mt_mask(struct kvm_vcpu · line 7606

Quoted 3 lines

arch/x86/kvm/vmx/vmx.c +0 -0

 	/*
 	 * Force WB and ignore guest PAT if the VM does NOT have a non-

coherent

Quoted 2 lines

arch/x86/kvm/vmx/vmx.c static u8 vmx_get_mt_mask(struct kvm_vcpu · line 7608

Quoted 2 lines

arch/x86/kvm/vmx/vmx.c +0 -2

-	 * device attached.  Letting the guest control memory types on Intel
-	 * CPUs may result in unexpected behavior, and so KVM's ABI is to

trust

Quoted 2 lines

arch/x86/kvm/vmx/vmx.c static u8 vmx_get_mt_mask(struct kvm_vcpu · line 7607

Quoted 2 lines

arch/x86/kvm/vmx/vmx.c +1 -1

-	 * the guest to behave only as a last resort.
+	 * device attached and the CPU doesn't support self-snoop.  Letting

the

Quoted 8 lines

arch/x86/kvm/vmx/vmx.c static u8 vmx_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)

Quoted 8 lines

arch/x86/kvm/vmx/vmx.c +5 -1

+	 * guest control memory types on Intel CPUs without self-snoop may
+	 * result in unexpected behavior, and so KVM's (historical) ABI is to
+	 * trust the guest to behave only as a last resort.
 	 */
-	if (!kvm_arch_has_noncoherent_dma(vcpu->kvm))
+	if (!static_cpu_has(X86_FEATURE_SELFSNOOP) &&
+	    !kvm_arch_has_noncoherent_dma(vcpu->kvm))
 		return (MTRR_TYPE_WRBACK << VMX_EPT_MT_EPTE_SHIFT)

| VMX_EPT_IPAT_BIT;

Quoted 3 lines

For the case of !static_cpu_has(X86_FEATURE_SELFSNOOP) && kvm_arch_has_noncoherent_dma(vcpu->kvm), I think we at least should

warn

Quoted 1 line

about unsafe before honoring guest memory type.

I don't think it gains us enough to offset the potential pain such a message would bring. Assuming the warning isn't outright ignored, the most likely scenario
is that the warning will cause random end users to worry that the setup they've
been running for years is broken, when in reality it's probably just fine for their
use case.

I would be quite surprised if there are people running untrusted workloads on
10+ year old silicon *and* have passthrough devices and non-coherent IOMMUs/DMA.

And anyone exposing a device directly to an untrusted workload really should have
done their homework.

And it's not like we're going to change KVM's historical behavior at this point.

Quoted 8 lines

From: Sean Christopherson <[email protected]>
Sent: Tuesday, March 12, 2024 8:26 AM

Quoted 3 lines · Yan Zhao

For the case of !static_cpu_has(X86_FEATURE_SELFSNOOP) && kvm_arch_has_noncoherent_dma(vcpu->kvm), I think we at least should warn about unsafe before honoring guest memory type.

Quoted 6 lines

I don't think it gains us enough to offset the potential pain such a message would bring. Assuming the warning isn't outright ignored, the most likely scenario is that the warning will cause random end users to worry that the setup they've been running for years is broken, when in reality it's probably just fine for their
use case.

Isn't the 'worry' necessary to allow end users evaluate whether "it's probably just fine for their use case"?

I saw the old comment already mentioned that doing so may lead to unexpected behaviors. But I'm not sure whether such code-level caveat has been visible enough to end users.

Quoted 3 lines

I would be quite surprised if there are people running untrusted workloads on 10+ year old silicon *and* have passthrough devices and non-coherent IOMMUs/DMA.

this is probably true.

Quoted 2 lines

And anyone exposing a device directly to an untrusted workload really should have done their homework.

or they run trusted workloads which might be tampered by virus to exceed the scope of their homework. 😊

Quoted 1 line

And it's not like we're going to change KVM's historical behavior at this point.

I agree with your point of not breaking userspace. But still think a warning might be informative to let users evaluate their setup against a newly identified "unexpected behavior" which has security implication beyond the guest, while the previous interpretation of "unexpected behavior" might be that the guest can at most shoot its own foot...

Quoted 15 lines · Kevin Tian

Quoted 8 lines

From: Sean Christopherson <[email protected]>
Sent: Tuesday, March 12, 2024 8:26 AM

Quoted 3 lines · Yan Zhao

For the case of !static_cpu_has(X86_FEATURE_SELFSNOOP) && kvm_arch_has_noncoherent_dma(vcpu->kvm), I think we at least should warn about unsafe before honoring guest memory type.

Quoted 6 lines

I don't think it gains us enough to offset the potential pain such a message would bring. Assuming the warning isn't outright ignored, the most likely scenario is that the warning will cause random end users to worry that the setup they've been running for years is broken, when in reality it's probably just fine for their
use case.

Quoted 2 lines

Isn't the 'worry' necessary to allow end users evaluate whether "it's probably just fine for their use case"?

Realistically, outside of large scale deployments, no end user is going to be able to make that evaluation, because practically speaking it requires someone with quite low-level hardware knowledge to be able to make that judgment call. And counting by number of human end users (as opposed to number of VMs being run), I am willing to bet that the overwhelming majority of KVM users aren't kernel or systems engineers.

Understandably, users tend to be alarmed by (or suspicious of) new warnings that show up. E.g. see the ancient KVM_SET_TSS_ADDR pr_warn[*]. And recently, we had an internal bug report filed against KVM because they observed a performance regression when booting a KVM guest, and saw a new message about some CPU vulnerability being mitigated on VM-Exit that showed up in their *guest* kernel.

In short, my concern is that adding a new pr_warn() will generate noise for end users *and* for KVM developers/maintainers, because even if we phrase the message to talk specifically about "untrusted workloads", the majority of affected users will not have the necessary knowledge to make an informed decision.

[*] https://lore.kernel.org/all/[email protected]

Quoted 3 lines

I saw the old comment already mentioned that doing so may lead to unexpected behaviors. But I'm not sure whether such code-level caveat has been visible enough to end users.

Another point to consider: KVM is _always_ potentially broken on such CPUs, as KVM forces WB for guest accesses. I.e. KVM will create memory aliasing if the host has guest memory mapped as non-WB in the PAT, without non-coherent DMA exposed to the guest.

Quoted 3 lines

Quoted 3 lines

I would be quite surprised if there are people running untrusted workloads on 10+ year old silicon *and* have passthrough devices and non-coherent IOMMUs/DMA.

Quoted 7 lines

this is probably true.

Quoted 2 lines

And anyone exposing a device directly to an untrusted workload really should have done their homework.

or they run trusted workloads which might be tampered by virus to exceed the scope of their homework. 😊

If a workload is being run in a KVM guest for host isolation/security purposes, and a device was exposed to said workload, then I would firmly consider analyzing the impact of a compromised guest to be part of their homework.

Quoted 7 lines

Quoted 1 line

And it's not like we're going to change KVM's historical behavior at this point.

I agree with your point of not breaking userspace. But still think a warning might be informative to let users evaluate their setup against a newly identified "unexpected behavior" which has security implication beyond the guest, while the previous interpretation of "unexpected behavior" might be that the guest can at most shoot its own foot...

If this issue weren't limited to 10+ year old hardware, I would be more inclined to add a message. But at this point, realistically the only thing KVM would be saying is "you're running old hardware, that might be unsafe in today's world".

For users that care about security, we'd be telling them something they already know (and if they don't know, they've got bigger problems). And for everyone else, it'd be scary noise without any meaningful benefit.

From: Zhao, Yan Y <[email protected]>
Sent: Wednesday, March 13, 2024 9:19 AM

Quoted 2 lines · Sean Christopherson

Quoted 1 line · Kevin Tian

I saw the old comment already mentioned that doing so may lead to

unexpected

Quoted 1 line

Quoted 1 line

behaviors. But I'm not sure whether such code-level caveat has been

visible

Quoted 2 lines

Quoted 1 line

enough to end users.

What about add a new module parameter to turn on honoring guest for non-coherent DMAs on CPUs without self-snoop?
A previous example is VFIO's "allow_unsafe_interrupts" parameter.

Quoted 2 lines

Another point to consider: KVM is _always_ potentially broken on such

CPUs, as

Quoted 1 line

KVM forces WB for guest accesses. I.e. KVM will create memory aliasing if

the

Quoted 1 line

host has guest memory mapped as non-WB in the PAT, without non-

coherent DMA

Quoted 1 line

exposed to the guest.

In this case, memory aliasing may only lead to guest not function well, since guest is not using WC/UC (which can bypass host initialization data in cache). But if guest has any chance to read information not intended to it, I believe we need to fix it as well.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

I would be quite surprised if there are people running untrusted

workloads

Quoted 1 line

Quoted 1 line

Quoted 1 line

on 10+ year old silicon *and* have passthrough devices and non-

coherent

Quoted 1 line

Quoted 1 line

Quoted 1 line

IOMMUs/DMA.

What if the guest is a totally malicious one?
Previously we trust the guest in the case of noncoherent DMA is because we believe a malicious guest will only meet data corruption and shoot his own
foot.

But as Jason raised the security problem in another mail thread [1], this will expose security hole if CPUs have no self-snoop. So, we need to fix it, right?
+ Jason, in case I didn't understand this problem correctly.

[1] https://lore.kernel.org/all/[email protected]/

We'll certain fix the security hole on CPUs w/ self-snoop. In this case CPU accesses are guaranteed to be coherent and the vulnerability can only be exposed via non-coherent DMA which is supposed to be fixed by your coming series.

But for old CPUs w/o self-snoop the hole can be exploited using either CPU or non-coherent DMA once the guest PAT is honored. As long as nobody is willing to actually fix the CPU path (is it possible?) I'm kind of convinced

by Sean that sustaining the old behavior is probably the best option...

Quoted 8 lines

We'll certain fix the security hole on CPUs w/ self-snoop. In this case CPU accesses are guaranteed to be coherent and the vulnerability can only be exposed via non-coherent DMA which is supposed to be fixed by your coming series.

But for old CPUs w/o self-snoop the hole can be exploited using either CPU or non-coherent DMA once the guest PAT is honored. As long as nobody is willing to actually fix the CPU path (is it possible?) I'm kind of convinced

We can cook a patch to check CPU self-snoop and force WB in EPT even for non-coherent DMA if no self-snoop. Then back porting such a patch together with the IOMMU side mitigation for non-coherent DMA.

Otherwise, IOMMU side mitigation alone is meaningless for platforms of CPU of no self-snoop.

Quoted 1 line

by Sean that sustaining the old behavior is probably the best option...

Yes, as long as we think exposing secuirty hole on those platforms is acceptable.

Quoted 11 lines · Yan Zhao

Quoted 8 lines

We'll certain fix the security hole on CPUs w/ self-snoop. In this case CPU accesses are guaranteed to be coherent and the vulnerability can only be exposed via non-coherent DMA which is supposed to be fixed by your coming series.

But for old CPUs w/o self-snoop the hole can be exploited using either CPU or non-coherent DMA once the guest PAT is honored. As long as nobody is willing to actually fix the CPU path (is it possible?) I'm kind of convinced

We can cook a patch to check CPU self-snoop and force WB in EPT even for non-coherent DMA if no self-snoop. Then back porting such a patch together with the IOMMU side mitigation for non-coherent DMA.

Please don't. This is a "let sleeping dogs lie" situation.

let sleeping dogs lie - avoid interfering in a situation that is currently causing no problems but might do so as a result of such interference.

Yes, there is technically a flaw, but we have zero evidence that anyone cares or that it is actually problematic in practice. On the other hand, any functional change we make has a non-zero changes of breaking existing setups that have worked for many years.

Quoted 5 lines

Otherwise, IOMMU side mitigation alone is meaningless for platforms of CPU of no self-snoop.

Quoted 1 line

by Sean that sustaining the old behavior is probably the best option...

Yes, as long as we think exposing secuirty hole on those platforms is acceptable.

Yes, I think it's acceptable. Obviously not ideal, but given the alternatives, I think it is a reasonable risk.

Being 100% secure is simply not possible. Security is often about balancing the risk/threat against the cost. In this case, the risk is low (old hardware, uncommon setup for untrusted guests, small window of opportunity, and limited data exposure), whereas the cost is high (decent chance of breaking existing VMs).

Quoted 32 lines · Sean Christopherson

Quoted 11 lines · Yan Zhao

Quoted 8 lines

We'll certain fix the security hole on CPUs w/ self-snoop. In this case CPU accesses are guaranteed to be coherent and the vulnerability can only be exposed via non-coherent DMA which is supposed to be fixed by your coming series.

But for old CPUs w/o self-snoop the hole can be exploited using either CPU or non-coherent DMA once the guest PAT is honored. As long as nobody is willing to actually fix the CPU path (is it possible?) I'm kind of convinced

We can cook a patch to check CPU self-snoop and force WB in EPT even for non-coherent DMA if no self-snoop. Then back porting such a patch together with the IOMMU side mitigation for non-coherent DMA.

Please don't. This is a "let sleeping dogs lie" situation.

let sleeping dogs lie - avoid interfering in a situation that is currently causing no problems but might do so as a result of such interference.

Yes, there is technically a flaw, but we have zero evidence that anyone cares or that it is actually problematic in practice. On the other hand, any functional change we make has a non-zero changes of breaking existing setups that have worked for many years.

Quoted 5 lines

Otherwise, IOMMU side mitigation alone is meaningless for platforms of CPU of no self-snoop.

Quoted 1 line

by Sean that sustaining the old behavior is probably the best option...

Yes, as long as we think exposing secuirty hole on those platforms is acceptable.

Yes, I think it's acceptable. Obviously not ideal, but given the alternatives, I think it is a reasonable risk.

Quoted 4 lines

Being 100% secure is simply not possible. Security is often about balancing the risk/threat against the cost. In this case, the risk is low (old hardware, uncommon setup for untrusted guests, small window of opportunity, and limited data exposure), whereas the cost is high (decent chance of breaking existing VMs).

Ok, thanks for explanation!
I still have one last question: if in future there are CPUs with no selfsnoop (for some unknown reason, or just paranoid),

do we allow this unsafe honoring of guest memory type for non-coherent DMAs?

Unconditionally honor guest PAT on CPUs that support self-snoop, as Intel has confirmed that CPUs that support self-snoop always snoop caches and store buffers. I.e. CPUs with self-snoop maintain cache coherency even in the presence of aliased memtypes, thus there is no need to trust the guest behaves and only honor PAT as a last resort, as KVM does today.

Honoring guest PAT is desirable for use cases where the guest has access to non-coherent DMA _without_ bouncing through VFIO, e.g. when a virtual (mediated, for all intents and purposes) GPU is exposed to the guest, along with buffers that are consumed directly by the physical GPU, i.e. which can't be proxied by the host to ensure writes from the guest are performed with the correct memory type for the GPU.

Cc Yiwei Zhang <[email protected]>

Suggested-by Yan Zhao <[email protected]>

Suggested-by Kevin Tian <[email protected]>

Signed-off-by Sean Christopherson <[email protected]>

---
arch/x86/kvm/mmu/mmu.c | 8 +++++---
arch/x86/kvm/vmx/vmx.c | 10 ++++++----

2 files changed, 11 insertions(+), 7 deletions(-)

arch/x86/kvm/mmu/mmu.c +5 -3

index 403cd8f914cd..7fa514830628 100644
@@ -4622,14 +4622,16 @@ static int kvm_tdp_mmu_page_fault(struct kvm_vcpu *vcpu,
 bool kvm_mmu_may_ignore_guest_pat(void)
 {
 	/*
-	 * When EPT is enabled (shadow_memtype_mask is non-zero), and the VM
+	 * When EPT is enabled (shadow_memtype_mask is non-zero), the CPU does
+	 * not support self-snoop (or is affected by an erratum), and the VM
 	 * has non-coherent DMA (DMA doesn't snoop CPU caches), KVM's ABI is to
 	 * honor the memtype from the guest's PAT so that guest accesses to
 	 * memory that is DMA'd aren't cached against the guest's wishes.  As a
 	 * result, KVM _may_ ignore guest PAT, whereas without non-coherent DMA,
-	 * KVM _always_ ignores guest PAT (when EPT is enabled).
+	 * KVM _always_ ignores or honors guest PAT, i.e. doesn't toggle SPTE
+	 * bits in response to non-coherent device (un)registration.
 	 */
-	return shadow_memtype_mask;
+	return !static_cpu_has(X86_FEATURE_SELFSNOOP) && shadow_memtype_mask;
 }
 
 int kvm_tdp_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault)

arch/x86/kvm/vmx/vmx.c +0 -2

index 17a8e4fdf9c4..5dc4c24ae203 100644
@@ -7605,11 +7605,13 @@ static u8 vmx_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
 
 	/*
 	 * Force WB and ignore guest PAT if the VM does NOT have a non-coherent
-	 * device attached.  Letting the guest control memory types on Intel
-	 * CPUs may result in unexpected behavior, and so KVM's ABI is to trust

Quoted 12 lines · Sean Christopherson

Unconditionally honor guest PAT on CPUs that support self-snoop, as Intel has confirmed that CPUs that support self-snoop always snoop caches and store buffers. I.e. CPUs with self-snoop maintain cache coherency even in the presence of aliased memtypes, thus there is no need to trust the guest behaves and only honor PAT as a last resort, as KVM does today.

Honoring guest PAT is desirable for use cases where the guest has access to non-coherent DMA _without_ bouncing through VFIO, e.g. when a virtual (mediated, for all intents and purposes) GPU is exposed to the guest, along with buffers that are consumed directly by the physical GPU, i.e. which can't be proxied by the host to ensure writes from the guest are performed with the correct memory type for the GPU.

Quoted 17 lines

int kvm_tdp_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault)

arch/x86/kvm/vmx/vmx.c +4 -3

index 17a8e4fdf9c4..5dc4c24ae203 100644
@@ -7605,11 +7605,13 @@ static u8 vmx_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
 
 	/*
 	 * Force WB and ignore guest PAT if the VM does NOT have a non-coherent
-	 * device attached.  Letting the guest control memory types on Intel
-	 * CPUs may result in unexpected behavior, and so KVM's ABI is to trust
-	 * the guest to behave only as a last resort.
+	 * device attached and the CPU doesn't support self-snoop.  Letting the
+	 * guest control memory types on Intel CPUs without self-snoop may
+	 * result in unexpected behavior, and so KVM's (historical) ABI is to
+	 * trust the guest to behave only as a last resort.
 	 */

arch/x86/kvm/vmx/vmx.c static u8 vmx_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio) · line 7615

Quoted 4 lines

arch/x86/kvm/vmx/vmx.c +2 -1

-	if (!kvm_arch_has_noncoherent_dma(vcpu->kvm))
+	if (!static_cpu_has(X86_FEATURE_SELFSNOOP) &&
+	    !kvm_arch_has_noncoherent_dma(vcpu->kvm))
 		return (MTRR_TYPE_WRBACK << VMX_EPT_MT_EPTE_SHIFT) | VMX_EPT_IPAT_BIT;

W/ this change, guests w/o pass-thru devices can also access UC memory. Locking UC memory leads to bus lock. So, guests w/o pass-thru devices can potentially launch DOS attacks on other CPUs on host. isn't it a problem?

Unconditionally honor guest PAT on CPUs that support self-snoop, as Intel has confirmed that CPUs that support self-snoop always snoop caches and store buffers. I.e. CPUs with self-snoop maintain cache coherency even in the presence of aliased memtypes, thus there is no need to trust the guest behaves and only honor PAT as a last resort, as KVM does today.

Honoring guest PAT is desirable for use cases where the guest has access to non-coherent DMA _without_ bouncing through VFIO, e.g. when a virtual (mediated, for all intents and purposes) GPU is exposed to the guest, along with buffers that are consumed directly by the physical GPU, i.e. which can't be proxied by the host to ensure writes from the guest are performed with the correct memory type for the GPU.

Cc Yiwei Zhang <[email protected]>

Suggested-by Yan Zhao <[email protected]>

Suggested-by Kevin Tian <[email protected]>

Signed-off-by Sean Christopherson <[email protected]>

---
 arch/x86/kvm/mmu/mmu.c |  8 +++++---
 arch/x86/kvm/vmx/vmx.c | 10 ++++++----
 2 files changed, 11 insertions(+), 7 deletions(-)

arch/x86/kvm/mmu/mmu.c +5 -3

index 403cd8f914cd..7fa514830628 100644
@@ -4622,14 +4622,16 @@ static int kvm_tdp_mmu_page_fault(struct kvm_vcpu *vcpu,
 bool kvm_mmu_may_ignore_guest_pat(void)
 {
 	/*
-	 * When EPT is enabled (shadow_memtype_mask is non-zero), and the VM
+	 * When EPT is enabled (shadow_memtype_mask is non-zero), the CPU does
+	 * not support self-snoop (or is affected by an erratum), and the VM
 	 * has non-coherent DMA (DMA doesn't snoop CPU caches), KVM's ABI is to
 	 * honor the memtype from the guest's PAT so that guest accesses to
 	 * memory that is DMA'd aren't cached against the guest's wishes.  As a
 	 * result, KVM _may_ ignore guest PAT, whereas without non-coherent DMA,
-	 * KVM _always_ ignores guest PAT (when EPT is enabled).
+	 * KVM _always_ ignores or honors guest PAT, i.e. doesn't toggle SPTE
+	 * bits in response to non-coherent device (un)registration.
 	 */
-	return shadow_memtype_mask;
+	return !static_cpu_has(X86_FEATURE_SELFSNOOP) && shadow_memtype_mask;
 }
 
 int kvm_tdp_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault)

arch/x86/kvm/vmx/vmx.c +1 -3

index 17a8e4fdf9c4..5dc4c24ae203 100644
@@ -7605,11 +7605,13 @@ static u8 vmx_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
 
 	/*
 	 * Force WB and ignore guest PAT if the VM does NOT have a non-coherent
-	 * device attached.  Letting the guest control memory types on Intel
-	 * CPUs may result in unexpected behavior, and so KVM's ABI is to trust
-	 * the guest to behave only as a last resort.
+	 * device attached and the CPU doesn't support self-snoop.  Letting the

Necroposting!

Turns out that this change broke "bochs-display" driver in QEMU even when the guest is modern (don't ask me 'who the hell uses bochs for modern guests', it was basically a configuration error :-). E.g:

The CPU where this reproduces is fairly modern too (Intel(R) Xeon(R) Silver 4410Y, Sapphire Rapids). I wish I could give additional details to what exactly happens in the guest but I can't find anything useful in the logs ("WARNING: Application 'org.gnome.Shell.desktop' killed by signal 9") and I know too little (nothing?) about how modern Linux graphics stack is organized :-( Cc: Gerd just in case.

Quoted 5 lines

Necroposting!

Turns out that this change broke "bochs-display" driver in QEMU even when the guest is modern (don't ask me 'who the hell uses bochs for modern guests', it was basically a configuration error :-). E.g:

qemu stdvga (the default display device) is affected too.

Quoted 8 lines · Gerd Hoffmann

Quoted 5 lines

Necroposting!

Turns out that this change broke "bochs-display" driver in QEMU even when the guest is modern (don't ask me 'who the hell uses bochs for modern guests', it was basically a configuration error :-). E.g:

qemu stdvga (the default display device) is affected too.

So far, I was only able to verify that the issue has nothing to do with OVMF and multi-vcpu, it reproduces very well with

$ qemu-kvm -machine q35,accel=kvm,kernel-irqchip=split -name guest=c10s

(quoted diff) +0 -3

-cpu host -smp 1 -m 16384 -drive file=/var/lib/libvirt/images/c10s-bios.qcow2,if=none,id=drive-ide0-0-0
-device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1
-vnc :0 -device VGA -monitor stdio --no-reboot

Comparing traces of working and broken cases, I couldn't find anything suspicious but I may had missed something of course. For now, it seems like a userspace misbehavior resulting in a segfault.

Quoted 19 lines · Vitaly Kuznetsov

Quoted 8 lines · Gerd Hoffmann

Quoted 5 lines

Necroposting!

Turns out that this change broke "bochs-display" driver in QEMU even when the guest is modern (don't ask me 'who the hell uses bochs for modern guests', it was basically a configuration error :-). E.g:

qemu stdvga (the default display device) is affected too.

So far, I was only able to verify that the issue has nothing to do with OVMF and multi-vcpu, it reproduces very well with

$ qemu-kvm -machine q35,accel=kvm,kernel-irqchip=split -name guest=c10s

(quoted diff) +0 -3

-cpu host -smp 1 -m 16384 -drive file=/var/lib/libvirt/images/c10s-bios.qcow2,if=none,id=drive-ide0-0-0
-device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1
-vnc :0 -device VGA -monitor stdio --no-reboot

Quoted 3 lines

Comparing traces of working and broken cases, I couldn't find anything suspicious but I may had missed something of course. For now, it seems like a userspace misbehavior resulting in a segfault.

Guest userspace?

Quoted 20 lines · Sean Christopherson

Quoted 19 lines · Vitaly Kuznetsov

Quoted 8 lines · Gerd Hoffmann

Quoted 5 lines

Necroposting!

Turns out that this change broke "bochs-display" driver in QEMU even when the guest is modern (don't ask me 'who the hell uses bochs for modern guests', it was basically a configuration error :-). E.g:

qemu stdvga (the default display device) is affected too.

So far, I was only able to verify that the issue has nothing to do with OVMF and multi-vcpu, it reproduces very well with

$ qemu-kvm -machine q35,accel=kvm,kernel-irqchip=split -name guest=c10s

(quoted diff) +0 -3

-cpu host -smp 1 -m 16384 -drive file=/var/lib/libvirt/images/c10s-bios.qcow2,if=none,id=drive-ide0-0-0
-device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1
-vnc :0 -device VGA -monitor stdio --no-reboot

Quoted 6 lines

Quoted 3 lines

Comparing traces of working and broken cases, I couldn't find anything suspicious but I may had missed something of course. For now, it seems like a userspace misbehavior resulting in a segfault.

Guest userspace?

Yes? :-) As Gerd described, video memory is "mapped into userspace so the wayland / X11 display server can software-render into the buffer" and it seems that wayland gets something unexpected in this memory and crashes.

Quoted 29 lines · Vitaly Kuznetsov

Quoted 20 lines · Sean Christopherson

Quoted 19 lines · Vitaly Kuznetsov

Quoted 8 lines · Gerd Hoffmann

Quoted 5 lines

Necroposting!

Turns out that this change broke "bochs-display" driver in QEMU even when the guest is modern (don't ask me 'who the hell uses bochs for modern guests', it was basically a configuration error :-). E.g:

qemu stdvga (the default display device) is affected too.

So far, I was only able to verify that the issue has nothing to do with OVMF and multi-vcpu, it reproduces very well with

$ qemu-kvm -machine q35,accel=kvm,kernel-irqchip=split -name guest=c10s

(quoted diff) +0 -3

-cpu host -smp 1 -m 16384 -drive file=/var/lib/libvirt/images/c10s-bios.qcow2,if=none,id=drive-ide0-0-0
-device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1
-vnc :0 -device VGA -monitor stdio --no-reboot

Quoted 6 lines

Quoted 3 lines

Comparing traces of working and broken cases, I couldn't find anything suspicious but I may had missed something of course. For now, it seems like a userspace misbehavior resulting in a segfault.

Guest userspace?

Quoted 4 lines

Yes? :-) As Gerd described, video memory is "mapped into userspace so the wayland / X11 display server can software-render into the buffer" and it seems that wayland gets something unexpected in this memory and crashes.

Also, I don't know if it helps or not, but out of two hunks in 377b2f359d1f, it is the vmx_get_mt_mask() one which brings the issue. I.e. the following is enough to fix things:

arch/x86/kvm/vmx/vmx.c +0 -4

index f18c2d8c7476..733a0c45d1a6 100644
@@ -7659,13 +7659,11 @@ u8 vmx_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
 
        /*
         * Force WB and ignore guest PAT if the VM does NOT have a non-coherent
-        * device attached and the CPU doesn't support self-snoop.  Letting the
-        * guest control memory types on Intel CPUs without self-snoop may
-        * result in unexpected behavior, and so KVM's (historical) ABI is to
-        * trust the guest to behave only as a last resort.

Quoted 30 lines

Quoted 4 lines

Yes? :-) As Gerd described, video memory is "mapped into userspace so the wayland / X11 display server can software-render into the buffer" and it seems that wayland gets something unexpected in this memory and crashes.

Also, I don't know if it helps or not, but out of two hunks in 377b2f359d1f, it is the vmx_get_mt_mask() one which brings the issue. I.e. the following is enough to fix things:

arch/x86/kvm/vmx/vmx.c +4 -6

index f18c2d8c7476..733a0c45d1a6 100644
@@ -7659,13 +7659,11 @@ u8 vmx_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
 
        /*
         * Force WB and ignore guest PAT if the VM does NOT have a non-coherent
-        * device attached and the CPU doesn't support self-snoop.  Letting the
-        * guest control memory types on Intel CPUs without self-snoop may
-        * result in unexpected behavior, and so KVM's (historical) ABI is to
-        * trust the guest to behave only as a last resort.
+        * device attached.  Letting the guest control memory types on Intel
+        * CPUs may result in unexpected behavior, and so KVM's ABI is to trust
+        * the guest to behave only as a last resort.
         */
-       if (!static_cpu_has(X86_FEATURE_SELFSNOOP) &&
-           !kvm_arch_has_noncoherent_dma(vcpu->kvm))
+       if (!kvm_arch_has_noncoherent_dma(vcpu->kvm))
                return (MTRR_TYPE_WRBACK << VMX_EPT_MT_EPTE_SHIFT) | VMX_EPT_IPAT_BIT;
 

arch/x86/kvm/vmx/vmx.c u8 vmx_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio) · line 7669

Quoted 1 line

arch/x86/kvm/vmx/vmx.c +0 -0

        return (MTRR_TYPE_WRBACK << VMX_EPT_MT_EPTE_SHIFT);

Hmm, that suggests the guest kernel maps the buffer as WC. And looking at the bochs driver, IIUC, the kernel mappings via ioremap() are UC-, not WC. So it could be that userspace doesn't play nice with WC, but could it also be that the QEMU backend doesn't play nice with WC (on Intel)?

Given that this is a purely synthetic device, is there any reason to use UC or WC?

I.e. can the bochs driver configure its VRAM buffers to be WB? It doesn't look super easy (the DRM/TTM code has so. many. layers), but it appears doable. Since the device only exists in VMs, it's possible the bochs driver has never run on Intel CPUs with WC memtype.

Quoted 8 lines · Gerd Hoffmann

Quoted 5 lines

Necroposting!

Turns out that this change broke "bochs-display" driver in QEMU even when the guest is modern (don't ask me 'who the hell uses bochs for modern guests', it was basically a configuration error :-). E.g:

qemu stdvga (the default display device) is affected too.

So far, I was only able to verify that the issue has nothing to do with OVMF and multi-vcpu, it reproduces very well with

$ qemu-kvm -machine q35,accel=kvm,kernel-irqchip=split -name guest=c10s

(quoted diff) +0 -3

-cpu host -smp 1 -m 16384 -drive file=/var/lib/libvirt/images/c10s-bios.qcow2,if=none,id=drive-ide0-0-0
-device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1
-vnc :0 -device VGA -monitor stdio --no-reboot

Comparing traces of working and broken cases, I couldn't find anything suspicious but I may had missed something of course. For now, it seems like a userspace misbehavior resulting in a segfault.

Quoted 19 lines · Vitaly Kuznetsov

Quoted 8 lines · Gerd Hoffmann

Quoted 5 lines

Necroposting!

Turns out that this change broke "bochs-display" driver in QEMU even when the guest is modern (don't ask me 'who the hell uses bochs for modern guests', it was basically a configuration error :-). E.g:

qemu stdvga (the default display device) is affected too.

So far, I was only able to verify that the issue has nothing to do with OVMF and multi-vcpu, it reproduces very well with

$ qemu-kvm -machine q35,accel=kvm,kernel-irqchip=split -name guest=c10s

(quoted diff) +0 -3

-cpu host -smp 1 -m 16384 -drive file=/var/lib/libvirt/images/c10s-bios.qcow2,if=none,id=drive-ide0-0-0
-device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1
-vnc :0 -device VGA -monitor stdio --no-reboot

Quoted 3 lines

Comparing traces of working and broken cases, I couldn't find anything suspicious but I may had missed something of course. For now, it seems like a userspace misbehavior resulting in a segfault.

Could you please share steps launch the broken guest desktop? (better also with guest kernel version, name of desktop processes,
name of X server)

Quoted 20 lines · Yan Zhao

Quoted 19 lines · Vitaly Kuznetsov

Quoted 8 lines · Gerd Hoffmann

Quoted 5 lines

Necroposting!

Turns out that this change broke "bochs-display" driver in QEMU even when the guest is modern (don't ask me 'who the hell uses bochs for modern guests', it was basically a configuration error :-). E.g:

qemu stdvga (the default display device) is affected too.

So far, I was only able to verify that the issue has nothing to do with OVMF and multi-vcpu, it reproduces very well with

$ qemu-kvm -machine q35,accel=kvm,kernel-irqchip=split -name guest=c10s

(quoted diff) +0 -3

-cpu host -smp 1 -m 16384 -drive file=/var/lib/libvirt/images/c10s-bios.qcow2,if=none,id=drive-ide0-0-0
-device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1
-vnc :0 -device VGA -monitor stdio --no-reboot

Quoted 6 lines

Quoted 3 lines

Comparing traces of working and broken cases, I couldn't find anything suspicious but I may had missed something of course. For now, it seems like a userspace misbehavior resulting in a segfault.

Could you please share steps launch the broken guest desktop? (better also with guest kernel version, name of desktop processes,
name of X server)

I think the easiest would be to download the latest Centos Stream 10 iso, e.g:

https://composes.stream.centos.org/stream-10/development/CentOS-Stream-10-20240902.d.0/compose/BaseOS/x86_64/iso/CentOS-Stream-10-20240902.d.0-x86_64-dvd1.iso
(the link is probably not eternal but should work for a couple weeks, check https://composes.stream.centos.org/stream-10/development/ it it doesn't work anymore).

Then, just run it:

$ /usr/libexec/qemu-kvm -machine q35,accel=kvm,kernel-irqchip=split -name guest=c10s -cpu host -smp 1 -m 16384 -cdrom CentOS-Stream-10-20240902.d.0-x86_64-dvd1.iso -vnc :0 -device VGA -monitor stdio --no-reboot

and connect to VNC console. To speed things up, pick 'Install Centos Stream 10' in the boot menu to avoid ISO integrity check.

With "KVM: VMX: Always honor guest PAT on CPUs that support self-snoop" commit included, you will see the following on the VNC console:
installer tries starting Wayland, crashes and drops back into text console. If you revert the commit and start over, Wayland will normally start and you will see the installer.

If the installer environment is inconvenient for debugging, then you can install in text mode (or with the commit reverted :-) and then the same problem will be observed when gdm starts.

FWIW, I use QEMU-9.0 from the same C10S (qemu-kvm-9.0.0-7.el10.x86_64) but I don't think it matters in this case. My CPU is "Intel(R) Xeon(R) Silver 4410Y".

FWIW, I use QEMU-9.0 from the same C10S (qemu-kvm-9.0.0-7.el10.x86_64) but I don't think it matters in this case. My CPU is "Intel(R) Xeon(R) Silver 4410Y".

Quoted 3 lines · Vitaly Kuznetsov

FWIW, I use QEMU-9.0 from the same C10S (qemu-kvm-9.0.0-7.el10.x86_64) but I don't think it matters in this case. My CPU is "Intel(R) Xeon(R) Silver 4410Y".

Has this been reproduced on any other hardware besides SPR? I.e. did we stumble on another hardware issue?

Quoted 5 lines · Sean Christopherson

Quoted 3 lines · Vitaly Kuznetsov

FWIW, I use QEMU-9.0 from the same C10S (qemu-kvm-9.0.0-7.el10.x86_64) but I don't think it matters in this case. My CPU is "Intel(R) Xeon(R) Silver 4410Y".

Quoted 2 lines

Has this been reproduced on any other hardware besides SPR? I.e. did we stumble on another hardware issue?

Very possible, as according to Yan Zhao this doesn't reproduce on at least "Coffee Lake-S". Let me try to grab some random hardware around and I'll be back with my observations.

Quoted 7 lines · Vitaly Kuznetsov

Quoted 5 lines · Sean Christopherson

Quoted 3 lines · Vitaly Kuznetsov

FWIW, I use QEMU-9.0 from the same C10S (qemu-kvm-9.0.0-7.el10.x86_64) but I don't think it matters in this case. My CPU is "Intel(R) Xeon(R) Silver 4410Y".

Quoted 6 lines

Quoted 2 lines

Has this been reproduced on any other hardware besides SPR? I.e. did we stumble on another hardware issue?

Very possible, as according to Yan Zhao this doesn't reproduce on at least "Coffee Lake-S". Let me try to grab some random hardware around and I'll be back with my observations.

Update some new findings from my side:

BAR 0 of bochs VGA (fb_map) is used for frame buffer, covering phys range from 0xfd000000 to 0xfe000000.

On "Sapphire Rapids XCC":

1. If KVM forces this fb_map range to be WC+IPAT, installer/gdm can launch correctly.
i.e.

   if (gfn >= 0xfd000 && gfn < 0xfe000) {
   	return (MTRR_TYPE_WRCOMB << VMX_EPT_MT_EPTE_SHIFT) | VMX_EPT_IPAT_BIT;
   }
   return MTRR_TYPE_WRBACK << VMX_EPT_MT_EPTE_SHIFT;

2. If KVM forces this fb_map range to be UC+IPAT, installer failes to show / gdm restarts endlessly. (though on Coffee Lake-S, installer/gdm can launch correctly in this case).

3. On starting GDM, ttm_kmap_iter_linear_io_init() in guest is called to set this fb_map range as WC, with iosys_map_set_vaddr_iomem(&iter_io->dmap, ioremap_wc(mem->bus.offset, mem->size));

However, during bochs_pci_probe()-->bochs_load()-->bochs_hw_init(), pfns for this fb_map has been reserved as uc- by ioremap().
Then, the ioremap_wc() during starting GDM will only map guest PAT with UC-.

So, with KVM setting WB (no IPAT) to this fb_map range, the effective memory type is UC- and installer/gdm restarts endlessly.

4. If KVM sets WB (no IPAT) to this fb_map range, and changes guest bochs driver to call ioremap_wc() instead in bochs_hw_init(), gdm can launch correctly. (didn't verify the installer's case as I can't update the driver in that case).

The reason is that the ioremap_wc() called during starting GDM will no longer meet conflict and can map guest PAT as WC.

WIP to find out why effective UC in fb_map range will make gdm to restart endlessly.

Quoted 45 lines · Yan Zhao

Quoted 7 lines · Vitaly Kuznetsov

Quoted 5 lines · Sean Christopherson

Quoted 3 lines · Vitaly Kuznetsov

FWIW, I use QEMU-9.0 from the same C10S (qemu-kvm-9.0.0-7.el10.x86_64) but I don't think it matters in this case. My CPU is "Intel(R) Xeon(R) Silver 4410Y".

Quoted 6 lines

Quoted 2 lines

Has this been reproduced on any other hardware besides SPR? I.e. did we stumble on another hardware issue?

Very possible, as according to Yan Zhao this doesn't reproduce on at least "Coffee Lake-S". Let me try to grab some random hardware around and I'll be back with my observations.

Update some new findings from my side:

BAR 0 of bochs VGA (fb_map) is used for frame buffer, covering phys range from 0xfd000000 to 0xfe000000.

On "Sapphire Rapids XCC":

1. If KVM forces this fb_map range to be WC+IPAT, installer/gdm can launch correctly.
i.e.

   if (gfn >= 0xfd000 && gfn < 0xfe000) {
   	return (MTRR_TYPE_WRCOMB << VMX_EPT_MT_EPTE_SHIFT) | VMX_EPT_IPAT_BIT;
   }
   return MTRR_TYPE_WRBACK << VMX_EPT_MT_EPTE_SHIFT;

2. If KVM forces this fb_map range to be UC+IPAT, installer failes to show / gdm restarts endlessly. (though on Coffee Lake-S, installer/gdm can launch correctly in this case).

3. On starting GDM, ttm_kmap_iter_linear_io_init() in guest is called to set this fb_map range as WC, with iosys_map_set_vaddr_iomem(&iter_io->dmap, ioremap_wc(mem->bus.offset, mem->size));

However, during bochs_pci_probe()-->bochs_load()-->bochs_hw_init(), pfns for this fb_map has been reserved as uc- by ioremap().
Then, the ioremap_wc() during starting GDM will only map guest PAT with UC-.

So, with KVM setting WB (no IPAT) to this fb_map range, the effective memory type is UC- and installer/gdm restarts endlessly.

Quoted 6 lines

4. If KVM sets WB (no IPAT) to this fb_map range, and changes guest bochs driver to call ioremap_wc() instead in bochs_hw_init(), gdm can launch correctly. (didn't verify the installer's case as I can't update the driver in that case).

The reason is that the ioremap_wc() called during starting GDM will no longer meet conflict and can map guest PAT as WC.

Quoted 2 lines

WIP to find out why effective UC in fb_map range will make gdm to restart endlessly.

Not sure whether it's simply because UC is too slow.

T=Test execution time of a selftest in which guest writes to a GPA for
0x1000000UL times

              | Sapphire Rapids XCC  | Coffee Lake-S
--------------|----------------------|-----------------
KVM UC+IPAT   |    T=0m4.530s        |  T=0m0.622s

--------------|----------------------|-----------------
KVM WC+IPAT   |    T=0m0.149s        |  T=0m0.176s
--------------|----------------------|-----------------
KVM WB+IPAT   |    T=0m0.148s        |  T=0m0.148s

------------------------------------------------------

Quoted 50 lines · Yan Zhao

Quoted 49 lines · Yan Zhao

Quoted 7 lines · Vitaly Kuznetsov

Quoted 5 lines · Sean Christopherson

Quoted 3 lines · Vitaly Kuznetsov

FWIW, I use QEMU-9.0 from the same C10S (qemu-kvm-9.0.0-7.el10.x86_64) but I don't think it matters in this case. My CPU is "Intel(R) Xeon(R) Silver 4410Y".

Quoted 6 lines

Quoted 2 lines

Has this been reproduced on any other hardware besides SPR? I.e. did we stumble on another hardware issue?

Very possible, as according to Yan Zhao this doesn't reproduce on at least "Coffee Lake-S". Let me try to grab some random hardware around and I'll be back with my observations.

Update some new findings from my side:

BAR 0 of bochs VGA (fb_map) is used for frame buffer, covering phys range from 0xfd000000 to 0xfe000000.

On "Sapphire Rapids XCC":

1. If KVM forces this fb_map range to be WC+IPAT, installer/gdm can launch correctly.
i.e.

   if (gfn >= 0xfd000 && gfn < 0xfe000) {
   	return (MTRR_TYPE_WRCOMB << VMX_EPT_MT_EPTE_SHIFT) | VMX_EPT_IPAT_BIT;
   }
   return MTRR_TYPE_WRBACK << VMX_EPT_MT_EPTE_SHIFT;

2. If KVM forces this fb_map range to be UC+IPAT, installer failes to show / gdm restarts endlessly. (though on Coffee Lake-S, installer/gdm can launch correctly in this case).

3. On starting GDM, ttm_kmap_iter_linear_io_init() in guest is called to set this fb_map range as WC, with iosys_map_set_vaddr_iomem(&iter_io->dmap, ioremap_wc(mem->bus.offset, mem->size));

However, during bochs_pci_probe()-->bochs_load()-->bochs_hw_init(), pfns for this fb_map has been reserved as uc- by ioremap().
Then, the ioremap_wc() during starting GDM will only map guest PAT with UC-.

So, with KVM setting WB (no IPAT) to this fb_map range, the effective memory type is UC- and installer/gdm restarts endlessly.

4. If KVM sets WB (no IPAT) to this fb_map range, and changes guest bochs driver to call ioremap_wc() instead in bochs_hw_init(), gdm can launch correctly. (didn't verify the installer's case as I can't update the driver in that case).

Quoted 2 lines

Quoted 2 lines

The reason is that the ioremap_wc() called during starting GDM will no longer meet conflict and can map guest PAT as WC.

Huh. The upside of this is that it sounds like there's nothing broken with WC or self-snoop.

Quoted 8 lines

Quoted 3 lines

WIP to find out why effective UC in fb_map range will make gdm to restart endlessly.

Not sure whether it's simply because UC is too slow.

T=Test execution time of a selftest in which guest writes to a GPA for
0x1000000UL times

Quoted 3 lines

              | Sapphire Rapids XCC  | Coffee Lake-S
--------------|----------------------|-----------------
KVM UC+IPAT   |    T=0m4.530s        |  T=0m0.622s

Woah. Have you tried testing MOVDIR64 and/or WT? E.g. to see if the problem is with UC specifically, or if it occurs with any accesses that immediately write through to main memory.

Quoted 5 lines

--------------|----------------------|-----------------
KVM WC+IPAT   |    T=0m0.149s        |  T=0m0.176s
--------------|----------------------|-----------------
KVM WB+IPAT   |    T=0m0.148s        |  T=0m0.148s

------------------------------------------------------

Quoted 51 lines · Sean Christopherson

Quoted 50 lines · Yan Zhao

Quoted 49 lines · Yan Zhao

Quoted 7 lines · Vitaly Kuznetsov

Quoted 5 lines · Sean Christopherson

Quoted 3 lines · Vitaly Kuznetsov

FWIW, I use QEMU-9.0 from the same C10S (qemu-kvm-9.0.0-7.el10.x86_64) but I don't think it matters in this case. My CPU is "Intel(R) Xeon(R) Silver 4410Y".

Quoted 6 lines

Quoted 2 lines

Has this been reproduced on any other hardware besides SPR? I.e. did we stumble on another hardware issue?

Very possible, as according to Yan Zhao this doesn't reproduce on at least "Coffee Lake-S". Let me try to grab some random hardware around and I'll be back with my observations.

Update some new findings from my side:

BAR 0 of bochs VGA (fb_map) is used for frame buffer, covering phys range from 0xfd000000 to 0xfe000000.

On "Sapphire Rapids XCC":

1. If KVM forces this fb_map range to be WC+IPAT, installer/gdm can launch correctly.
i.e.

   if (gfn >= 0xfd000 && gfn < 0xfe000) {
   	return (MTRR_TYPE_WRCOMB << VMX_EPT_MT_EPTE_SHIFT) | VMX_EPT_IPAT_BIT;
   }
   return MTRR_TYPE_WRBACK << VMX_EPT_MT_EPTE_SHIFT;

2. If KVM forces this fb_map range to be UC+IPAT, installer failes to show / gdm restarts endlessly. (though on Coffee Lake-S, installer/gdm can launch correctly in this case).

3. On starting GDM, ttm_kmap_iter_linear_io_init() in guest is called to set this fb_map range as WC, with iosys_map_set_vaddr_iomem(&iter_io->dmap, ioremap_wc(mem->bus.offset, mem->size));

However, during bochs_pci_probe()-->bochs_load()-->bochs_hw_init(), pfns for this fb_map has been reserved as uc- by ioremap().
Then, the ioremap_wc() during starting GDM will only map guest PAT with UC-.

So, with KVM setting WB (no IPAT) to this fb_map range, the effective memory type is UC- and installer/gdm restarts endlessly.

4. If KVM sets WB (no IPAT) to this fb_map range, and changes guest bochs driver to call ioremap_wc() instead in bochs_hw_init(), gdm can launch correctly. (didn't verify the installer's case as I can't update the driver in that case).

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

The reason is that the ioremap_wc() called during starting GDM will no longer meet conflict and can map guest PAT as WC.

Huh. The upside of this is that it sounds like there's nothing broken with WC or self-snoop.

Considering a different perspective, the fb_map range is used as frame buffer (vram), with the guest writing to this range and the host reading from it. If the issue were related to self-snooping, we would expect the VNC window to display distorted data. However, the observed behavior is that the GDM window shows up correctly for a sec and restarts over and over.

So, do you think we can simply fix this issue by calling ioremap_wc() for the frame buffer/vram range in bochs driver, as is commonly done in other gpu drivers?

drivers/gpu/drm/tiny/bochs.c +1 -1

@@ -261,7 +261,9 @@ static int bochs_hw_init(struct drm_device *dev)
        if (pci_request_region(pdev, 0, "bochs-drm") != 0)
                DRM_WARN("Cannot request framebuffer, boot fb still active?\n");
 
-       bochs->fb_map = ioremap(addr, size);
+       bochs->fb_map = ioremap_wc(addr, size);
        if (bochs->fb_map == NULL) {
                DRM_ERROR("Cannot map framebuffer\n");
                return -ENOMEM;

Quoted 10 lines

Quoted 10 lines

Quoted 5 lines

WIP to find out why effective UC in fb_map range will make gdm to restart endlessly.

Not sure whether it's simply because UC is too slow.

T=Test execution time of a selftest in which guest writes to a GPA for
0x1000000UL times

Quoted 13 lines

Quoted 3 lines

              | Sapphire Rapids XCC  | Coffee Lake-S
--------------|----------------------|-----------------
KVM UC+IPAT   |    T=0m4.530s        |  T=0m0.622s

Woah. Have you tried testing MOVDIR64 and/or WT? E.g. to see if the problem is with UC specifically, or if it occurs with any accesses that immediately write through to main memory.

Quoted 5 lines

--------------|----------------------|-----------------
KVM WC+IPAT   |    T=0m0.149s        |  T=0m0.176s
--------------|----------------------|-----------------
KVM WB+IPAT   |    T=0m0.148s        |  T=0m0.148s

------------------------------------------------------

I re-run all the tests and collected an averaged data (10 times each) as below (previous data was just a single-run score):

T=Test execution time of a selftest in which guest writes to a GPA for
0x1000000UL times with WRITE_ONCE

KVM memtype  | Sapphire Rapids XCC | Coffee Lake-S
-------------|---------------------|----------------
 WB+IPAT     |     T=0.1511s       |    T=0.1661s
-------------|---------------------|----------------
 WC+IPAT     |     T=0.1411s       |    T=0.1656s
-------------|---------------------|----------------
 WT+IPAT     |     T=3.7527s       |    T=0.6156s
-------------|---------------------|----------------
 WP+IPAT     |     T=4.4663s       |    T=0.6203s
-------------|---------------------|----------------
 UC+IPAT     |     T=3.4632s       |    T=0.5868s

T=Test execution time of a selftest in which guest writes to a GPA for
0x1000000UL times with movdir64b.

(Coffee Lake-S has no feature movdir64).

KVM memtype  | Sapphire Rapids XCC | Coffee Lake-S
-------------|---------------------|----------------
 WB+IPAT     |     T=2.6142s       |       /     
-------------|---------------------|----------------
 WC+IPAT     |     T=2.8919s       |       /     
-------------|---------------------|----------------
 WT+IPAT     |     T=3.0966s       |       /      
-------------|---------------------|----------------
 WP+IPAT     |     T=2.4933s       |       /     
-------------|---------------------|----------------
 UC+IPAT     |     T=3.4606s       |       /     

Quoted 73 lines · Yan Zhao

Quoted 51 lines · Sean Christopherson

Quoted 50 lines · Yan Zhao

Quoted 49 lines · Yan Zhao

Quoted 7 lines · Vitaly Kuznetsov

Quoted 5 lines · Sean Christopherson

Quoted 3 lines · Vitaly Kuznetsov

FWIW, I use QEMU-9.0 from the same C10S (qemu-kvm-9.0.0-7.el10.x86_64) but I don't think it matters in this case. My CPU is "Intel(R) Xeon(R) Silver 4410Y".

Quoted 6 lines

Quoted 2 lines

Has this been reproduced on any other hardware besides SPR? I.e. did we stumble on another hardware issue?

Very possible, as according to Yan Zhao this doesn't reproduce on at least "Coffee Lake-S". Let me try to grab some random hardware around and I'll be back with my observations.

Update some new findings from my side:

BAR 0 of bochs VGA (fb_map) is used for frame buffer, covering phys range from 0xfd000000 to 0xfe000000.

On "Sapphire Rapids XCC":

1. If KVM forces this fb_map range to be WC+IPAT, installer/gdm can launch

    correctly.
    i.e.
    if (gfn >= 0xfd000 && gfn < 0xfe000) {
    	return (MTRR_TYPE_WRCOMB << VMX_EPT_MT_EPTE_SHIFT) | VMX_EPT_IPAT_BIT;
    }
    return MTRR_TYPE_WRBACK << VMX_EPT_MT_EPTE_SHIFT;

2. If KVM forces this fb_map range to be UC+IPAT, installer failes to show / gdm restarts endlessly. (though on Coffee Lake-S, installer/gdm can launch correctly in this case).

3. On starting GDM, ttm_kmap_iter_linear_io_init() in guest is called to set this fb_map range as WC, with iosys_map_set_vaddr_iomem(&iter_io->dmap, ioremap_wc(mem->bus.offset, mem->size));

However, during bochs_pci_probe()-->bochs_load()-->bochs_hw_init(), pfns for this fb_map has been reserved as uc- by ioremap().
Then, the ioremap_wc() during starting GDM will only map guest PAT with UC-.

So, with KVM setting WB (no IPAT) to this fb_map range, the effective memory type is UC- and installer/gdm restarts endlessly.

4. If KVM sets WB (no IPAT) to this fb_map range, and changes guest bochs driver to call ioremap_wc() instead in bochs_hw_init(), gdm can launch correctly. (didn't verify the installer's case as I can't update the driver in that case).

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

The reason is that the ioremap_wc() called during starting GDM will no longer meet conflict and can map guest PAT as WC.

Huh. The upside of this is that it sounds like there's nothing broken with WC or self-snoop.

Considering a different perspective, the fb_map range is used as frame buffer (vram), with the guest writing to this range and the host reading from it. If the issue were related to self-snooping, we would expect the VNC window to display distorted data. However, the observed behavior is that the GDM window shows up correctly for a sec and restarts over and over.

So, do you think we can simply fix this issue by calling ioremap_wc() for the frame buffer/vram range in bochs driver, as is commonly done in other gpu drivers?

drivers/gpu/drm/tiny/bochs.c +0 -0

@@ -261,7 +261,9 @@ static int bochs_hw_init(struct drm_device *dev)
         if (pci_request_region(pdev, 0, "bochs-drm") != 0)
                 DRM_WARN("Cannot request framebuffer, boot fb still active?\n");
 

drivers/gpu/drm/tiny/bochs.c static int bochs_hw_init(struct drm_device *dev) · line 267

Quoted 5 lines

drivers/gpu/drm/tiny/bochs.c +1 -1

-       bochs->fb_map = ioremap(addr, size);
+       bochs->fb_map = ioremap_wc(addr, size);
         if (bochs->fb_map == NULL) {
                 DRM_ERROR("Cannot map framebuffer\n");
                 return -ENOMEM;

Quoted 74 lines · Yan Zhao

Quoted 73 lines · Yan Zhao

Quoted 51 lines · Sean Christopherson

Quoted 50 lines · Yan Zhao

Quoted 49 lines · Yan Zhao

Quoted 7 lines · Vitaly Kuznetsov

Quoted 5 lines · Sean Christopherson

Quoted 3 lines · Vitaly Kuznetsov

FWIW, I use QEMU-9.0 from the same C10S (qemu-kvm-9.0.0-7.el10.x86_64) but I don't think it matters in this case. My CPU is "Intel(R) Xeon(R) Silver 4410Y".

Quoted 6 lines

Quoted 2 lines

Has this been reproduced on any other hardware besides SPR? I.e. did we stumble on another hardware issue?

Very possible, as according to Yan Zhao this doesn't reproduce on at least "Coffee Lake-S". Let me try to grab some random hardware around and I'll be back with my observations.

Update some new findings from my side:

BAR 0 of bochs VGA (fb_map) is used for frame buffer, covering phys range from 0xfd000000 to 0xfe000000.

On "Sapphire Rapids XCC":

1. If KVM forces this fb_map range to be WC+IPAT, installer/gdm can launch

    correctly.
    i.e.
    if (gfn >= 0xfd000 && gfn < 0xfe000) {
    	return (MTRR_TYPE_WRCOMB << VMX_EPT_MT_EPTE_SHIFT) | VMX_EPT_IPAT_BIT;
    }
    return MTRR_TYPE_WRBACK << VMX_EPT_MT_EPTE_SHIFT;

2. If KVM forces this fb_map range to be UC+IPAT, installer failes to show / gdm restarts endlessly. (though on Coffee Lake-S, installer/gdm can launch correctly in this case).

3. On starting GDM, ttm_kmap_iter_linear_io_init() in guest is called to set this fb_map range as WC, with iosys_map_set_vaddr_iomem(&iter_io->dmap, ioremap_wc(mem->bus.offset, mem->size));

However, during bochs_pci_probe()-->bochs_load()-->bochs_hw_init(), pfns for this fb_map has been reserved as uc- by ioremap().
Then, the ioremap_wc() during starting GDM will only map guest PAT with UC-.

So, with KVM setting WB (no IPAT) to this fb_map range, the effective memory type is UC- and installer/gdm restarts endlessly.

4. If KVM sets WB (no IPAT) to this fb_map range, and changes guest bochs driver to call ioremap_wc() instead in bochs_hw_init(), gdm can launch correctly. (didn't verify the installer's case as I can't update the driver in that case).

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

The reason is that the ioremap_wc() called during starting GDM will no longer meet conflict and can map guest PAT as WC.

Huh. The upside of this is that it sounds like there's nothing broken with WC or self-snoop.

Considering a different perspective, the fb_map range is used as frame buffer (vram), with the guest writing to this range and the host reading from it. If the issue were related to self-snooping, we would expect the VNC window to display distorted data. However, the observed behavior is that the GDM window shows up correctly for a sec and restarts over and over.

So, do you think we can simply fix this issue by calling ioremap_wc() for the frame buffer/vram range in bochs driver, as is commonly done in other gpu drivers?

drivers/gpu/drm/tiny/bochs.c +0 -0

@@ -261,7 +261,9 @@ static int bochs_hw_init(struct drm_device *dev)
         if (pci_request_region(pdev, 0, "bochs-drm") != 0)
                 DRM_WARN("Cannot request framebuffer, boot fb still active?\n");
 

Quoted 5 lines

drivers/gpu/drm/tiny/bochs.c static int bochs_hw_init(struct drm_device *dev) · line 267

Quoted 5 lines

drivers/gpu/drm/tiny/bochs.c +1 -1

-       bochs->fb_map = ioremap(addr, size);
+       bochs->fb_map = ioremap_wc(addr, size);
         if (bochs->fb_map == NULL) {
                 DRM_ERROR("Cannot map framebuffer\n");
                 return -ENOMEM;

While this is a fix for future kernels, it doesn't change the result for VMs already in existence.

I don't think there's an alternative to putting this behind a quirk.

While this is a fix for future kernels, it doesn't change the result for VMs already in existence.

I don't think there's an alternative to putting this behind a quirk.

Quoted 5 lines · Sean Christopherson

Quoted 3 lines · Vitaly Kuznetsov

FWIW, I use QEMU-9.0 from the same C10S (qemu-kvm-9.0.0-7.el10.x86_64) but I don't think it matters in this case. My CPU is "Intel(R) Xeon(R) Silver 4410Y".

Quoted 2 lines

Has this been reproduced on any other hardware besides SPR? I.e. did we stumble on another hardware issue?

Very possible, as according to Yan Zhao this doesn't reproduce on at least "Coffee Lake-S". Let me try to grab some random hardware around and I'll be back with my observations.

Quoted 6 lines · Sean Christopherson

Unconditionally honor guest PAT on CPUs that support self-snoop, as Intel has confirmed that CPUs that support self-snoop always snoop caches and store buffers. I.e. CPUs with self-snoop maintain cache coherency even in the presence of aliased memtypes, thus there is no need to trust the guest behaves and only honor PAT as a last resort, as KVM does today.

Quoted 6 lines

Honoring guest PAT is desirable for use cases where the guest has access to non-coherent DMA _without_ bouncing through VFIO, e.g. when a virtual (mediated, for all intents and purposes) GPU is exposed to the guest, along with buffers that are consumed directly by the physical GPU, i.e. which can't be proxied by the host to ensure writes from the guest are performed with the correct memory type for the GPU.

Necroposting!

Turns out that this change broke "bochs-display" driver in QEMU even when the guest is modern (don't ask me 'who the hell uses bochs for modern guests', it was basically a configuration error :-). E.g:
[...]

Quoted 15 lines · Vitaly Kuznetsov

Quoted 6 lines · Sean Christopherson

Unconditionally honor guest PAT on CPUs that support self-snoop, as Intel has confirmed that CPUs that support self-snoop always snoop caches and store buffers. I.e. CPUs with self-snoop maintain cache coherency even in the presence of aliased memtypes, thus there is no need to trust the guest behaves and only honor PAT as a last resort, as KVM does today.

Quoted 6 lines

Honoring guest PAT is desirable for use cases where the guest has access to non-coherent DMA _without_ bouncing through VFIO, e.g. when a virtual (mediated, for all intents and purposes) GPU is exposed to the guest, along with buffers that are consumed directly by the physical GPU, i.e. which can't be proxied by the host to ensure writes from the guest are performed with the correct memory type for the GPU.

Quoted 6 lines

Necroposting!

Turns out that this change broke "bochs-display" driver in QEMU even when the guest is modern (don't ask me 'who the hell uses bochs for modern guests', it was basically a configuration error :-). E.g:
[...]

This regression made it to the list of tracked regressions. It seems this thread stalled a while ago. Was this ever fixed? Does not look like it, but I might have missed something. Or is this a regression I should just ignore for one reason or another?

"Linux regression tracking (Thorsten Leemhuis)"

Quoted 23 lines · <[email protected]> writes:

Quoted 15 lines · Vitaly Kuznetsov

Quoted 6 lines · Sean Christopherson

Unconditionally honor guest PAT on CPUs that support self-snoop, as Intel has confirmed that CPUs that support self-snoop always snoop caches and store buffers. I.e. CPUs with self-snoop maintain cache coherency even in the presence of aliased memtypes, thus there is no need to trust the guest behaves and only honor PAT as a last resort, as KVM does today.

Quoted 6 lines

Honoring guest PAT is desirable for use cases where the guest has access to non-coherent DMA _without_ bouncing through VFIO, e.g. when a virtual (mediated, for all intents and purposes) GPU is exposed to the guest, along with buffers that are consumed directly by the physical GPU, i.e. which can't be proxied by the host to ensure writes from the guest are performed with the correct memory type for the GPU.

Quoted 6 lines

Necroposting!

Turns out that this change broke "bochs-display" driver in QEMU even when the guest is modern (don't ask me 'who the hell uses bochs for modern guests', it was basically a configuration error :-). E.g:
[...]

Quoted 5 lines

This regression made it to the list of tracked regressions. It seems this thread stalled a while ago. Was this ever fixed? Does not look like it, but I might have missed something. Or is this a regression I should just ignore for one reason or another?

The regression was addressed in by reverting 377b2f359d1f in 6.11

commit 9d70f3fec14421e793ffbc0ec2f739b24e534900

Author: Paolo Bonzini <[email protected]>
Date:   Sun Sep 15 02:49:33 2024 -0400

Revert "KVM: VMX: Always honor guest PAT on CPUs that support self-snoop"

Also, there's a (pending) DRM patch fixing it from the guest's side:
https://gitlab.freedesktop.org/drm/misc/kernel/-/commit/9388ccf69925223223c87355a417ba39b13a5e8e

First, rip out KVM's support for virtualizing guest MTRRs on VMX. The code is costly to main, a drag on guest boot performance, imperfect, and not required for functional correctness with modern guest kernels. Many details in patch 1's changelog.

With MTRR virtualization gone, always honor guest PAT on Intel CPUs that support self-snoop, as such CPUs are guaranteed to maintain coherency even if the guest is aliasing memtypes, e.g. if the host is using WB but the guest is using WC. Honoring guest PAT is desirable for use cases where the guest must use WC when accessing memory that is DMA'd from a non- coherent device that does NOT bounce through VFIO, e.g. for mediated virtual GPUs.

The SRCU patch adds an API that is effectively documentation for the memory barrier in srcu_read_lock(). Intel CPUs with self-snoop require a memory barrier after VM-Exit to ensure coherency, and KVM always does a srcu_read_lock() before reading guest memory after VM-Exit. Relying on SRCU to provide the barrier allows KVM to avoid emitting a redundant barrier of its own.

This series needs a _lot_ more testing; I arguably should have tagged it RFC, but I'm feeling lucky.

Sean Christopherson (3):
KVM: x86: Remove VMX support for virtualizing guest MTRR memtypes
KVM: VMX: Drop support for forcing UC memory when guest CR0.CD=1
KVM: VMX: Always honor guest PAT on CPUs that support self-snoop

Yan Zhao (2):
srcu: Add an API for a memory barrier after SRCU read lock
KVM: x86: Ensure a full memory barrier is emitted in the VM-Exit path

 Documentation/virt/kvm/api.rst        |   6 +-
 Documentation/virt/kvm/x86/errata.rst |  18 +
 arch/x86/include/asm/kvm_host.h       |  15 +-
 arch/x86/kvm/mmu.h                    |   7 +-
 arch/x86/kvm/mmu/mmu.c                |  35 +-
 arch/x86/kvm/mtrr.c                   | 644 ++------------------------
 arch/x86/kvm/vmx/vmx.c                |  40 +-
 arch/x86/kvm/x86.c                    |  24 +-
 arch/x86/kvm/x86.h                    |   4 -
 include/linux/srcu.h                  |  14 +
 10 files changed, 105 insertions(+), 702 deletions(-)

base-commit: 964d0c614c7f71917305a5afdca9178fe8231434 --
2.44.0.278.ge034bb2e1d-goog

First, rip out KVM's support for virtualizing guest MTRRs on VMX. The code is costly to main, a drag on guest boot performance, imperfect, and not required for functional correctness with modern guest kernels. Many details in patch 1's changelog.

With MTRR virtualization gone, always honor guest PAT on Intel CPUs that support self-snoop, as such CPUs are guaranteed to maintain coherency even if the guest is aliasing memtypes, e.g. if the host is using WB but the guest is using WC. Honoring guest PAT is desirable for use cases where the guest must use WC when accessing memory that is DMA'd from a non-coherent device that does NOT bounce through VFIO, e.g. for mediated virtual GPUs.

[...]

Quoted 13 lines · Sean Christopherson

First, rip out KVM's support for virtualizing guest MTRRs on VMX. The code is costly to main, a drag on guest boot performance, imperfect, and not required for functional correctness with modern guest kernels. Many details in patch 1's changelog.

With MTRR virtualization gone, always honor guest PAT on Intel CPUs that support self-snoop, as such CPUs are guaranteed to maintain coherency even if the guest is aliasing memtypes, e.g. if the host is using WB but the guest is using WC. Honoring guest PAT is desirable for use cases where the guest must use WC when accessing memory that is DMA'd from a non-coherent device that does NOT bounce through VFIO, e.g. for mediated virtual GPUs.

Quoted 1 line

[...]

Applied to kvm-x86 mtrrs, to get as much testing as possible before a potential merge in 6.11.

Paul, if you can take a gander at patch 3, it would be much appreciated.

Thanks!

[1/5] KVM: x86: Remove VMX support for virtualizing guest MTRR memtypes

      https://github.com/kvm-x86/linux/commit/0a7b73559b39
[2/5] KVM: VMX: Drop support for forcing UC memory when guest CR0.CD=1
      https://github.com/kvm-x86/linux/commit/e1548088ff54
[3/5] srcu: Add an API for a memory barrier after SRCU read lock
      https://github.com/kvm-x86/linux/commit/fcfe671e0879

[4/5] KVM: x86: Ensure a full memory barrier is emitted in the VM-Exit path

      https://github.com/kvm-x86/linux/commit/eb8d8fc29286
[5/5] KVM: VMX: Always honor guest PAT on CPUs that support self-snoop
      https://github.com/kvm-x86/linux/commit/95200f24b862

--
https://github.com/kvm-x86/linux/tree/next