[PATCH v4 0/2] x86/kvm: Enable MCE injection in the guest

[PATCH v4 0/2] x86/kvm: Enable MCE injection in the guest

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

Hi,

so AFAIR this got applied before the merge window but it is not
upstream. Very strange.

So let me send it again, might get more lucky this time. Rebased ontop
of 5.1-rc1.

Please apply,
thanks.

Changelog:
==========

v3:

here's a rediff ontop of -rc2. No changes, only added Yazen's Tested-by.

v2:

here's v2, dropping patch 3 and incorporating hopefully all of Radim's
feedback.

v1:

there's this mce-inject.ko module in the kernel which allows for
injecting real MCEs and thus test the MCE handling code.

It is doubly useful to be able to inject same MCEs in a guest so that
testing of the MCE handling code can happen even easier/faster. In order
to be able to do that on an AMD guest, we need to emulate some bits
and pieces like the HWCR[McStatusWrEn] bit which allows writes to the
MCi_STATUS registers without a #GP.

The below does that and with it I'm able to properly inject MCEs in said
guest.


Borislav Petkov (2):
  kvm/x86: Move MSR_K7_HWCR to svm.c
  x86/kvm: Implement MSR_HWCR support

 arch/x86/kvm/svm.c | 20 ++++++++++++++++++++
 arch/x86/kvm/x86.c | 46 +++++++++++++++++++++++++++++++---------------
 2 files changed, 51 insertions(+), 15 deletions(-)

-- 
2.21.0

[PATCH 1/2] kvm/x86: Move MSR_K7_HWCR to svm.c

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

This is an AMD-specific MSR. Put it where it belongs.

Signed-off-by: Borislav Petkov <bp@suse.de>
Tested-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
 arch/x86/kvm/svm.c | 14 ++++++++++++++
 arch/x86/kvm/x86.c | 12 ------------
 2 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index b5b128a0a051..00eb44a2a377 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -4201,6 +4201,9 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	case MSR_F10H_DECFG:
 		msr_info->data = svm->msr_decfg;
 		break;
+	case MSR_K7_HWCR:
+		msr_info->data = 0;
+		break;
 	default:
 		return kvm_get_msr_common(vcpu, msr_info);
 	}
@@ -4405,6 +4408,17 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
 		svm->msr_decfg = data;
 		break;
 	}
+	case MSR_K7_HWCR:
+		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) {
+			vcpu_unimpl(vcpu, "unimplemented HWCR wrmsr: 0x%llx\n",
+				    data);
+			return 1;
+		}
+		break;
 	case MSR_IA32_APICBASE:
 		if (kvm_vcpu_apicv_active(vcpu))
 			avic_update_vapic_bar(to_svm(vcpu), data);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 65e4559eef2f..e53d13cfceba 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2445,17 +2445,6 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		break;
 	case MSR_EFER:
 		return set_efer(vcpu, data);
-	case MSR_K7_HWCR:
-		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) {
-			vcpu_unimpl(vcpu, "unimplemented HWCR wrmsr: 0x%llx\n",
-				    data);
-			return 1;
-		}
-		break;
 	case MSR_FAM10H_MMIO_CONF_BASE:
 		if (data != 0) {
 			vcpu_unimpl(vcpu, "unimplemented MMIO_CONF_BASE wrmsr: "
@@ -2724,7 +2713,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:
-- 
2.21.0

[PATCH 2/2] x86/kvm: Implement MSR_HWCR support

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

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 <bp@suse.de>
Tested-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
 arch/x86/kvm/svm.c | 12 +++++++++---
 arch/x86/kvm/x86.c | 34 +++++++++++++++++++++++++++++++---
 2 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 00eb44a2a377..e5dfa00afe55 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -251,6 +251,9 @@ struct vcpu_svm {
 
 	/* which host CPU was used for running this vcpu */
 	unsigned int last_cpu;
+
+	/* MSRC001_0015 Hardware Configuration */
+	u64 msr_hwcr;
 };
 
 /*
@@ -4202,7 +4205,7 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		msr_info->data = svm->msr_decfg;
 		break;
 	case MSR_K7_HWCR:
-		msr_info->data = 0;
+		msr_info->data = svm->msr_hwcr;
 		break;
 	default:
 		return kvm_get_msr_common(vcpu, msr_info);
@@ -4412,8 +4415,11 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
 		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)) {
+			svm->msr_hwcr = data;
+		} else if (data != 0) {
 			vcpu_unimpl(vcpu, "unimplemented HWCR wrmsr: 0x%llx\n",
 				    data);
 			return 1;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index e53d13cfceba..dda7e1abb593 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2272,6 +2272,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)
+{
+	if (guest_cpuid_is_amd(vcpu)) {
+		struct msr_data tmp;
+
+		tmp.index = MSR_K7_HWCR;
+
+		if (kvm_x86_ops->get_msr(vcpu, &tmp))
+			return false;
+
+		/* McStatusWrEn enabled? */
+		if (tmp.data & BIT_ULL(18))
+			return true;
+	}
+
+	if (msr_info->data != 0)
+		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;
@@ -2303,9 +2327,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;
 		}
