kvm: x86: rewrite kvm_spec_ctrl_valid_bits

There are few cases when this function was creating a bogus #GP condition, for example case when and AMD host supports STIBP but doesn't support SSBD.

Follow the rules for AMD and Intel strictly instead.

AMD #GP rules for IA32_SPEC_CTRL can be found here:
https://bugzilla.kernel.org/show_bug.cgi?id=199889

Fixes 6441fa6178f5 ("KVM: x86: avoid incorrect writes to host MSR_IA32_SPEC_CTRL")

Signed-off-by Maxim Levitsky <[email protected]>

---
 arch/x86/kvm/x86.c | 57 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 42 insertions(+), 15 deletions(-)

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

index 00c88c2f34e4..a6bed4670b7f 100644
@@ -10670,27 +10670,54 @@ bool kvm_arch_no_poll(struct kvm_vcpu *vcpu)
 }
 EXPORT_SYMBOL_GPL(kvm_arch_no_poll);
 
-u64 kvm_spec_ctrl_valid_bits(struct kvm_vcpu *vcpu)
+
+static u64 kvm_spec_ctrl_valid_bits_host(void)
+{
+	uint64_t bits = 0;
+
+	if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
+		bits |= SPEC_CTRL_IBRS;
+	if (boot_cpu_has(X86_FEATURE_INTEL_STIBP))
+		bits |= SPEC_CTRL_STIBP;
+	if (boot_cpu_has(X86_FEATURE_SPEC_CTRL_SSBD))

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

+
+static u64 kvm_spec_ctrl_valid_bits_guest(struct kvm_vcpu *vcpu)
 {
-	uint64_t bits = SPEC_CTRL_IBRS | SPEC_CTRL_STIBP | SPEC_CTRL_SSBD;
+	uint64_t bits = 0;
 
-	/* The STIBP bit doesn't fault even if it's not advertised */
-	if (!guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL) &&
-	    !guest_cpuid_has(vcpu, X86_FEATURE_AMD_IBRS))
-		bits &= ~(SPEC_CTRL_IBRS | SPEC_CTRL_STIBP);
-	if (!boot_cpu_has(X86_FEATURE_SPEC_CTRL) &&
-	    !boot_cpu_has(X86_FEATURE_AMD_IBRS))
-		bits &= ~(SPEC_CTRL_IBRS | SPEC_CTRL_STIBP);
+	if (guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL))
+		bits |= SPEC_CTRL_IBRS;
+	if (guest_cpuid_has(vcpu, X86_FEATURE_INTEL_STIBP))
+		bits |= SPEC_CTRL_STIBP;
+	if (guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL_SSBD))

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

 	return bits;
 }
+
+u64 kvm_spec_ctrl_valid_bits(struct kvm_vcpu *vcpu)
+{
+	return kvm_spec_ctrl_valid_bits_host() &
+	       kvm_spec_ctrl_valid_bits_guest(vcpu);
+}
+
+
 EXPORT_SYMBOL_GPL(kvm_spec_ctrl_valid_bits);
 
 EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_exit);

Signature · 1 line

2.25.4

Quoted 4 lines · Maxim Levitsky

There are few cases when this function was creating a bogus #GP condition, for example case when and AMD host supports STIBP but doesn't support SSBD.

Follow the rules for AMD and Intel strictly instead.

Can you elaborate on the conditions that are problematic, e.g. what does the guest expect to exist that KVM isn't providing?

Quoted 30 lines

AMD #GP rules for IA32_SPEC_CTRL can be found here:
https://bugzilla.kernel.org/show_bug.cgi?id=199889

Fixes 6441fa6178f5 ("KVM: x86: avoid incorrect writes to host MSR_IA32_SPEC_CTRL")

Signed-off-by Maxim Levitsky <[email protected]>

---
 arch/x86/kvm/x86.c | 57 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 42 insertions(+), 15 deletions(-)

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

index 00c88c2f34e4..a6bed4670b7f 100644
@@ -10670,27 +10670,54 @@ bool kvm_arch_no_poll(struct kvm_vcpu *vcpu)
 }
 EXPORT_SYMBOL_GPL(kvm_arch_no_poll);
 
