[PATCH] kvm: svm: Only propagate next_rip when guest supports it

[PATCH] kvm: svm: Only propagate next_rip when guest supports it

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

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.

Cc: Bandan Das <bsd@redhat.com>
Cc: Dirk Mueller <dmueller@suse.com>
Tested-by: Dirk Mueller <dmueller@suse.com>
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 arch/x86/kvm/cpuid.h | 21 +++++++++++++++++++++
 arch/x86/kvm/svm.c   |  4 +++-
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
index dd05b9c..effca1f 100644
--- a/arch/x86/kvm/cpuid.h
+++ b/arch/x86/kvm/cpuid.h
@@ -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
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 2f9ed1f..4084b33 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -2365,7 +2365,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(&svm->vcpu))
+		nested_vmcb->control.next_rip  = vmcb->control.next_rip;
 
 	/*
 	 * If we emulate a VMRUN/#VMEXIT in the same host #vmexit cycle we have
-- 
1.9.1

Re: [PATCH] kvm: svm: Only propagate next_rip when guest supports it

[Paolo Bonzini]

On 09/10/2015 11:51, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
> 
> 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.
> 
> Cc: Bandan Das <bsd@redhat.com>
> Cc: Dirk Mueller <dmueller@suse.com>
> Tested-by: Dirk Mueller <dmueller@suse.com>
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>  arch/x86/kvm/cpuid.h | 21 +++++++++++++++++++++
>  arch/x86/kvm/svm.c   |  4 +++-
>  2 files changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
> index dd05b9c..effca1f 100644
> --- a/arch/x86/kvm/cpuid.h
> +++ b/arch/x86/kvm/cpuid.h
> @@ -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
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 2f9ed1f..4084b33 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -2365,7 +2365,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(&svm->vcpu))

This could be a bit expensive to do on every vmexit.  Can you benchmark
it with kvm-unit-tests, or just cache the result in struct vcpu_svm?

Thanks,

Paolo

> +		nested_vmcb->control.next_rip  = vmcb->control.next_rip;
>  
>  	/*
>  	 * If we emulate a VMRUN/#VMEXIT in the same host #vmexit cycle we have
> 

[PATCH v2] kvm: svm: Only propagate next_rip when guest supports it

[Joerg Roedel]

On Fri, Oct 09, 2015 at 01:15:11PM +0200, Paolo Bonzini wrote:
> This could be a bit expensive to do on every vmexit.  Can you benchmark
> it with kvm-unit-tests, or just cache the result in struct vcpu_svm?

Yes, caching it is certainly a good idea. I updated the patch:

>From 94ee662c527683c26ea5fa98a5a8f2c798c58470 Mon Sep 17 00:00:00 2001
From: Joerg Roedel <jroedel@suse.de>
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.

Cc: Bandan Das <bsd@redhat.com>
Cc: Dirk Mueller <dmueller@suse.com>
Tested-By: Dirk Mueller <dmueller@suse.com>
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 arch/x86/kvm/cpuid.h | 21 +++++++++++++++++++++
 arch/x86/kvm/svm.c   | 11 ++++++++++-
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
index dd05b9c..effca1f 100644
--- a/arch/x86/kvm/cpuid.h
+++ b/arch/x86/kvm/cpuid.h
@@ -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
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 2f9ed1f..e9e3294 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -159,6 +159,9 @@ struct vcpu_svm {
 	u32 apf_reason;
 
 	u64  tsc_ratio;
+
+	/* cached guest cpuid flags for faster access */
+	bool nrips_enabled	: 1;
 };
 
 static DEFINE_PER_CPU(u64, current_tsc_ratio);
@@ -2365,7 +2368,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 (svm->nrips_enabled)
+		nested_vmcb->control.next_rip  = vmcb->control.next_rip;
 
 	/*
 	 * If we emulate a VMRUN/#VMEXIT in the same host #vmexit cycle we have
@@ -4098,6 +4103,10 @@ static u64 svm_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
 
 static void svm_cpuid_update(struct kvm_vcpu *vcpu)
 {
+	struct vcpu_svm *svm = to_svm(vcpu);
+
+	/* Update nrips enabled cache */
+	svm->nrips_enabled = !!guest_cpuid_has_nrips(&svm->vcpu);
 }
 
 static void svm_set_supported_cpuid(u32 func, struct kvm_cpuid_entry2 *entry)
-- 
1.8.4.5

Re: [PATCH v2] kvm: svm: Only propagate next_rip when guest supports it

[Paolo Bonzini]