-- 
2.21.0

Re: [PATCH 1/2] kvm/x86: Move MSR_K7_HWCR to svm.c

[Jim Mattson]

On Mon, Mar 25, 2019 at 10:17 AM Borislav Petkov <bp@alien8.de> wrote:
>
> From: Borislav Petkov <bp@suse.de>
>
> This is an AMD-specific MSR. Put it where it belongs.
>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> Tested-by: Yazen Ghannam <yazen.ghannam@amd.com>
> ---
>  arch/x86/kvm/svm.c | 14 ++++++++++++++
>  arch/x86/kvm/x86.c | 12 ------------
>  2 files changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index b5b128a0a051..00eb44a2a377 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -4201,6 +4201,9 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>         case MSR_F10H_DECFG:
>                 msr_info->data = svm->msr_decfg;
>                 break;
> +       case MSR_K7_HWCR:
> +               msr_info->data = 0;
> +               break;

If the guest has written a non-zero value to this MSR, shouldn't it be
able to read that value back?

Re: [PATCH 1/2] kvm/x86: Move MSR_K7_HWCR to svm.c

[Borislav Petkov]

On Mon, Mar 25, 2019 at 11:05:48AM -0700, Jim Mattson wrote:
> If the guest has written a non-zero value to this MSR, shouldn't it be
> able to read that value back?

See the second patch.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH 1/2] kvm/x86: Move MSR_K7_HWCR to svm.c

[Sean Christopherson]