-u64 kvm_spec_ctrl_valid_bits(struct kvm_vcpu *vcpu)
+
+static u64 kvm_spec_ctrl_valid_bits_host(void)
+{
+	uint64_t bits = 0;
+
+	if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
+		bits |= SPEC_CTRL_IBRS;
+	if (boot_cpu_has(X86_FEATURE_INTEL_STIBP))
+		bits |= SPEC_CTRL_STIBP;
+	if (boot_cpu_has(X86_FEATURE_SPEC_CTRL_SSBD))

arch/x86/kvm/x86.c bool kvm_arch_no_poll(struct kvm_vcpu *vcpu) · line 10692

Quoted 10 lines

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

+		bits |= SPEC_CTRL_SSBD;
+
+	if (boot_cpu_has(X86_FEATURE_AMD_IBRS) || boot_cpu_has(X86_FEATURE_AMD_STIBP))
+		bits |= SPEC_CTRL_STIBP | SPEC_CTRL_IBRS;
+
+	if (boot_cpu_has(X86_FEATURE_AMD_SSBD))
+		bits |= SPEC_CTRL_STIBP | SPEC_CTRL_IBRS | SPEC_CTRL_SSBD;
+
+	return bits;
+}

Rather than compute the mask every time, it can be computed once on module load and stashed in a global. Note, there's a RFC series[*] to support reprobing bugs at runtime, but that has bigger issues with existing KVM functionality to be addressed, i.e. it's not our problem, yet :-).

[*] https://lkml.kernel.org/r/[email protected]

Quoted 21 lines

arch/x86/kvm/x86.c +9 -8

+
+static u64 kvm_spec_ctrl_valid_bits_guest(struct kvm_vcpu *vcpu)
 {
-	uint64_t bits = SPEC_CTRL_IBRS | SPEC_CTRL_STIBP | SPEC_CTRL_SSBD;
+	uint64_t bits = 0;
 
-	/* The STIBP bit doesn't fault even if it's not advertised */
-	if (!guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL) &&
-	    !guest_cpuid_has(vcpu, X86_FEATURE_AMD_IBRS))
-		bits &= ~(SPEC_CTRL_IBRS | SPEC_CTRL_STIBP);
-	if (!boot_cpu_has(X86_FEATURE_SPEC_CTRL) &&
-	    !boot_cpu_has(X86_FEATURE_AMD_IBRS))
-		bits &= ~(SPEC_CTRL_IBRS | SPEC_CTRL_STIBP);
+	if (guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL))
+		bits |= SPEC_CTRL_IBRS;
+	if (guest_cpuid_has(vcpu, X86_FEATURE_INTEL_STIBP))
+		bits |= SPEC_CTRL_STIBP;
+	if (guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL_SSBD))
+		bits |= SPEC_CTRL_SSBD;
 

arch/x86/kvm/x86.c bool kvm_arch_no_poll(struct kvm_vcpu *vcpu) · line 10706

Quoted 8 lines

arch/x86/kvm/x86.c +2 -6

-	if (!guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL_SSBD) &&
-	    !guest_cpuid_has(vcpu, X86_FEATURE_AMD_SSBD))
-		bits &= ~SPEC_CTRL_SSBD;
-	if (!boot_cpu_has(X86_FEATURE_SPEC_CTRL_SSBD) &&
-	    !boot_cpu_has(X86_FEATURE_AMD_SSBD))
-		bits &= ~SPEC_CTRL_SSBD;
+	if (guest_cpuid_has(vcpu, X86_FEATURE_AMD_IBRS) ||
+			guest_cpuid_has(vcpu, X86_FEATURE_AMD_STIBP))

Bad indentation.

arch/x86/kvm/x86.c bool kvm_arch_no_poll(struct kvm_vcpu *vcpu) · line 10709

Quoted 4 lines

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

+		bits |= SPEC_CTRL_STIBP | SPEC_CTRL_IBRS;
+	if (guest_cpuid_has(vcpu, X86_FEATURE_AMD_SSBD))
+		bits |= SPEC_CTRL_STIBP | SPEC_CTRL_IBRS | SPEC_CTRL_SSBD;