On 14/10/2015 15:10, Joerg Roedel wrote:
> From 94ee662c527683c26ea5fa98a5a8f2c798c58470 Mon Sep 17 00:00:00 2001
> From: Joerg Roedel <jroedel@suse.de>
> 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.
> 
> Cc: Bandan Das <bsd@redhat.com>
> Cc: Dirk Mueller <dmueller@suse.com>
> Tested-By: Dirk Mueller <dmueller@suse.com>
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>  arch/x86/kvm/cpuid.h | 21 +++++++++++++++++++++
>  arch/x86/kvm/svm.c   | 11 ++++++++++-
>  2 files changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
> index dd05b9c..effca1f 100644
> --- a/arch/x86/kvm/cpuid.h
> +++ b/arch/x86/kvm/cpuid.h
> @@ -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
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 2f9ed1f..e9e3294 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -159,6 +159,9 @@ struct vcpu_svm {
>  	u32 apf_reason;
>  
>  	u64  tsc_ratio;
> +
> +	/* cached guest cpuid flags for faster access */
> +	bool nrips_enabled	: 1;
>  };
>  
>  static DEFINE_PER_CPU(u64, current_tsc_ratio);
> @@ -2365,7 +2368,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 (svm->nrips_enabled)
> +		nested_vmcb->control.next_rip  = vmcb->control.next_rip;
>  
>  	/*
>  	 * If we emulate a VMRUN/#VMEXIT in the same host #vmexit cycle we have
> @@ -4098,6 +4103,10 @@ static u64 svm_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
>  
>  static void svm_cpuid_update(struct kvm_vcpu *vcpu)
>  {
> +	struct vcpu_svm *svm = to_svm(vcpu);
> +
> +	/* Update nrips enabled cache */
> +	svm->nrips_enabled = !!guest_cpuid_has_nrips(&svm->vcpu);
>  }
>  
>  static void svm_set_supported_cpuid(u32 func, struct kvm_cpuid_entry2 *entry)
> 

Applied to kvm/queue, thanks.

Paolo

[PATCH] Use WARN_ON_ONCE for missing X86_FEATURE_NRIPS

[Dirk Müller]

[-- Attachment #1: Type: text/plain, Size: 294 bytes --]

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 <dmueller@suse.com>

-- 
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, 
HRB 21284 (AG Nürnberg)

[-- Attachment #2: 0001-Use-WARN_ON_ONCE-for-missing-X86_FEATURE_NRIPS.patch --]
[-- Type: text/x-patch, Size: 943 bytes --]

>From 40c7213e34a1a1caf0b98db832c06c593f888b11 Mon Sep 17 00:00:00 2001
From: Dirk Mueller <dirk@dmllr.de>
Date: Thu, 1 Oct 2015 13:10:10 +0200
Subject: [PATCH] Use WARN_ON_ONCE for missing X86_FEATURE_NRIPS

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 <dmueller@suse.com>
---
 arch/x86/kvm/svm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 94b7d15..0a42859 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -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;
 	}
 
-- 
2.5.1

Re: [PATCH] Use WARN_ON_ONCE for missing X86_FEATURE_NRIPS

[Paolo Bonzini]

On 01/10/2015 13:43, Dirk Müller wrote:
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 94b7d15..0a42859 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -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;
>  	}
>  

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

Re: [PATCH] Use WARN_ON_ONCE for missing X86_FEATURE_NRIPS

[Bandan Das]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 01/10/2015 13:43, Dirk Müller wrote:
>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> index 94b7d15..0a42859 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -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;
>>  	}
>>  
>
> 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.

