Use WARN_ON_ONCE for missing X86_FEATURE_NRIPS

From 40c7213e34a1a1caf0b98db832c06c593f888b11 Mon Sep 17 00:00:00 2001

The cpu feature flags are not ever going to change, so warning everytime can cause a lot of kernel log spam
(in our case more than 10GB/hour).

Signed-off-by Dirk Mueller <[email protected]>

Applied. Do you also know what caused the warning, and/or would you like me to take a look?

Quoted 7 lines · Dirk Müller

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

index 94b7d15..0a42859 100644
@@ -514,7 +514,7 @@ static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
 	struct vcpu_svm *svm = to_svm(vcpu);
 

arch/x86/kvm/svm.c static void skip_emulated_instruction(struct kvm_vcpu *vcpu) · line 520

Quoted 6 lines

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

 	if (svm->vmcb->control.next_rip != 0) {
-		WARN_ON(!static_cpu_has(X86_FEATURE_NRIPS));
+		WARN_ON_ONCE(!static_cpu_has(X86_FEATURE_NRIPS));
 		svm->next_rip = svm->vmcb->control.next_rip;
 	}
 

Bandan, what was the reason for warning here?

Should we change the "if" condition to static_cpu_has(X86_FEATURE_NRIPS) instead of Dirk's patch?

Paolo