On Mon, Mar 25, 2019 at 06:16:48PM +0100, Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.de>
> 
> This is an AMD-specific MSR. Put it where it belongs.
> 
> Signed-off-by: Borislav Petkov <bp@suse.de>
> Tested-by: Yazen Ghannam <yazen.ghannam@amd.com>
> ---
>  arch/x86/kvm/svm.c | 14 ++++++++++++++
>  arch/x86/kvm/x86.c | 12 ------------
>  2 files changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index b5b128a0a051..00eb44a2a377 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -4201,6 +4201,9 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  	case MSR_F10H_DECFG:
>  		msr_info->data = svm->msr_decfg;
>  		break;
> +	case MSR_K7_HWCR:
> +		msr_info->data = 0;
> +		break;
>  	default:
>  		return kvm_get_msr_common(vcpu, msr_info);
>  	}
> @@ -4405,6 +4408,17 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
>  		svm->msr_decfg = data;
>  		break;
>  	}
> +	case MSR_K7_HWCR:
> +		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) {
> +			vcpu_unimpl(vcpu, "unimplemented HWCR wrmsr: 0x%llx\n",
> +				    data);
> +			return 1;
> +		}
> +		break;
>  	case MSR_IA32_APICBASE:
>  		if (kvm_vcpu_apicv_active(vcpu))
>  			avic_update_vapic_bar(to_svm(vcpu), data);
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 65e4559eef2f..e53d13cfceba 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2445,17 +2445,6 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  		break;
>  	case MSR_EFER:
>  		return set_efer(vcpu, data);
> -	case MSR_K7_HWCR:
> -		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) {
> -			vcpu_unimpl(vcpu, "unimplemented HWCR wrmsr: 0x%llx\n",
> -				    data);
> -			return 1;
> -		}
> -		break;
>  	case MSR_FAM10H_MMIO_CONF_BASE:
>  		if (data != 0) {
>  			vcpu_unimpl(vcpu, "unimplemented MMIO_CONF_BASE wrmsr: "
> @@ -2724,7 +2713,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:

Won't this prevent emulating an AMD guest on Intel hardware, e.g. due to
injecting #GPs during boot?  Keeping support in kvm_{get,set}_msr_common
doesn't preclude svm_{get,set}_msr() from having SVM-specific handling for
the MSR.

>  	case MSR_VM_HSAVE_PA:
>  	case MSR_K8_INT_PENDING_MSG:
>  	case MSR_AMD64_NB_CFG:
> -- 
> 2.21.0
> 

Re: [PATCH 1/2] kvm/x86: Move MSR_K7_HWCR to svm.c

[Borislav Petkov]

On Mon, Mar 25, 2019 at 11:21:33AM -0700, Sean Christopherson wrote:
> Won't this prevent emulating an AMD guest on Intel hardware, e.g. due to
> injecting #GPs during boot?

I guess, but...

> Keeping support in kvm_{get,set}_msr_common
> doesn't preclude svm_{get,set}_msr() from having SVM-specific handling for
> the MSR.

... is kvm_{get,set}_msr_common() supposed to cover for all those
overlapping MSRs between AMD and Intel? svm_{get,set}_msr() have a lot
more AMD-specific MSRs just like vmx_{get,set}_msr() respectively for
Intel.

Which would mean that if you really want to support those cross-vendor
emulations, you don't need the svm* and vmx* MSR accessors... or am I
missing something?

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH 1/2] kvm/x86: Move MSR_K7_HWCR to svm.c

[Sean Christopherson]

On Mon, Mar 25, 2019 at 07:39:09PM +0100, Borislav Petkov wrote:
> On Mon, Mar 25, 2019 at 11:21:33AM -0700, Sean Christopherson wrote:
> > Won't this prevent emulating an AMD guest on Intel hardware, e.g. due to
> > injecting #GPs during boot?
> 
> I guess, but...
> 
> > Keeping support in kvm_{get,set}_msr_common
> > doesn't preclude svm_{get,set}_msr() from having SVM-specific handling for
> > the MSR.
> 
> ... is kvm_{get,set}_msr_common() supposed to cover for all those
> overlapping MSRs between AMD and Intel? svm_{get,set}_msr() have a lot
> more AMD-specific MSRs just like vmx_{get,set}_msr() respectively for
> Intel.
> 
> Which would mean that if you really want to support those cross-vendor
> emulations, you don't need the svm* and vmx* MSR accessors... or am I
> missing something?

Generally speaking, the goal is to support cross-vendor VMs without having
to modify the guest kernel, i.e. exact emulation is out of scope.  This
means "emulating" cross-vendor MSRs that the guest expects to exist to the
point where the guest won't explode, e.g. in the case of MSR_K7_HWCR, Linux
expects the MSR to exist on all AMD platforms and AFAICT will die during
boot if it doesn't.

The rule of thumb for "what MSRs can a guest reasonably expect to exist"
is fluid.  The most clear cut cases are when support is explicitly
enumerated via some feature bit and KVM reports support for said feature
to userspace, e.g. if userspace advertises a feature (to the guest) that
KVM doesn't support, then it's a userspace bug.

But for MSRs like MSR_K7_HWCR and MSR_F10H_DECFG where their existence is
implicit, whoever came first often wins.  For example, MSR_K7_HWCR existed
long before KVM and guest kernels expect it to exist on all AMD CPUs, so
KVM emulates it unconditionally.  Whereas MSR_F10H_DECFG was recently
added and obviously not emulated on existing hypervisors, so the kernel
has to assume the MSR might not exist when running under a hypervisor,
which means KVM doesn't need to pretend the MSR uncondtionally exists.

Re: [PATCH 1/2] kvm/x86: Move MSR_K7_HWCR to svm.c

[Borislav Petkov]

On Mon, Mar 25, 2019 at 12:21:11PM -0700, Sean Christopherson wrote:
> Generally speaking, the goal is to support cross-vendor VMs without having
> to modify the guest kernel, i.e. exact emulation is out of scope.  This
> means "emulating" cross-vendor MSRs that the guest expects to exist to the
> point where the guest won't explode, e.g. in the case of MSR_K7_HWCR, Linux
> expects the MSR to exist on all AMD platforms and AFAICT will die during
> boot if it doesn't.
> 
> The rule of thumb for "what MSRs can a guest reasonably expect to exist"
> is fluid.

Ok, I'll keep it in the common MSR accessors in the next version.

Thx for confirming what I was suspecting.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.