From the commit:

 
-	if (svm->vmcb->control.next_rip != 0)
+	if (svm->vmcb->control.next_rip != 0) {
+		WARN_ON(!static_cpu_has(X86_FEATURE_NRIPS));
 		svm->next_rip = svm->vmcb->control.next_rip;
+	}
 
 	if (!svm->next_rip) {
 		if (emulate_instruction(vcpu, EMULTYPE_SKIP) !=
@@ -4317,7 +4319,9 @@ static int svm_check_intercept(struct kvm_vcpu *vcpu,
 		break;
 	}
 
-	vmcb->control.next_rip  = info->next_rip;
+	/* TODO: Advertise NRIPS to guest hypervisor unconditionally */
+	if (static_cpu_has(X86_FEATURE_NRIPS))
+		vmcb->control.next_rip  = info->next_rip;
 	vmcb->control.exit_code = icpt_info.exit_code;
 	vmexit = nested_svm_exit_handled(svm);
...

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

Yes, seems ok to me. If decodeassist isn't supported then it's
mostly a stale value. It's interesting that that L1 still works even
after we hit this warning!

> Paolo

Re: [PATCH] Use WARN_ON_ONCE for missing X86_FEATURE_NRIPS

[Joerg Roedel]

On Thu, Oct 01, 2015 at 06:31:27PM -0400, Bandan Das wrote:
> >> @@ -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

Re: [PATCH] Use WARN_ON_ONCE for missing X86_FEATURE_NRIPS

[Bandan Das]

Joerg Roedel <joro@8bytes.org> writes:

> On Thu, Oct 01, 2015 at 06:31:27PM -0400, Bandan Das wrote:
>> >> @@ -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

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

> 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

Re: [PATCH] Use WARN_ON_ONCE for missing X86_FEATURE_NRIPS

[Joerg Roedel]

On Tue, Oct 06, 2015 at 01:59:27PM -0400, Bandan Das wrote:
> Joerg Roedel <joro@8bytes.org> writes:
> >
> > 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.


	Joerg

[PATCH] kvm: svm: Only propagate next_rip when guest supports it

[Joerg Roedel]

On Wed, Oct 07, 2015 at 01:03:35PM +0200, Joerg Roedel wrote:
> 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 <jroedel@suse.de>
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 <jroedel@suse.de>
---
 arch/x86/kvm/cpuid.h | 21 +++++++++++++++++++++
 arch/x86/kvm/svm.c   |  7 ++++++-
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
index dd05b9c..effca1f 100644
--- a/arch/x86/kvm/cpuid.h
+++ b/arch/x86/kvm/cpuid.h
@@ -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
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 94b7d15..e1a8824 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -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
@@ -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;
 
+	/* Clear next_rip, as real hardware would do */
+	nested_vmcb->control.next_rip = 0;
+
 	nested_svm_unmap(page);
 
 	/* Enter Guest-Mode */
-- 
1.8.4.5

Re: [PATCH] kvm: svm: Only propagate next_rip when guest supports it

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1334 bytes --]

Hi Joerg,

[auto build test ERROR on v4.3-rc4 -- if it's inappropriate base, please ignore]

config: i386-randconfig-x009-201540 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   arch/x86/kvm/svm.c: In function 'nested_svm_vmexit':
>> arch/x86/kvm/svm.c:2369:28: error: 'vcpu' undeclared (first use in this function)
     if (guest_cpuid_has_nrips(vcpu))
                               ^
   arch/x86/kvm/svm.c:2369:28: note: each undeclared identifier is reported only once for each function it appears in

vim +/vcpu +2369 arch/x86/kvm/svm.c

  2363		nested_vmcb->control.exit_code_hi      = vmcb->control.exit_code_hi;
  2364		nested_vmcb->control.exit_info_1       = vmcb->control.exit_info_1;
  2365		nested_vmcb->control.exit_info_2       = vmcb->control.exit_info_2;
  2366		nested_vmcb->control.exit_int_info     = vmcb->control.exit_int_info;
  2367		nested_vmcb->control.exit_int_info_err = vmcb->control.exit_int_info_err;
  2368	
> 2369		if (guest_cpuid_has_nrips(vcpu))
  2370			nested_vmcb->control.next_rip  = vmcb->control.next_rip;
  2371	
  2372		/*

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 24334 bytes --]

Re: [PATCH] kvm: svm: Only propagate next_rip when guest supports it

[Bandan Das]

Joerg Roedel <joro@8bytes.org> writes:

> On Wed, Oct 07, 2015 at 01:03:35PM +0200, Joerg Roedel wrote:
>> 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).

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. 

> 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:

Yep, agreed.

> From 019afc60507618b8e44e0c67d5ea2d850d88c9dd Mon Sep 17 00:00:00 2001
> From: Joerg Roedel <jroedel@suse.de>
> 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 <jroedel@suse.de>
> ---
>  arch/x86/kvm/cpuid.h | 21 +++++++++++++++++++++
>  arch/x86/kvm/svm.c   |  7 ++++++-
>  2 files changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
> index dd05b9c..effca1f 100644
> --- a/arch/x86/kvm/cpuid.h
> +++ b/arch/x86/kvm/cpuid.h
> @@ -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
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 94b7d15..e1a8824 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -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
> @@ -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;
>  
> +	/* 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.

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

Re: [PATCH] kvm: svm: Only propagate next_rip when guest supports it

[Joerg Roedel]

On Wed, Oct 07, 2015 at 11:48:36AM -0400, Bandan Das wrote:
> 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.

Okay, so I think the warning can be removed.

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

Note that there is a bug here, instead of vcpu it must be &svm->vcpu.
Somehow I missed to at least compile-test this.

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

> >  
> >  	/*
> >  	 * 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;
> >  
> > +	/* 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.

Yeah, probably right. Since we only write guests next_rip when the guest
supports it via cpuid, there is probably no point in resetting it at
vmrun emulation.


	Joerg

Re: [PATCH] kvm: svm: Only propagate next_rip when guest supports it

[Dirk Müller]

On 07.10.2015 18:14, Joerg Roedel wrote:

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

Tested-By: Dirk Mueller <dmueller@suse.com>

Works fine here. Thanks!


Greetings,
Dirk