Would it be feasible to split into two patches? The first (tagged Fixes:) to make the functional changes without inverting the logic or splitting, and then do the cleanup? It's really hard to review this patch because I can't easily tease out what's different in terms of functionality.

Quoted 13 lines

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

 	return bits;
 }
+
+u64 kvm_spec_ctrl_valid_bits(struct kvm_vcpu *vcpu)
+{
+	return kvm_spec_ctrl_valid_bits_host() &
+	       kvm_spec_ctrl_valid_bits_guest(vcpu);
+}
+
+
 EXPORT_SYMBOL_GPL(kvm_spec_ctrl_valid_bits);
 

Quoted 4 lines

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

 EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_exit);

Signature · 1 line

2.25.4

Quoted 5 lines

Rather than compute the mask every time, it can be computed once on module load and stashed in a global. Note, there's a RFC series[*] to support reprobing bugs at runtime, but that has bigger issues with existing KVM functionality to be addressed, i.e. it's not our problem, yet :-).

Quoted 1 line

[*] https://lkml.kernel.org/r/[email protected]

Thanks for the pointer!

Note though that the above code only runs once, since after a single successful (non #GP) set of it to non-zero value, it is cleared in MSR bitmap for both reads and writes on both VMX and SVM.

This is done because of performance reasons which in this case are more important than absolute correctness. Thus to some extent the guest checks in the above are pointless.

If you ask me, I would just remove the kvm_spec_ctrl_valid_bits, and pass this msr to guest right away and not on first access.

One oddity with this whole thing is that by passing through the MSR, KVM is allowing the guest to write bits it doesn't know about, which is definitely not normal. It also means the guest could write bits that the host VMM can't.

Somehwat crazy idea inbound... rather than calculating the valid bits in software, what if we throw the value at the CPU and see if it fails? At least that way the host and guest are subject to the same rules. E.g.

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

@@ -2062,11 +2062,19 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
                    !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL))
                        return 1;
 
-               if (data & ~kvm_spec_ctrl_valid_bits(vcpu))
-                       return 1;
-
+               ret = 0;
                vmx->spec_ctrl = data;
