Fix a long-standing bug where KVM could clobber its OS-visible workarounds
handling (not that anyone would notice), and then clean up the code to make
it easier understand and maintain (I didn't even know what "osvw" stood for
until I ran into this code when trying to moving actual SVM pieces of
svm_enable_virtualization_cpu() out of KVM (for TDX purposes)).

Tested by running in a VM and generating unique per-vCPU MSR values in the
host (see below), and verifying KVM ended up with the right values.

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c9c2aa6f4705..d8b8eff733d8 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4631,12 +4631,20 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
        case MSR_AMD64_OSVW_ID_LENGTH:
                if (!guest_cpu_cap_has(vcpu, X86_FEATURE_OSVW))
                        return 1;
+
+               if (vcpu->vcpu_idx == 64)
+                       return 1;
+
                msr_info->data = vcpu->arch.osvw.length;
+               if (vcpu->vcpu_idx < 64)
+                       msr_info->data = max(vcpu->vcpu_idx, 8);
                break;
        case MSR_AMD64_OSVW_STATUS:
                if (!guest_cpu_cap_has(vcpu, X86_FEATURE_OSVW))
                        return 1;
                msr_info->data = vcpu->arch.osvw.status;
+               if (vcpu->vcpu_idx < 64)
+                       msr_info->data |= BIT_ULL(vcpu->vcpu_idx);
                break;
        case MSR_PLATFORM_INFO:
                if (!msr_info->host_initiated &&

Sean Christopherson (5):
  KVM: SVM: Serialize updates to global OS-Visible Workarounds variables
  KVM: SVM: Skip OSVW MSR reads if KVM is treating all errata as present
  KVM: SVM: Extract OS-visible workarounds setup to helper function
  KVM: SVM: Skip OSVW variable updates if current CPU's errata are a
    subset
  KVM: SVM: Skip OSVW MSR reads if current CPU doesn't support the
    feature

 arch/x86/kvm/svm/svm.c | 72 ++++++++++++++++++++++++++----------------
 1 file changed, 44 insertions(+), 28 deletions(-)


base-commit: 16ec4fb4ac95d878b879192d280db2baeec43272
-- 
2.52.0.rc1.455.g30608eb744-goog

Guard writes to the global osvw_status and osvw_len variables with a
spinlock to ensure enabling virtualization on multiple CPUs in parallel
doesn't effectively drop any writes due to writing back stale data.  Don't
bother taking the lock when the boot CPU doesn't support the feature, as
that check is constant for all CPUs, i.e. racing writes will always write
the same value (zero).

Note, the bug was inadvertently "fixed" by commit 9a798b1337af ("KVM:
Register cpuhp and syscore callbacks when enabling hardware"), which
effectively serialized calls to enable virtualization due to how the cpuhp
framework "brings up" CPU.  But KVM shouldn't rely on the mechanics of
cphup to provide serialization.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/svm.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index fc42bcdbb520..5612e46e481c 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -77,6 +77,7 @@ static bool erratum_383_found __read_mostly;
  * are published and we know what the new status bits are
  */
 static uint64_t osvw_len = 4, osvw_status;
+static DEFINE_SPINLOCK(osvw_lock);
 
 static DEFINE_PER_CPU(u64, current_tsc_ratio);
 
@@ -554,16 +555,19 @@ static int svm_enable_virtualization_cpu(void)
 		if (!err)
 			err = native_read_msr_safe(MSR_AMD64_OSVW_STATUS, &status);
 
-		if (err)
+		guard(spinlock)(&osvw_lock);
+
+		if (err) {
 			osvw_status = osvw_len = 0;
-		else {
+		} else {
 			if (len < osvw_len)
 				osvw_len = len;
 			osvw_status |= status;
 			osvw_status &= (1ULL << osvw_len) - 1;
 		}
-	} else
+	} else {
 		osvw_status = osvw_len = 0;
+	}
 
 	svm_init_erratum_383();
 
-- 
2.52.0.rc1.455.g30608eb744-goog

Don't bother reading the OSVW MSRs if osvw_len is already zero, i.e. if
KVM is already treating all errata as present, in which case the positive
path of the if-statement is one giant nop.

Opportunistically update the comment to more thoroughly explain how the
MSRs work and why the code does what it does.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/svm.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 5612e46e481c..0101da1a3c26 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -539,15 +539,25 @@ static int svm_enable_virtualization_cpu(void)
 
 
 	/*
-	 * Get OSVW bits.
+	 * Get OS-Visible Workarounds (OSVW) bits.
 	 *
 	 * Note that it is possible to have a system with mixed processor
 	 * revisions and therefore different OSVW bits. If bits are not the same
 	 * on different processors then choose the worst case (i.e. if erratum
 	 * is present on one processor and not on another then assume that the
 	 * erratum is present everywhere).
+	 *
+	 * Note #2!  The OSVW MSRs are used to communciate that an erratum is
+	 * NOT present!  Software must assume erratum as present if its bit is
+	 * set in OSVW_STATUS *or* the bit number exceeds OSVW_ID_LENGTH.  If
+	 * either RDMSR fails, simply zero out the length to treat all errata
+	 * as being present.  Similarly, use the *minimum* length across all
+	 * CPUs, not the maximum length.
+	 *
+	 * If the length is zero, then is KVM already treating all errata as
+	 * being present and there's nothing left to do.
 	 */
-	if (cpu_has(&boot_cpu_data, X86_FEATURE_OSVW)) {
+	if (osvw_len && cpu_has(&boot_cpu_data, X86_FEATURE_OSVW)) {
 		u64 len, status = 0;
 		int err;
 
-- 
2.52.0.rc1.455.g30608eb744-goog

Move the initialization of the global OSVW variables to a helper function
so that svm_enable_virtualization_cpu() isn't polluted with a pile of what
is effectively legacy code.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/svm.c | 90 +++++++++++++++++++++++-------------------
 1 file changed, 49 insertions(+), 41 deletions(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 0101da1a3c26..d3f0cc5632d1 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -417,6 +417,54 @@ static void svm_init_osvw(struct kvm_vcpu *vcpu)
 		vcpu->arch.osvw.status |= 1;
 }
 
+static void svm_init_os_visible_workarounds(void)
+{
+	u64 len, status;
+	int err;
+
+	/*
+	 * Get OS-Visible Workarounds (OSVW) bits.
+	 *
+	 * Note that it is possible to have a system with mixed processor
+	 * revisions and therefore different OSVW bits. If bits are not the same
+	 * on different processors then choose the worst case (i.e. if erratum
+	 * is present on one processor and not on another then assume that the
+	 * erratum is present everywhere).
+	 *
+	 * Note #2!  The OSVW MSRs are used to communciate that an erratum is
+	 * NOT present!  Software must assume erratum as present if its bit is
+	 * set in OSVW_STATUS *or* the bit number exceeds OSVW_ID_LENGTH.  If
+	 * either RDMSR fails, simply zero out the length to treat all errata
+	 * as being present.  Similarly, use the *minimum* length across all
+	 * CPUs, not the maximum length.
+	 *
+	 * If the length is zero, then is KVM already treating all errata as
+	 * being present and there's nothing left to do.
+	 */
+	if (!osvw_len)
+		return;
+
+	if (!boot_cpu_has(X86_FEATURE_OSVW)) {
+		osvw_status = osvw_len = 0;
+		return;
+	}
+
+	err = native_read_msr_safe(MSR_AMD64_OSVW_ID_LENGTH, &len);
+	if (!err)
+		err = native_read_msr_safe(MSR_AMD64_OSVW_STATUS, &status);
+
+	guard(spinlock)(&osvw_lock);
+
+	if (err) {
+		osvw_status = osvw_len = 0;
+	} else {
+		if (len < osvw_len)
+			osvw_len = len;
+		osvw_status |= status;
+		osvw_status &= (1ULL << osvw_len) - 1;
+	}
+}
+
 static bool __kvm_is_svm_supported(void)
 {
 	int cpu = smp_processor_id();
@@ -537,47 +585,7 @@ static int svm_enable_virtualization_cpu(void)
 		__svm_write_tsc_multiplier(SVM_TSC_RATIO_DEFAULT);
 	}
 
-
-	/*
-	 * Get OS-Visible Workarounds (OSVW) bits.
-	 *
-	 * Note that it is possible to have a system with mixed processor
-	 * revisions and therefore different OSVW bits. If bits are not the same
-	 * on different processors then choose the worst case (i.e. if erratum
-	 * is present on one processor and not on another then assume that the
-	 * erratum is present everywhere).
-	 *
-	 * Note #2!  The OSVW MSRs are used to communciate that an erratum is
-	 * NOT present!  Software must assume erratum as present if its bit is
-	 * set in OSVW_STATUS *or* the bit number exceeds OSVW_ID_LENGTH.  If
-	 * either RDMSR fails, simply zero out the length to treat all errata
-	 * as being present.  Similarly, use the *minimum* length across all
-	 * CPUs, not the maximum length.
-	 *
-	 * If the length is zero, then is KVM already treating all errata as
-	 * being present and there's nothing left to do.
-	 */
-	if (osvw_len && cpu_has(&boot_cpu_data, X86_FEATURE_OSVW)) {
-		u64 len, status = 0;
-		int err;
-
-		err = native_read_msr_safe(MSR_AMD64_OSVW_ID_LENGTH, &len);
-		if (!err)
-			err = native_read_msr_safe(MSR_AMD64_OSVW_STATUS, &status);
-
-		guard(spinlock)(&osvw_lock);
-
-		if (err) {
-			osvw_status = osvw_len = 0;
-		} else {
-			if (len < osvw_len)
-				osvw_len = len;
-			osvw_status |= status;
-			osvw_status &= (1ULL << osvw_len) - 1;
-		}
-	} else {
-		osvw_status = osvw_len = 0;
-	}
+	svm_init_os_visible_workarounds();
 
 	svm_init_erratum_383();
 
-- 
2.52.0.rc1.455.g30608eb744-goog

Elide the OSVW variable updates if the current CPU's set of errata are a
subset of the errata tracked in the global values, i.e. if no update is
needed.  There's no danger of under-reporting errata due to bailing early
as KVM is purely reducing the set of "known fixed" errata.  I.e. a racing
update on a different CPU with _more_ errata doesn't change anything if
the current CPU has the same or fewer errata relative to the status quo.

If another CPU is writing osvw_len, then "len" is guaranteed to be larger
than the new osvw_len and so the osvw_len update would be skipped anyways.

If another CPU is setting new bits in osvw_status, then "status" is
guaranteed to be a subset of the new osvw_status and the bitwise-OR would
be an effective nop anyways.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/svm.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index d3f0cc5632d1..152c56762b26 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -420,7 +420,6 @@ static void svm_init_osvw(struct kvm_vcpu *vcpu)
 static void svm_init_os_visible_workarounds(void)
 {
 	u64 len, status;
-	int err;
 
 	/*
 	 * Get OS-Visible Workarounds (OSVW) bits.
@@ -449,20 +448,19 @@ static void svm_init_os_visible_workarounds(void)
 		return;
 	}
 
-	err = native_read_msr_safe(MSR_AMD64_OSVW_ID_LENGTH, &len);
-	if (!err)
-		err = native_read_msr_safe(MSR_AMD64_OSVW_STATUS, &status);
+	if (native_read_msr_safe(MSR_AMD64_OSVW_ID_LENGTH, &len) ||
+	    native_read_msr_safe(MSR_AMD64_OSVW_STATUS, &status))
+		len = status = 0;
+
+	if (status == READ_ONCE(osvw_status) && len >= READ_ONCE(osvw_len))
+		return;
 
 	guard(spinlock)(&osvw_lock);
 
-	if (err) {
-		osvw_status = osvw_len = 0;
-	} else {
-		if (len < osvw_len)
-			osvw_len = len;
-		osvw_status |= status;
-		osvw_status &= (1ULL << osvw_len) - 1;
-	}
+	if (len < osvw_len)
+		osvw_len = len;
+	osvw_status |= status;
+	osvw_status &= (1ULL << osvw_len) - 1;
 }
 
 static bool __kvm_is_svm_supported(void)
-- 
2.52.0.rc1.455.g30608eb744-goog

Skip the OSVW RDMSRs if the current CPU doesn't enumerate support for the
MSRs.  In practice, checking only the boot CPU's capabilities is
sufficient, as the RDMSRs should fault when unsupported, but there's no
downside to being more precise, and checking only the boot CPU _looks_
wrong given the rather odd semantics of the MSRs.  E.g. if a CPU doesn't
support OVSW, then KVM must assume all errata are present.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/svm.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 152c56762b26..c971a15453be 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -443,12 +443,8 @@ static void svm_init_os_visible_workarounds(void)
 	if (!osvw_len)
 		return;
 
-	if (!boot_cpu_has(X86_FEATURE_OSVW)) {
-		osvw_status = osvw_len = 0;
-		return;
-	}
-
-	if (native_read_msr_safe(MSR_AMD64_OSVW_ID_LENGTH, &len) ||
+	if (!this_cpu_has(X86_FEATURE_OSVW) ||
+	    native_read_msr_safe(MSR_AMD64_OSVW_ID_LENGTH, &len) ||
 	    native_read_msr_safe(MSR_AMD64_OSVW_STATUS, &status))
 		len = status = 0;
 
-- 
2.52.0.rc1.455.g30608eb744-goog