Skip to contentSkip to conversation

x86/kvm: Implement HWCR support

PATCH In review Borislav Petkov 3 participants 9 messages Apr 8, 2019 – Apr 18, 2019

Subsystems (3)
x86 KVM KVM x86
lore
Share & tools
lore

Borislav Petkov

author maintainer
#0 lore raw

PATCH 1/1 x86/kvm: Implement HWCR support

Hi all,

here's v5 which keeps the HWCR functionality in kvm/x86.c so that emulation of AMD guests on Intel hw still can work.

--
From: Borislav Petkov <[email protected]>

The AMD hardware configuration register has some useful bits which can be used by guests. Implement McStatusWrEn which can be used by guests when injecting MCEs with the in-kernel mce-inject module.

For that, we need to set bit 18 - McStatusWrEn - first, before writing the MCi_STATUS registers (otherwise we #GP).

Add the required machinery to do so.

Signed-off-by Borislav Petkov <[email protected]>
Cc Jim Mattson <[email protected]>
Cc Joerg Roedel <[email protected]>
Cc KVM <[email protected]>
Cc Paolo Bonzini <[email protected]>
Cc Radim Krčmář <[email protected]>
Cc Sean Christopherson <[email protected]>
Cc Tom Lendacky <[email protected]>
Cc Tony Luck <[email protected]>
Cc Yazen Ghannam <[email protected]>
---
 arch/x86/include/asm/kvm_host.h |  3 +++
 arch/x86/kvm/x86.c              | 45 ++++++++++++++++++++++++++++-----
 2 files changed, 42 insertions(+), 6 deletions(-)
arch/x86/include/asm/kvm_host.h +3 -0
index 159b5988292f..541c431df806 100644
@@ -780,6 +780,9 @@ struct kvm_vcpu_arch {
 
 	/* Flush the L1 Data cache for L1TF mitigation on VMENTER */
 	bool l1tf_flush_l1d;
+
+	/* AMD MSRC001_0015 Hardware Configuration */
+	u64 msr_hwcr;
 };
 
 struct kvm_lpage_info {
arch/x86/kvm/x86.c +39 -6

75 lines not on this page · raw message

Signature · 8 lines
2.21.0


-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
#1 lore raw
Quoted 51 lines · Borislav Petkov

Hi all,

here's v5 which keeps the HWCR functionality in kvm/x86.c so that emulation of AMD guests on Intel hw still can work.

--
From: Borislav Petkov <[email protected]>

The AMD hardware configuration register has some useful bits which can be used by guests. Implement McStatusWrEn which can be used by guests when injecting MCEs with the in-kernel mce-inject module.

For that, we need to set bit 18 - McStatusWrEn - first, before writing the MCi_STATUS registers (otherwise we #GP).

Add the required machinery to do so.

Signed-off-by Borislav Petkov <[email protected]>
Cc Jim Mattson <[email protected]>
Cc Joerg Roedel <[email protected]>
Cc KVM <[email protected]>
Cc Paolo Bonzini <[email protected]>
Cc Radim Krčmář <[email protected]>
Cc Sean Christopherson <[email protected]>
Cc Tom Lendacky <[email protected]>
Cc Tony Luck <[email protected]>
Cc Yazen Ghannam <[email protected]>
---
 arch/x86/include/asm/kvm_host.h |  3 +++
 arch/x86/kvm/x86.c              | 45 ++++++++++++++++++++++++++++-----
 2 files changed, 42 insertions(+), 6 deletions(-)
arch/x86/include/asm/kvm_host.h +3 -0
index 159b5988292f..541c431df806 100644
@@ -780,6 +780,9 @@ struct kvm_vcpu_arch {
 
 	/* Flush the L1 Data cache for L1TF mitigation on VMENTER */
 	bool l1tf_flush_l1d;
+
+	/* AMD MSRC001_0015 Hardware Configuration */
+	u64 msr_hwcr;
 };
 
 struct kvm_lpage_info {
arch/x86/kvm/x86.c +0 -0
index 099b851dabaf..e07f21c1d2d4 100644

arch/x86/kvm/x86.c static void kvmclock_sync_fn(struct work_struct *work) · line 2279

Quoted 7 lines
arch/x86/kvm/x86.c +4 -0
@@ -2273,6 +2273,30 @@ static void kvmclock_sync_fn(struct work_struct *work)
 					KVMCLOCK_SYNC_PERIOD);
 }
 
+/*
+ * On AMD, HWCR[McStatusWrEn] controls whether setting MCi_STATUS results in #GP.
+ */
+static bool __set_mci_status(struct kvm_vcpu *vcpu, struct msr_data *msr_info)

The name and return value semantics are confusing since kvm_set_msr*() returns '1' to signal fault, '0' to signal success. Most MSR helpers have "valid" somewhere in the function name to avoid confusion regarding the return value.

arch/x86/kvm/x86.c static void kvmclock_sync_fn(struct work_struct *work) · line 2286

Quoted 7 lines
arch/x86/kvm/x86.c +7 -0
+{
+	if (guest_cpuid_is_amd(vcpu)) {
+		struct msr_data tmp;
+
+		tmp.index = MSR_K7_HWCR;
+
+		if (kvm_get_msr_common(vcpu, &tmp))

No need to get through kvm_get_msr_common(), vcpu->arch.msr_hwcr can be queried directly. Going that route would likely eliminate the need for a helper func, i.e. avoid the naming confusion and the comment below.

arch/x86/kvm/x86.c static void kvmclock_sync_fn(struct work_struct *work) · line 2294

Quoted 8 lines
arch/x86/kvm/x86.c +8 -0
+			return false;
+
+		/* McStatusWrEn enabled? */
+		if (tmp.data & BIT_ULL(18))
+			return true;
+	}
+
+	if (msr_info->data != 0)

Irrevelant if this helper gets dropped, but why not "return !msr_info->data;"?

Quoted 66 lines
arch/x86/kvm/x86.c +20 -6
+		return false;
+
+	return true;
+}
+
 static int set_msr_mce(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 {
 	u64 mcg_cap = vcpu->arch.mcg_cap;

@@ -2304,9 +2328,13 @@ static int set_msr_mce(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 			if ((offset & 0x3) == 0 &&
 			    data != 0 && (data | (1 << 10)) != ~(u64)0)
 				return -1;
-			if (!msr_info->host_initiated &&
-				(offset & 0x3) == 1 && data != 0)
-				return -1;
+
+			/* MCi_STATUS */
+			if ((offset & 0x3) == 1 && !msr_info->host_initiated) {
+				if (!__set_mci_status(vcpu, msr_info))
+						return -1;
+			}
+
 			vcpu->arch.mce_banks[offset] = data;
 			break;
 		}

@@ -2455,8 +2483,11 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		data &= ~(u64)0x40;	/* ignore flush filter disable */
 		data &= ~(u64)0x100;	/* ignore ignne emulation enable */
 		data &= ~(u64)0x8;	/* ignore TLB cache disable */
-		data &= ~(u64)0x40000;  /* ignore Mc status write enable */
-		if (data != 0) {
+
+		/* Handle McStatusWrEn */
+		if (data == BIT_ULL(18)) {
+			vcpu->arch.msr_hwcr = data;
+		} else if (data != 0) {
 			vcpu_unimpl(vcpu, "unimplemented HWCR wrmsr: 0x%llx\n",
 				    data);
 			return 1;

@@ -2730,7 +2761,6 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	case MSR_K8_SYSCFG:
 	case MSR_K8_TSEG_ADDR:
 	case MSR_K8_TSEG_MASK:
-	case MSR_K7_HWCR:
 	case MSR_VM_HSAVE_PA:
 	case MSR_K8_INT_PENDING_MSG:
 	case MSR_AMD64_NB_CFG:

@@ -2894,6 +2924,9 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	case MSR_MISC_FEATURES_ENABLES:
 		msr_info->data = vcpu->arch.msr_misc_features_enables;
 		break;
+	case MSR_K7_HWCR:
+		msr_info->data = vcpu->arch.msr_hwcr;
+		break;
 	default:
 		if (kvm_pmu_is_valid_msr(vcpu, msr_info->index))
 			return kvm_pmu_get_msr(vcpu, msr_info->index, &msr_info->data);
Signature · 8 lines
2.21.0


-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
author maintainer
#2 lore raw
Quoted 11 lines · Sean Christopherson

arch/x86/kvm/x86.c static void kvmclock_sync_fn(struct work_struct *work) · line 2286

Quoted 7 lines
arch/x86/kvm/x86.c +7 -0
+{
+	if (guest_cpuid_is_amd(vcpu)) {
+		struct msr_data tmp;
+
+		tmp.index = MSR_K7_HWCR;
+
+		if (kvm_get_msr_common(vcpu, &tmp))

No need to get through kvm_get_msr_common(), vcpu->arch.msr_hwcr can be queried directly. Going that route would likely eliminate the need for a helper func, i.e. avoid the naming confusion and the comment below.

Agree with every point except this: the helper function abstracts the guest cpuid check and the rest of the handling nicely. If I put everything in an already too big default: label in set_msr_mce(), then the code there will become even more unreadable than it is.

Thx.

Signature · 4 lines
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
author maintainer
#3 lore raw

PATCH 1/1 x86/kvm: Implement HWCR support

Hi all,

ok here's v5.1 with most of Sean's feedback addressed. The function checking whether HWCR[18] is set, I've renamed to can_set_mci_status() and left it to return bool because it really is used in boolean context, answering the question "Can I set MCi_STATUS MSRs?"

And now it all looks simple and clean, just how I like it! :-)

Thx.

---
From: Borislav Petkov <[email protected]>

The hardware configuration register has some useful bits which can be used by guests. Implement McStatusWrEn which can be used by guests when injecting MCEs with the in-kernel mce-inject module.

For that, we need to set bit 18 - McStatusWrEn - first, before writing the MCi_STATUS registers (otherwise we #GP).

Add the required machinery to do so.

Signed-off-by Borislav Petkov <[email protected]>
Cc Jim Mattson <[email protected]>
Cc Joerg Roedel <[email protected]>
Cc KVM <[email protected]>
Cc Paolo Bonzini <[email protected]>
Cc Radim Krčmář <[email protected]>
Cc Sean Christopherson <[email protected]>
Cc Tom Lendacky <[email protected]>
Cc Tony Luck <[email protected]>
Cc Yazen Ghannam <[email protected]>
---
 arch/x86/include/asm/kvm_host.h |  3 +++
 arch/x86/kvm/x86.c              | 33 +++++++++++++++++++++++++++------
 2 files changed, 30 insertions(+), 6 deletions(-)
arch/x86/include/asm/kvm_host.h +3 -0
index 159b5988292f..541c431df806 100644
@@ -780,6 +780,9 @@ struct kvm_vcpu_arch {
 
 	/* Flush the L1 Data cache for L1TF mitigation on VMENTER */
 	bool l1tf_flush_l1d;
+
+	/* AMD MSRC001_0015 Hardware Configuration */
+	u64 msr_hwcr;
 };
 
 struct kvm_lpage_info {
arch/x86/kvm/x86.c +27 -6
index 099b851dabaf..10f6acc6494c 100644
@@ -2273,6 +2273,18 @@ static void kvmclock_sync_fn(struct work_struct *work)
 					KVMCLOCK_SYNC_PERIOD);
 }
 
+/*
+ * On AMD, HWCR[McStatusWrEn] controls whether setting MCi_STATUS results in #GP.
+ */
+static bool can_set_mci_status(struct kvm_vcpu *vcpu)
+{
+	/* McStatusWrEn enabled? */
+	if (guest_cpuid_is_amd(vcpu))
+		return !!(vcpu->arch.msr_hwcr & BIT_ULL(18));
+
+	return false;
+}
+
 static int set_msr_mce(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 {
 	u64 mcg_cap = vcpu->arch.mcg_cap;

@@ -2304,9 +2316,13 @@ static int set_msr_mce(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 			if ((offset & 0x3) == 0 &&
 			    data != 0 && (data | (1 << 10)) != ~(u64)0)
 				return -1;
-			if (!msr_info->host_initiated &&
-				(offset & 0x3) == 1 && data != 0)
-				return -1;
+
+			/* MCi_STATUS */
+			if ((offset & 0x3) == 1 && !msr_info->host_initiated) {
+				if (!can_set_mci_status(vcpu))
+					return -1;
+			}
+
 			vcpu->arch.mce_banks[offset] = data;
 			break;
 		}

@@ -2455,8 +2471,11 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		data &= ~(u64)0x40;	/* ignore flush filter disable */
 		data &= ~(u64)0x100;	/* ignore ignne emulation enable */
 		data &= ~(u64)0x8;	/* ignore TLB cache disable */
-		data &= ~(u64)0x40000;  /* ignore Mc status write enable */
-		if (data != 0) {
+
+		/* Handle McStatusWrEn */
+		if (data == BIT_ULL(18)) {
+			vcpu->arch.msr_hwcr = data;
+		} else if (data != 0) {
 			vcpu_unimpl(vcpu, "unimplemented HWCR wrmsr: 0x%llx\n",
 				    data);
 			return 1;

@@ -2730,7 +2749,6 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	case MSR_K8_SYSCFG:
 	case MSR_K8_TSEG_ADDR:
 	case MSR_K8_TSEG_MASK:
-	case MSR_K7_HWCR:
 	case MSR_VM_HSAVE_PA:
 	case MSR_K8_INT_PENDING_MSG:
 	case MSR_AMD64_NB_CFG:

@@ -2894,6 +2912,9 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	case MSR_MISC_FEATURES_ENABLES:
 		msr_info->data = vcpu->arch.msr_misc_features_enables;
 		break;
+	case MSR_K7_HWCR:
+		msr_info->data = vcpu->arch.msr_hwcr;
+		break;
 	default:
 		if (kvm_pmu_is_valid_msr(vcpu, msr_info->index))
 			return kvm_pmu_get_msr(vcpu, msr_info->index, &msr_info->data);
Signature · 7 lines
2.21.0

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
#4 lore raw
Quoted 76 lines · Borislav Petkov

Hi all,

ok here's v5.1 with most of Sean's feedback addressed. The function checking whether HWCR[18] is set, I've renamed to can_set_mci_status() and left it to return bool because it really is used in boolean context, answering the question "Can I set MCi_STATUS MSRs?"

And now it all looks simple and clean, just how I like it! :-)

Thx.

---
From: Borislav Petkov <[email protected]>

The hardware configuration register has some useful bits which can be used by guests. Implement McStatusWrEn which can be used by guests when injecting MCEs with the in-kernel mce-inject module.

For that, we need to set bit 18 - McStatusWrEn - first, before writing the MCi_STATUS registers (otherwise we #GP).

Add the required machinery to do so.

Signed-off-by Borislav Petkov <[email protected]>
Cc Jim Mattson <[email protected]>
Cc Joerg Roedel <[email protected]>
Cc KVM <[email protected]>
Cc Paolo Bonzini <[email protected]>
Cc Radim Krčmář <[email protected]>
Cc Sean Christopherson <[email protected]>
Cc Tom Lendacky <[email protected]>
Cc Tony Luck <[email protected]>
Cc Yazen Ghannam <[email protected]>
---
 arch/x86/include/asm/kvm_host.h |  3 +++
 arch/x86/kvm/x86.c              | 33 +++++++++++++++++++++++++++------
 2 files changed, 30 insertions(+), 6 deletions(-)
arch/x86/include/asm/kvm_host.h +3 -0
index 159b5988292f..541c431df806 100644
@@ -780,6 +780,9 @@ struct kvm_vcpu_arch {
 
 	/* Flush the L1 Data cache for L1TF mitigation on VMENTER */
 	bool l1tf_flush_l1d;
+
+	/* AMD MSRC001_0015 Hardware Configuration */
+	u64 msr_hwcr;
 };
 
 struct kvm_lpage_info {
arch/x86/kvm/x86.c +12 -0
index 099b851dabaf..10f6acc6494c 100644
@@ -2273,6 +2273,18 @@ static void kvmclock_sync_fn(struct work_struct *work)
 					KVMCLOCK_SYNC_PERIOD);
 }
 
+/*
+ * On AMD, HWCR[McStatusWrEn] controls whether setting MCi_STATUS results in #GP.
+ */
+static bool can_set_mci_status(struct kvm_vcpu *vcpu)
+{
+	/* McStatusWrEn enabled? */
+	if (guest_cpuid_is_amd(vcpu))
+		return !!(vcpu->arch.msr_hwcr & BIT_ULL(18));
+
+	return false;
+}
+
 static int set_msr_mce(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 {
 	u64 mcg_cap = vcpu->arch.mcg_cap;

arch/x86/kvm/x86.c static int set_msr_mce(struct kvm_vcpu *vcpu, struct msr_data *msr_info) · line 2322

Quoted 10 lines
arch/x86/kvm/x86.c +4 -3
@@ -2304,9 +2316,13 @@ static int set_msr_mce(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 			if ((offset & 0x3) == 0 &&
 			    data != 0 && (data | (1 << 10)) != ~(u64)0)
 				return -1;
-			if (!msr_info->host_initiated &&
-				(offset & 0x3) == 1 && data != 0)
-				return -1;
+
+			/* MCi_STATUS */
+			if ((offset & 0x3) == 1 && !msr_info->host_initiated) {
+				if (!can_set_mci_status(vcpu))

This doesn't allow writing '0' regardless of msr_hwcr.BIT(18), which was previously supported. And there's no need for multiple if statements.

Quoted 46 lines
arch/x86/kvm/x86.c +11 -3
+					return -1;
+			}
+
 			vcpu->arch.mce_banks[offset] = data;
 			break;
 		}

@@ -2455,8 +2471,11 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		data &= ~(u64)0x40;	/* ignore flush filter disable */
 		data &= ~(u64)0x100;	/* ignore ignne emulation enable */
 		data &= ~(u64)0x8;	/* ignore TLB cache disable */
-		data &= ~(u64)0x40000;  /* ignore Mc status write enable */
-		if (data != 0) {
+
+		/* Handle McStatusWrEn */
+		if (data == BIT_ULL(18)) {
+			vcpu->arch.msr_hwcr = data;
+		} else if (data != 0) {
 			vcpu_unimpl(vcpu, "unimplemented HWCR wrmsr: 0x%llx\n",
 				    data);
 			return 1;

@@ -2730,7 +2749,6 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	case MSR_K8_SYSCFG:
 	case MSR_K8_TSEG_ADDR:
 	case MSR_K8_TSEG_MASK:
-	case MSR_K7_HWCR:
 	case MSR_VM_HSAVE_PA:
 	case MSR_K8_INT_PENDING_MSG:
 	case MSR_AMD64_NB_CFG:

@@ -2894,6 +2912,9 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	case MSR_MISC_FEATURES_ENABLES:
 		msr_info->data = vcpu->arch.msr_misc_features_enables;
 		break;
+	case MSR_K7_HWCR:
+		msr_info->data = vcpu->arch.msr_hwcr;
+		break;
 	default:
 		if (kvm_pmu_is_valid_msr(vcpu, msr_info->index))
 			return kvm_pmu_get_msr(vcpu, msr_info->index, &msr_info->data);
Signature · 7 lines
2.21.0

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
author maintainer
#5 lore raw
Quoted 2 lines · Sean Christopherson

This doesn't allow writing '0' regardless of msr_hwcr.BIT(18), which was previously supported.

Restored the old conditional:

arch/x86/kvm/x86.c +2 -1
index 10f6acc6494c..f74f1280745b 100644
@@ -2318,7 +2318,8 @@ static int set_msr_mce(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 				return -1;
 
 			/* MCi_STATUS */
-			if ((offset & 0x3) == 1 && !msr_info->host_initiated) {
+			if (!msr_info->host_initiated &&
+			    (offset & 0x3) == 1 && data != 0) {
 				if (!can_set_mci_status(vcpu))
 					return -1;
 			}
Quoted 1 line

And there's no need for multiple if statements.

It is a bit more readable this way.

Actually, I'd break that if above into smaller if-statements with flipped logic to make it even more readable:

	if (msr_info->host_initiated)
		goto set_msr;

	if (!(offset & 0x3))
		goto set_msr;

	if (!data)
		goto set_msr;

	if (!can_set_mci_status(vcpu))
		return -1;

set_msr:

	...

Anything else? Can I send v5.2 now?

Signature · 4 lines
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
#6 lore raw
Quoted 28 lines · Borislav Petkov
Quoted 2 lines · Sean Christopherson

This doesn't allow writing '0' regardless of msr_hwcr.BIT(18), which was previously supported.

Restored the old conditional:

arch/x86/kvm/x86.c +2 -1
index 10f6acc6494c..f74f1280745b 100644
@@ -2318,7 +2318,8 @@ static int set_msr_mce(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 				return -1;
 
 			/* MCi_STATUS */
-			if ((offset & 0x3) == 1 && !msr_info->host_initiated) {
+			if (!msr_info->host_initiated &&
+			    (offset & 0x3) == 1 && data != 0) {
 				if (!can_set_mci_status(vcpu))
 					return -1;
 			}
Quoted 2 lines

And there's no need for multiple if statements.

It is a bit more readable this way.

Actually, I'd break that if above into smaller if-statements with flipped logic to make it even more readable:

Quoted 11 lines
	if (msr_info->host_initiated)
		goto set_msr;

	if (!(offset & 0x3))
		goto set_msr;

	if (!data)
		goto set_msr;

	if (!can_set_mci_status(vcpu))
		return -1;

At that point it probably makes sense to rework the whole block as there are existing issues in both the CTL and STATUS flows:

- Fault cases should return '1', not '-1'.

- host_initiated should be checked for the CTL case

My personal preference would be to combine the host_initiated and !data checks for brevity, so something like:

	if (msr_info->host_initiated || !data)
		goto set_msr;

	offset = msr - MSR_IA32_MC0_CTL;
	if ((offset & 0x3) == 0 && (data | (1 << 10)) != ~(u64)0)
		return 1;
	if ((offset & 0x3) == 1 && !can_set_mci_status(vcpu))
		return 1;

set_msr:

Quoted 12 lines

set_msr:

	...

Anything else? Can I send v5.2 now?

Signature · 4 lines
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
maintainer
#7 lore raw
In reply to Borislav Petkov
Quoted 40 lines · Borislav Petkov
Quoted 2 lines · Sean Christopherson

This doesn't allow writing '0' regardless of msr_hwcr.BIT(18), which was previously supported.

Restored the old conditional:

arch/x86/kvm/x86.c +2 -1
index 10f6acc6494c..f74f1280745b 100644
@@ -2318,7 +2318,8 @@ static int set_msr_mce(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 				return -1;
 
 			/* MCi_STATUS */
-			if ((offset & 0x3) == 1 && !msr_info->host_initiated) {
+			if (!msr_info->host_initiated &&
+			    (offset & 0x3) == 1 && data != 0) {
 				if (!can_set_mci_status(vcpu))
 					return -1;
 			}
Quoted 2 lines

And there's no need for multiple if statements.

It is a bit more readable this way.

Actually, I'd break that if above into smaller if-statements with flipped logic to make it even more readable:

	if (msr_info->host_initiated)
		goto set_msr;

	if (!(offset & 0x3))
		goto set_msr;

	if (!data)
		goto set_msr;

	if (!can_set_mci_status(vcpu))
		return -1;
Quoted 5 lines

set_msr:

	...

Anything else? Can I send v5.2 now?

No need, squashing this in before running away for vacation.

Paolo

maintainer Applied
#8 lore raw
In reply to Borislav Petkov
Quoted 8 lines · Borislav Petkov

The hardware configuration register has some useful bits which can be used by guests. Implement McStatusWrEn which can be used by guests when injecting MCEs with the in-kernel mce-inject module.

For that, we need to set bit 18 - McStatusWrEn - first, before writing the MCi_STATUS registers (otherwise we #GP).

Add the required machinery to do so.

Just one little thing missing:

arch/x86/kvm/x86.c +2 -0
index eb49d2cee68a..6e23343c6b36 100644
@@ -1172,6 +1172,8 @@ bool kvm_rdpmc(struct kvm_vcpu *vcpu)
 	MSR_MISC_FEATURES_ENABLES,
 	MSR_AMD64_VIRT_SPEC_CTRL,
 	MSR_IA32_POWER_CTL,
+
+	MSR_K7_HWCR,
 };
 
 static unsigned num_emulated_msrs;

Queued with this change.

Paolo

Quoted 93 lines
Signed-off-by Borislav Petkov <[email protected]>
Cc Jim Mattson <[email protected]>
Cc Joerg Roedel <[email protected]>
Cc KVM <[email protected]>
Cc Paolo Bonzini <[email protected]>
Cc Radim Krčmář <[email protected]>
Cc Sean Christopherson <[email protected]>
Cc Tom Lendacky <[email protected]>
Cc Tony Luck <[email protected]>
Cc Yazen Ghannam <[email protected]>
---
 arch/x86/include/asm/kvm_host.h |  3 +++
 arch/x86/kvm/x86.c              | 33 +++++++++++++++++++++++++++------
 2 files changed, 30 insertions(+), 6 deletions(-)
arch/x86/include/asm/kvm_host.h +3 -0
index 159b5988292f..541c431df806 100644
@@ -780,6 +780,9 @@ struct kvm_vcpu_arch {
 
 	/* Flush the L1 Data cache for L1TF mitigation on VMENTER */
 	bool l1tf_flush_l1d;
+
+	/* AMD MSRC001_0015 Hardware Configuration */
+	u64 msr_hwcr;
 };
 
 struct kvm_lpage_info {
arch/x86/kvm/x86.c +24 -6
index 099b851dabaf..10f6acc6494c 100644
@@ -2273,6 +2273,18 @@ static void kvmclock_sync_fn(struct work_struct *work)
 					KVMCLOCK_SYNC_PERIOD);
 }
 
+/*
+ * On AMD, HWCR[McStatusWrEn] controls whether setting MCi_STATUS results in #GP.
+ */
+static bool can_set_mci_status(struct kvm_vcpu *vcpu)
+{
+	/* McStatusWrEn enabled? */
+	if (guest_cpuid_is_amd(vcpu))
+		return !!(vcpu->arch.msr_hwcr & BIT_ULL(18));
+
+	return false;
+}
+
 static int set_msr_mce(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 {
 	u64 mcg_cap = vcpu->arch.mcg_cap;

@@ -2304,9 +2316,13 @@ static int set_msr_mce(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 			if ((offset & 0x3) == 0 &&
 			    data != 0 && (data | (1 << 10)) != ~(u64)0)
 				return -1;
-			if (!msr_info->host_initiated &&
-				(offset & 0x3) == 1 && data != 0)
-				return -1;
+
+			/* MCi_STATUS */
+			if ((offset & 0x3) == 1 && !msr_info->host_initiated) {
+				if (!can_set_mci_status(vcpu))
+					return -1;
+			}
+
 			vcpu->arch.mce_banks[offset] = data;
 			break;
 		}

@@ -2455,8 +2471,11 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		data &= ~(u64)0x40;	/* ignore flush filter disable */
 		data &= ~(u64)0x100;	/* ignore ignne emulation enable */
 		data &= ~(u64)0x8;	/* ignore TLB cache disable */
-		data &= ~(u64)0x40000;  /* ignore Mc status write enable */
-		if (data != 0) {
+
+		/* Handle McStatusWrEn */
+		if (data == BIT_ULL(18)) {
+			vcpu->arch.msr_hwcr = data;
+		} else if (data != 0) {
 			vcpu_unimpl(vcpu, "unimplemented HWCR wrmsr: 0x%llx\n",
 				    data);
 			return 1;

@@ -2730,7 +2749,6 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	case MSR_K8_SYSCFG:
 	case MSR_K8_TSEG_ADDR:
 	case MSR_K8_TSEG_MASK:
-	case MSR_K7_HWCR:
 	case MSR_VM_HSAVE_PA:
 	case MSR_K8_INT_PENDING_MSG:
 	case MSR_AMD64_NB_CFG:

arch/x86/kvm/x86.c int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)

Quoted 10 lines
arch/x86/kvm/x86.c +3 -1
@@ -2894,6 +2912,9 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	case MSR_MISC_FEATURES_ENABLES:
 		msr_info->data = vcpu->arch.msr_misc_features_enables;
 		break;
+	case MSR_K7_HWCR:
+		msr_info->data = vcpu->arch.msr_hwcr;
+		break;
 	default:
 		if (kvm_pmu_is_valid_msr(vcpu, msr_info->index))
 			return kvm_pmu_get_msr(vcpu, msr_info->index, &msr_info->data);
-- 2.21.0

Queued, thanks.