-               if (!data)
+
+               local_irq_disable();
+               if (rdmsrl_safe(MSR_IA32_SPEC_CTRL, &data))
+                       ret = 1;
+               else if (wrmsrl_safe(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl))
+                       ret = 1;
+               else
+                       wrmsrl(MSR_IA32_SPEC_CTRL, data))
+               local_irq_enable();
+
+               if (ret || !vmx->spec_ctrl)
                        break;
 
                /*

Quoted 4 lines · Sean Christopherson

One oddity with this whole thing is that by passing through the MSR, KVM is allowing the guest to write bits it doesn't know about, which is definitely not normal. It also means the guest could write bits that the host VMM can't.

That's true. However, the main purpose of the kvm_spec_ctrl_valid_bits check is to ensure that host-initiated writes are valid; this way, you don't get a #GP on the next vmentry's WRMSR to MSR_IA32_SPEC_CTRL. Checking the guest CPUID bit is not even necessary.

Quoted 4 lines

Quoted 4 lines

One oddity with this whole thing is that by passing through the MSR, KVM is allowing the guest to write bits it doesn't know about, which is definitely not normal. It also means the guest could write bits that the host VMM can't.

Quoted 4 lines

That's true. However, the main purpose of the kvm_spec_ctrl_valid_bits check is to ensure that host-initiated writes are valid; this way, you don't get a #GP on the next vmentry's WRMSR to MSR_IA32_SPEC_CTRL. Checking the guest CPUID bit is not even necessary.

Right, what I'm saying is that rather than try and decipher specs to determine what bits are supported, just throw the value at hardware and go from there. That's effectively what we end up doing for the guest writes anyways.

Actually, the current behavior will break migration if there are ever legal bits that KVM doesn't recognize, e.g. guest writes a value that KVM doesn't allow and then migration fails when the destination tries to stuff the value into KVM.

Quoted 4 lines · Sean Christopherson

Quoted 4 lines

Quoted 4 lines

One oddity with this whole thing is that by passing through the MSR, KVM is allowing the guest to write bits it doesn't know about, which is definitely not normal. It also means the guest could write bits that the host VMM can't.

Quoted 8 lines

Quoted 4 lines

That's true. However, the main purpose of the kvm_spec_ctrl_valid_bits check is to ensure that host-initiated writes are valid; this way, you don't get a #GP on the next vmentry's WRMSR to MSR_IA32_SPEC_CTRL. Checking the guest CPUID bit is not even necessary.

Right, what I'm saying is that rather than try and decipher specs to determine what bits are supported, just throw the value at hardware and go from there. That's effectively what we end up doing for the guest writes anyways.

Yes, it would prevent the #GP.

Quoted 4 lines

Actually, the current behavior will break migration if there are ever legal bits that KVM doesn't recognize, e.g. guest writes a value that KVM doesn't allow and then migration fails when the destination tries to stuff the value into KVM.

Yes, unfortunately migration would also be broken if the target (and the guest CPUID) is an older CPU. But that's not something we can fix without trapping all writes which would be unacceptably slow.

Aren't you supposed to be on vacation? :-)

Quoted 10 lines · Sean Christopherson

Quoted 4 lines

One oddity with this whole thing is that by passing through the MSR, KVM is allowing the guest to write bits it doesn't know about, which is definitely not normal. It also means the guest could write bits that the host VMM can't.

That's true. However, the main purpose of the kvm_spec_ctrl_valid_bits check is to ensure that host-initiated writes are valid; this way, you don't get a #GP on the next vmentry's WRMSR to MSR_IA32_SPEC_CTRL. Checking the guest CPUID bit is not even necessary.

Right, what I'm saying is that rather than try and decipher specs to determine what bits are supported, just throw the value at hardware and go from there. That's effectively what we end up doing for the guest writes anyways.

Actually, the current behavior will break migration if there are ever legal bits that KVM doesn't recognize, e.g. guest writes a value that KVM doesn't allow and then migration fails when the destination tries to stuff the value into KVM.

Aren't you supposed to be on vacation? :-)

Quoted 10 lines · Paolo Bonzini

Quoted 4 lines · Sean Christopherson

One oddity with this whole thing is that by passing through the MSR, KVM is allowing the guest to write bits it doesn't know about, which is definitely not normal. It also means the guest could write bits that the host VMM can't.

That's true. However, the main purpose of the kvm_spec_ctrl_valid_bits check is to ensure that host-initiated writes are valid; this way, you don't get a #GP on the next vmentry's WRMSR to MSR_IA32_SPEC_CTRL. Checking the guest CPUID bit is not even necessary.

Right, what I'm saying is that rather than try and decipher specs to determine what bits are supported, just throw the value at hardware and go from there. That's effectively what we end up doing for the guest writes anyways.

Actually, the current behavior will break migration if there are ever legal bits that KVM doesn't recognize, e.g. guest writes a value that KVM doesn't allow and then migration fails when the destination tries to stuff the value into KVM.

After thinking about this, I am thinking that we should apply similiar logic as done with the 'cpu-pm' related features.
This way the user can choose between passing through the IA32_SPEC_CTRL, (and in this case, we can since the user choose it, pass it right away, and thus avoid using kvm_spec_ctrl_valid_bits completely), and between correctness, in which case we can always emulate this msr, and therefore check all the bits, both regard to guest and host supported values.

Does this makes sense, or do you think that this is overkill?

Quoted 19 lines · Sean Christopherson

Aren't you supposed to be on vacation? :-)

Quoted 10 lines · Paolo Bonzini

Quoted 4 lines · Sean Christopherson

One oddity with this whole thing is that by passing through the MSR, KVM is allowing the guest to write bits it doesn't know about, which is definitely not normal. It also means the guest could write bits that the host VMM can't.

That's true. However, the main purpose of the kvm_spec_ctrl_valid_bits check is to ensure that host-initiated writes are valid; this way, you don't get a #GP on the next vmentry's WRMSR to MSR_IA32_SPEC_CTRL. Checking the guest CPUID bit is not even necessary.

Right, what I'm saying is that rather than try and decipher specs to determine what bits are supported, just throw the value at hardware and go from there. That's effectively what we end up doing for the guest writes anyways.

Quoted 4 lines

Actually, the current behavior will break migration if there are ever legal bits that KVM doesn't recognize, e.g. guest writes a value that KVM doesn't allow and then migration fails when the destination tries to stuff the value into KVM.

After thinking about this, I am thinking that we should apply similiar logic as done with the 'cpu-pm' related features.
This way the user can choose between passing through the IA32_SPEC_CTRL, (and in this case, we can since the user choose it, pass it right away, and thus avoid using kvm_spec_ctrl_valid_bits completely), and between correctness, in which case we can always emulate this msr, and therefore check all the bits, both regard to guest and host supported values.
Does this makes sense, or do you think that this is overkill?

Quoted 5 lines · Maxim Levitsky

Quoted 5 lines

Rather than compute the mask every time, it can be computed once on module load and stashed in a global. Note, there's a RFC series[*] to support reprobing bugs at runtime, but that has bigger issues with existing KVM functionality to be addressed, i.e. it's not our problem, yet :-).

Quoted 7 lines

Quoted 1 line

[*] https://lkml.kernel.org/r/[email protected]

Thanks for the pointer!

Note though that the above code only runs once, since after a single successful (non #GP) set of it to non-zero value, it is cleared in MSR bitmap for both reads and writes on both VMX and SVM.

For me the performance is secondary to documenting the fact that the host valid bits are fixed for a given instance of the kernel. There's enough going on in kvm_spec_ctrl_valid_bits_host() that's it's not super easy to see that it's a "constant" value.

Quoted 4 lines

This is done because of performance reasons which in this case are more important than absolute correctness. Thus to some extent the guest checks in the above are pointless.

Quoted 2 lines

If you ask me, I would just remove the kvm_spec_ctrl_valid_bits, and pass this msr to guest right away and not on first access.

That would unnecessarily penalize guests that don't utilize the MSR as KVM would need to do a RDMSR on every VM-Exit to grab the guest's value.

One oddity with this whole thing is that by passing through the MSR, KVM is allowing the guest to write bits it doesn't know about, which is definitely not normal. It also means the guest could write bits that the host VMM can't.

Somehwat crazy idea inbound... rather than calculating the valid bits in software, what if we throw the value at the CPU and see if it fails? At least that way the host and guest are subject to the same rules. E.g.

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

@@ -2062,11 +2062,19 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
                    !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL))
                        return 1;
 
-               if (data & ~kvm_spec_ctrl_valid_bits(vcpu))
-                       return 1;
-
+               ret = 0;
                vmx->spec_ctrl = data;
-               if (!data)
+
+               local_irq_disable();
+               if (rdmsrl_safe(MSR_IA32_SPEC_CTRL, &data))
+                       ret = 1;

Quoted 29 lines

Somehwat crazy idea inbound... rather than calculating the valid bits in software, what if we throw the value at the CPU and see if it fails? At least that way the host and guest are subject to the same rules. E.g.

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

@@ -2062,11 +2062,19 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
                    !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL))
                        return 1;
 
-               if (data & ~kvm_spec_ctrl_valid_bits(vcpu))
-                       return 1;
-
+               ret = 0;
                vmx->spec_ctrl = data;
-               if (!data)
+
+               local_irq_disable();
+               if (rdmsrl_safe(MSR_IA32_SPEC_CTRL, &data))
+                       ret = 1;
+               else if (wrmsrl_safe(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl))
+                       ret = 1;
+               else
+                       wrmsrl(MSR_IA32_SPEC_CTRL, data))
+               local_irq_enable();
+
+               if (ret || !vmx->spec_ctrl)
                        break;
 

arch/x86/kvm/vmx/vmx.c static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) · line 2080

Quoted 2 lines

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

                /*

I don't mind this as well, knowing that this is done only one per VM run anyway.