I added the warning so that we catch if the next_rip field is being written to (even if the feature isn't supported) by a buggy L1 hypervisor.

Quoted 2 lines

I added the warning so that we catch if the next_rip field is being written to (even if the feature isn't supported) by a buggy L1 hypervisor.

Interesting, so how about this patch?

From c5c8ea255d680f972cbdfc835cdf352fa78897ae Mon Sep 17 00:00:00 2001
From: Dirk Mueller <[email protected]>
Date: Fri, 2 Oct 2015 08:35:24 +0200
Subject: [PATCH] KVM: nSVM: Check for NRIP support before accepting
control.next_rip

NRIP support itself depends on cpuid Fn8000_000A_EDX[NRIPS], remove a WARN_ON_(once) and check for it directly.

Signed-off-by Dirk Mueller <[email protected]>

---
 arch/x86/kvm/svm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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

index 0a42859..33d36da 100644

Quoted 8 lines · Paolo Bonzini

Quoted 7 lines · Dirk Müller

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

index 94b7d15..0a42859 100644
@@ -514,7 +514,7 @@ static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
 	struct vcpu_svm *svm = to_svm(vcpu);
 

Quoted 8 lines

arch/x86/kvm/svm.c static void skip_emulated_instruction(struct kvm_vcpu *vcpu) · line 520

Quoted 6 lines

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

 	if (svm->vmcb->control.next_rip != 0) {
-		WARN_ON(!static_cpu_has(X86_FEATURE_NRIPS));
+		WARN_ON_ONCE(!static_cpu_has(X86_FEATURE_NRIPS));
 		svm->next_rip = svm->vmcb->control.next_rip;
 	}
 

Bandan, what was the reason for warning here?

I added the warning so that we catch if the next_rip field is being written to (even if the feature isn't supported) by a buggy L1 hypervisor.

Quoted 19 lines · Bandan Das

Quoted 8 lines · Paolo Bonzini

Quoted 7 lines · Dirk Müller

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

index 94b7d15..0a42859 100644
@@ -514,7 +514,7 @@ static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
 	struct vcpu_svm *svm = to_svm(vcpu);
 

Quoted 8 lines

arch/x86/kvm/svm.c static void skip_emulated_instruction(struct kvm_vcpu *vcpu) · line 520

Quoted 6 lines

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

 	if (svm->vmcb->control.next_rip != 0) {
-		WARN_ON(!static_cpu_has(X86_FEATURE_NRIPS));
+		WARN_ON_ONCE(!static_cpu_has(X86_FEATURE_NRIPS));
 		svm->next_rip = svm->vmcb->control.next_rip;
 	}
 

Bandan, what was the reason for warning here?

Quoted 2 lines

I added the warning so that we catch if the next_rip field is being written to (even if the feature isn't supported) by a buggy L1 hypervisor.

Even if the L1 hypervisor writes to the next_rip field in the VMCB, we would never see it in this code path, as we access the shadow VMCB in this statement.

We don't even care if the L1 hypervisor writes to its next_rip field because we only write to this field on an emulatated VMEXIT and never read it back.

So what's the point in adding a guest-triggerable warning at all?

Joerg

Joerg Roedel <[email protected]> writes:
The problems is that the next_rip field could be stale. If the processor supports next_rip, then it will clear it out on the next entry. If it doesn't, an old value just sits there (no matter who wrote it) and the problem happens when skip_emulated_instruction advances the rip with an incorrect value.

Quoted 6 lines · Bandan Das

Joerg Roedel <[email protected]> writes:
The problems is that the next_rip field could be stale. If the processor supports next_rip, then it will clear it out on the next entry. If it doesn't, an old value just sits there (no matter who wrote it) and the problem happens when skip_emulated_instruction advances the rip with an incorrect value.

So the right fix would be to just set the guests next_rip to 0 when we emulate vmrun, just like real hardware does, no?

Joerg

--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 8 lines · Joerg Roedel

Quoted 6 lines · Bandan Das

Joerg Roedel <[email protected]> writes:
The problems is that the next_rip field could be stale. If the processor supports next_rip, then it will clear it out on the next entry. If it doesn't, an old value just sits there (no matter who wrote it) and the problem happens when skip_emulated_instruction advances the rip with an incorrect value.

Quoted 2 lines

So the right fix would be to just set the guests next_rip to 0 when we emulate vmrun, just like real hardware does, no?

Agreed, resetting to 0 if nrips isn't supported seems right. It would still help having a printk_once in this case IMO :)

Quoted 10 lines · Bandan Das

Quoted 8 lines · Joerg Roedel

Quoted 6 lines · Bandan Das

Joerg Roedel <[email protected]> writes:
The problems is that the next_rip field could be stale. If the processor supports next_rip, then it will clear it out on the next entry. If it doesn't, an old value just sits there (no matter who wrote it) and the problem happens when skip_emulated_instruction advances the rip with an incorrect value.

Quoted 5 lines

Quoted 2 lines

So the right fix would be to just set the guests next_rip to 0 when we emulate vmrun, just like real hardware does, no?

Agreed, resetting to 0 if nrips isn't supported seems right. It would still help having a printk_once in this case IMO :)

I meant to reset it always to 0 on vmrun, like real hardware does.

Joerg

So the right fix would be to just set the guests next_rip to 0 when we emulate vmrun, just like real hardware does, no?

Quoted 2 lines

So the right fix would be to just set the guests next_rip to 0 when we emulate vmrun, just like real hardware does, no?

Like this? (Note: I’m not sure what I’m doing here..). I Agree with you that the warning for this seems excessive, I’ve just removed it.

From 47db81837b6fe58aa302317bbf2a092b40af0a36 Mon Sep 17 00:00:00 2001
From: Dirk Mueller <[email protected]>
Date: Fri, 2 Oct 2015 08:35:24 +0200
Subject: [PATCH] KVM: nSVM: Check for NRIP support before passing on
control.next_rip

NRIP support itself depends on cpuid Fn8000_000A_EDX[NRIPS], so ignore it if the cpu feature is not available. Remove the guest-triggerable WARN_ON for this as it just emulates real hardware behavior.

Signed-off-by Dirk Mueller <[email protected]>

---
 arch/x86/kvm/svm.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

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

index 0a42859..66e3a4c 100644

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

 	if (!svm->next_rip) {

Quoted 8 lines

arch/x86/kvm/svm.c static void skip_emulated_instruction(struct kvm_vcpu *vcpu) · line 519

Quoted 8 lines

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

@@ -514,7 +514,7 @@ static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
 	struct vcpu_svm *svm = to_svm(vcpu);
 
 	if (svm->vmcb->control.next_rip != 0) {
-		WARN_ON(!static_cpu_has(X86_FEATURE_NRIPS));
+		WARN_ON_ONCE(!static_cpu_has(X86_FEATURE_NRIPS));
 		svm->next_rip = svm->vmcb->control.next_rip;
 	}

Quoted 8 lines · Bandan Das

Quoted 8 lines

arch/x86/kvm/svm.c static void skip_emulated_instruction(struct kvm_vcpu *vcpu) · line 519

Quoted 8 lines

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

@@ -514,7 +514,7 @@ static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
 	struct vcpu_svm *svm = to_svm(vcpu);
 
 	if (svm->vmcb->control.next_rip != 0) {
-		WARN_ON(!static_cpu_has(X86_FEATURE_NRIPS));
+		WARN_ON_ONCE(!static_cpu_has(X86_FEATURE_NRIPS));
 		svm->next_rip = svm->vmcb->control.next_rip;
 	}

I looked again how this could possibly be triggered, and I am somewhat confused now.

So svm->vmcb->control.next_rip is only written by hardware or in svm_check_intercept(). Both cases write only to this field, if the hardware supports X86_FEATURE_NRIPS. The write in nested_svm_vmexit only

targets the guests VMCB, and we don't use that one again.

So I can't see how the WARN_ON above could be triggered. Do I miss something or might this also be a miscompilation of static_cpu_has?

Joerg

Quoted 4 lines · Joerg Roedel

So svm->vmcb->control.next_rip is only written by hardware or in svm_check_intercept(). Both cases write only to this field, if the hardware supports X86_FEATURE_NRIPS. The write in nested_svm_vmexit only

Not until commit f104765b4f81fd74d69e0eb161e89096deade2db. So, an older L1 kernel will trigger it.

But we don't care if L1 writes something into its own next_rip, as we never read this value from its VMCB. We only copy the next_rip value we get from our shadow-vmcb to it on an emulated vmexit. So I still don't understand what triggers the reported problem or why the WARN_ON is necessary.

From 019afc60507618b8e44e0c67d5ea2d850d88c9dd Mon Sep 17 00:00:00 2001

Quoted 5 lines · Joerg Roedel

But we don't care if L1 writes something into its own next_rip, as we never read this value from its VMCB. We only copy the next_rip value we get from our shadow-vmcb to it on an emulated vmexit. So I still don't understand what triggers the reported problem or why the WARN_ON is necessary.

Okay, I think I have an idea now. I talked a bit with Dirk and the WARN_ON triggers in the guest, and not on the host. This makes a lot more sense.

In nested-svm we always copy the next_rip from the shadow-vmcb to the guests vmcb, even when the nrips bit in cpuid is not set for the guest. This obviously triggers the WARN_ON() in the L1 KVM (I still don't understand why the WARN_ON was introduced in the first place).

So the right fix is to only copy next_rip to the guests vmcb when its cpuid indicates that next_rip is supported there, like in this patch:

From 019afc60507618b8e44e0c67d5ea2d850d88c9dd Mon Sep 17 00:00:00 2001
From: Joerg Roedel <[email protected]>
Date: Wed, 7 Oct 2015 13:38:19 +0200
Subject: [PATCH] kvm: svm: Only propagate next_rip when guest supports it

Currently we always write the next_rip of the shadow vmcb to the guests vmcb when we emulate a vmexit. This could confuse the guest when its cpuid indicated no support for the next_rip feature.

Fix this by only propagating next_rip if the guest actually supports it.

Signed-off-by Joerg Roedel <[email protected]>

---
 arch/x86/kvm/cpuid.h | 21 +++++++++++++++++++++
 arch/x86/kvm/svm.c   |  7 ++++++-
 2 files changed, 27 insertions(+), 1 deletion(-)

arch/x86/kvm/cpuid.h +21 -0

index dd05b9c..effca1f 100644
@@ -133,4 +133,25 @@ static inline bool guest_cpuid_has_mpx(struct kvm_vcpu *vcpu)
 	best = kvm_find_cpuid_entry(vcpu, 7, 0);
 	return best && (best->ebx & bit(X86_FEATURE_MPX));
 }
+
+/*
+ * NRIPS is provided through cpuidfn 0x8000000a.edx bit 3
+ */
+#define BIT_NRIPS	3
+
+static inline bool guest_cpuid_has_nrips(struct kvm_vcpu *vcpu)
+{
+	struct kvm_cpuid_entry2 *best;
+
+	best = kvm_find_cpuid_entry(vcpu, 0x8000000a, 0);
+
+	/*
+	 * NRIPS is a scattered cpuid feature, so we can't use
+	 * X86_FEATURE_NRIPS here (X86_FEATURE_NRIPS would be bit
+	 * position 8, not 3).
+	 */
+	return best && (best->edx & bit(BIT_NRIPS));
+}
+#undef BIT_NRIPS
+
 #endif

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

index 94b7d15..e1a8824 100644
@@ -2459,7 +2459,9 @@ static int nested_svm_vmexit(struct vcpu_svm *svm)
 	nested_vmcb->control.exit_info_2       = vmcb->control.exit_info_2;
 	nested_vmcb->control.exit_int_info     = vmcb->control.exit_int_info;
 	nested_vmcb->control.exit_int_info_err = vmcb->control.exit_int_info_err;
-	nested_vmcb->control.next_rip          = vmcb->control.next_rip;
+
+	if (guest_cpuid_has_nrips(vcpu))
+		nested_vmcb->control.next_rip  = vmcb->control.next_rip;
 
 	/*
 	 * If we emulate a VMRUN/#VMEXIT in the same host #vmexit cycle we have

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

 	nested_svm_unmap(page);
 
 	/* Enter Guest-Mode */

Ok, understood now. The warn_on would trigger in L1 only if it has decided to disable nrips for some reason as was the case here. So, my reasoning behind putting the warning was incorrect.

arch/x86/kvm/svm.c static int nested_svm_vmexit(struct vcpu_svm *svm) · line 2464

Quoted 3 lines

arch/x86/kvm/svm.c +3 -0

+
+	if (guest_cpuid_has_nrips(vcpu))
+		nested_vmcb->control.next_rip  = vmcb->control.next_rip;

Quoted 7 lines

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

 	/*
 	 * If we emulate a VMRUN/#VMEXIT in the same host #vmexit cycle we have

@@ -2714,6 +2716,9 @@ static bool nested_svm_vmrun(struct vcpu_svm *svm)
 	svm->vmcb->control.event_inj = nested_vmcb->control.event_inj;
 	svm->vmcb->control.event_inj_err = nested_vmcb->control.event_inj_err;
 

arch/x86/kvm/svm.c static bool nested_svm_vmrun(struct vcpu_svm *svm) · line 2721

Quoted 3 lines

arch/x86/kvm/svm.c +3 -0

+	/* Clear next_rip, as real hardware would do */
+	nested_vmcb->control.next_rip = 0;
+

Why do we need this ? And are you sure this is what real hardware does ? I couldn't find anything in the spec.

Dirk is currently testing whether this (fixed) patch solves the problem in his setup.

Quoted 6 lines · Bandan Das

Quoted 4 lines · Joerg Roedel

So svm->vmcb->control.next_rip is only written by hardware or in svm_check_intercept(). Both cases write only to this field, if the hardware supports X86_FEATURE_NRIPS. The write in nested_svm_vmexit only

Quoted 2 lines

Not until commit f104765b4f81fd74d69e0eb161e89096deade2db. So, an older L1 kernel will trigger it.

But we don't care if L1 writes something into its own next_rip, as we never read this value from its VMCB. We only copy the next_rip value we get from our shadow-vmcb to it on an emulated vmexit. So I still don't understand what triggers the reported problem or why the WARN_ON is necessary.

Joerg

Ok, looks like I am making some incorrect "vmx" assumptions here. What happens when we exit from L2 to L0, arent' we looking at the VMCB L1 is using to run L2 ? Wouldn't that trigger the warning if the host processor does not support nrips and the field is set ?